| Message ID | 20230814211116.3224759-7-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 uses the FL_SLEEP flag in struct file_lock to check if it's a > blocking request in case if the request coming from nfs lockd process > indicated by lm_grant() is set. > > IF FL_SLEEP is set a asynchronous blocking request is being made and > it's waiting for lm_grant() callback being called to signal the lock was > granted. If it's not set a synchronous non-blocking request is being made. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > fs/dlm/plock.c | 38 ++++++++++++++++++++++---------------- > 1 file changed, 22 insertions(+), 16 deletions(-) > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c > index 0094fa4004cc..524771002a2f 100644 > --- a/fs/dlm/plock.c > +++ b/fs/dlm/plock.c > @@ -140,7 +140,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > op->info.optype = DLM_PLOCK_OP_LOCK; > op->info.pid = fl->fl_pid; > op->info.ex = (fl->fl_type == F_WRLCK); > - op->info.wait = IS_SETLKW(cmd); > op->info.fsid = ls->ls_global_id; > op->info.number = number; > op->info.start = fl->fl_start; > @@ -148,24 +147,31 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > op->info.owner = (__u64)(long)fl->fl_owner; > /* async handling */ > if (fl->fl_lmops && fl->fl_lmops->lm_grant) { > - op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > - if (!op_data) { > - dlm_release_plock_op(op); > - rv = -ENOMEM; > - goto out; > - } > + if (fl->fl_flags & FL_SLEEP) { > + op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > + if (!op_data) { > + dlm_release_plock_op(op); > + rv = -ENOMEM; > + goto out; > + } > > - op_data->callback = fl->fl_lmops->lm_grant; > - locks_init_lock(&op_data->flc); > - locks_copy_lock(&op_data->flc, fl); > - op_data->fl = fl; > - op_data->file = file; > + op->info.wait = 1; > + op_data->callback = fl->fl_lmops->lm_grant; > + locks_init_lock(&op_data->flc); > + locks_copy_lock(&op_data->flc, fl); > + op_data->fl = fl; > + op_data->file = file; > > - op->data = op_data; > + op->data = op_data; > > - send_op(op); > - rv = FILE_LOCK_DEFERRED; > - goto out; > + send_op(op); > + rv = FILE_LOCK_DEFERRED; > + goto out; A question...we're returning FILE_LOCK_DEFERRED after the DLM request is sent. If it ends up being blocked, what happens? Does it do a lm_grant downcall with -EAGAIN or something as the result? > + } else { > + op->info.wait = 0; > + } > + } else { > + op->info.wait = IS_SETLKW(cmd); > } > > send_op(op); Looks reasonable overall. Now that I look, we have quite a number of places in the kernel that seem to check for F_SETLKW, when what they really want is to check FL_SLEEP.
Hi, On Wed, Aug 16, 2023 at 9:07 AM Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote: > > This patch uses the FL_SLEEP flag in struct file_lock to check if it's a > > blocking request in case if the request coming from nfs lockd process > > indicated by lm_grant() is set. > > > > IF FL_SLEEP is set a asynchronous blocking request is being made and > > it's waiting for lm_grant() callback being called to signal the lock was > > granted. If it's not set a synchronous non-blocking request is being made. > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > fs/dlm/plock.c | 38 ++++++++++++++++++++++---------------- > > 1 file changed, 22 insertions(+), 16 deletions(-) > > > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c > > index 0094fa4004cc..524771002a2f 100644 > > --- a/fs/dlm/plock.c > > +++ b/fs/dlm/plock.c > > @@ -140,7 +140,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > > op->info.optype = DLM_PLOCK_OP_LOCK; > > op->info.pid = fl->fl_pid; > > op->info.ex = (fl->fl_type == F_WRLCK); > > - op->info.wait = IS_SETLKW(cmd); > > op->info.fsid = ls->ls_global_id; > > op->info.number = number; > > op->info.start = fl->fl_start; > > @@ -148,24 +147,31 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > > op->info.owner = (__u64)(long)fl->fl_owner; > > /* async handling */ > > if (fl->fl_lmops && fl->fl_lmops->lm_grant) { > > - op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > > - if (!op_data) { > > - dlm_release_plock_op(op); > > - rv = -ENOMEM; > > - goto out; > > - } > > + if (fl->fl_flags & FL_SLEEP) { > > + op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > > + if (!op_data) { > > + dlm_release_plock_op(op); > > + rv = -ENOMEM; > > + goto out; > > + } > > > > - op_data->callback = fl->fl_lmops->lm_grant; > > - locks_init_lock(&op_data->flc); > > - locks_copy_lock(&op_data->flc, fl); > > - op_data->fl = fl; > > - op_data->file = file; > > + op->info.wait = 1; > > + op_data->callback = fl->fl_lmops->lm_grant; > > + locks_init_lock(&op_data->flc); > > + locks_copy_lock(&op_data->flc, fl); > > + op_data->fl = fl; > > + op_data->file = file; > > > > - op->data = op_data; > > + op->data = op_data; > > > > - send_op(op); > > - rv = FILE_LOCK_DEFERRED; > > - goto out; > > + send_op(op); > > + rv = FILE_LOCK_DEFERRED; > > + goto out; > > A question...we're returning FILE_LOCK_DEFERRED after the DLM request is > sent. If it ends up being blocked, what happens? Does it do a lm_grant > downcall with -EAGAIN or something as the result? > no, when info->wait is set then it is a blocked lock request, which means lm_grant() will be called when the lock request is granted. > > > + } else { > > + op->info.wait = 0; > > + } > > + } else { > > + op->info.wait = IS_SETLKW(cmd); > > } > > > > send_op(op); > > Looks reasonable overall. > > Now that I look, we have quite a number of places in the kernel that > seem to check for F_SETLKW, when what they really want is to check > FL_SLEEP. Yes, so far I understand FL_SLEEP is F_SETLKW when you get only F_SETLK in case of fl->fl_lmops && fl->fl_lmops->lm_grant is true. It is confusing but this is how it works... if it's not set we will get F_SETLKW and this should imply FL_SLEEP is set. - Alex
On Wed, 2023-08-16 at 21:19 -0400, Alexander Aring wrote: > Hi, > > On Wed, Aug 16, 2023 at 9:07 AM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote: > > > This patch uses the FL_SLEEP flag in struct file_lock to check if it's a > > > blocking request in case if the request coming from nfs lockd process > > > indicated by lm_grant() is set. > > > > > > IF FL_SLEEP is set a asynchronous blocking request is being made and > > > it's waiting for lm_grant() callback being called to signal the lock was > > > granted. If it's not set a synchronous non-blocking request is being made. > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > --- > > > fs/dlm/plock.c | 38 ++++++++++++++++++++++---------------- > > > 1 file changed, 22 insertions(+), 16 deletions(-) > > > > > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c > > > index 0094fa4004cc..524771002a2f 100644 > > > --- a/fs/dlm/plock.c > > > +++ b/fs/dlm/plock.c > > > @@ -140,7 +140,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > > > op->info.optype = DLM_PLOCK_OP_LOCK; > > > op->info.pid = fl->fl_pid; > > > op->info.ex = (fl->fl_type == F_WRLCK); > > > - op->info.wait = IS_SETLKW(cmd); > > > op->info.fsid = ls->ls_global_id; > > > op->info.number = number; > > > op->info.start = fl->fl_start; > > > @@ -148,24 +147,31 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > > > op->info.owner = (__u64)(long)fl->fl_owner; > > > /* async handling */ > > > if (fl->fl_lmops && fl->fl_lmops->lm_grant) { > > > - op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > > > - if (!op_data) { > > > - dlm_release_plock_op(op); > > > - rv = -ENOMEM; > > > - goto out; > > > - } > > > + if (fl->fl_flags & FL_SLEEP) { > > > + op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > > > + if (!op_data) { > > > + dlm_release_plock_op(op); > > > + rv = -ENOMEM; > > > + goto out; > > > + } > > > > > > - op_data->callback = fl->fl_lmops->lm_grant; > > > - locks_init_lock(&op_data->flc); > > > - locks_copy_lock(&op_data->flc, fl); > > > - op_data->fl = fl; > > > - op_data->file = file; > > > + op->info.wait = 1; > > > + op_data->callback = fl->fl_lmops->lm_grant; > > > + locks_init_lock(&op_data->flc); > > > + locks_copy_lock(&op_data->flc, fl); > > > + op_data->fl = fl; > > > + op_data->file = file; > > > > > > - op->data = op_data; > > > + op->data = op_data; > > > > > > - send_op(op); > > > - rv = FILE_LOCK_DEFERRED; > > > - goto out; > > > + send_op(op); > > > + rv = FILE_LOCK_DEFERRED; > > > + goto out; > > > > A question...we're returning FILE_LOCK_DEFERRED after the DLM request is > > sent. If it ends up being blocked, what happens? Does it do a lm_grant > > downcall with -EAGAIN or something as the result? > > > > no, when info->wait is set then it is a blocked lock request, which > means lm_grant() will be called when the lock request is granted. > Ok, that's probably problematic with the current code too. lockd will time out the block after 7s, so if the lock isn't granted in that time it'll give up on it. > > > > > + } else { > > > + op->info.wait = 0; > > > + } > > > + } else { > > > + op->info.wait = IS_SETLKW(cmd); > > > } > > > > > > send_op(op); > > > > Looks reasonable overall. > > > > Now that I look, we have quite a number of places in the kernel that > > seem to check for F_SETLKW, when what they really want is to check > > FL_SLEEP. > > Yes, so far I understand FL_SLEEP is F_SETLKW when you get only > F_SETLK in case of fl->fl_lmops && fl->fl_lmops->lm_grant is true. It > is confusing but this is how it works... if it's not set we will get > F_SETLKW and this should imply FL_SLEEP is set. > > Yeah. Might be good to consider how to make this more consistent across the kernel.
Hi, On Thu, Aug 17, 2023 at 7:27 AM Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, 2023-08-16 at 21:19 -0400, Alexander Aring wrote: > > Hi, > > > > On Wed, Aug 16, 2023 at 9:07 AM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote: > > > > This patch uses the FL_SLEEP flag in struct file_lock to check if it's a > > > > blocking request in case if the request coming from nfs lockd process > > > > indicated by lm_grant() is set. > > > > > > > > IF FL_SLEEP is set a asynchronous blocking request is being made and > > > > it's waiting for lm_grant() callback being called to signal the lock was > > > > granted. If it's not set a synchronous non-blocking request is being made. > > > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > > --- > > > > fs/dlm/plock.c | 38 ++++++++++++++++++++++---------------- > > > > 1 file changed, 22 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c > > > > index 0094fa4004cc..524771002a2f 100644 > > > > --- a/fs/dlm/plock.c > > > > +++ b/fs/dlm/plock.c > > > > @@ -140,7 +140,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > > > > op->info.optype = DLM_PLOCK_OP_LOCK; > > > > op->info.pid = fl->fl_pid; > > > > op->info.ex = (fl->fl_type == F_WRLCK); > > > > - op->info.wait = IS_SETLKW(cmd); > > > > op->info.fsid = ls->ls_global_id; > > > > op->info.number = number; > > > > op->info.start = fl->fl_start; > > > > @@ -148,24 +147,31 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, > > > > op->info.owner = (__u64)(long)fl->fl_owner; > > > > /* async handling */ > > > > if (fl->fl_lmops && fl->fl_lmops->lm_grant) { > > > > - op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > > > > - if (!op_data) { > > > > - dlm_release_plock_op(op); > > > > - rv = -ENOMEM; > > > > - goto out; > > > > - } > > > > + if (fl->fl_flags & FL_SLEEP) { > > > > + op_data = kzalloc(sizeof(*op_data), GFP_NOFS); > > > > + if (!op_data) { > > > > + dlm_release_plock_op(op); > > > > + rv = -ENOMEM; > > > > + goto out; > > > > + } > > > > > > > > - op_data->callback = fl->fl_lmops->lm_grant; > > > > - locks_init_lock(&op_data->flc); > > > > - locks_copy_lock(&op_data->flc, fl); > > > > - op_data->fl = fl; > > > > - op_data->file = file; > > > > + op->info.wait = 1; > > > > + op_data->callback = fl->fl_lmops->lm_grant; > > > > + locks_init_lock(&op_data->flc); > > > > + locks_copy_lock(&op_data->flc, fl); > > > > + op_data->fl = fl; > > > > + op_data->file = file; > > > > > > > > - op->data = op_data; > > > > + op->data = op_data; > > > > > > > > - send_op(op); > > > > - rv = FILE_LOCK_DEFERRED; > > > > - goto out; > > > > + send_op(op); > > > > + rv = FILE_LOCK_DEFERRED; > > > > + goto out; > > > > > > A question...we're returning FILE_LOCK_DEFERRED after the DLM request is > > > sent. If it ends up being blocked, what happens? Does it do a lm_grant > > > downcall with -EAGAIN or something as the result? > > > > > > > no, when info->wait is set then it is a blocked lock request, which > > means lm_grant() will be called when the lock request is granted. > > > > Ok, that's probably problematic with the current code too. lockd will > time out the block after 7s, so if the lock isn't granted in that time > it'll give up on it. > that's why blocked lock requests (meaning F_SETLKW, FL_SLEEP is set) need to use NLM_NEVER? It will never give up on it. I thought NLM_TIMEOUT is just the polling frequency for doing a blocked request by doing non-blocking requests. Means trylocks in a loop with a polling frequency. > > > > > > > + } else { > > > > + op->info.wait = 0; > > > > + } > > > > + } else { > > > > + op->info.wait = IS_SETLKW(cmd); > > > > } > > > > > > > > send_op(op); > > > > > > Looks reasonable overall. > > > > > > Now that I look, we have quite a number of places in the kernel that > > > seem to check for F_SETLKW, when what they really want is to check > > > FL_SLEEP. > > > > Yes, so far I understand FL_SLEEP is F_SETLKW when you get only > > F_SETLK in case of fl->fl_lmops && fl->fl_lmops->lm_grant is true. It > > is confusing but this is how it works... if it's not set we will get > > F_SETLKW and this should imply FL_SLEEP is set. > > > > > > Yeah. Might be good to consider how to make this more consistent across > the kernel. ok. I will do some grep and try to find those places. - Alex
diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c index 0094fa4004cc..524771002a2f 100644 --- a/fs/dlm/plock.c +++ b/fs/dlm/plock.c @@ -140,7 +140,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, op->info.optype = DLM_PLOCK_OP_LOCK; op->info.pid = fl->fl_pid; op->info.ex = (fl->fl_type == F_WRLCK); - op->info.wait = IS_SETLKW(cmd); op->info.fsid = ls->ls_global_id; op->info.number = number; op->info.start = fl->fl_start; @@ -148,24 +147,31 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file, op->info.owner = (__u64)(long)fl->fl_owner; /* async handling */ if (fl->fl_lmops && fl->fl_lmops->lm_grant) { - op_data = kzalloc(sizeof(*op_data), GFP_NOFS); - if (!op_data) { - dlm_release_plock_op(op); - rv = -ENOMEM; - goto out; - } + if (fl->fl_flags & FL_SLEEP) { + op_data = kzalloc(sizeof(*op_data), GFP_NOFS); + if (!op_data) { + dlm_release_plock_op(op); + rv = -ENOMEM; + goto out; + } - op_data->callback = fl->fl_lmops->lm_grant; - locks_init_lock(&op_data->flc); - locks_copy_lock(&op_data->flc, fl); - op_data->fl = fl; - op_data->file = file; + op->info.wait = 1; + op_data->callback = fl->fl_lmops->lm_grant; + locks_init_lock(&op_data->flc); + locks_copy_lock(&op_data->flc, fl); + op_data->fl = fl; + op_data->file = file; - op->data = op_data; + op->data = op_data; - send_op(op); - rv = FILE_LOCK_DEFERRED; - goto out; + send_op(op); + rv = FILE_LOCK_DEFERRED; + goto out; + } else { + op->info.wait = 0; + } + } else { + op->info.wait = IS_SETLKW(cmd); } send_op(op);
This patch uses the FL_SLEEP flag in struct file_lock to check if it's a blocking request in case if the request coming from nfs lockd process indicated by lm_grant() is set. IF FL_SLEEP is set a asynchronous blocking request is being made and it's waiting for lm_grant() callback being called to signal the lock was granted. If it's not set a synchronous non-blocking request is being made. Signed-off-by: Alexander Aring <aahringo@redhat.com> --- fs/dlm/plock.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)