Hi,
On Fri, Aug 25, 2023 at 2:10 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Wed, 2023-08-23 at 17:33 -0400, Alexander Aring wrote:
> > This patch returns nlm_lck_blocked in nlmsvc_lock() when an asynchronous
> > lock request is pending. During testing I ran into the case with the
> > side-effects that lockd is waiting for only one lm_grant() callback
> > because it's already part of the nlm_blocked list. If another
> > asynchronous for the same nlm_block is triggered two lm_grant()
> > callbacks will occur but lockd was only waiting for one.
> >
> > To avoid any change of existing users this handling will only being made
> > when export_op_support_safe_async_lock() returns true.
> >
> > Signed-off-by: Alexander Aring <aahringo@xxxxxxxxxx>
> > ---
> > fs/lockd/svclock.c | 24 +++++++++++++++++-------
> > 1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > index 6e3b230e8317..aa4174fbaf5b 100644
> > --- a/fs/lockd/svclock.c
> > +++ b/fs/lockd/svclock.c
> > @@ -531,6 +531,23 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> > goto out;
> > }
> >
> > + spin_lock(&nlm_blocked_lock);
> > + /*
> > + * If this is a lock request for an already pending
> > + * lock request we return nlm_lck_blocked without calling
> > + * vfs_lock_file() again. Otherwise we have two pending
> > + * requests on the underlaying ->lock() implementation but
> > + * only one nlm_block to being granted by lm_grant().
> > + */
> > + if (export_op_support_safe_async_lock(inode->i_sb->s_export_op,
> > + nlmsvc_file_file(file)->f_op) &&
> > + !list_empty(&block->b_list)) {
> > + spin_unlock(&nlm_blocked_lock);
> > + ret = nlm_lck_blocked;
> > + goto out;
> > + }
>
> Looks reasonable. The block->b_list check is subtle, but the comment
> helps.
thanks. To be honest, I am "a little bit" worried (I am thinking of
this scenario) that we might have a problem here with multiple
identically lock requests being granted at the same time. In such
cases the most fields of struct file_lock are mostly the same and
nlm_compare_locks() checks exactly on those fields. I am concerned
this corner case could cause problems, but it is a very rare case and
it makes totally no sense that an application is doing such a request.
I am currently trying to get an xfstest for this upstream.
- Alex
