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

[Viacheslav Dubeyko]

On Fri, 2026-09-18 at 15:36 +0200, Christoph Hellwig wrote:
> 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.

OK. Sounds good.

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

I like the suggestion.

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

Agreed.

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

Let me take a look into the concept and rework the current code.

Thanks,
Slava.

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