Re: [PATCH v4 1/2] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[Nguyen Ngoc Thang]

Hi Slava,

Thanks a lot for the detailed review, this caught a real bug. Replies
inline, v5 diff at the bottom.

> static inline?
>
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < 8; ext++, i++)
>
> I am introducing the special constant for the 8 extents of the fork in
> HFS+ iomap patchset. How can we handle this? Because I would like to
> see the named constant instead of hardcoded value.

Added HFSPLUS_EXTENT_COUNT in hfsplus_fs.h and used it here. I don't
know what name you're using in the iomap patchset -- happy to rename
to match once you let me know, so we don't end up with two constants
for the same thing when that series lands.

I kept hfsplus_ext_fork_full() itself as plain "static", not "static
inline": it's not a single-line wrapper, and the compiler already
inlines small static functions like this at -O2, so an explicit
"inline" in a .c file (as opposed to a header) doesn't buy us
anything here. I did make the new one-line is_extents_btree() helper
below "static inline", since that one really is just a trivial
predicate wrapper.

> The comment is not fully correct. We should not be here for the case of
> Extents Overflow file because there is no forks other than in
> superblock. It's not about the lock issue. We simply should not be here
> at all.

You're right, fixed. The comment now says: "Per the HFS+ format, the
extents overflow file is fully described by its own eight fork
extents and can never have an overflow extent of its own recorded in
the tree; this function should never legitimately be reached for it."

> Maybe, we need to introduce something like is_extents_btree() method?
> What do you think?

Done -- added is_extents_btree() and used it at all three call sites
in this patch (hfsplus_ext_read_extent(), the fork-full check in
hfsplus_file_extend(), and the insert_extent backstop).

> It looks like complicated condition and it deserves a static inline
> function, from my point of view.

Extracted into hfsplus_ext_file_needs_contig_grow().

> Maybe, instead of this long comment we need to introduce a dedicated
> method for processing Extents Overflow file allocation case?

Extracted into hfsplus_ext_file_grow(), replacing the inline comment
with a doc comment on the function itself.

> Maybe, I am missing something here. But goal + 1 sounds like we request
> to allocate only one block. Is it correct? If yes, why only one block?
> Usually, we need to try to allocate a clumpSize.

You're right, and this was an actual bug, not just a readability
issue. I traced hfsplus_block_allocate(): the `size` argument bounds
both where the scan stops *and* the returned run length via
`len = min(size - start, len)`. With `size = goal + 1` and
`start = goal`, that clamps `len` to 1 no matter what clump_blocks
was, so this path only ever allocated a single block. Fixed to use
`goal + *len` (the original clump_blocks) as the bound instead, in
hfsplus_ext_file_grow(). That keeps the "must start exactly at goal"
rejection (still checked via `start != goal` by the caller) while
allowing a full clump to be granted when the space is there.

> Can we be here at all? If start != goal, then we cannot allocate at
> all. And we can be here only if we have empty slot it the fork. Am I
> right?

The other way around: this branch is taken when
hfsplus_ext_fork_full() returns true, i.e. there is *no* free slot
left in the fork. If there is a free slot, we fall through to the
regular allocate-anywhere path and hfsplus_add_extent() just records
it in that slot -- no special-casing needed. I renamed the condition
to hfsplus_ext_file_needs_contig_grow() to make that unambiguous.

> checkpatch.pl --strict flags one alignment style issue [...]
> continuation should align with the open paren — cosmetic only

Fixed.

Thanks again for catching the goal+1 bug in particular -- v5 below.

---
Changes since v4:
 - Fix hfsplus_file_extend() requesting only 1 block instead of a
   full clump when growing the extents overflow file's fork
   (goal + 1 -> goal + len in the block_allocate() call).
 - Add HFSPLUS_EXTENT_COUNT instead of hardcoding 8.
 - Add is_extents_btree() instead of repeating the i_ino comparison.
 - Extract hfsplus_ext_file_needs_contig_grow() and
   hfsplus_ext_file_grow() out of hfsplus_file_extend().
 - Fix comment on the HFSPLUS_EXT_CNID guard in
   hfsplus_ext_read_extent() to state the real reason.
 - Fix checkpatch --strict alignment nit on pr_err() continuation.
(all per Slava's review)

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index eb7c11524d18..f3a4b8fd567f 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -84,6 +84,23 @@ static u32 hfsplus_ext_lastblock(struct hfsplus_extent *ext)
 	return be32_to_cpu(ext->start_block) + be32_to_cpu(ext->block_count);
 }

