On Sun, Sep 8, 2013 at 10:32 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Sat, 7 Sep 2013 14:49:24 -0400
> Jim McDonough <jmcd@xxxxxxxxx> wrote:
>
>> Here's my second try. I hope there aren't missed ops which update the
>> inode without having nlink updated. At worst, they should stop
>> clobbering valid values, which was definitely happening before.
>>
>> Signed-off-by: Jim McDonough <jmcdonough@xxxxxxx>
>>
>> ---
>> fs/cifs/cifsglob.h | 1 +
>> fs/cifs/inode.c | 21 ++++++++++++++++++++-
>> fs/cifs/readdir.c | 3 +++
>> 3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -712,6 +712,7 @@ struct dfs_info3_param {
>> #define CIFS_FATTR_DELETE_PENDING 0x2
>> #define CIFS_FATTR_NEED_REVAL 0x4
>> #define CIFS_FATTR_INO_COLLISION 0x8
>> +#define CIFS_FATTR_MISSING_NLINK 0x10
>>
>> struct cifs_fattr {
>> u32 cf_flags;
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -118,6 +118,25 @@ cifs_revalidate_cache(struct inode *inod
>> cifs_i->invalid_mapping = true;
>> }
>>
>> +/* copy nlink to the inode, unless it wasn't provided. Provide
>> + sane values if we don't have an existing one and none was provided */
>> +static void
>> +cifs_nlink_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
>> +{
>> + if (fattr->cf_nlink) {
>> + inode->i_nlink = fattr->cf_nlink;
>> + return;
>> + }
>> +
>
> I think it would be best to get rid of the above if block and add an
> else block to the end of the following if clause that just sets
> inode->i_nlink = fattr->cf_nlink unconditionally.
>
> The problem with the logic here is that you're ignoring te
> (legitimate) case where cf_nlink might be 0 as reported by the server.
>
Yah, true. I am assuming it won't change to zero after it's been set
to something else.
>> + if ((fattr->cf_flags & CIFS_FATTR_MISSING_NLINK) &&
>> + (inode->i_state & I_NEW)) {
>> + if (fattr->cf_cifsattrs & ATTR_DIRECTORY)
>> + inode->i_nlink = 2;
>> + else
>> + inode->i_nlink = 1;
>> + }
>> +}
>> +
>> /* populate an inode with info from a cifs_fattr struct */
>> void
>> cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
>> @@ -132,7 +151,7 @@ cifs_fattr_to_inode(struct inode *inode,
>> inode->i_mtime = fattr->cf_mtime;
>> inode->i_ctime = fattr->cf_ctime;
>> inode->i_rdev = fattr->cf_rdev;
>> - inode->i_nlink = fattr->cf_nlink;
>> + cifs_nlink_fattr_to_inode(inode, fattr);
>> inode->i_uid = fattr->cf_uid;
>> inode->i_gid = fattr->cf_gid;
>>
>> --- a/fs/cifs/readdir.c
>> +++ b/fs/cifs/readdir.c
>> @@ -130,6 +130,9 @@ cifs_fill_common_info(struct cifs_fattr
>> fattr->cf_dtype = DT_REG;
>> }
>>
>> + /* non-unix readdir doesn't provide nlink */
>> + fattr->cf_flags &= CIFS_FATTR_MISSING_NLINK
>> +
>
> Shouldn't that be "|=" ?
Uh, yeah. I'd already fixed that but posted the wrong one. Sigh.
>
>> if (fattr->cf_cifsattrs & ATTR_READONLY)
>> fattr->cf_mode &= ~S_IWUGO;
>>
>>
>>
>>
>
> The only other thing I see is that the code in cifs_all_info_to_fattr
> ought to be changed to use this flag and not try to fake up the
> cf_nlink value.
Ok, I'll add that as well.
--
Jim McDonough
Samba Team
SUSE labs
jmcd at samba dot org
jmcd at themcdonoughs dot org
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
