From: Jeff Layton <jeff.layton@primarydata.com>
To: "J. Bruce Fields" <bfields@fieldses.org>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH] nfsd: bump dp->dl_time when unhashing delegation
Date: Tue, 22 Jul 2014 13:52:06 -0400 [thread overview]
Message-ID: <20140722135206.7dbfbac5@tlielax.poochiereds.net> (raw)
In-Reply-To: <20140722174552.GA27277@fieldses.org>
On Tue, 22 Jul 2014 13:45:52 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Tue, Jul 22, 2014 at 12:41:31PM -0400, Jeff Layton wrote:
> > There's a potential race between a lease break and DELEGRETURN call.
> >
> > Suppose a lease break comes in and queues the workqueue job for a
> > delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in
> > finds the delegation and calls destroy_delegation on it to unhash it and
> > put its primary reference.
> >
> > Next, the workqueue job runs and queues the delegation back onto the
> > del_recall_lru list, issues the CB_RECALL and puts the final reference.
> > With that, the final reference to the delegation is put, but it's still
> > on the LRU list.
> >
> > When we go to unhash a delegation, it's because we intend to get rid of
> > it soon afterward, so we don't want lease breaks to mess with it once
> > that occurs. Fix this by bumping the dl_time whenever we unhash a
> > delegation, to ensure that lease breaks don't monkey with it.
>
> Makes sense, thanks. Repeating from IRC: this fixes a regression from
> 02e1215f9f7 "nfsd: Avoid taking state_lock while holding inode lock in
> nfsd_break_one_deleg". (In my tree only.)
>
> --b.
>
> >
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> > fs/nfsd/nfs4state.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 72da0d44e66b..a3a828d17563 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)
> >
> > spin_lock(&state_lock);
> > dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> > + /* Ensure that deleg break won't try to requeue it */
> > + ++dp->dl_time;
> > spin_lock(&fp->fi_lock);
> > list_del_init(&dp->dl_perclnt);
> > list_del_init(&dp->dl_recall_lru);
> > --
> > 1.9.3
> >
Sorry, I think I sent you a version with an earlier description. Here's
the one I meant to send:
--------------------[snip]---------------------
[PATCH] nfsd: bump dl_time when unhashing delegation
There's a potential race between a lease break and DELEGRETURN call.
Suppose a lease break comes in and queues the workqueue job for a
delegation, but it doesn't run just yet. Then, a DELEGRETURN comes in
finds the delegation and calls destroy_delegation on it to unhash it and
put its primary reference.
Next, the workqueue job runs and queues the delegation back onto the
del_recall_lru list, issues the CB_RECALL and puts the final reference.
With that, the final reference to the delegation is put, but it's still
on the LRU list.
When we go to unhash a delegation, it's because we intend to get rid of
it soon afterward, so we don't want lease breaks to mess with it once
that occurs. Fix this by bumping the dl_time whenever we unhash a
delegation, to ensure that lease breaks don't monkey with it.
I believe this is a regression due to commit 02e1215f9f7 (nfsd: Avoid
taking state_lock while holding inode lock in nfsd_break_one_deleg).
Prior to that, the state_lock was held in the lm_break callback itself,
and that would have prevented this race.
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
fs/nfsd/nfs4state.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 72da0d44e66b..a3a828d17563 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -660,6 +660,8 @@ unhash_delegation(struct nfs4_delegation *dp)
spin_lock(&state_lock);
dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
+ /* Ensure that deleg break won't try to requeue it */
+ ++dp->dl_time;
spin_lock(&fp->fi_lock);
list_del_init(&dp->dl_perclnt);
list_del_init(&dp->dl_recall_lru);
--
1.9.3
next prev parent reply other threads:[~2014-07-22 17:52 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-22 16:41 [PATCH] nfsd: bump dp->dl_time when unhashing delegation Jeff Layton
2014-07-22 17:45 ` J. Bruce Fields
2014-07-22 17:52 ` Jeff Layton [this message]
2014-07-22 19:36 ` J. Bruce Fields
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=20140722135206.7dbfbac5@tlielax.poochiereds.net \
--to=jeff.layton@primarydata.com \
--cc=bfields@fieldses.org \
--cc=linux-nfs@vger.kernel.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.