| Message ID | 20240218-flsplit4-v1-1-26454fc090f2@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | filelock: fix deadlock detection in POSIX locking | expand |
On Mon, 19 Feb 2024, Jeff Layton wrote: > The FL_POSIX check in __locks_insert_block was inadvertantly broken > recently and is now inserting only OFD locks instead of only legacy > POSIX locks. > > This breaks deadlock detection in POSIX locks, and may also be the root > cause of a performance regression noted by the kernel test robot. > Restore the proper sense of the test. > > Fixes: b6be3714005c ("filelock: convert __locks_insert_block, conflict and deadlock checks to use file_lock_core") > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202402181229.f8147f40-oliver.sang@intel.com > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > Disregard what I said earlier about this bug being harmless. It broke > deadlock detection in POSIX locks (LTP fcntl17 shows the bug). This > patch fixes it. It may be best to squash this into the patch that > introduced the regression. > > I'm not certain if this fixes the performance regression that the KTR > noticed recently in this patch, but that's what got me looking more > closely, so I'll give it credit for reporting this. Hopefully it'll > confirm that result for us. > --- > fs/locks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/locks.c b/fs/locks.c > index 26d52ef5314a..90c8746874de 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -812,7 +812,7 @@ static void __locks_insert_block(struct file_lock_core *blocker, > list_add_tail(&waiter->flc_blocked_member, > &blocker->flc_blocked_requests); > > - if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == (FL_POSIX|FL_OFDLCK)) > + if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == FL_POSIX) > locks_insert_global_blocked(waiter); I wonder how that happened... sorry I didn't notice it in my review. Reviewed-by: NeilBrown <neilb@suse.de> Thanks, NeilBrown > > /* The requests in waiter->flc_blocked are known to conflict with > > --- > base-commit: 292fcaa1f937345cb65f3af82a1ee6692c8df9eb > change-id: 20240218-flsplit4-e843536f4c11 > > Best regards, > -- > Jeff Layton <jlayton@kernel.org> > >
On Mon, 2024-02-19 at 09:28 +1100, NeilBrown wrote: > On Mon, 19 Feb 2024, Jeff Layton wrote: > > The FL_POSIX check in __locks_insert_block was inadvertantly broken > > recently and is now inserting only OFD locks instead of only legacy > > POSIX locks. > > > > This breaks deadlock detection in POSIX locks, and may also be the root > > cause of a performance regression noted by the kernel test robot. > > Restore the proper sense of the test. > > > > Fixes: b6be3714005c ("filelock: convert __locks_insert_block, conflict and deadlock checks to use file_lock_core") > > Reported-by: kernel test robot <oliver.sang@intel.com> > > Closes: https://lore.kernel.org/oe-lkp/202402181229.f8147f40-oliver.sang@intel.com > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > Disregard what I said earlier about this bug being harmless. It broke > > deadlock detection in POSIX locks (LTP fcntl17 shows the bug). This > > patch fixes it. It may be best to squash this into the patch that > > introduced the regression. > > > > I'm not certain if this fixes the performance regression that the KTR > > noticed recently in this patch, but that's what got me looking more > > closely, so I'll give it credit for reporting this. Hopefully it'll > > confirm that result for us. > > --- > > fs/locks.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index 26d52ef5314a..90c8746874de 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -812,7 +812,7 @@ static void __locks_insert_block(struct file_lock_core *blocker, > > list_add_tail(&waiter->flc_blocked_member, > > &blocker->flc_blocked_requests); > > > > - if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == (FL_POSIX|FL_OFDLCK)) > > + if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == FL_POSIX) > > locks_insert_global_blocked(waiter); > > I wonder how that happened... sorry I didn't notice it in my review. > > Reviewed-by: NeilBrown <neilb@suse.de> > Mea culpa. I had this bug in the original version of the series, fixed it and then reverted that fix by accident while rebasing to clean up and reorganize things.
On Sun, 18 Feb 2024 08:33:28 -0500, Jeff Layton wrote: > The FL_POSIX check in __locks_insert_block was inadvertantly broken > recently and is now inserting only OFD locks instead of only legacy > POSIX locks. > > This breaks deadlock detection in POSIX locks, and may also be the root > cause of a performance regression noted by the kernel test robot. > Restore the proper sense of the test. > > [...] Applied to the vfs.file branch of the vfs/vfs.git tree. Patches in the vfs.file branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.file [1/1] filelock: fix deadlock detection in POSIX locking https://git.kernel.org/vfs/vfs/c/14786d949a3b
diff --git a/fs/locks.c b/fs/locks.c index 26d52ef5314a..90c8746874de 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -812,7 +812,7 @@ static void __locks_insert_block(struct file_lock_core *blocker, list_add_tail(&waiter->flc_blocked_member, &blocker->flc_blocked_requests); - if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == (FL_POSIX|FL_OFDLCK)) + if ((blocker->flc_flags & (FL_POSIX|FL_OFDLCK)) == FL_POSIX) locks_insert_global_blocked(waiter); /* The requests in waiter->flc_blocked are known to conflict with
The FL_POSIX check in __locks_insert_block was inadvertantly broken recently and is now inserting only OFD locks instead of only legacy POSIX locks. This breaks deadlock detection in POSIX locks, and may also be the root cause of a performance regression noted by the kernel test robot. Restore the proper sense of the test. Fixes: b6be3714005c ("filelock: convert __locks_insert_block, conflict and deadlock checks to use file_lock_core") Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202402181229.f8147f40-oliver.sang@intel.com Signed-off-by: Jeff Layton <jlayton@kernel.org> --- Disregard what I said earlier about this bug being harmless. It broke deadlock detection in POSIX locks (LTP fcntl17 shows the bug). This patch fixes it. It may be best to squash this into the patch that introduced the regression. I'm not certain if this fixes the performance regression that the KTR noticed recently in this patch, but that's what got me looking more closely, so I'll give it credit for reporting this. Hopefully it'll confirm that result for us. --- fs/locks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 292fcaa1f937345cb65f3af82a1ee6692c8df9eb change-id: 20240218-flsplit4-e843536f4c11 Best regards,