| Message ID | 20230823213352.1971009-7-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 uses the FL_SLEEP flag in struct file_lock to determine if > the lock request is a blocking or non-blocking request. Before dlm was > using IS_SETLKW() was being used which is not usable for lock requests > coming from lockd when EXPORT_OP_SAFE_ASYNC_LOCK inside the export flags > is set. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > fs/dlm/plock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c > index 0094fa4004cc..0c6ed5eeb840 100644 > --- a/fs/dlm/plock.c > +++ b/fs/dlm/plock.c > @@ -140,7 +140,7 @@ 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.wait = !!(fl->fl_flags & FL_SLEEP); > op->info.fsid = ls->ls_global_id; > op->info.number = number; > op->info.start = fl->fl_start; Not sure you really need the !!, but ok... Reviewed-by: Jeff Layton <jlayton@kernel.org>
Hi, On Fri, Aug 25, 2023 at 2:18 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, 2023-08-23 at 17:33 -0400, Alexander Aring wrote: > > This patch uses the FL_SLEEP flag in struct file_lock to determine if > > the lock request is a blocking or non-blocking request. Before dlm was > > using IS_SETLKW() was being used which is not usable for lock requests > > coming from lockd when EXPORT_OP_SAFE_ASYNC_LOCK inside the export flags > > is set. > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > fs/dlm/plock.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c > > index 0094fa4004cc..0c6ed5eeb840 100644 > > --- a/fs/dlm/plock.c > > +++ b/fs/dlm/plock.c > > @@ -140,7 +140,7 @@ 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.wait = !!(fl->fl_flags & FL_SLEEP); > > op->info.fsid = ls->ls_global_id; > > op->info.number = number; > > op->info.start = fl->fl_start; > > Not sure you really need the !!, but ok... > The wait is a byte value and FL_SLEEP doesn't fit into it, I already run into problems with it. I don't think somebody does a if (foo->wait == 1) but it should be set to 1 or 0. An alternative would be: ((fl->fl_flags & FL_SLEEP) == FL_SLEEP). I am not sure what the coding style says here. I think it's more important what the C standard says about !!(condition), but there are other users of this in the Linux kernel. :-/ - Alex
On Wed, 2023-08-30 at 08:38 -0400, Alexander Aring wrote: > Hi, > > On Fri, Aug 25, 2023 at 2:18 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Wed, 2023-08-23 at 17:33 -0400, Alexander Aring wrote: > > > This patch uses the FL_SLEEP flag in struct file_lock to determine if > > > the lock request is a blocking or non-blocking request. Before dlm was > > > using IS_SETLKW() was being used which is not usable for lock requests > > > coming from lockd when EXPORT_OP_SAFE_ASYNC_LOCK inside the export flags > > > is set. > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > --- > > > fs/dlm/plock.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c > > > index 0094fa4004cc..0c6ed5eeb840 100644 > > > --- a/fs/dlm/plock.c > > > +++ b/fs/dlm/plock.c > > > @@ -140,7 +140,7 @@ 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.wait = !!(fl->fl_flags & FL_SLEEP); > > > op->info.fsid = ls->ls_global_id; > > > op->info.number = number; > > > op->info.start = fl->fl_start; > > > > Not sure you really need the !!, but ok... > > > > The wait is a byte value and FL_SLEEP doesn't fit into it, I already > run into problems with it. I don't think somebody does a if (foo->wait > == 1) but it should be set to 1 or 0. > AIUI, any halfway decent compiler should take the result of the &, and implicitly cast that properly to bool. Basically, any value other than 0 should be true. If the compiler just blindly casts the lowest byte though, then you do need the double-negative. > An alternative would be: ((fl->fl_flags & FL_SLEEP) == FL_SLEEP). I am > not sure what the coding style says here. I think it's more important > what the C standard says about !!(condition), but there are other > users of this in the Linux kernel. :-/ I don't care too much either way, but my understanding was that you don't need to do the !! trick in most cases with modern compilers.
diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c index 0094fa4004cc..0c6ed5eeb840 100644 --- a/fs/dlm/plock.c +++ b/fs/dlm/plock.c @@ -140,7 +140,7 @@ 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.wait = !!(fl->fl_flags & FL_SLEEP); op->info.fsid = ls->ls_global_id; op->info.number = number; op->info.start = fl->fl_start;
This patch uses the FL_SLEEP flag in struct file_lock to determine if the lock request is a blocking or non-blocking request. Before dlm was using IS_SETLKW() was being used which is not usable for lock requests coming from lockd when EXPORT_OP_SAFE_ASYNC_LOCK inside the export flags is set. Signed-off-by: Alexander Aring <aahringo@redhat.com> --- fs/dlm/plock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)