| Message ID | 20230814211116.3224759-3-aahringo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: nfs: async lock request changes | expand |
On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote: > This patch removes to handle non-blocking lock requests as asynchronous > lock request returning FILE_LOCK_DEFERRED. When fl_lmops and lm_grant() > is set and a non-blocking lock request returns FILE_LOCK_DEFERRED will > end in an WARNING to signal the user the misusage of the API. > Probably need to rephrase the word salad in the first sentence of the commit log. I had to go over it a few times to understand what was going on here. In any case, I'm guessing that the idea here is that GFS2/DLM shouldn't ever return FILE_LOCK_DEFERRED if this is a non-wait request (i.e. someone called F_SETLK instead of F_SETLKW)? That may be ok, but again, lockd goes to great lengths to avoid blocking and I think it's generally a good idea. If an upcall to DLM can take a long time, it might be a good idea to continue to allow a !wait request to return FILE_LOCK_DEFERRED. I guess this really depends on the current behavior today though. Does DLM ever return FILE_LOCK_DEFERRED on a non-blocking lock request? > The reason why we moving to make non-blocking lock request as > synchronized call is that we already doing this behaviour for unlock or > cancellation as well. Those are POSIX lock operations which are handled > in an synchronized way and waiting for an answer. For non-blocking lock > requests the answer will probably arrive in the same time as unlock or > cancellation operations as those are trylock operations only. > > In case of a blocking lock request we need to have it asynchronously > because the time when the lock request getting granted is unknown. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > fs/lockd/svclock.c | 39 +++++++-------------------------------- > 1 file changed, 7 insertions(+), 32 deletions(-) > > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c > index 7d63524bdb81..1e74a578d7de 100644 > --- a/fs/lockd/svclock.c > +++ b/fs/lockd/svclock.c > @@ -440,31 +440,6 @@ static void nlmsvc_freegrantargs(struct nlm_rqst *call) > locks_release_private(&call->a_args.lock.fl); > } > > -/* > - * Deferred lock request handling for non-blocking lock > - */ > -static __be32 > -nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block) > -{ > - __be32 status = nlm_lck_denied_nolocks; > - > - block->b_flags |= B_QUEUED; > - > - nlmsvc_insert_block(block, NLM_TIMEOUT); > - > - block->b_cache_req = &rqstp->rq_chandle; > - if (rqstp->rq_chandle.defer) { > - block->b_deferred_req = > - rqstp->rq_chandle.defer(block->b_cache_req); > - if (block->b_deferred_req != NULL) > - status = nlm_drop_reply; > - } > - dprintk("lockd: nlmsvc_defer_lock_rqst block %p flags %d status %d\n", > - block, block->b_flags, ntohl(status)); > - > - return status; > -} > - > /* > * Attempt to establish a lock, and if it can't be granted, block it > * if required. > @@ -569,14 +544,14 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, > ret = async_block ? nlm_lck_blocked : nlm_lck_denied; > goto out_cb_mutex; > case FILE_LOCK_DEFERRED: > - block->b_flags |= B_PENDING_CALLBACK; > + /* lock requests without waiters are handled in > + * a non async way. Let assert this to inform > + * the user about a API violation. > + */ > + WARN_ON_ONCE(!wait); > > - if (wait) > - break; > - /* Filesystem lock operation is in progress > - Add it to the queue waiting for callback */ > - ret = nlmsvc_defer_lock_rqst(rqstp, block); > - goto out_cb_mutex; > + block->b_flags |= B_PENDING_CALLBACK; > + break; > case -EDEADLK: > nlmsvc_remove_block(block); > ret = nlm_deadlock;
Hi, On Wed, Aug 16, 2023 at 7:37 AM Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote: > > This patch removes to handle non-blocking lock requests as asynchronous > > lock request returning FILE_LOCK_DEFERRED. When fl_lmops and lm_grant() > > is set and a non-blocking lock request returns FILE_LOCK_DEFERRED will > > end in an WARNING to signal the user the misusage of the API. > > > > Probably need to rephrase the word salad in the first sentence of the > commit log. I had to go over it a few times to understand what was going > on here. > ok. I will go over it again. > In any case, I'm guessing that the idea here is that GFS2/DLM shouldn't > ever return FILE_LOCK_DEFERRED if this is a non-wait request (i.e. > someone called F_SETLK instead of F_SETLKW)? > Yes, non-wait requests (meaning trylock) does not return FILE_LOCK_DEFERRED. I added in some patch a WARN_ON() if this would be the case. However I mentioned in other patches there is this mismatch between F_SETLK from lockd and figure out if it's wait and non-wait if FL_SLEEP is set, but somehow if it's not coming from lockd (lm_grant is not set) it's going over the cmd if it's F_SETLKW. So far I understand DLM should never make this decision over the F_SETLK vs F_SETLKW it should always check on FL_SLEEP. I can change it to use FL_SLEEP only. > That may be ok, but again, lockd goes to great lengths to avoid blocking > and I think it's generally a good idea. If an upcall to DLM can take a > long time, it might be a good idea to continue to allow a !wait request > to return FILE_LOCK_DEFERRED. > In the case of DLM there is no difference between upcall/downcall if lockd does other operations like unlock/cancellation requests. We don't do the optimization there, why are we doing it for !wait requests... but okay I can change it. > I guess this really depends on the current behavior today though. Does > DLM ever return FILE_LOCK_DEFERRED on a non-blocking lock request? > I change it so that it doesn't do it, but I can change it !wait requests will return FILE_LOCK_DEFERRED and be handled asynchronously. - Alex
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c index 7d63524bdb81..1e74a578d7de 100644 --- a/fs/lockd/svclock.c +++ b/fs/lockd/svclock.c @@ -440,31 +440,6 @@ static void nlmsvc_freegrantargs(struct nlm_rqst *call) locks_release_private(&call->a_args.lock.fl); } -/* - * Deferred lock request handling for non-blocking lock - */ -static __be32 -nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block) -{ - __be32 status = nlm_lck_denied_nolocks; - - block->b_flags |= B_QUEUED; - - nlmsvc_insert_block(block, NLM_TIMEOUT); - - block->b_cache_req = &rqstp->rq_chandle; - if (rqstp->rq_chandle.defer) { - block->b_deferred_req = - rqstp->rq_chandle.defer(block->b_cache_req); - if (block->b_deferred_req != NULL) - status = nlm_drop_reply; - } - dprintk("lockd: nlmsvc_defer_lock_rqst block %p flags %d status %d\n", - block, block->b_flags, ntohl(status)); - - return status; -} - /* * Attempt to establish a lock, and if it can't be granted, block it * if required. @@ -569,14 +544,14 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file, ret = async_block ? nlm_lck_blocked : nlm_lck_denied; goto out_cb_mutex; case FILE_LOCK_DEFERRED: - block->b_flags |= B_PENDING_CALLBACK; + /* lock requests without waiters are handled in + * a non async way. Let assert this to inform + * the user about a API violation. + */ + WARN_ON_ONCE(!wait); - if (wait) - break; - /* Filesystem lock operation is in progress - Add it to the queue waiting for callback */ - ret = nlmsvc_defer_lock_rqst(rqstp, block); - goto out_cb_mutex; + block->b_flags |= B_PENDING_CALLBACK; + break; case -EDEADLK: nlmsvc_remove_block(block); ret = nlm_deadlock;
This patch removes to handle non-blocking lock requests as asynchronous lock request returning FILE_LOCK_DEFERRED. When fl_lmops and lm_grant() is set and a non-blocking lock request returns FILE_LOCK_DEFERRED will end in an WARNING to signal the user the misusage of the API. The reason why we moving to make non-blocking lock request as synchronized call is that we already doing this behaviour for unlock or cancellation as well. Those are POSIX lock operations which are handled in an synchronized way and waiting for an answer. For non-blocking lock requests the answer will probably arrive in the same time as unlock or cancellation operations as those are trylock operations only. In case of a blocking lock request we need to have it asynchronously because the time when the lock request getting granted is unknown. Signed-off-by: Alexander Aring <aahringo@redhat.com> --- fs/lockd/svclock.c | 39 +++++++-------------------------------- 1 file changed, 7 insertions(+), 32 deletions(-)