Re: [PATCH v4 2/3] minix: add iomap functions and definitions

[Christoph Hellwig <hch@infradead.org>]

On Wed, Aug 26, 2026 at 02:41:56PM -0700, Jeremy Bingham wrote:
> This patch also wires iomap.c into itree_v1.c and itree_v2.c, similarly
> to how itree_common.c is included in those files, and exports version
> specific versions of minix_iomap_begin and iomap_ops.

Can we keep the iomap code out of this double build mess?  Just define
some low-level helper in it, but keep everything else in an only
once build iomap.c

> Also updates 'unsigned' to 'unsigned int' a few places that got picked
> up by checkpatch.pl.

If you want to do these cleanups please split them into a separate
patch.

> +++ b/fs/minix/iomap.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * iomap functions for minix.
> + */

Not really a very useful comment :)  OTOH this lacks a copyright
statement for your work.

> +static int minix_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> +	unsigned int flags, struct iomap *iomap, struct iomap *srcmap)

This mostly does not need the iomap.  It basically returns a physical
block and a trystate of HOLE/FOUND/ALLOCATED.  If you change the
interface to that you can keep it in itree_common by directly morphing
the existing block lookup into that, or even do it as a prep patch,
which first refactors get_block to this interface, and then do the
iomap mapping on top of that, and in a place that only gets built
once.

> +/*
> + * minix_iomap_end ends up being a nop; since minix doesn't have any extents or
> + * transactions to worry about, there isn't anything to update here. The on-disk
> + * indirect blocks get dirtied in minix_iomap_begin.
> + */
> +static int minix_iomap_end(struct inode *inode, loff_t offset, loff_t length,
> +	ssize_t written, unsigned int flags, struct iomap *iomap)
> +{
> +	return 0;
> +}

In which case we can just drop it.  Note that for the next version
against 7.3-rc should also move from the begin/end calls to the
single next one using the DEFINE_IOMAP_ITER_NEXT helper macro.

> -static inline unsigned minix_blocks_needed(unsigned bits, unsigned blocksize)
> +static inline unsigned int minix_blocks_needed(unsigned int bits, unsigned int blocksize)

Overly long line.  I'd much rather generate the baseless checkpath.pl
complaint and leave the plain unsgineds in place.  There is absolutely
no technical downside in using that notation.

> +static inline const struct iomap_ops *minix_iomap_ops_ver(struct inode *inode)
> +{
> +	return (INODE_VERSION(inode) == MINIX_V1) ?

No need for the braces.