Re: [PATCH v4 2/7] hfsplus: rework hfsplus_get_block() logic

[Darrick J. Wong]

On Mon, Sep 14, 2026 at 04:39:36PM -0700, Viacheslav Dubeyko wrote:
> Split the extent-lookup/allocate logic out of hfsplus_get_block() into
> a new hfsplus_map_extent(), which reports the mapping as
> (dblock, max_blocks, balloc) instead of filling in a buffer_head.
> hfsplus_get_block() becomes a thin wrapper around it for the
> buffer_head-based callers (B-tree metadata, symlinks).
> 
> No functional change to the existing buffer_head path. This is
> preparation for the iomap-based regular file I/O path added in a
> later patch, which will call hfsplus_map_extent() directly.
> 
> cc: Christoph Hellwig <hch@xxxxxx>
> cc: John Paul Adrian Glaubitz <glaubitz@xxxxxxxxxxxxxxxxxxx>
> cc: Yangtao Li <frank.li@xxxxxxxx>
> cc: linux-fsdevel@xxxxxxxxxxxxxxx
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Viacheslav Dubeyko <slava@xxxxxxxxxxx>
> ---
>  fs/hfsplus/extents.c    | 118 +++++++++++++++++++++++++++++-----------
>  fs/hfsplus/hfsplus_fs.h |   2 +
>  2 files changed, 87 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index eb7c11524d18..ffd52ad8867c 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -48,18 +48,29 @@ static void hfsplus_ext_build_key(hfsplus_btree_key *key, u32 cnid,
>  	key->ext.pad = 0;
>  }
>  
> -static u32 hfsplus_ext_find_block(struct hfsplus_extent *ext, u32 off)
> +/*
> + * hfsplus_ext_find_block() - find contiguous sequence of block
> + *
> + * Find the disk allocation block for 'off' within an 8-entry
> + * extent record, and the number of further allocation blocks
> + * that are contiguous with it in the same extent entry.
> + */
> +static u32 hfsplus_ext_find_block(struct hfsplus_extent *ext, u32 off,
> +				 u32 *dblock)

Hmm.  Does this function take an hfs+ extent and a file block offset
within that extent as inputs?  And are its outputs the physical block
number in @dblock, and the block count as the return value?

(Wow, TN1150 is still available!)

It's sorta too bad that this code doesn't just return a mapping
structure with some nice names to make the code easier to understand,
but, eh, whatever.  Old fs code isn't always pretty. :)

This took me a while to understand, but it seems reasonable to me so
Acked-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>

--D


