| Message ID | CAH2r5mtyaufgrOssd872OZzq32TQ8fyprE82aK8FV7MbxceU7g@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2013-07-04 at 14:43 -0500, Steve French wrote: > I merged an equivalent but slightly smaller version of your patch into > cifs-2.6.git (see below) and added stable since we should not be > returning nlink of 0. Let me know if any objections. > > [CIFS] use sensible file nlink values if unprovided > > Certain servers may not set the NumberOfLinks field in query file/path > info responses. In such a case, cifs_inode_needs_reval() assumes that > all regular files are hardlinks and triggers revalidation, leading to > excessive and unnecessary network traffic. > > This change hardcodes cf_nlink (and subsequently i_nlink) when not > returned by the server, similar to what already occurs in cifs_mkdir(). > > Cc: <stable@vger.kernel.org> > Signed-off-by: David Disseldorp <ddiss@suse.de> > Signed-off-by: Steve French <smfrench@gmail.com> > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 20efd81..449b6cf 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -558,6 +558,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, > FILE_ALL_INFO *info, > fattr->cf_mode &= ~(S_IWUGO); > > fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > + if (fattr->cf_nlink < 1) { > + cifs_dbg(1, "replacing bogus file nlink value %u\n", > + fattr->cf_nlink); > + fattr->cf_nlink = 1; > + } > } > > fattr->cf_uid = cifs_sb->mnt_uid; > > > On Thu, Jul 4, 2013 at 1:06 PM, David Disseldorp <ddiss@suse.de> wrote: > > Certain servers may not set the NumberOfLinks field in query file/path > > info responses. In such a case, cifs_inode_needs_reval() assumes that > > all regular files are hardlinks and triggers revalidation, leading to > > excessive and unnecessary network traffic. > > > > This change hardcodes cf_nlink (and subsequently i_nlink) when not > > returned by the server, similar to what already occurs in cifs_mkdir(). > > > > Signed-off-by: David Disseldorp <ddiss@suse.de> > > --- > > fs/cifs/inode.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > > index 6f37228..8c48027 100644 > > --- a/fs/cifs/inode.c > > +++ b/fs/cifs/inode.c > > @@ -532,6 +532,8 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > > fattr->cf_bytes = le64_to_cpu(info->AllocationSize); > > fattr->cf_createtime = le64_to_cpu(info->CreationTime); > > > > + fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > > + > > if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > > fattr->cf_dtype = DT_DIR; > > @@ -542,9 +544,13 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > > /* clear write bits if ATTR_READONLY is set */ > > if (fattr->cf_cifsattrs & ATTR_READONLY) > > fattr->cf_mode &= ~(S_IWUGO); > > - } > > > > - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > > + if (fattr->cf_nlink < 1) { > > + cFYI(1, "replacing bogus file nlink value %u", > > + fattr->cf_nlink); > > + fattr->cf_nlink = 1; > > + } > > + } > > > > fattr->cf_uid = cifs_sb->mnt_uid; > > fattr->cf_gid = cifs_sb->mnt_gid; > > -- > > 1.8.1.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > What about readdir? Is a similar fix needed in cifs_fill_common_info or one of its callers?
On Thu, 4 Jul 2013 14:43:23 -0500 Steve French <smfrench@gmail.com> wrote: > I merged an equivalent but slightly smaller version of your patch into > cifs-2.6.git (see below) and added stable since we should not be > returning nlink of 0. Let me know if any objections. Looks good to me Steve. FWIW, I've been using the following patch to replicate the NumberOfLinks=0 behaviour on Samba: http://gitweb.samba.org/?p=ddiss/samba.git;a=commitdiff;h=93c8795c9a9e9f Cheers, David -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-07-05 at 18:40 +0200, David Disseldorp wrote: > On Thu, 4 Jul 2013 14:43:23 -0500 > Steve French <smfrench@gmail.com> wrote: > > > I merged an equivalent but slightly smaller version of your patch into > > cifs-2.6.git (see below) and added stable since we should not be > > returning nlink of 0. Let me know if any objections. > > Looks good to me Steve. > FWIW, I've been using the following patch to replicate the > NumberOfLinks=0 behaviour on Samba: > http://gitweb.samba.org/?p=ddiss/samba.git;a=commitdiff;h=93c8795c9a9e9f > > Cheers, David > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html I'm sorry, this patch is incomplete. It ignores the readdir codepath completely. If Steve has already merged this, then it needs a follow-on patch that fixes the readdir code too, probably with the same workaround in cifs_fill_common_info. Otherwise, you're going to end up with different st_nlink values depending on whether it got instantated by a readdir() or stat().
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index 20efd81..449b6cf 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -558,6 +558,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, fattr->cf_mode &= ~(S_IWUGO); fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); + if (fattr->cf_nlink < 1) { + cifs_dbg(1, "replacing bogus file nlink value %u\n", + fattr->cf_nlink); + fattr->cf_nlink = 1; + } }
I merged an equivalent but slightly smaller version of your patch into cifs-2.6.git (see below) and added stable since we should not be returning nlink of 0. Let me know if any objections. [CIFS] use sensible file nlink values if unprovided Certain servers may not set the NumberOfLinks field in query file/path info responses. In such a case, cifs_inode_needs_reval() assumes that all regular files are hardlinks and triggers revalidation, leading to excessive and unnecessary network traffic. This change hardcodes cf_nlink (and subsequently i_nlink) when not returned by the server, similar to what already occurs in cifs_mkdir(). Cc: <stable@vger.kernel.org> Signed-off-by: David Disseldorp <ddiss@suse.de> Signed-off-by: Steve French <smfrench@gmail.com> fattr->cf_uid = cifs_sb->mnt_uid; On Thu, Jul 4, 2013 at 1:06 PM, David Disseldorp <ddiss@suse.de> wrote: > Certain servers may not set the NumberOfLinks field in query file/path > info responses. In such a case, cifs_inode_needs_reval() assumes that > all regular files are hardlinks and triggers revalidation, leading to > excessive and unnecessary network traffic. > > This change hardcodes cf_nlink (and subsequently i_nlink) when not > returned by the server, similar to what already occurs in cifs_mkdir(). > > Signed-off-by: David Disseldorp <ddiss@suse.de> > --- > fs/cifs/inode.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 6f37228..8c48027 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -532,6 +532,8 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > fattr->cf_bytes = le64_to_cpu(info->AllocationSize); > fattr->cf_createtime = le64_to_cpu(info->CreationTime); > > + fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > + > if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > @@ -542,9 +544,13 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > /* clear write bits if ATTR_READONLY is set */ > if (fattr->cf_cifsattrs & ATTR_READONLY) > fattr->cf_mode &= ~(S_IWUGO); > - } > > - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > + if (fattr->cf_nlink < 1) { > + cFYI(1, "replacing bogus file nlink value %u", > + fattr->cf_nlink); > + fattr->cf_nlink = 1; > + } > + } > > fattr->cf_uid = cifs_sb->mnt_uid; > fattr->cf_gid = cifs_sb->mnt_gid; > -- > 1.8.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html