LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: NeilBrown <neilb@suse.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Christoph Hellwig <hch@lst.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Eric Biederman <ebiederm@xmission.com>
Subject: Re: dcache: remove trylock loops (was Re: [BUG] lock_parent() breakage when used from shrink_dentry_list())
Date: Tue, 13 Mar 2018 12:12:51 +1100	[thread overview]
Message-ID: <87o9jsdbho.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <20180312200540.GO30522@ZenIV.linux.org.uk>

[-- Attachment #1: Type: text/plain, Size: 4152 bytes --]

On Mon, Mar 12 2018, Al Viro wrote:

> On Mon, Mar 12, 2018 at 07:13:51PM +0000, Al Viro wrote:
>
>> There's an unpleasant area around the ->s_root vs. NFS.  There's
>> code that makes assumptions about ->s_root that are simply not true
>> for NFS.  Is path_connected() correct wrt NFS multiple imports from
>> the same server?  Ditto for mnt_already_visible() (that one might
>> be mitigated at the moment, but probably won't last).  Eric, am
>> I missing something subtle in there?
>
> BTW, another thing spotted while trying to document the stuff:
> Neil's "VFS: don't keep disconnected dentries on d_anon" has a subtle
> side effect I hadn't spotted when applying.  Namely, for
> DCACHE_DISCONNECTED non-directory IS_ROOT dentries we now have
> d_unhashed() true.  That makes d_find_alias() logics around
> discon_alias dead code:
>                 if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
>                         if (IS_ROOT(alias) &&
>                             (alias->d_flags & DCACHE_DISCONNECTED)) {
>                                 discon_alias = alias;
>                         } else {
>                                 __dget_dlock(alias);
>                                 spin_unlock(&alias->d_lock);
>                                 return alias;
>                         }
>                 }

I agree that this code is now more complex than needed.

>
> Directory case is a red herring - we'll end up picking one and only
> alias, whether it's IS_ROOT/disconnected or not.  For non-directories,
> though, IS_ROOT and disconnected implies d_unhashed() now, so we might
> as well turn that into
> 	if directory
> 		return d_find_any_alias
> 	else
> 		return the first hashed alias

Yes, this looks like a good simplification.

>
> Does any of the d_find_alias() callers want those dentries?  That is,
> non-directories from d_obtain_alias() still not attached to a parent;
> note that exportfs_decode_fh() is *not* one of such places - we don't
> use d_find_alias() there at all.  If there's no such caller, we can
> bloody well just drop the discon_alias logics and be done with that;
> if there is, that commit has introduced a bug.  I might have missed
> a part of threads related to that patch, so my apologies if it had
> been discussed.
>
> Neil, what's the situation there?  A lot of those callers clearly treat
> the "only disconnected IS_ROOT alias exist" same as "no aliases at all";
> it looks like the change might have been the right thing, but it sure
> as hell shouldn't be an undocumented side effect...

Many of the callers really want a name, which these disconnected
dentries didn't have, so the new code is more correct in those cases.

d_find_alias() is documented as returning a hashed dentry (for
non-directories) and I suspect most people would be surprised to find
that a disconnected dentry with no name was considered to be "hashed",
so I think the patch could be seen as fixing the documentation.

Of the three users that you didn't classify as "clearly" not wanting the
disconnected dentries:

>	     * ceph invalidate_aliases()

This wants to ensure the dentries are discarded on final dput().  This
is already the case for disconnected dentries, so it doesn't need to be
told about them.  Code is correct.


>	     * cap_inode_getsecurity()

If this gets invoked when the nfsd server calls vfs_getxattr() on a
disconnected dentry, it will return -EINVAL.  This looks like a
potential bug.
nfsd only calls vfs_getxattr() in nfsd4_is_junction() so this bug would
not affect many use cases.
I think cap_inode_getsecurity() should really be calling
d_find_any_alias().

>	     * selinux inode_doinit_with_dentry() (two call sites, very alike)

I'm less sure about this one, but I think it probably wants
d_find_any_alias() as well.  Like cap_inode_getsecurity() it only wants
a dentry so that it can pass something to __vfs_getxattr(),
and that only wants a dentry so it can pass something to ->get.

Possibly we should rename d_find_alias() to d_find_hashed_alias() so that
people need to make a conscious choice between d_find_hashed_alias() and
d_find_any_alias() ??

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

  parent reply	other threads:[~2018-03-13  1:13 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-22 23:50 [PATCH v2 0/6] fs/dcache: avoid trylock loops John Ogness
2018-02-22 23:50 ` [PATCH v2 1/6] fs/dcache: Remove stale comment from dentry_kill() John Ogness
2018-02-22 23:50 ` [PATCH v2 2/6] fs/dcache: Move dentry_kill() below lock_parent() John Ogness
2018-02-22 23:50 ` [PATCH v2 3/6] fs/dcache: Avoid the try_lock loop in d_delete() John Ogness
2018-02-23  2:08   ` Al Viro
2018-02-22 23:50 ` [PATCH v2 4/6] fs/dcache: Avoid the try_lock loops in dentry_kill() John Ogness
2018-02-23  2:22   ` Al Viro
2018-02-23  3:12     ` Al Viro
2018-02-23  3:16       ` Al Viro
2018-02-23  5:46       ` Al Viro
2018-02-22 23:50 ` [PATCH v2 5/6] fs/dcache: Avoid a try_lock loop in shrink_dentry_list() John Ogness
2018-02-23  3:48   ` Al Viro
2018-02-22 23:50 ` [PATCH v2 6/6] fs/dcache: Avoid remaining " John Ogness
2018-02-23  3:58   ` Al Viro
2018-02-23  4:08     ` Al Viro
2018-02-23 13:57       ` John Ogness
2018-02-23 15:09         ` Al Viro
2018-02-23 17:42           ` Al Viro
2018-02-23 20:13             ` [BUG] lock_parent() breakage when used from shrink_dentry_list() (was Re: [PATCH v2 6/6] fs/dcache: Avoid remaining try_lock loop in shrink_dentry_list()) Al Viro
2018-02-23 21:35               ` Linus Torvalds
2018-02-24  0:22                 ` Al Viro
2018-02-25  7:40                   ` Al Viro
2018-02-27  5:16                     ` dcache: remove trylock loops (was Re: [BUG] lock_parent() breakage when used from shrink_dentry_list()) John Ogness
2018-03-12 19:13                       ` Al Viro
2018-03-12 20:05                         ` Al Viro
2018-03-12 20:33                           ` Al Viro
2018-03-13  1:12                           ` NeilBrown [this message]
2018-04-28  0:10                             ` Al Viro
2018-03-12 20:23                         ` Eric W. Biederman
2018-03-12 20:39                           ` Al Viro
2018-03-12 23:28                             ` Eric W. Biederman
2018-03-12 23:52                               ` Eric W. Biederman
2018-03-13  0:37                                 ` Al Viro
2018-03-13  0:50                                   ` Al Viro
2018-03-13  4:02                                     ` Eric W. Biederman
2018-03-14 23:20                                     ` [PATCH] fs: Teach path_connected to handle nfs filesystems with multiple roots Eric W. Biederman
2018-03-15 22:34                                       ` Al Viro
2018-03-13  0:36                               ` dcache: remove trylock loops (was Re: [BUG] lock_parent() breakage when used from shrink_dentry_list()) Al Viro
2018-03-12 22:14                         ` Thomas Gleixner
2018-03-13 20:46                         ` John Ogness
2018-03-13 21:05                           ` John Ogness
2018-03-13 23:59                             ` Al Viro
2018-03-14  2:58                               ` Matthew Wilcox
2018-03-14  8:18                               ` John Ogness
2018-03-02  9:04                     ` [BUG] lock_parent() breakage when used from shrink_dentry_list() (was Re: [PATCH v2 6/6] fs/dcache: Avoid remaining try_lock loop in shrink_dentry_list()) Sebastian Andrzej Siewior
2018-02-23  0:59 ` [PATCH v2 0/6] fs/dcache: avoid trylock loops Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87o9jsdbho.fsf@notabene.neil.brown.name \
    --to=neilb@suse.com \
    --cc=bigeasy@linutronix.de \
    --cc=ebiederm@xmission.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ZenIV.linux.org.uk \
    --subject='Re: dcache: remove trylock loops (was Re: [BUG] lock_parent() breakage when used from shrink_dentry_list())' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).