From: Alexander Aring <aahringo@redhat.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: linux-nfs@vger.kernel.org, cluster-devel@redhat.com,
ocfs2-devel@lists.linux.dev, chuck.lever@oracle.com,
anna@kernel.org, linux-fsdevel@vger.kernel.org,
trond.myklebust@hammerspace.com
Subject: Re: [Cluster-devel] [RFCv2 1/7] lockd: fix race in async lock request handling
Date: Thu, 17 Aug 2023 14:39:00 -0400 [thread overview]
Message-ID: <CAK-6q+iB9AWqbe-cJfHi=nRAsdxXQ_KjpZLd4Hsj1VEtj8CNHQ@mail.gmail.com> (raw)
In-Reply-To: <350395c8906f46ec4634392adb8a9e3927763ef1.camel@kernel.org>
Hi,
On Tue, Aug 15, 2023 at 2:21 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2023-08-15 at 13:49 -0400, Jeff Layton wrote:
> > On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote:
> > > This patch fixes a race in async lock request handling between adding
> > > the relevant struct nlm_block to nlm_blocked list after the request was
> > > sent by vfs_lock_file() and nlmsvc_grant_deferred() does a lookup of the
> > > nlm_block in the nlm_blocked list. It could be that the async request is
> > > completed before the nlm_block was added to the list. This would end
> > > in a -ENOENT and a kernel log message of "lockd: grant for unknown
> > > block".
> > >
> > > To solve this issue we add the nlm_block before the vfs_lock_file() call
> > > to be sure it has been added when a possible nlmsvc_grant_deferred() is
> > > called. If the vfs_lock_file() results in an case when it wouldn't be
> > > added to nlm_blocked list, the nlm_block struct will be removed from
> > > this list again.
> > >
> > > The introducing of the new B_PENDING_CALLBACK nlm_block flag will handle
> > > async lock requests on a pending lock requests as a retry on the caller
> > > level to hit the -EAGAIN case.
> > >
> > > Signed-off-by: Alexander Aring <aahringo@redhat.com>
> > > ---
> > > fs/lockd/svclock.c | 100 ++++++++++++++++++++++++++----------
> > > include/linux/lockd/lockd.h | 2 +
> > > 2 files changed, 74 insertions(+), 28 deletions(-)
> > >
> > >
>
> [...]
>
> > > diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> > > index f42594a9efe0..91f55458f5fc 100644
> > > --- a/include/linux/lockd/lockd.h
> > > +++ b/include/linux/lockd/lockd.h
> > > @@ -185,10 +185,12 @@ struct nlm_block {
> > > struct nlm_file * b_file; /* file in question */
> > > struct cache_req * b_cache_req; /* deferred request handling */
> > > struct cache_deferred_req * b_deferred_req
> > > + struct mutex b_cb_mutex; /* callback mutex */
> >
> > There is no mention at all of this new mutex in the changelog or
> > comments. It's not at all clear to me what this is intended to protect.
> > In general, with lockd being a single-threaded service, we want to avoid
> > sleeping locks. This will need some clear justification.
> >
> > At a glance, it looks like you're trying to use this to hold
> > B_PENDING_CALLBACK steady while a lock request is being handled. That
> > suggests that you're using this mutex to serialize access to a section
> > of code and not one or more specific data structures. We usually like to
> > avoid that sort of thing, since locks that protect arbitrary sections of
> > code become difficult to work with over time.
> >
> > I'm going to go out on a limb here though and suggest that there is
> > probably a way to solve this problem that doesn't involve adding new
> > locks.
> >
> > > unsigned int b_flags; /* block flags */
> > > #define B_QUEUED 1 /* lock queued */
> > > #define B_GOT_CALLBACK 2 /* got lock or conflicting lock */
> > > #define B_TIMED_OUT 4 /* filesystem too slow to respond */
> > > +#define B_PENDING_CALLBACK 8 /* pending callback for lock request */
> > > };
> > >
> > > /*
> >
> > Do we need this new flag at all? It seems redundant. If we have a block
> > on the list, then it is sort of by definition "pending callback". If
> > it's not on the list anymore, then it's not. No?
> >
>
> Do we need anything more than a patch along these lines? Note that this
> is untested, so RFC:
>
> ---------------------8<-----------------------
>
> [RFC PATCH] lockd: alternate fix for race between deferred lock and grant
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/lockd/svclock.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index c43ccdf28ed9..e9a84363c26e 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -446,6 +446,8 @@ nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block)
>
> block->b_flags |= B_QUEUED;
>
> + /* FIXME: remove and reinsert w/o dropping spinlock */
> + nlmsvc_remove_block(block);
> nlmsvc_insert_block(block, NLM_TIMEOUT);
>
a insert should just be okay, because there is an atomic switch if
it's already part of nlm_blocked and it will just update the timeout
of nlm_block in the list and it's order (because nlm_blocked is kind
of sorted according their timeouts in nlm_blocked).
> block->b_cache_req = &rqstp->rq_chandle;
> @@ -535,6 +537,9 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> if (!wait)
> lock->fl.fl_flags &= ~FL_SLEEP;
> mode = lock_to_openmode(&lock->fl);
> +
> + /* Append to list of blocked */
> + nlmsvc_insert_block(block, NLM_NEVER);
> error = vfs_lock_file(file->f_file[mode], F_SETLK, &lock->fl, NULL);
> lock->fl.fl_flags &= ~FL_SLEEP;
>
> @@ -542,6 +547,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> switch (error) {
> case 0:
> ret = nlm_granted;
> + nlmsvc_remove_block(block);
> goto out;
> case -EAGAIN:
> /*
> @@ -552,6 +558,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> if (wait)
> break;
> ret = async_block ? nlm_lck_blocked : nlm_lck_denied;
> + nlmsvc_remove_block(block);
> goto out;
> case FILE_LOCK_DEFERRED:
> if (wait)
> @@ -570,8 +577,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
>
> ret = nlm_lck_blocked;
>
> - /* Append to list of blocked */
> - nlmsvc_insert_block(block, NLM_NEVER);
ok. I will try to start with that.
- Alex
next prev parent reply other threads:[~2023-08-17 18:39 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-14 21:11 [Cluster-devel] [RFCv2 0/7] fs: nfs: async lock request changes Alexander Aring
2023-08-14 21:11 ` [Cluster-devel] [RFCv2 1/7] lockd: fix race in async lock request handling Alexander Aring
2023-08-15 17:49 ` Jeff Layton
2023-08-15 18:21 ` Jeff Layton
2023-08-17 18:39 ` Alexander Aring [this message]
2023-08-17 18:36 ` Alexander Aring
2023-08-14 21:11 ` [Cluster-devel] [RFCv2 2/7] lockd: FILE_LOCK_DEFERRED only on FL_SLEEP Alexander Aring
2023-08-16 11:37 ` Jeff Layton
2023-08-17 1:40 ` Alexander Aring
2023-08-14 21:11 ` [Cluster-devel] [RFCv2 3/7] lockd: introduce safe async lock op Alexander Aring
2023-08-16 11:43 ` Jeff Layton
2023-08-14 21:11 ` [Cluster-devel] [RFCv2 4/7] locks: update lock callback documentation Alexander Aring
2023-08-16 12:01 ` Jeff Layton
2023-08-17 1:23 ` Alexander Aring
2023-08-14 21:11 ` [Cluster-devel] [RFCv2 5/7] dlm: use fl_owner from lockd Alexander Aring
2023-08-16 12:02 ` Jeff Layton
2023-08-14 21:11 ` [Cluster-devel] [RFCv2 6/7] dlm: use FL_SLEEP to check if blocking request Alexander Aring
2023-08-16 13:07 ` Jeff Layton
2023-08-17 1:19 ` Alexander Aring
2023-08-17 11:27 ` Jeff Layton
2023-08-17 13:02 ` Alexander Aring
2023-08-14 21:11 ` [Cluster-devel] [RFCv2 7/7] dlm: implement EXPORT_OP_SAFE_ASYNC_LOCK Alexander Aring
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='CAK-6q+iB9AWqbe-cJfHi=nRAsdxXQ_KjpZLd4Hsj1VEtj8CNHQ@mail.gmail.com' \
--to=aahringo@redhat.com \
--cc=anna@kernel.org \
--cc=chuck.lever@oracle.com \
--cc=cluster-devel@redhat.com \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=ocfs2-devel@lists.linux.dev \
--cc=trond.myklebust@hammerspace.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 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 read-only IMAP folder(s) and NNTP newsgroup(s).