Re: [PATCH] hfsplus: free failed b-tree nodes in hfs_bnode_put()

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Mon, 2026-11-02 at 01:29 +0900, Song jeonghak wrote:
> Hi Markus,
> 
> (Resending as plain text; my previous reply was rejected by the lists
> because it contained an HTML part. Sorry for the duplicate.)
> 
> > How do you think about to avoid a bit of duplicate source code?
> 
> Thanks for looking at the patch. The two branches share the
> unhash/unlock/free steps, but they handle different situations:
> 
>  - HFS_BNODE_DELETED is only set by hfs_bnode_unlink(), when the node
>    is removed from the tree. Its slot becomes unused, so
>    hfs_bnode_put() also clears its bit in the allocation map
>    (hfs_bmap_free()) and, if HFSPLUS_VOL_UNUSED_NODE_FIX is set,
>    zeroes it (hfs_bnode_clear()).
> 
>  - HFS_BNODE_ERROR means the node failed to load. It is still part of
>    the on-disk tree; only the in-memory copy is dropped. Its map bit
>    must stay set, otherwise the allocator could hand out a node that
>    is still linked, and its on-disk contents should not be
>    overwritten. In my reproduction the failing node is the header
>    node (node 0), which must always stay allocated. From reading the
>    code, hfs_bmap_free() would hit its BUG_ON(!node->this) for it;
>    I have not run that.
> 
> I looked at how other put functions that take the lock with
> atomic_dec_and_lock()/refcount_dec_and_lock() deal with this, and
> both forms are used. ceph_put_snap_realm() in fs/ceph/snap.c repeats
> the unlock in each branch, while fsnotify_put_mark() in
> fs/notify/mark.c records the difference in a flag and does the common
> part once. Following the second style, the two branches could become:
> 
> 		bool deleted = test_bit(HFS_BNODE_DELETED, &node-
> >flags);
> 
> 		if (deleted || test_bit(HFS_BNODE_ERROR, &node-
> >flags)) {
> 			hfs_bnode_unhash(node);
> 			spin_unlock(&tree->hash_lock);
> 			if (deleted) {
> 				if (hfs_bnode_need_zeroout(tree))
> 					hfs_bnode_clear(node, 0,
> tree->node_size);
> 				hfs_bmap_free(node);
> 			}
> 			hfs_bnode_free(node);
> 			return;
> 		}
> 		spin_unlock(&tree->hash_lock);
> 
> I am fine with either form and will use whichever the maintainers
> prefer in v2.
> 
> While looking into this I found a mistake in my changelog. It says a
> failed node "has no b-tree map bit to release and no contents to zero
> out". That is wrong: the bit is set and the contents are on disk.
> They
> are left alone on purpose, as described above. I will correct this in
> v2.
> 
> 

Somehow I cannot find the patch in my email client. Please, double
check that you send the patch in plain text and, please, ping me if I
don't answer on v2 of the patch.

Thanks,
Slava.