[PATCH] hfsplus: remove mutex_lock check in hfsplus_free_extents

[PATCH] hfsplus: remove mutex_lock check in hfsplus_free_extents

[Yangtao Li]

Syzbot reported an issue in hfsplus subsystem:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 4400 at fs/hfsplus/extents.c:346
	hfsplus_free_extents+0x700/0xad0
Call Trace:
<TASK>
hfsplus_file_truncate+0x768/0xbb0 fs/hfsplus/extents.c:606
hfsplus_write_begin+0xc2/0xd0 fs/hfsplus/inode.c:56
cont_expand_zero fs/buffer.c:2383 [inline]
cont_write_begin+0x2cf/0x860 fs/buffer.c:2446
hfsplus_write_begin+0x86/0xd0 fs/hfsplus/inode.c:52
generic_cont_expand_simple+0x151/0x250 fs/buffer.c:2347
hfsplus_setattr+0x168/0x280 fs/hfsplus/inode.c:263
notify_change+0xe38/0x10f0 fs/attr.c:420
do_truncate+0x1fb/0x2e0 fs/open.c:65
do_sys_ftruncate+0x2eb/0x380 fs/open.c:193
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

To avoid deadlock, Commit 31651c607151 ("hfsplus: avoid deadlock
on file truncation") unlock extree before hfsplus_free_extents(),
and add check wheather extree is locked in hfsplus_free_extents().

However, when operations such as hfsplus_file_release,
hfsplus_setattr, hfsplus_unlink, and hfsplus_unlink are executed
concurrently in different files, it is very likely to trigger the
WARN_ON, which will lead syzbot and xfstest to consider it as an
abnormality.

Since we already have alloc_mutex to protect alloc file modify,
let's remove mutex_lock check.

Fixes: 31651c607151f ("hfsplus: avoid deadlock on file truncation")
Reported-by: syzbot+8c0bc9f818702ff75b76@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/00000000000057fa4605ef101c4c@google.com/
Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/hfsplus/extents.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index a6d61685ae79..b1699b3c246a 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -342,9 +342,6 @@ static int hfsplus_free_extents(struct super_block *sb,
 	int i;
 	int err = 0;
 
-	/* Mapping the allocation file may lock the extent tree */
-	WARN_ON(mutex_is_locked(&HFSPLUS_SB(sb)->ext_tree->tree_lock));
-
 	hfsplus_dump_extent(extent);
 	for (i = 0; i < 8; extent++, i++) {
 		count = be32_to_cpu(extent->block_count);
-- 
2.48.1

Re: [PATCH] hfsplus: remove mutex_lock check in hfsplus_free_extents

[Viacheslav Dubeyko]

On Sun, 2025-05-11 at 05:08 -0600, Yangtao Li wrote:
> Syzbot reported an issue in hfsplus subsystem:
> 

Could you please add the issue into the issues list [1] (if it is not
there yet) and to assign it on yourself?

> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 4400 at fs/hfsplus/extents.c:346
> 	hfsplus_free_extents+0x700/0xad0
> Call Trace:
> <TASK>
> hfsplus_file_truncate+0x768/0xbb0 fs/hfsplus/extents.c:606
> hfsplus_write_begin+0xc2/0xd0 fs/hfsplus/inode.c:56
> cont_expand_zero fs/buffer.c:2383 [inline]
> cont_write_begin+0x2cf/0x860 fs/buffer.c:2446
> hfsplus_write_begin+0x86/0xd0 fs/hfsplus/inode.c:52
> generic_cont_expand_simple+0x151/0x250 fs/buffer.c:2347
> hfsplus_setattr+0x168/0x280 fs/hfsplus/inode.c:263
> notify_change+0xe38/0x10f0 fs/attr.c:420
> do_truncate+0x1fb/0x2e0 fs/open.c:65
> do_sys_ftruncate+0x2eb/0x380 fs/open.c:193
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> To avoid deadlock, Commit 31651c607151 ("hfsplus: avoid deadlock
> on file truncation") unlock extree before hfsplus_free_extents(),
> and add check wheather extree is locked in hfsplus_free_extents().
> 
> However, when operations such as hfsplus_file_release,
> hfsplus_setattr, hfsplus_unlink, and hfsplus_unlink are executed
> concurrently in different files, it is very likely to trigger the
> WARN_ON, which will lead syzbot and xfstest to consider it as an
> abnormality.
> 

Which particular xfstests' test-case(s) triggers the issue? Do we have
the easy reproducing path of it? How can I check the fix, finally?

> Since we already have alloc_mutex to protect alloc file modify,
> let's remove mutex_lock check.
> 

I don't think that I follow the point. The two mutexes are namely the
basis for potential deadlocks. Currently, I am not sure that we are
fixing the issue. Probably, we are trying to hide the symptoms of the
real issue without the clear understanding what is going wrong. I would
like to hear the explanation how the issue is happening and why the
warning removal can help here.

> Fixes: 31651c607151f ("hfsplus: avoid deadlock on file truncation")
> Reported-by: syzbot+8c0bc9f818702ff75b76@syzkaller.appspotmail.com
> Closes:
> https://lore.kernel.org/all/00000000000057fa4605ef101c4c@google.com/
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/hfsplus/extents.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index a6d61685ae79..b1699b3c246a 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -342,9 +342,6 @@ static int hfsplus_free_extents(struct
> super_block *sb,
>  	int i;
>  	int err = 0;
>  
> -	/* Mapping the allocation file may lock the extent tree */
> -	WARN_ON(mutex_is_locked(&HFSPLUS_SB(sb)->ext_tree-
> >tree_lock));
> -

I am not sure that it's the good idea to remove any warning because,
probably, we could not understand the real reason of the issue and we
simply trying to hind the symptoms of something more serious.

Current explanation doesn't sound reasonably well to me. I am not
convinced yet that it is proper fix and we see the reason of the issue.
I would like to hear more clear justification that we have to remove
this check.

Thanks,
Slava. 

>  	hfsplus_dump_extent(extent);
>  	for (i = 0; i < 8; extent++, i++) {
>  		count = be32_to_cpu(extent->block_count);

[1] https://github.com/hfs-linux-kernel/hfs-linux-kernel/issues

回复: [PATCH] hfsplus: remove mutex_lock check in hfsplus_free_extents

[李扬韬]

Hi Slava,

> Which particular xfstests' test-case(s) triggers the issue? Do we have the easy reproducing path of it? How can I check the fix, finally?

generic/013 triggers the issue. Here is the reproducing path.

[Origin]

We got fsck error, reason is the same as [1].

[1] https://lore.kernel.org/all/20250430001211.1912533-1-slava@dubeyko.com/

root@ubuntu:/home/ubuntu/xfstests-dev# ./check generic/013
FSTYP         -- hfsplus
PLATFORM      -- Linux/x86_64 ubuntu 6.15.0-rc4-00055-g71bfd66b8583-dirty #421 SMP PREEMPT_DYNAMIC Fri May 23 18:30:10 CST 2025
MKFS_OPTIONS  -- /dev/nvme1n1
MOUNT_OPTIONS -- /dev/nvme1n1 /mnt/scratch

generic/013 35s ... [  380.286618] hfsplus: xattr exists yet
[  382.410297] hfsplus: xattr exists yet
[  383.872844] hfsplus: cannot replace xattr
[  385.802529] hfsplus: cannot replace xattr
[  393.125897] hfsplus: xattr exists yet
[  396.222921] hfsplus: cannot replace xattr
[  399.084012] hfsplus: cannot replace xattr
[  403.233816] hfsplus: cannot replace xattr
_check_generic_filesystem: filesystem on /dev/nvme0n1 is inconsistent
(see /home/ubuntu/xfstests-dev/results//generic/013.full for details)
_check_dmesg: something found in dmesg (see /home/ubuntu/xfstests-dev/results//generic/013.dmesg)

Ran: generic/013
Failures: generic/013
Failed 1 of 1 tests

[w/ bnode patch]

The fsck error is related to the node not being cleared, which may be related to the implementation of the fsck tool. 
We can continue to discuss this in the previous email. For this, we can ignore it and continue the analysis based on the bnode patch.

diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 079ea80534f7..f2424acd3636 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -633,7 +633,7 @@ void hfs_bnode_put(struct hfs_bnode *node)
                if (test_bit(HFS_BNODE_DELETED, &node->flags)) {
                        hfs_bnode_unhash(node);
                        spin_unlock(&tree->hash_lock);
-                       if (hfs_bnode_need_zeroout(tree))
+               //      if (hfs_bnode_need_zeroout(tree))
                                hfs_bnode_clear(node, 0, tree->node_size);
                        hfs_bmap_free(node);
                        hfs_bnode_free(node);

After apply bnode patch. We got error from dmesg, which warn at fs/hfsplus/extents.c:346.

root@ubuntu:/home/ubuntu/xfstests-dev# ./check generic/013
FSTYP         -- hfsplus
PLATFORM      -- Linux/x86_64 ubuntu 6.15.0-rc4-00055-g71bfd66b8583-dirty #422 SMP PREEMPT_DYNAMIC Sun May 25 22:37:55 CST 2025
MKFS_OPTIONS  -- /dev/nvme1n1
MOUNT_OPTIONS -- /dev/nvme1n1 /mnt/scratch

generic/013 35s ... [  236.356697] hfsplus: xattr exists yet
[  238.288269] hfsplus: xattr exists yet
[  240.673488] hfsplus: cannot replace xattr
[  242.133163] hfsplus: xattr exists yet
[  242.172538] hfsplus: xattr exists yet
[  243.702797] hfsplus: xattr exists yet
[  245.943067] hfsplus: xattr exists yet
[  249.502186] hfsplus: cannot replace xattr
[  252.544517] hfsplus: xattr exists yet
[  253.538462] hfsplus: cannot replace xattr
[  263.456784] hfsplus: cannot replace xattr
_check_dmesg: something found in dmesg (see /home/ubuntu/xfstests-dev/results//generic/013.dmesg)

Ran: generic/013
Failures: generic/013
Failed 1 of 1 tests

# demsg
[  225.975852] run fstests generic/013 at 2025-05-25 14:42:11
[  231.718234] ------------[ cut here ]------------
[  231.718677] WARNING: CPU: 3 PID: 1091 at fs/hfsplus/extents.c:346 hfsplus_free_extents+0xfc/0x110
[  231.719117] Modules linked in:
[  231.719895] CPU: 3 UID: 0 PID: 1091 Comm: fsstress Not tainted 6.15.0-rc4-00055-g71bfd66b8583-dirty #422 PREEMPT(voluntary)
[  231.719996] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[  231.720170] RIP: 0010:hfsplus_free_extents+0xfc/0x110
[  231.720383] Code: 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 48 c7 c7 4d 62 46 b2 e8 b5 58 cf ff eb 95 48 c7 c7 4d 62 46 b2 e8 a7 58 cf ff eb cd 90 <0f> 0b 90 e9 30 ff ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90
[  231.720492] RSP: 0018:ffffaaa9813bbd28 EFLAGS: 00000202
[  231.720563] RAX: ffff995582666701 RBX: 00000000000000ed RCX: 00000000000001b5
[  231.720592] RDX: 00000000000000ed RSI: ffff9955818e8ad8 RDI: ffff995588c80048
[  231.720617] RBP: ffff9955818e8ad8 R08: 0000000000000002 R09: ffffaaa9813bbcda
[  231.720641] R10: ffff995585cfdb90 R11: 0000000000000002 R12: 0000000000000000
[  231.720672] R13: 00000000000001b5 R14: 00000000000000c8 R15: ffff995587f40800
[  231.720778] FS:  00007f81e3bd8740(0000) GS:ffff99564ac46000(0000) knlGS:0000000000000000
[  231.720813] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  231.720838] CR2: 00005634430a6108 CR3: 00000000147d5000 CR4: 00000000000006f0
[  231.720960] Call Trace:
[  231.721831]  <TASK>
[  231.722111]  hfsplus_file_truncate+0x2b6/0x3e0
[  231.722222]  hfsplus_delete_inode+0x54/0x70
[  231.722325]  hfsplus_unlink+0x17f/0x1c0
[  231.722384]  ? security_inode_permission+0x23/0x40
[  231.722415]  vfs_unlink+0x110/0x2b0
[  231.722442]  do_unlinkat+0x251/0x2c0
[  231.722471]  __x64_sys_unlink+0x1c/0x30
[  231.722491]  do_syscall_64+0x9e/0x190
[  231.722551]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[  231.722726] RIP: 0033:0x7f81e3cf740b
[  231.723023] Code: 30 ff ff ff e9 74 fd ff ff e8 a1 ba 01 00 90 f3 0f 1e fa b8 5f 00 00 00 0f 05 c3 0f 1f 40 00 f3 0f 1e fa b8 57 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 d9 69 0e 00 f7 d8
[  231.723047] RSP: 002b:00007ffe97ed66b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000057
[  231.723089] RAX: ffffffffffffffda RBX: 0000000000000035 RCX: 00007f81e3cf740b
[  231.723104] RDX: 0000000000000000 RSI: 00007ffe97ed6690 RDI: 00005634430875c0
[  231.723116] RBP: 00007ffe97ed6830 R08: 000000000000ffff R09: 0000000000000000
[  231.723128] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe97ed66d0
[  231.723141] R13: 8f5c28f5c28f5c29 R14: 00007ffe97ed68a0 R15: 000056341fe3c7c0
[  231.723219]  </TASK>
[  231.723427] ---[ end trace 0000000000000000 ]---
[  233.296305] ------------[ cut here ]------------

[w/ bnode &this patch]

Test pass without any error.

root@ubuntu:/home/ubuntu/xfstests-dev# ./check generic/013
FSTYP         -- hfsplus
PLATFORM      -- Linux/x86_64 ubuntu 6.15.0-rc4-00055-g71bfd66b8583-dirty #423 SMP PREEMPT_DYNAMIC Sun May 25 22:54:51 CST 2025
MKFS_OPTIONS  -- /dev/nvme1n1
MOUNT_OPTIONS -- /dev/nvme1n1 /mnt/scratch

generic/013 35s ... [  106.018643] hfsplus: xattr exists yet
[  110.155138] hfsplus: cannot replace xattr
[  112.061738] hfsplus: cannot replace xattr
[  113.215120] hfsplus: cannot replace xattr
[  118.308974] hfsplus: xattr exists yet
[  133.279630] hfsplus: cannot replace xattr
[  134.581764] hfsplus: cannot replace xattr
[  135.557120] hfsplus: xattr exists yet
 46s
Ran: generic/013
Passed all 1 tests

> I don't think that I follow the point. The two mutexes are namely the basis for potential deadlocks. Currently, I am not sure that we are fixing the issue. Probably, we are trying to hide the symptoms of the real issue without the clear understanding what is going wrong. I would like to hear the explanation how the issue is happening and why the warning removal can help here.

I don't know if the above description is clear enough. Actually, this warning is not helpful at all. 
The comment above this warning also describes one of the easy triggering situations, which can easily trigger and cause xfstest&syzbot to report errors.

> I am not sure that it's the good idea to remove any warning because, probably, we could not understand the real reason of the issue and we simply trying to hind the symptoms of something more serious.
> 
> Current explanation doesn't sound reasonably well to me. I am not convinced yet that it is proper fix and we see the reason of the issue.
> I would like to hear more clear justification that we have to remove this check.

If there is indeed an exception or you can point out the problem, then we should fix it. Otherwise, in my opinion, this warning has no purpose and should be removed.

Thx,
Yangtao

Re: 回复: [PATCH] hfsplus: remove mutex_lock check in hfsplus_free_extents

[Viacheslav Dubeyko]

On Sun, 2025-05-25 at 15:03 +0000, 李扬韬 wrote:
> Hi Slava,
> 
> > Which particular xfstests' test-case(s) triggers the issue? Do we
> > have the easy reproducing path of it? How can I check the fix,
> > finally?
> 
> generic/013 triggers the issue. Here is the reproducing path.
> 

Great! Could you please add generic/013 issues analysis in the patch
comment? I mean the dmesg output here. 

> [Origin]
> 
> We got fsck error, reason is the same as [1].
> 
> [1]
> https://lore.kernel.org/all/20250430001211.1912533-1-slava@dubeyko.com/
> 

Mentioning [1] could be confusing because it was HFS related fix. But
we are discussion HFS+ here.

> root@ubuntu:/home/ubuntu/xfstests-dev# ./check generic/013
> FSTYP         -- hfsplus
> PLATFORM      -- Linux/x86_64 ubuntu 6.15.0-rc4-00055-g71bfd66b8583-
> dirty #421 SMP PREEMPT_DYNAMIC Fri May 23 18:30:10 CST 2025
> MKFS_OPTIONS  -- /dev/nvme1n1
> MOUNT_OPTIONS -- /dev/nvme1n1 /mnt/scratch
> 
> generic/013 35s ... [  380.286618] hfsplus: xattr exists yet
> [  382.410297] hfsplus: xattr exists yet
> [  383.872844] hfsplus: cannot replace xattr
> [  385.802529] hfsplus: cannot replace xattr
> [  393.125897] hfsplus: xattr exists yet
> [  396.222921] hfsplus: cannot replace xattr
> [  399.084012] hfsplus: cannot replace xattr
> [  403.233816] hfsplus: cannot replace xattr
> _check_generic_filesystem: filesystem on /dev/nvme0n1 is inconsistent
> (see /home/ubuntu/xfstests-dev/results//generic/013.full for details)
> _check_dmesg: something found in dmesg (see /home/ubuntu/xfstests-
> dev/results//generic/013.dmesg)
> 
> Ran: generic/013
> Failures: generic/013
> Failed 1 of 1 tests
> 
> [w/ bnode patch]
> 
> The fsck error is related to the node not being cleared, which may be
> related to the implementation of the fsck tool. 
> We can continue to discuss this in the previous email. For this, we
> can ignore it and continue the analysis based on the bnode patch.

Let's discuss this issue in independent thread. I assume you would like
to share the patch with the fix. Do you mean that
hfs_bnode_need_zeroout() works not completely correct? Because, I had
impression that HFS+ makes clearing of deleted nodes.

> 
> diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
> index 079ea80534f7..f2424acd3636 100644
> --- a/fs/hfsplus/bnode.c
> +++ b/fs/hfsplus/bnode.c
> @@ -633,7 +633,7 @@ void hfs_bnode_put(struct hfs_bnode *node)
>                 if (test_bit(HFS_BNODE_DELETED, &node->flags)) {
>                         hfs_bnode_unhash(node);
>                         spin_unlock(&tree->hash_lock);
> -                       if (hfs_bnode_need_zeroout(tree))
> +               //      if (hfs_bnode_need_zeroout(tree))
>                                 hfs_bnode_clear(node, 0, tree-
> >node_size);
>                         hfs_bmap_free(node);
>                         hfs_bnode_free(node);
> 
> After apply bnode patch. We got error from dmesg, which warn at
> fs/hfsplus/extents.c:346.
> 
> root@ubuntu:/home/ubuntu/xfstests-dev# ./check generic/013
> FSTYP         -- hfsplus
> PLATFORM      -- Linux/x86_64 ubuntu 6.15.0-rc4-00055-g71bfd66b8583-
> dirty #422 SMP PREEMPT_DYNAMIC Sun May 25 22:37:55 CST 2025
> MKFS_OPTIONS  -- /dev/nvme1n1
> MOUNT_OPTIONS -- /dev/nvme1n1 /mnt/scratch
> 
> generic/013 35s ... [  236.356697] hfsplus: xattr exists yet
> [  238.288269] hfsplus: xattr exists yet
> [  240.673488] hfsplus: cannot replace xattr
> [  242.133163] hfsplus: xattr exists yet
> [  242.172538] hfsplus: xattr exists yet
> [  243.702797] hfsplus: xattr exists yet
> [  245.943067] hfsplus: xattr exists yet
> [  249.502186] hfsplus: cannot replace xattr
> [  252.544517] hfsplus: xattr exists yet
> [  253.538462] hfsplus: cannot replace xattr
> [  263.456784] hfsplus: cannot replace xattr
> _check_dmesg: something found in dmesg (see /home/ubuntu/xfstests-
> dev/results//generic/013.dmesg)
> 
> Ran: generic/013
> Failures: generic/013
> Failed 1 of 1 tests
> 
> # demsg
> [  225.975852] run fstests generic/013 at 2025-05-25 14:42:11
> [  231.718234] ------------[ cut here ]------------
> [  231.718677] WARNING: CPU: 3 PID: 1091 at fs/hfsplus/extents.c:346
> hfsplus_free_extents+0xfc/0x110
> [  231.719117] Modules linked in:
> [  231.719895] CPU: 3 UID: 0 PID: 1091 Comm: fsstress Not tainted
> 6.15.0-rc4-00055-g71bfd66b8583-dirty #422 PREEMPT(voluntary)
> [  231.719996] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [  231.720170] RIP: 0010:hfsplus_free_extents+0xfc/0x110
> [  231.720383] Code: 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 48 c7 c7
> 4d 62 46 b2 e8 b5 58 cf ff eb 95 48 c7 c7 4d 62 46 b2 e8 a7 58 cf ff
> eb cd 90 <0f> 0b 90 e9 30 ff ff ff 66 66 2e 0f 1f 84 00 00 00 00 00
> 90 90 90
> [  231.720492] RSP: 0018:ffffaaa9813bbd28 EFLAGS: 00000202
> [  231.720563] RAX: ffff995582666701 RBX: 00000000000000ed RCX:
> 00000000000001b5
> [  231.720592] RDX: 00000000000000ed RSI: ffff9955818e8ad8 RDI:
> ffff995588c80048
> [  231.720617] RBP: ffff9955818e8ad8 R08: 0000000000000002 R09:
> ffffaaa9813bbcda
> [  231.720641] R10: ffff995585cfdb90 R11: 0000000000000002 R12:
> 0000000000000000
> [  231.720672] R13: 00000000000001b5 R14: 00000000000000c8 R15:
> ffff995587f40800
> [  231.720778] FS:  00007f81e3bd8740(0000) GS:ffff99564ac46000(0000)
> knlGS:0000000000000000
> [  231.720813] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  231.720838] CR2: 00005634430a6108 CR3: 00000000147d5000 CR4:
> 00000000000006f0
> [  231.720960] Call Trace:
> [  231.721831]  <TASK>
> [  231.722111]  hfsplus_file_truncate+0x2b6/0x3e0
> [  231.722222]  hfsplus_delete_inode+0x54/0x70
> [  231.722325]  hfsplus_unlink+0x17f/0x1c0
> [  231.722384]  ? security_inode_permission+0x23/0x40
> [  231.722415]  vfs_unlink+0x110/0x2b0
> [  231.722442]  do_unlinkat+0x251/0x2c0
> [  231.722471]  __x64_sys_unlink+0x1c/0x30
> [  231.722491]  do_syscall_64+0x9e/0x190
> [  231.722551]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> [  231.722726] RIP: 0033:0x7f81e3cf740b
> [  231.723023] Code: 30 ff ff ff e9 74 fd ff ff e8 a1 ba 01 00 90 f3
> 0f 1e fa b8 5f 00 00 00 0f 05 c3 0f 1f 40 00 f3 0f 1e fa b8 57 00 00
> 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 d9 69 0e
> 00 f7 d8
> [  231.723047] RSP: 002b:00007ffe97ed66b8 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000057
> [  231.723089] RAX: ffffffffffffffda RBX: 0000000000000035 RCX:
> 00007f81e3cf740b
> [  231.723104] RDX: 0000000000000000 RSI: 00007ffe97ed6690 RDI:
> 00005634430875c0
> [  231.723116] RBP: 00007ffe97ed6830 R08: 000000000000ffff R09:
> 0000000000000000
> [  231.723128] R10: 0000000000000000 R11: 0000000000000246 R12:
> 00007ffe97ed66d0
> [  231.723141] R13: 8f5c28f5c28f5c29 R14: 00007ffe97ed68a0 R15:
> 000056341fe3c7c0
> [  231.723219]  </TASK>
> [  231.723427] ---[ end trace 0000000000000000 ]---
> [  233.296305] ------------[ cut here ]------------
> 
> [w/ bnode &this patch]
> 
> Test pass without any error.
> 
> root@ubuntu:/home/ubuntu/xfstests-dev# ./check generic/013
> FSTYP         -- hfsplus
> PLATFORM      -- Linux/x86_64 ubuntu 6.15.0-rc4-00055-g71bfd66b8583-
> dirty #423 SMP PREEMPT_DYNAMIC Sun May 25 22:54:51 CST 2025
> MKFS_OPTIONS  -- /dev/nvme1n1
> MOUNT_OPTIONS -- /dev/nvme1n1 /mnt/scratch
> 
> generic/013 35s ... [  106.018643] hfsplus: xattr exists yet
> [  110.155138] hfsplus: cannot replace xattr
> [  112.061738] hfsplus: cannot replace xattr
> [  113.215120] hfsplus: cannot replace xattr
> [  118.308974] hfsplus: xattr exists yet
> [  133.279630] hfsplus: cannot replace xattr
> [  134.581764] hfsplus: cannot replace xattr
> [  135.557120] hfsplus: xattr exists yet
>  46s
> Ran: generic/013
> Passed all 1 tests
> 
> > I don't think that I follow the point. The two mutexes are namely
> > the basis for potential deadlocks. Currently, I am not sure that we
> > are fixing the issue. Probably, we are trying to hide the symptoms
> > of the real issue without the clear understanding what is going
> > wrong. I would like to hear the explanation how the issue is
> > happening and why the warning removal can help here.
> 
> I don't know if the above description is clear enough. Actually, this
> warning is not helpful at all. 
> The comment above this warning also describes one of the easy
> triggering situations, which can easily trigger and cause
> xfstest&syzbot to report errors.
> 

I see your point. We need accurately explain here that several threads
could try to lock the shared extents tree. And warning can be triggered
in one thread when another thread has locked the tree. This is the
wrong behavior of the code and we need to remove the warning.

Could you please rework the patch comment by means of adding precise
explanation of this?  

> > I am not sure that it's the good idea to remove any warning
> > because, probably, we could not understand the real reason of the
> > issue and we simply trying to hind the symptoms of something more
> > serious.
> > 
> > Current explanation doesn't sound reasonably well to me. I am not
> > convinced yet that it is proper fix and we see the reason of the
> > issue.
> > I would like to hear more clear justification that we have to
> > remove this check.
> 
> If there is indeed an exception or you can point out the problem,
> then we should fix it. Otherwise, in my opinion, this warning has no
> purpose and should be removed.

We have a lot of problems in the code and good warnings make sense. The
intentions of this warning was to prevent wrong using of locks. But it
was missed that multiple threads can try to lock the shared extents
tree. So, yes, it makes sense to remove this particular warning but,
potentially, we still could have issues in the code. However, we need
to explain why this warning works in wrong way in really precise
manner. :)

Thanks,
Slava.

回复: 回复: [PATCH] hfsplus: remove mutex_lock check in hfsplus_free_extents

[李扬韬]

Hi Slava,

> Could you please rework the patch comment by means of adding precise explanation of this?  

I don't quite get your point. Would you mind adding a comment or editing it here? : ) 

Thx,
Yangtao

Re: 回复: 回复: [PATCH] hfsplus: remove mutex_lock check in hfsplus_free_extents

[Viacheslav Dubeyko]

On Wed, 2025-05-28 at 17:01 +0000, 李扬韬 wrote:
> Hi Slava,
> 
> > Could you please rework the patch comment by means of adding
> > precise explanation of this?  
> 
> I don't quite get your point. Would you mind adding a comment or
> editing it here? : ) 
> 

I see your point. We need accurately explain here that several threads
could try to lock the shared extents tree. And warning can be triggered
in one thread when another thread has locked the tree. This is the
wrong behavior of the code and we need to remove the warning.

This is what I meant here.

Thanks,
Slava.