| Message ID | 20230823213352.1971009-3-aahringo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lockd: dlm: async lock request changes | expand |
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@redhat.com> > --- > 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. > + spin_unlock(&nlm_blocked_lock); > + > if (!wait) > lock->fl.fl_flags &= ~FL_SLEEP; > mode = lock_to_openmode(&lock->fl); > @@ -543,13 +560,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, > ret = nlm_granted; > goto out; > case -EAGAIN: > - /* > - * If this is a blocking request for an > - * already pending lock request then we need > - * to put it back on lockd's block list > - */ > - if (wait) > - break; > ret = async_block ? nlm_lck_blocked : nlm_lck_denied; > goto out; > case FILE_LOCK_DEFERRED: Reviewed-by: Jeff Layton <jlayton@kernel.org>
Hi, On Fri, Aug 25, 2023 at 2:10 PM Jeff Layton <jlayton@kernel.org> 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@redhat.com> > > --- > > 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
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; + } + spin_unlock(&nlm_blocked_lock); + if (!wait) lock->fl.fl_flags &= ~FL_SLEEP; mode = lock_to_openmode(&lock->fl); @@ -543,13 +560,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, ret = nlm_granted; goto out; case -EAGAIN: - /* - * If this is a blocking request for an - * already pending lock request then we need - * to put it back on lockd's block list - */ - if (wait) - break; ret = async_block ? nlm_lck_blocked : nlm_lck_denied; goto out; case FILE_LOCK_DEFERRED:
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@redhat.com> --- fs/lockd/svclock.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)