| Message ID | 1358276491-4835-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 15, 2013 at 1:01 PM, Jeff Layton <jlayton@redhat.com> wrote: > We've had a long-standing problem with DFS referral points. CIFS servers > generally try to make them look like directories in FIND_FIRST/NEXT > responses. When you go to try to do a FIND_FIRST on them though, the > server will then (correctly) return STATUS_PATH_NOT_COVERED. Mostly this > manifests as spurious EREMOTE errors back to userland. Why would we ever return STATUS_PATH_NOT_COVERED (converted to EREMOTE)? If we do a find first on a directory and get status path not covered - why don't we simply chase the DFS referral then rather than greatly slow down findfirst?
On Tue, 15 Jan 2013 13:34:07 -0600 Steve French <smfrench@gmail.com> wrote: > On Tue, Jan 15, 2013 at 1:01 PM, Jeff Layton <jlayton@redhat.com> wrote: > > We've had a long-standing problem with DFS referral points. CIFS servers > > generally try to make them look like directories in FIND_FIRST/NEXT > > responses. When you go to try to do a FIND_FIRST on them though, the > > server will then (correctly) return STATUS_PATH_NOT_COVERED. Mostly this > > manifests as spurious EREMOTE errors back to userland. > > Why would we ever return STATUS_PATH_NOT_COVERED (converted to > EREMOTE)? If we do a find first on a directory and get status path > not covered - why don't we simply chase the DFS referral then rather > than greatly slow down findfirst? > Note that I marked this RFC since I'm not 100% sold on the approach... This doesn't slow down FIND_FIRST, it just means that we revalidate the inode before trying to use it. The scenario goes something like this: Suppose we have a share like this: //server/share ...and under the top level of that share is a DFS referral "dfslink". Now, we go and do a readdir on the top level directory and that causes a FIND_FIRST to go out on the wire. The response has "dfslink" in it, but it looks just like a directory. So, we instantiate a new dentry for it with a directory inode, based on the info in the FIND_FIRST response. Now, we go to try and do a readdir() on "dfslink". We do an opendir() which doesn't cause any sort of call to go out onto the wire because we just instantiated the dentry. So, we end up doing a FIND_FIRST against //server/share/dfslink, and get back STATUS_PATH_NOT_COVERED. This patch fixes that by forcing the client to revalidate the dentry at lookup time (i.e. when the directory is opened in the above example). The lookup will be redone (via QPathInfo) and we'll end up chasing the referral as expected.
We had a problem reported by a user which I found is fixed by this patch. The customer reported a crash where they were able to cd into a DFS share where the target doesn't exist. The machine eventually crashes when the lookup code attempts to access the inode->i_ops->lookup which is set to NULL for DFS shares. I was able to recreate this problem and noticed that this also affects a standard DFS share. To recreate the problem, simply mount the DFS share on a mount point and run the following commands. # mount -t cifs -o username=user,password=mysecret //vm140-52/DFS_Public/ /mnt # cd /mnt # ls; cd n1/ Where n1 is a DFS share. 1) The first command 'ls', results in the readdir system call which ends up calling FIND_FIRST SMB command to the server. 2) An inode is initiated but no S_AUTOMOUNT flag is set at this stage. 3) The dentry initiated doesn't contain the DCACHE_NEED_AUTOMOUNT flag. 4) When we cd into this directory immediately, the dentry returned was just created(ie. not yet timed out) and is valid. 5) We then called for this dentry. Since we do not have the DCACHE_NEED_AUTOMOUNT flag set in dentry->d_flags, we do not trigger the automount. 6) After we have done the chdir, By attempting a few lookups, we eventually hit a problem because the inode functions in inode->i_ops are all set to NULL. I have tested with this patch and it does work since we force a QUERY_PATH_LOOKUP on all directories returned in FIND_FIRST. The downside ofcourse is increase in the QUERY_PATH_LOOKUPs. Sachin Prabhu On Tue, 2013-01-15 at 14:01 -0500, Jeff Layton wrote: > We've had a long-standing problem with DFS referral points. CIFS servers > generally try to make them look like directories in FIND_FIRST/NEXT > responses. When you go to try to do a FIND_FIRST on them though, the > server will then (correctly) return STATUS_PATH_NOT_COVERED. Mostly this > manifests as spurious EREMOTE errors back to userland. > > This patch attempts to fix this by marking directories that are > discovered via FIND_FIRST/NEXT for revaldiation. When the lookup code > runs across them again, we'll reissue a QPathInfo against them and that > will make it chase the referral properly. > > There is some performance penalty involved here and no I haven't > measured it -- it'll be highly dependent upon the workload and contents > of the mounted share. To try and mitigate that though, the code only > marks the inode for revalidation when it's possible to run across a DFS > referral. i.e.: when the kernel has DFS support built in and the share > is "in DFS". > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/readdir.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index cdd6ff4..5fc9b4a 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -128,6 +128,22 @@ out: > dput(dentry); > } > > +/* > + * Is it possible that this directory might turn out to be a DFS referral > + * once we go to try and use it? > + */ > +static bool > +cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) > +{ > +#ifdef CONFIG_CIFS_DFS_UPCALL > + struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > + > + if (tcon->Flags & SMB_SHARE_IS_IN_DFS) > + return true; > +#endif > + return false; > +} > + > static void > cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > { > @@ -137,6 +153,14 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > + /* > + * CIFS servers generally make DFS referrals look like > + * directories in FIND_* responses. Since there's no way to > + * tell the difference, we must revalidate directory inodes > + * before trying to use them. > + */ > + if (cifs_dfs_is_possible(cifs_sb)) > + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; -- 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
Hi Jeff, On 01/15/2013 08:01 PM, Jeff Layton wrote: > We've had a long-standing problem with DFS referral points. CIFS servers > generally try to make them look like directories in FIND_FIRST/NEXT > responses. When you go to try to do a FIND_FIRST on them though, the > server will then (correctly) return STATUS_PATH_NOT_COVERED. Mostly this > manifests as spurious EREMOTE errors back to userland. > > This patch attempts to fix this by marking directories that are > discovered via FIND_FIRST/NEXT for revaldiation. When the lookup code > runs across them again, we'll reissue a QPathInfo against them and that > will make it chase the referral properly. > > There is some performance penalty involved here and no I haven't > measured it -- it'll be highly dependent upon the workload and contents > of the mounted share. To try and mitigate that though, the code only > marks the inode for revalidation when it's possible to run across a DFS > referral. i.e.: when the kernel has DFS support built in and the share > is "in DFS". Are you planning to get this patch into mainline? I've applied this patch to 3.7.7, otherwise we where not able to mount dfs shares. It's running for over 2 months now on our servers and we have not seen any problems whatsoever. Regards, Martijn
On Tue, 23 Apr 2013 11:59:45 +0200 Martijn de Gouw <martijn.de.gouw@prodrive.nl> wrote: > Hi Jeff, > > On 01/15/2013 08:01 PM, Jeff Layton wrote: > > We've had a long-standing problem with DFS referral points. CIFS servers > > generally try to make them look like directories in FIND_FIRST/NEXT > > responses. When you go to try to do a FIND_FIRST on them though, the > > server will then (correctly) return STATUS_PATH_NOT_COVERED. Mostly this > > manifests as spurious EREMOTE errors back to userland. > > > > This patch attempts to fix this by marking directories that are > > discovered via FIND_FIRST/NEXT for revaldiation. When the lookup code > > runs across them again, we'll reissue a QPathInfo against them and that > > will make it chase the referral properly. > > > > There is some performance penalty involved here and no I haven't > > measured it -- it'll be highly dependent upon the workload and contents > > of the mounted share. To try and mitigate that though, the code only > > marks the inode for revalidation when it's possible to run across a DFS > > referral. i.e.: when the kernel has DFS support built in and the share > > is "in DFS". > > Are you planning to get this patch into mainline? > > I've applied this patch to 3.7.7, otherwise we where not able to mount > dfs shares. It's running for over 2 months now on our servers and we > have not seen any problems whatsoever. > > Regards, Martijn > I was hoping that it would go into the upcoming 3.10 merge window, but Steve won't take it as he's concerned about the performance impact. The current story is "maybe 3.11". One possibility to mitigate the performance impact might be to switch the default to "nodfs". IOW, make it so that you consciously have to mount with "-o dfs" in order to get it. That's a departure from how it works today though.
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index cdd6ff4..5fc9b4a 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -128,6 +128,22 @@ out: dput(dentry); } +/* + * Is it possible that this directory might turn out to be a DFS referral + * once we go to try and use it? + */ +static bool +cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) +{ +#ifdef CONFIG_CIFS_DFS_UPCALL + struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); + + if (tcon->Flags & SMB_SHARE_IS_IN_DFS) + return true; +#endif + return false; +} + static void cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) { @@ -137,6 +153,14 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; fattr->cf_dtype = DT_DIR; + /* + * CIFS servers generally make DFS referrals look like + * directories in FIND_* responses. Since there's no way to + * tell the difference, we must revalidate directory inodes + * before trying to use them. + */ + if (cifs_dfs_is_possible(cifs_sb)) + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; } else { fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; fattr->cf_dtype = DT_REG;
We've had a long-standing problem with DFS referral points. CIFS servers generally try to make them look like directories in FIND_FIRST/NEXT responses. When you go to try to do a FIND_FIRST on them though, the server will then (correctly) return STATUS_PATH_NOT_COVERED. Mostly this manifests as spurious EREMOTE errors back to userland. This patch attempts to fix this by marking directories that are discovered via FIND_FIRST/NEXT for revaldiation. When the lookup code runs across them again, we'll reissue a QPathInfo against them and that will make it chase the referral properly. There is some performance penalty involved here and no I haven't measured it -- it'll be highly dependent upon the workload and contents of the mounted share. To try and mitigate that though, the code only marks the inode for revalidation when it's possible to run across a DFS referral. i.e.: when the kernel has DFS support built in and the share is "in DFS". Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/readdir.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)