diff mbox series

filelock: fix deadlock detection in POSIX locking

Message ID 20240218-flsplit4-v1-1-26454fc090f2@kernel.org
State New
Headers
Series filelock: fix deadlock detection in POSIX locking |

Commit Message

Jeff Layton Feb. 18, 2024, 1:33 p.m. UTC 
  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,

Comments

NeilBrown Feb. 18, 2024, 10:28 p.m. UTC | #1 
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>
> 
>
Jeff Layton Feb. 19, 2024, 3:10 p.m. UTC | #2 
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.
Christian Brauner Feb. 20, 2024, 8:54 a.m. UTC | #3 
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 mbox series

Patch

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