>  {
>  	int i;
>  	u32 count;
>  
> -	for (i = 0; i < 8; ext++, i++) {
> +	for (i = 0; i < HFSPLUS_FORK_EXTENT_COUNT; ext++, i++) {
>  		count = be32_to_cpu(ext->block_count);
> -		if (off < count)
> -			return be32_to_cpu(ext->start_block) + off;
> +		if (off < count) {
> +			*dblock = be32_to_cpu(ext->start_block) + off;
> +			return count - off;
> +		}
>  		off -= count;
>  	}
>  	/* panic? */
> +	*dblock = 0;
>  	return 0;
>  }
>  
> @@ -68,7 +79,7 @@ static int hfsplus_ext_block_count(struct hfsplus_extent *ext)
>  	int i;
>  	u32 count = 0;
>  
> -	for (i = 0; i < 8; ext++, i++)
> +	for (i = 0; i < HFSPLUS_FORK_EXTENT_COUNT; ext++, i++)
>  		count += be32_to_cpu(ext->block_count);
>  	return count;
>  }
> @@ -225,37 +236,46 @@ static int hfsplus_ext_read_extent(struct inode *inode, u32 block)
>  	return res;
>  }
>  
> -/* Get a block at iblock for inode, possibly allocating if create */
> -int hfsplus_get_block(struct inode *inode, sector_t iblock,
> -		      struct buffer_head *bh_result, int create)
> +/*
> + * hfsplus_map_extent() - find or allocate a sequence of allocation blocks
> + *
> + * Looks up the allocation block at 'ablock' for inode, extending the
> + * file (via hfsplus_file_extend(), in clump_blocks-sized chunks) when
> + * 'create' is set and 'ablock' lies beyond the current allocation.
> + *
> + * On success, *dblock is the disk allocation block backing 'ablock',
> + * and *max_blocks is the number of further allocation blocks that are
> + * contiguous with it (i.e. the remaining length of the extent entry
> + * that contains 'ablock'), which may be smaller than the whole file's
> + * remaining allocation when the fork is fragmented across several
> + * extent entries. If a new extent had to be allocated to satisfy the
> + * request, *balloc (when non-NULL) is set to true.
> + */
> +int hfsplus_map_extent(struct inode *inode, u32 ablock, int create,
> +			u32 *dblock, u32 *max_blocks, bool *balloc)
>  {
> -	struct super_block *sb = inode->i_sb;
> -	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
>  	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
> -	int res = -EIO;
> -	u32 ablock, dblock, mask;
> -	sector_t sector;
> -	int was_dirty = 0;
> +	int was_dirty;
> +	int res;
>  
> -	/* Convert inode block to disk allocation block */
> -	ablock = iblock >> sbi->fs_shift;
> +	if (balloc)
> +		*balloc = false;
>  
> -	if (iblock >= hip->fs_blocks) {
> +	if (ablock >= hip->alloc_blocks) {
>  		if (!create)
> -			return 0;
> -		if (iblock > hip->fs_blocks)
>  			return -EIO;
> -		if (ablock >= hip->alloc_blocks) {
> -			res = hfsplus_file_extend(inode, false);
> -			if (res)
> -				return res;
> -		}
> -	} else
> -		create = 0;
> +		res = hfsplus_file_extend(inode, false);
> +		if (res)
> +			return res;
> +		if (balloc)
> +			*balloc = true;
> +	}
>  
>  	if (ablock < hip->first_blocks) {
> -		dblock = hfsplus_ext_find_block(hip->first_extents, ablock);
> -		goto done;
> +		*max_blocks = hfsplus_ext_find_block(hip->first_extents,
> +						     ablock,
> +						     dblock);
> +		return 0;
>  	}
>  
>  	if (inode->i_ino == HFSPLUS_EXT_CNID)
> @@ -274,11 +294,44 @@ int hfsplus_get_block(struct inode *inode, sector_t iblock,
>  		mutex_unlock(&hip->extents_lock);
>  		return -EIO;
>  	}
> -	dblock = hfsplus_ext_find_block(hip->cached_extents,
> -					ablock - hip->cached_start);
> +	*max_blocks = hfsplus_ext_find_block(hip->cached_extents,
> +					     ablock - hip->cached_start,
> +					     dblock);
>  	mutex_unlock(&hip->extents_lock);
>  
> -done:
> +	if (was_dirty)
> +		mark_inode_dirty(inode);
> +
> +	return 0;
> +}
> +
> +/* Get a block at iblock for inode, possibly allocating if create */
> +int hfsplus_get_block(struct inode *inode, sector_t iblock,
> +		      struct buffer_head *bh_result, int create)
> +{
> +	struct super_block *sb = inode->i_sb;
> +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
> +	u32 ablock, dblock, mask, max_blocks;
> +	sector_t sector;
> +	int res;
> +
> +	/* Convert inode block to disk allocation block */
> +	ablock = iblock >> sbi->fs_shift;
> +
> +	if (iblock >= hip->fs_blocks) {
> +		if (!create)
> +			return 0;
> +		if (iblock > hip->fs_blocks)
> +			return -EIO;
> +	} else
> +		create = 0;
> +
> +	res = hfsplus_map_extent(inode, ablock, create, &dblock, &max_blocks,
> +				  NULL);
> +	if (res)
> +		return res;
> +
>  	hfs_dbg("ino %llu, iblock %llu - dblock %u\n",
>  		inode->i_ino, (long long)iblock, dblock);
>  
> @@ -292,9 +345,8 @@ int hfsplus_get_block(struct inode *inode, sector_t iblock,
>  		hip->phys_size += sb->s_blocksize;
>  		hip->fs_blocks++;
>  		inode_add_bytes(inode, sb->s_blocksize);
> -	}
> -	if (create || was_dirty)
>  		mark_inode_dirty(inode);
> +	}
>  	return 0;
>  }
>  
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 1e5b58e6a13f..67586382269b 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -436,6 +436,8 @@ int hfsplus_ext_cmp_key(const hfsplus_btree_key *k1,
>  int hfsplus_ext_write_extent(struct inode *inode);
>  int hfsplus_get_block(struct inode *inode, sector_t iblock,
>  		      struct buffer_head *bh_result, int create);
> +int hfsplus_map_extent(struct inode *inode, u32 ablock, int create,
> +			u32 *dblock, u32 *max_blocks, bool *balloc);
>  int hfsplus_free_fork(struct super_block *sb, u32 cnid,
>  		      struct hfsplus_fork_raw *fork, int type);
>  int hfsplus_file_extend(struct inode *inode, bool zeroout);
> -- 
> 2.43.0
> 
>