Message-ID: <13f85ee0265f7a41ef99f151c9a4185f9d9ab0a0.camel@dubeyko.com>
Date: Tue, 27 May 2025 14:49:30 -0700
From: Viacheslav Dubeyko <slava@...eyko.com>
To: 李扬韬 <frank.li@...o.com>, 
 "glaubitz@...sik.fu-berlin.de"	 <glaubitz@...sik.fu-berlin.de>, Andrew
 Morton <akpm@...ux-foundation.org>,  "Ernesto A."
 Fernández	 <ernesto.mnd.fernandez@...il.com>
Cc: "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>, 
 "linux-kernel@...r.kernel.org"
	 <linux-kernel@...r.kernel.org>, 
 "syzbot+8c0bc9f818702ff75b76@...kaller.appspotmail.com"
	 <syzbot+8c0bc9f818702ff75b76@...kaller.appspotmail.com>, 
	Slava.Dubeyko@....com
Subject: Re: 回复: [PATCH] hfsplus: remove
 mutex_lock check in hfsplus_free_extents

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@...ntu:/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@...ntu:/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@...ntu:/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.

Powered by blists - more mailing lists