Re: [PATCH v4 4/7] hfsplus: add iomap operations for regular file data

[Christoph Hellwig]

On Mon, Sep 14, 2026 at 04:39:38PM -0700, Viacheslav Dubeyko wrote:
> This patch adds iomap.h and iomap.c files. The iomap.h contains
> declarations of hfsplus_iomap_ops, hfsplus_write_iomap_ops,
> hfsplus_writeback_ops, and hfsplus_write_dio_ops operations.
> The iomap.c implements __hfsplus_iomap_begin(),
> hfsplus_write_iomap_end(), hfsplus_iomap_cont_expand(),
> hfsplus_writeback_range() methods that become the basis of
> HFS+ iomap operations.

Adding this without the users in the last two patches is odd, as this
isn't really the kind of atomic change we're usually doing in Linux.

I'd vote for merging this and the last two patches into one.

> index 000000000000..5723e854e58e
> --- /dev/null
> +++ b/fs/hfsplus/iomap.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * iomap callback functions for the hfsplus filesystem

In Linux terminology these are methods, not callbacks.  I'd probably just
drop this comment entirely, as it doesn't really add value, though.

> +static int hfsplus_iomap_begin(struct inode *inode, loff_t offset,
> +				loff_t length, unsigned int flags,
> +				struct iomap *iomap, struct iomap *srcmap)
> +{
> +	return __hfsplus_iomap_begin(inode,
> +				     offset, length, flags,
> +				     iomap, false);

Odd formatting.  Why not:

	return __hfsplus_iomap_begin(inode, offset, length, flags, iomap,
				     false);

?

Also may_alloc as flags with a readable flag name might be nicer here.

> +}
> +
> +static int hfsplus_write_iomap_begin(struct inode *inode, loff_t offset,
> +				     loff_t length, unsigned int flags,
> +				     struct iomap *iomap, struct iomap *srcmap)
> +{
> +	return __hfsplus_iomap_begin(inode,
> +				     offset, length, flags,
> +				     iomap, true);
> +}

Same.

> +const struct iomap_ops hfsplus_iomap_ops = {
> +	.iomap_begin = hfsplus_iomap_begin,
> +};

Please use the new iomap next scheme merged in 7.3-rc.  In fact
the old begin/end methods were supposed to be removed after -rc1,
but someone they managed to still stay around.

> +
> +/*
> + * hfsplus_write_iomap_end()

Duplicating the function name adds no value.
Same for the other functions.

> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * iomap callback declarations for the hfsplus filesystem
> + */

Same comment as for iomap.c.

> +static inline
> +int iomap_dio_end_io(struct kiocb *iocb, ssize_t size,
> +		     int error, unsigned int flags)

Odd formatting, normally we'd do:

static inline int iomap_dio_end_io(struct kiocb *iocb, ssize_t size, int error,
		unsigned int flags)

or:

static inline int
iomap_dio_end_io(struct kiocb *iocb, ssize_t size, int error,
		unsigned int flags)