From: "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
To: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
Cc: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>,
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
Subject: Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries
Date: Thu, 28 Jun 2012 09:59:27 -0400 [thread overview]
Message-ID: <20120628135927.GA6406@fieldses.org> (raw)
In-Reply-To: <20110311150749.2fa2be66-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
Coming back to this now, just trying to review the
filehandle-lookup/dcache interactions:
On Fri, Mar 11, 2011 at 03:07:49PM +1100, NeilBrown wrote:
> 1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence
> was only a hint, its absence was a strong statement.
> If the flag is set, the dentry might not be linked to the root.
> If it is clear, it definitely is link through to the root.
> However I think it was used with stronger intent than that.
>
> Now it seems to mean a little bit more: If it is set and the dentry
> is hashed, then it must be on the sb->s_anon list.
The code that makes that assumption is __d_shrink (which does the work
of d_drop)--it uses DCACHE_DISCONECTED to decide which hash chain to
lock.
I can't find any basis for that assumption. The only code that clears
DCACHE_DISCONNECTED is in expfs.c, and it isn't done at the same time as
hashing. Am I missing something?
> This is a significant
> which I never noticed (I haven't been watching). Originally a
> disconnected dentry would be attached (and hashed) to its parent. Then
> that parent would get its own parent and so on until it was attached all
> the way to the root. Only then would be start clearing
> DCACHE_DISCONNECTED. It seems we must clear it sooner now... I wonder if
> that is correct.
It looks wrong to me:
If we clear DCACHE_DISCONNECTED too early, then we risk a filehandle
lookup thinking the dentry is OK to use. That could mean for example
trying to rename across directories that don't have any ancestor
relationship to each other in the dcache yet.
So we need to wait to clear DCACHE_DISCONNECTED until we *know* the
dentry's parents go all the way back to the root. As you say, that's
what the current code does.
But that means DCACHE_DISCONNECTED dentries can be hashed to their
parents, and __d_shrink can be handed such dentries and then get the
locking wrong.
It looks like this bug might originate with Nick Piggin's ceb5bdc2d246
"fs: dcache per-bucket dcache hash locking"? There's no discussion in
the changelog, so probably it was just based on an unexamined assumption
about DCACHE_DISCONNECTED.
I wonder if an IS_ROOT() test could replace the DCACHE_DISCONNECTED test
in __d_shrink(), or if we need another flag, or ?
--b.
--
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
WARNING: multiple messages have this Message-ID (diff)
From: "J. Bruce Fields" <bfields@fieldses.org>
To: NeilBrown <neilb@suse.de>
Cc: Al Viro <viro@ZenIV.linux.org.uk>,
Nick Piggin <npiggin@gmail.com>, Nick Piggin <npiggin@kernel.dk>,
linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries
Date: Thu, 28 Jun 2012 09:59:27 -0400 [thread overview]
Message-ID: <20120628135927.GA6406@fieldses.org> (raw)
In-Reply-To: <20110311150749.2fa2be66@notabene.brown>
Coming back to this now, just trying to review the
filehandle-lookup/dcache interactions:
On Fri, Mar 11, 2011 at 03:07:49PM +1100, NeilBrown wrote:
> 1/ Originally DCACHE_DISCONNECTED didn't really mean much - it's presence
> was only a hint, its absence was a strong statement.
> If the flag is set, the dentry might not be linked to the root.
> If it is clear, it definitely is link through to the root.
> However I think it was used with stronger intent than that.
>
> Now it seems to mean a little bit more: If it is set and the dentry
> is hashed, then it must be on the sb->s_anon list.
The code that makes that assumption is __d_shrink (which does the work
of d_drop)--it uses DCACHE_DISCONECTED to decide which hash chain to
lock.
I can't find any basis for that assumption. The only code that clears
DCACHE_DISCONNECTED is in expfs.c, and it isn't done at the same time as
hashing. Am I missing something?
> This is a significant
> which I never noticed (I haven't been watching). Originally a
> disconnected dentry would be attached (and hashed) to its parent. Then
> that parent would get its own parent and so on until it was attached all
> the way to the root. Only then would be start clearing
> DCACHE_DISCONNECTED. It seems we must clear it sooner now... I wonder if
> that is correct.
It looks wrong to me:
If we clear DCACHE_DISCONNECTED too early, then we risk a filehandle
lookup thinking the dentry is OK to use. That could mean for example
trying to rename across directories that don't have any ancestor
relationship to each other in the dcache yet.
So we need to wait to clear DCACHE_DISCONNECTED until we *know* the
dentry's parents go all the way back to the root. As you say, that's
what the current code does.
But that means DCACHE_DISCONNECTED dentries can be hashed to their
parents, and __d_shrink can be handed such dentries and then get the
locking wrong.
It looks like this bug might originate with Nick Piggin's ceb5bdc2d246
"fs: dcache per-bucket dcache hash locking"? There's no discussion in
the changelog, so probably it was just based on an unexamined assumption
about DCACHE_DISCONNECTED.
I wonder if an IS_ROOT() test could replace the DCACHE_DISCONNECTED test
in __d_shrink(), or if we need another flag, or ?
--b.
next prev parent reply other threads:[~2012-06-28 13:59 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
[not found] ` <20101115174837.GB10044-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
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
[not found] ` <20101129193248.GA9897-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2010-11-30 1:00 ` Nick Piggin
2010-11-30 1:00 ` Nick Piggin
[not found] ` <AANLkTikwzDJ_q65==uxDsAhp3h8bU7Rkt7U9gVgRAK0D-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
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
[not found] ` <20101214220102.GM24828-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
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
[not found] ` <20101218161609.GA22150-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2010-12-19 14:53 ` Nick Piggin
2010-12-19 14:53 ` Nick Piggin
[not found] ` <AANLkTingRv_gtRSctGzMfYrKg02M_sKj97HSQPRm_mA_-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-12-27 23:46 ` [PATCH] " J. Bruce Fields
2010-12-27 23:46 ` J. Bruce Fields
[not found] ` <20101227234641.GA22248-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2011-01-18 20:45 ` J. Bruce Fields
2011-01-18 20:45 ` J. Bruce Fields
[not found] ` <20110118204509.GA10903-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2011-01-18 22:02 ` Nick Piggin
2011-01-18 22:02 ` Nick Piggin
[not found] ` <AANLkTikL2CDSWQJ1QH_Y4G-j70Vd=VesNMMnYTmMGHC9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-01-18 22:08 ` J. Bruce Fields
2011-01-18 22:08 ` J. Bruce Fields
[not found] ` <20110118220817.GF10903-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2011-03-08 18:13 ` J. Bruce Fields
2011-03-08 18:13 ` J. Bruce Fields
[not found] ` <20110308181320.GA15566-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2011-03-10 10:58 ` Al Viro
2011-03-10 10:58 ` Al Viro
[not found] ` <20110310105821.GE22723-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
2011-03-11 4:07 ` NeilBrown
2011-03-11 4:07 ` NeilBrown
[not found] ` <20110311150749.2fa2be66-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2012-02-14 17:03 ` J. Bruce Fields
2012-02-14 17:03 ` J. Bruce Fields
[not found] ` <20120214170300.GA4309-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2012-02-15 16:56 ` J. Bruce Fields
2012-02-15 16:56 ` J. Bruce Fields
[not found] ` <20120215165633.GE12490-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2012-02-16 3:06 ` NeilBrown
2012-02-16 3:06 ` NeilBrown
[not found] ` <20120216140603.08cb4900-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2012-02-16 11:51 ` J. Bruce Fields
2012-02-16 11:51 ` J. Bruce Fields
[not found] ` <20120216115133.GA20279-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2012-02-16 16:08 ` J. Bruce Fields
2012-02-16 16:08 ` J. Bruce Fields
2012-02-16 22:30 ` J. Bruce Fields
[not found] ` <20120216223011.GA23997-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
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
[not found] ` <20120220135537.3078e20b-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2012-02-29 23:10 ` J. Bruce Fields
2012-02-29 23:10 ` J. Bruce Fields
2012-06-28 13:59 ` J. Bruce Fields [this message]
2012-06-28 13:59 ` J. Bruce Fields
[not found] ` <20120628135927.GA6406-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2012-06-29 20:10 ` J. Bruce Fields
2012-06-29 20:10 ` J. Bruce Fields
[not found] ` <20120629201034.GA17103-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
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=20120628135927.GA6406@fieldses.org \
--to=bfields-uc3wqj2krung9huczpvpmw@public.gmane.org \
--cc=linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=neilb-l3A5Bk7waGM@public.gmane.org \
--cc=npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=npiggin-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org \
--cc=viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org \
/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.