cifs: revalidate directories instiantiated via FIND_* in order to handle DFS referrals

Commit Message

Steve French June 25, 2013, 5:30 p.m. UTC

On Tue, Jun 25, 2013 at 10:00 AM, Jeff Layton <[e-mail]> wrote:
> On Tue, 25 Jun 2013 01:42:44 -0500
> Steve French <[e-mail]> wrote:
>
>> Looking at this issue at the plugfest this week - the recommendation
>> is to add a check for ATTR_REPARSE
>> since this is usually set (for the Windows case) for DFS links (DFS
>> links are one case of reparse points - but the performance hit for
>> revalidating all reparse points is not bad).   Tomorrow we can check
>> for the Samba case in more detail to see how the scenario looks and
>> whether we can workaround it.
>>
>> We did find another workaround which could be checked, but decided
>> against using that one in preference to simply checking for the
>> reparse attribute.   In the Windows case since you don't create
>> subdirectories under a DFS link on the server side - the creation time
>> and last write time of the directory will match for the case of a DFS
>> link returned by QueryDir or FindFirst (so we also could have refused
>> to cache directories whose creation time and last write time match -
>> which would also mean that we don't cache empty subdirectories which
>> is also a trivial perf hit to not cache those - it would be easy
>> enough to refuse to cache those directories too if you want).
>>
>
> Sounds reasonable -- you should probably post the patch you've already
> merged into your for-next branch so others can comment and review
> though...
>
> --
> Jeff Layton <[e-mail]>


commit 732e0143e8811afcb79d4e1b308536e4a7a1d791
Author: Jeff Layton <[e-mail]>
Date:   Tue Jun 25 01:32:17 2013 -0500

    [CIFS] revalidate directories instiantiated via FIND_* in order to
handle DFS referrals

    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"

    [At the Microsoft plugfest we noted that usually the DFS links had
    the REPARSE attribute tag enabled - DFS junctions are reparse points
    after all - so I just added a check for that flag too so the
    performance impact should be smaller - Steve]

    Signed-off-by: Jeff Layton <[e-mail]>
    Reviewed-by: Sachin Prabhu <[e-mail]>
    Signed-off-by: Steve French <[e-mail]>

 		fattr->cf_dtype = DT_REG;

Comments

Jeff Layton June 25, 2013, 5:38 p.m. UTC | #1

On Tue, 25 Jun 2013 12:30:11 -0500
Steve French <[e-mail]> wrote:

> On Tue, Jun 25, 2013 at 10:00 AM, Jeff Layton <[e-mail]> wrote:
> > On Tue, 25 Jun 2013 01:42:44 -0500
> > Steve French <[e-mail]> wrote:
> >
> >> Looking at this issue at the plugfest this week - the recommendation
> >> is to add a check for ATTR_REPARSE
> >> since this is usually set (for the Windows case) for DFS links (DFS
> >> links are one case of reparse points - but the performance hit for
> >> revalidating all reparse points is not bad).   Tomorrow we can check
> >> for the Samba case in more detail to see how the scenario looks and
> >> whether we can workaround it.
> >>
> >> We did find another workaround which could be checked, but decided
> >> against using that one in preference to simply checking for the
> >> reparse attribute.   In the Windows case since you don't create
> >> subdirectories under a DFS link on the server side - the creation time
> >> and last write time of the directory will match for the case of a DFS
> >> link returned by QueryDir or FindFirst (so we also could have refused
> >> to cache directories whose creation time and last write time match -
> >> which would also mean that we don't cache empty subdirectories which
> >> is also a trivial perf hit to not cache those - it would be easy
> >> enough to refuse to cache those directories too if you want).
> >>
> >
> > Sounds reasonable -- you should probably post the patch you've already
> > merged into your for-next branch so others can comment and review
> > though...
> >
> > --
> > Jeff Layton <[e-mail]>
> 
> 
> commit 732e0143e8811afcb79d4e1b308536e4a7a1d791
> Author: Jeff Layton <[e-mail]>
> Date:   Tue Jun 25 01:32:17 2013 -0500
> 
>     [CIFS] revalidate directories instiantiated via FIND_* in order to
> handle DFS referrals
> 
>     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"
> 
>     [At the Microsoft plugfest we noted that usually the DFS links had
>     the REPARSE attribute tag enabled - DFS junctions are reparse points
>     after all - so I just added a check for that flag too so the
>     performance impact should be smaller - Steve]
> 
>     Signed-off-by: Jeff Layton <[e-mail]>
>     Reviewed-by: Sachin Prabhu <[e-mail]>
>     Signed-off-by: Steve French <[e-mail]>
> 
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 770d5a9..94d6201 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -126,6 +126,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)
>  {
> @@ -135,6 +151,19 @@ 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;
> +		/*
> +		 * Windows CIFS servers generally make DFS referrals look
> +		 * like directories in FIND_* responses with the reparse
> +		 * attribute flag also set (since DFS junctions are
> +		 * reparse points). We must revalidate at least these
> +		 * directory inodes before trying to use them (if
> +		 * they are DFS we will get PATH_NOT_COVERED back
> +		 * when queried directly and can then try to connect
> +		 * to the DFS target)
> +		 */
> +		if (cifs_dfs_is_possible(cifs_sb) &&
> +		    (fattr->cf_cifsattrs & ATTR_REPARSE))
> +			fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
>  	} else {
>  		fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>  		fattr->cf_dtype = DT_REG;
> 
> 
> 

Looks good to me -- ACK on the changes...

Patch

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 770d5a9..94d6201 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -126,6 +126,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)
 {
@@ -135,6 +151,19 @@  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;
+		/*
+		 * Windows CIFS servers generally make DFS referrals look
+		 * like directories in FIND_* responses with the reparse
+		 * attribute flag also set (since DFS junctions are
+		 * reparse points). We must revalidate at least these
+		 * directory inodes before trying to use them (if
+		 * they are DFS we will get PATH_NOT_COVERED back
+		 * when queried directly and can then try to connect
+		 * to the DFS target)
+		 */
+		if (cifs_dfs_is_possible(cifs_sb) &&
+		    (fattr->cf_cifsattrs & ATTR_REPARSE))
+			fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
 	} else {
 		fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;