+/* True if the inode is the extents overflow file's own inode */
+static inline bool is_extents_btree(struct inode *inode)
+{
+	return inode->i_ino == HFSPLUS_EXT_CNID;
+}
+
+/* True if all extents of a fork are in use (no free slot left) */
+static bool hfsplus_ext_fork_full(struct hfsplus_extent *ext)
+{
+	int i;
+
+	for (i = 0; i < HFSPLUS_EXTENT_COUNT; ext++, i++)
+		if (!ext->block_count)
+			return false;
+	return true;
+}
+
 static int __hfsplus_ext_write_extent(struct inode *inode,
 		struct hfs_find_data *fd)
 {
@@ -217,6 +234,15 @@ static int hfsplus_ext_read_extent(struct inode *inode, u32 block)
 	    block < hip->cached_start + hip->cached_blocks)
 		return 0;

+	/*
+	 * Per the HFS+ format, the extents overflow file is fully
+	 * described by its own eight fork extents and can never have an
+	 * overflow extent of its own recorded in the tree; this function
+	 * should never legitimately be reached for it.
+	 */
+	if (is_extents_btree(inode))
+		return -ENOSPC;
+
 	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
 	if (!res) {
 		res = __hfsplus_ext_cache_extent(&fd, inode, block);
@@ -392,6 +418,34 @@ static int hfsplus_free_extents(struct super_block *sb,
 	}
 }

+/*
+ * True when growing the extents overflow file's own inode needs the
+ * contiguous-only special case below: its fork's eight extents are
+ * all in use, so there is no free slot left to record a new extent
+ * for it.
+ */
+static bool hfsplus_ext_file_needs_contig_grow(struct inode *inode,
+					       struct hfsplus_inode_info *hip)
+{
+	return is_extents_btree(inode) &&
+	       hip->alloc_blocks == hip->first_blocks &&
+	       hfsplus_ext_fork_full(hip->first_extents);
+}
+
+/*
+ * Allocate blocks to grow the extents overflow file itself once its
+ * fork is full (see hfsplus_ext_file_needs_contig_grow()). Per the
+ * HFS+ format this file can never record an overflow extent of its
+ * own, so the only way to grow it further is a contiguous extension
+ * of the last extent already in the fork: search for up to *len free
+ * blocks starting exactly at goal, and return a start block other
+ * than goal if the block at goal itself isn't free.
+ */
+static u32 hfsplus_ext_file_grow(struct super_block *sb, u32 goal, u32 *len)
+{
+	return hfsplus_block_allocate(sb, goal + *len, goal, len);
+}
+
 int hfsplus_free_fork(struct super_block *sb, u32 cnid,
 		struct hfsplus_fork_raw *fork, int type)
 {
@@ -465,13 +519,21 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	}

 	len = hip->clump_blocks;
-	start = hfsplus_block_allocate(sb, sbi->total_blocks, goal, &len);
-	if (start >= sbi->total_blocks) {
-		start = hfsplus_block_allocate(sb, goal, 0, &len);
-		if (start >= goal) {
+	if (hfsplus_ext_file_needs_contig_grow(inode, hip)) {
+		start = hfsplus_ext_file_grow(sb, goal, &len);
+		if (start != goal) {
 			res = -ENOSPC;
 			goto out;
 		}
+	} else {
+		start = hfsplus_block_allocate(sb, sbi->total_blocks, goal, &len);
+		if (start >= sbi->total_blocks) {
+			start = hfsplus_block_allocate(sb, goal, 0, &len);
+			if (start >= goal) {
+				res = -ENOSPC;
+				goto out;
+			}
+		}
 	}

 	if (zeroout) {
@@ -526,6 +588,20 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 	return res;

 insert_extent:
+	/*
+	 * The fork-full precheck above keeps the extents overflow file's
+	 * own inode from ever landing here with blocks already allocated;
+	 * this is a backstop, so still free what was allocated rather
+	 * than leak it.
+	 */
+	if (is_extents_btree(inode)) {
+		if (hfsplus_block_free(sb, start, len))
+			pr_err("can't free extent: start %u, count %u\n",
+			       start, len);
+		res = -ENOSPC;
+		goto out;
+	}
+
 	hfs_dbg("insert new extent\n");
 	res = hfsplus_ext_write_extent_locked(inode);
 	if (res)
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 1e5b58e6a13f..7c53832f2784 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -24,6 +24,9 @@
 #define HFSPLUS_TYPE_DATA 0x00
 #define HFSPLUS_TYPE_RSRC 0xFF

+/* Number of extent slots in a fork (hfsplus_extent_rec, hfs_common.h) */
+#define HFSPLUS_EXTENT_COUNT 8
+
 typedef int (*btree_keycmp)(const hfsplus_btree_key *,
 		const hfsplus_btree_key *);

--
Thanks,
Nguyen Ngoc Thang