[PATCH] filelock: fix deadlock detection in POSIX locking

[PATCH] filelock: fix deadlock detection in POSIX locking

[Jeff Layton]

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);
 
 	/* 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>

Re: [PATCH] filelock: fix deadlock detection in POSIX locking

[NeilBrown]

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>
> 
> 

Re: [PATCH] filelock: fix deadlock detection in POSIX locking

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] filelock: fix deadlock detection in POSIX locking

[Christian Brauner]

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