All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Nick Piggin <npiggin@gmail.com>, Nick Piggin <npiggin@kernel.dk>,
	linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Neil Brown <neilb@suse.de>
Subject: Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries
Date: Thu, 10 Mar 2011 10:58:21 +0000	[thread overview]
Message-ID: <20110310105821.GE22723@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20110308181320.GA15566@fieldses.org>

On Tue, Mar 08, 2011 at 01:13:20PM -0500, J. Bruce Fields wrote:

> Al, do you have this in your queue to look at?  Need me to resend?  Or
> should it take some other route?

It's in queue, but I'd be a lot happier if I understood what's going on
with __d_find_alias() elsewhere.  Namely, in d_splice_alias().  The thing
is, unless I'm missing something we ought to use __d_find_any_alias()
there as well.  Directories really, _really_ should not have more than
one alias.  And what we get is really weird:
	* find (the only) alias
	* if it doesn't exist, create one (OK, no problem)
	* if it does exist and happens to be IS_ROOT and DCACHE_DISCONNECTED,
move it (also fine, modulo rather useless BUG_ON() in there)
	* if it does exist and either isn't IS_ROOT or not DCACHE_DISCONNECTED,
add a new alias and say nothing.

The last part looks very strange.  I'd been staring at the history of this
function and I _think_ it's a rudiment of period when we used that stuff for
non-directories as well.  It used to be directory-only; then Neil had
switched it to non-directories as well (in "Fix disconnected dentries on NFS
exports" back in 2004), adding want_discon argument to __d_find_alias() in
process and using it instead of open-coded equivalent of __d_find_any_alias().
Then, two years later, in "knfsd: close a race-opportunity in d_splice_alias"
he'd made that code directory-only again, at which point we arrived to the
current situation.

Neil, is there some reason I'm missing that makes __d_find_alias() right in
there?  And do we still need the second argument in __d_find_alias()?

Anyway, Bruce's patch goes in tonight's push, but I'd love to see that
mess cleaned up as well or at least explained.

WARNING: multiple messages have this Message-ID (diff)
From: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
To: "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
Cc: Nick Piggin <npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Nick Piggin <npiggin-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Neil Brown <neilb-l3A5Bk7waGM@public.gmane.org>
Subject: Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries
Date: Thu, 10 Mar 2011 10:58:21 +0000	[thread overview]
Message-ID: <20110310105821.GE22723@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20110308181320.GA15566-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>

On Tue, Mar 08, 2011 at 01:13:20PM -0500, J. Bruce Fields wrote:

> Al, do you have this in your queue to look at?  Need me to resend?  Or
> should it take some other route?

It's in queue, but I'd be a lot happier if I understood what's going on
with __d_find_alias() elsewhere.  Namely, in d_splice_alias().  The thing
is, unless I'm missing something we ought to use __d_find_any_alias()
there as well.  Directories really, _really_ should not have more than
one alias.  And what we get is really weird:
	* find (the only) alias
	* if it doesn't exist, create one (OK, no problem)
	* if it does exist and happens to be IS_ROOT and DCACHE_DISCONNECTED,
move it (also fine, modulo rather useless BUG_ON() in there)
	* if it does exist and either isn't IS_ROOT or not DCACHE_DISCONNECTED,
add a new alias and say nothing.

The last part looks very strange.  I'd been staring at the history of this
function and I _think_ it's a rudiment of period when we used that stuff for
non-directories as well.  It used to be directory-only; then Neil had
switched it to non-directories as well (in "Fix disconnected dentries on NFS
exports" back in 2004), adding want_discon argument to __d_find_alias() in
process and using it instead of open-coded equivalent of __d_find_any_alias().
Then, two years later, in "knfsd: close a race-opportunity in d_splice_alias"
he'd made that code directory-only again, at which point we arrived to the
current situation.

Neil, is there some reason I'm missing that makes __d_find_alias() right in
there?  And do we still need the second argument in __d_find_alias()?

Anyway, Bruce's patch goes in tonight's push, but I'd love to see that
mess cleaned up as well or at least explained.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2011-03-10 10:58 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-11-12 18:43 lifetime of DCACHE_DISCONECTED dentries J. Bruce Fields
2010-11-13 11:53 ` Nick Piggin
2010-11-13 11:53   ` Nick Piggin
2010-11-15 17:48   ` J. Bruce Fields
2010-11-15 17:48     ` J. Bruce Fields
2010-11-16  6:45     ` Nick Piggin
2010-11-16  6:45       ` Nick Piggin
2010-11-29  3:56       ` Nick Piggin
2010-11-29  3:56         ` Nick Piggin
2010-11-29 19:32         ` J. Bruce Fields
2010-11-29 19:32           ` J. Bruce Fields
2010-11-30  1:00           ` Nick Piggin
2010-11-30  1:00             ` Nick Piggin
2010-11-30 18:39             ` J. Bruce Fields
2010-11-30 18:39               ` J. Bruce Fields
2010-12-03 22:33             ` [PATCH] nfsd4: allow __d_obtain_alias() to return unhashed dentries J. Bruce Fields
2010-12-03 22:33               ` J. Bruce Fields
2010-12-13  5:19               ` Nick Piggin
2010-12-13  5:19                 ` Nick Piggin
2010-12-14 22:01                 ` J. Bruce Fields
2010-12-14 22:01                   ` J. Bruce Fields
2010-12-17 17:53                   ` [PATCH] fs/dcache: use standard list macro for d_find_alias J. Bruce Fields
2010-12-17 17:53                     ` J. Bruce Fields
2010-12-17 18:00                   ` [PATCH 2/2] fs/dcache: allow __d_obtain_alias() to return unhashed dentries J. Bruce Fields
2010-12-17 18:00                     ` J. Bruce Fields
2010-12-18  2:01                     ` Nick Piggin
2010-12-18  2:01                       ` Nick Piggin
2010-12-18 16:16                       ` J. Bruce Fields
2010-12-18 16:16                         ` J. Bruce Fields
2010-12-19 14:53                         ` Nick Piggin
2010-12-19 14:53                           ` Nick Piggin
2010-12-27 23:46                           ` [PATCH] " J. Bruce Fields
2010-12-27 23:46                             ` J. Bruce Fields
2011-01-18 20:45                             ` J. Bruce Fields
2011-01-18 20:45                               ` J. Bruce Fields
2011-01-18 22:02                               ` Nick Piggin
2011-01-18 22:02                                 ` Nick Piggin
2011-01-18 22:08                                 ` J. Bruce Fields
2011-01-18 22:08                                   ` J. Bruce Fields
2011-03-08 18:13                                   ` J. Bruce Fields
2011-03-08 18:13                                     ` J. Bruce Fields
2011-03-10 10:58                                     ` Al Viro [this message]
2011-03-10 10:58                                       ` Al Viro
2011-03-11  4:07                                       ` NeilBrown
2011-03-11  4:07                                         ` NeilBrown
2012-02-14 17:03                                         ` J. Bruce Fields
2012-02-14 17:03                                           ` J. Bruce Fields
2012-02-15 16:56                                           ` J. Bruce Fields
2012-02-15 16:56                                             ` J. Bruce Fields
2012-02-16  3:06                                             ` NeilBrown
2012-02-16  3:06                                               ` NeilBrown
2012-02-16 11:51                                               ` J. Bruce Fields
2012-02-16 11:51                                                 ` J. Bruce Fields
2012-02-16 16:08                                                 ` J. Bruce Fields
2012-02-16 16:08                                                   ` J. Bruce Fields
2012-02-16 22:30                                               ` J. Bruce Fields
2012-02-17 16:34                                                 ` Peng Tao
2012-02-17 16:34                                                   ` Peng Tao
2012-03-13 20:55                                                   ` J. Bruce Fields
2012-03-13 20:55                                                     ` J. Bruce Fields
2012-03-13 20:58                                                     ` [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases J. Bruce Fields
2012-03-13 20:58                                                     ` [PATCH 2/2] vfs: remove unused __d_splice_alias argument J. Bruce Fields
2012-02-20  2:55                                                 ` [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries NeilBrown
2012-02-20  2:55                                                   ` NeilBrown
2012-02-29 23:10                                                   ` J. Bruce Fields
2012-02-29 23:10                                                     ` J. Bruce Fields
2012-06-28 13:59                                         ` J. Bruce Fields
2012-06-28 13:59                                           ` J. Bruce Fields
2012-06-29 20:10                                           ` J. Bruce Fields
2012-06-29 20:10                                             ` J. Bruce Fields
2012-06-29 20:29                                             ` J. Bruce Fields
2012-06-29 20:29                                               ` J. Bruce Fields
2012-07-01 23:15                                               ` NeilBrown

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=20110310105821.GE22723@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=bfields@fieldses.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neilb@suse.de \
    --cc=npiggin@gmail.com \
    --cc=npiggin@kernel.dk \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.