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

[Song jeonghak]

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.

Regards,
Song jeonghak