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

[Viacheslav Dubeyko]

On Tue, 2026-09-15 at 21:07 +0700, Nguyen Ngoc Thang wrote:
> 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.

You cannot simply declare the same constant because our patchsets will
conflict. Probably, you need to keep 8 as hardcoded value now. And it
will be good to make the refactoring after my patchset will be in
HFS/HFS+ git tree.

> 
> 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.

I cannot treat as a v5 of the patch because it's not the patch but
simple discussion. And it makes the review process really complicated.
Please, don't mess the discussion with the formal patches.

> 
> ---
> 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 */

This comment is useless because the name is informative enough.

> +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) */

I assume that you are practicing AI assistant a lot. Please, clean upo
useless comments after this stuff. The name of function is informative
enough.

> +static bool hfsplus_ext_fork_full(struct hfsplus_extent *ext)

hfsplus_fork_full()...

If you would like to be sure that fork is not corrupted and it is full,
then you need to analyze the fork structure. Otherwise, it is enough to
check the latest extent in the fork.

> +{
> +	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.
> + */

Comment is longer than the function itself. It is not necessary at all.

> +static bool hfsplus_ext_file_needs_contig_grow(struct inode *inode,
> +					       struct
> hfsplus_inode_info *hip)

is_ext_file_need_grow() ?

> +{
> +	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.
> + */

Ditto.

> +static u32 hfsplus_ext_file_grow(struct super_block *sb, u32 goal,
> u32 *len)
> +{
> +	return hfsplus_block_allocate(sb, goal + *len, goal, len);
> +}

This doesn't make sense at all.

> +
>  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;
> +			}
> +		}

This didn't make the situation better. Probably, all this piece of code
should be one function.

> 
>  	}
> 
>  	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;
> +	}

It's hard to discuss if you are moving discussion out of the code. My
question still the same here. Because I cannot connect your answer with
my question and code.

Thanks,
Slava. 

> +
>  	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