From patchwork Tue Jun 25 17:30:11 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steve French X-Patchwork-Id: 2778211 Return-Path: X-Original-To: patchwork-cifs-client@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 71BBA9F3A0 for ; Tue, 25 Jun 2013 17:30:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2700C202BC for ; Tue, 25 Jun 2013 17:30:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C7C20202CE for ; Tue, 25 Jun 2013 17:30:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751358Ab3FYRaO (ORCPT ); Tue, 25 Jun 2013 13:30:14 -0400 Received: from mail-pb0-f47.google.com ([209.85.160.47]:64717 "EHLO mail-pb0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751825Ab3FYRaL (ORCPT ); Tue, 25 Jun 2013 13:30:11 -0400 Received: by mail-pb0-f47.google.com with SMTP id rr13so12859419pbb.6 for ; Tue, 25 Jun 2013 10:30:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=hSkqXgfbpD4B3WBahx23A1EqDyvMMquJq+6pqAczkL4=; b=q8hhnNyI3gVzDoKVvfNwyolqrmTxxRlrWSaRkDCAbixqI/O6BG/zgRaehKBKyxFvh0 X+pZvgH8MJgEArJhbk6Zd5jkE8gtB8T5tDmEtntitB6VRIuwfghSO54OUvxaxz0tlawd zpRZqBJaVEcxcfsfJn8HwLdUKNCd5nLq659oGMb8wLpOzquvqmIGbEQ8ZIssULeK+EHq DfRpkvCE/8M0clu7WnLklfj/oTzZ/fAh2H08mXvu9dOjn0Fn5yLQsrOMpKfsUzuAl4LQ wKGl/zZ21nZwT/n1pp88E1bUg6TPxilzff2gYoYXhvB0ixIABfrLAphGh6gQ1y7hn807 oTuA== MIME-Version: 1.0 X-Received: by 10.68.252.194 with SMTP id zu2mr124053pbc.58.1372181411279; Tue, 25 Jun 2013 10:30:11 -0700 (PDT) Received: by 10.68.128.9 with HTTP; Tue, 25 Jun 2013 10:30:11 -0700 (PDT) In-Reply-To: <20130625110018.0f5fe0eb@tlielax.poochiereds.net> References: <20130625110018.0f5fe0eb@tlielax.poochiereds.net> Date: Tue, 25 Jun 2013 12:30:11 -0500 Message-ID: Subject: Re: cifs: revalidate directories instiantiated via FIND_* in order to handle DFS referrals From: Steve French To: Jeff Layton Cc: linux-cifs@vger.kernel.org Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jun 25, 2013 at 10:00 AM, Jeff Layton wrote: > On Tue, 25 Jun 2013 01:42:44 -0500 > Steve French 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 commit 732e0143e8811afcb79d4e1b308536e4a7a1d791 Author: Jeff Layton 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 Reviewed-by: Sachin Prabhu Signed-off-by: Steve French fattr->cf_dtype = DT_REG; 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;