From: "J. Bruce Fields" <bfields@fieldses.org>
To: Nick Piggin <npiggin@gmail.com>
Cc: linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: lifetime of DCACHE_DISCONECTED dentries
Date: Mon, 29 Nov 2010 14:32:48 -0500 [thread overview]
Message-ID: <20101129193248.GA9897@fieldses.org> (raw)
In-Reply-To: <AANLkTinMq1W1vx-jZnf7cb1cAnh9OyoVRYtVS7zpujfs@mail.gmail.com>
On Mon, Nov 29, 2010 at 02:56:22PM +1100, Nick Piggin wrote:
> On Tue, Nov 16, 2010 at 5:45 PM, Nick Piggin <npiggin@gmail.com> wrote:
> > On Tue, Nov 16, 2010 at 4:48 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> On Sat, Nov 13, 2010 at 10:53:12PM +1100, Nick Piggin wrote:
> >>> On Sat, Nov 13, 2010 at 5:43 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> >>> > - putfh: look up the filehandle. The only alias found for the
> >>> > inode will be DCACHE_UNHASHED alias referenced by the filp
> >>> > associated with the nfsd open. d_obtain_alias() doesn't like
> >>> > this, so it creates a new DCACHE_DISCONECTED dentry and
> >>> > returns that instead.
> >>>
> >>> This seems to be where the thing goes wrong. It isn't a hashed dentry at
> >>> this point here, so d_obtain_alias should not be making one.
> >>
> >> Sounds sensible. (But can you think of any actual bugs that will result
> >> from trying to add a new hashed dentry in this case?)
> >
> > Well, this one? :)
> >
> >
> >>> I think the inode i_nlink games are much more appropriate on this side of
> >>> the equation, rather than the dput side (after all, d_obtain_alias is setting
> >>> up an alias for the inode).
> >>>
> >>> Can you even put the link check into __d_find_alias?
> >>>
> >>> - if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
> >>> + if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||
> >>> !d_unhashed(alias)) {
> >>>
> >>> Something like that?
> >>
> >> The immediate result of that would be for the close rpc (or any rpc's
> >> sent after the file was unlinked) to fail with ESTALE.
> >
> > Why is that? Seems like it would be a bug, because a hashed dentry may
> > be unhashed at any time concurrently to nfsd operation, so it should be
> > able to tolerate that so long as it has a ref on the inode?
>
> Ping? Did you work out why nfs fails with ESTALE in that case? It seems
> to work in my testing (and do the right thing with freeing the inode).
Bah, sorry, I read too quickly, got the sense of the test backwards, and
thought you were suggesting __d_find_alias() shouldn't return an alias
in the i_nlink == 0 case!
Yes, agreed, that should solve my problem.
But what's the reason for the d_unhashed() check now? Could we get rid
of it entirely?
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.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: Nick Piggin <npiggin@gmail.com>
Cc: linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: lifetime of DCACHE_DISCONECTED dentries
Date: Mon, 29 Nov 2010 14:32:48 -0500 [thread overview]
Message-ID: <20101129193248.GA9897@fieldses.org> (raw)
In-Reply-To: <AANLkTinMq1W1vx-jZnf7cb1cAnh9OyoVRYtVS7zpujfs@mail.gmail.com>
On Mon, Nov 29, 2010 at 02:56:22PM +1100, Nick Piggin wrote:
> On Tue, Nov 16, 2010 at 5:45 PM, Nick Piggin <npiggin@gmail.com> wrote:
> > On Tue, Nov 16, 2010 at 4:48 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> On Sat, Nov 13, 2010 at 10:53:12PM +1100, Nick Piggin wrote:
> >>> On Sat, Nov 13, 2010 at 5:43 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> >>> > - putfh: look up the filehandle. The only alias found for the
> >>> > inode will be DCACHE_UNHASHED alias referenced by the filp
> >>> > associated with the nfsd open. d_obtain_alias() doesn't like
> >>> > this, so it creates a new DCACHE_DISCONECTED dentry and
> >>> > returns that instead.
> >>>
> >>> This seems to be where the thing goes wrong. It isn't a hashed dentry at
> >>> this point here, so d_obtain_alias should not be making one.
> >>
> >> Sounds sensible. (But can you think of any actual bugs that will result
> >> from trying to add a new hashed dentry in this case?)
> >
> > Well, this one? :)
> >
> >
> >>> I think the inode i_nlink games are much more appropriate on this side of
> >>> the equation, rather than the dput side (after all, d_obtain_alias is setting
> >>> up an alias for the inode).
> >>>
> >>> Can you even put the link check into __d_find_alias?
> >>>
> >>> - if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
> >>> + if (S_ISDIR(inode->i_mode) || !inode->i_nlink ||
> >>> !d_unhashed(alias)) {
> >>>
> >>> Something like that?
> >>
> >> The immediate result of that would be for the close rpc (or any rpc's
> >> sent after the file was unlinked) to fail with ESTALE.
> >
> > Why is that? Seems like it would be a bug, because a hashed dentry may
> > be unhashed at any time concurrently to nfsd operation, so it should be
> > able to tolerate that so long as it has a ref on the inode?
>
> Ping? Did you work out why nfs fails with ESTALE in that case? It seems
> to work in my testing (and do the right thing with freeing the inode).
Bah, sorry, I read too quickly, got the sense of the test backwards, and
thought you were suggesting __d_find_alias() shouldn't return an alias
in the i_nlink == 0 case!
Yes, agreed, that should solve my problem.
But what's the reason for the d_unhashed() check now? Could we get rid
of it entirely?
--b.
next prev parent reply other threads:[~2010-11-29 19:32 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 [this message]
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
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=20101129193248.GA9897@fieldses.org \
--to=bfields@fieldses.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=npiggin@gmail.com \
/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.