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

[Jeremy Bingham]

On Fri, Sep 18, 2026 at 7:12 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
> 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

OK. I went back and forth a bunch on how to best do this, because it's *such* a
mess. I had a lot of reservations about the way I had done it, but
it's so all around
messy I ended up there. When I refactor it I'll either move that function to
itree_common.c or craft a way to get that depth information out.

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

Noted. I did submit a patch like that back in I think late June or
early July, but got
told (politely) to knock it off. I had kept these cleanups because of
checkpatch.pl
griping, but as you said below there's no technical downside to leaving it.

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

I didn't want to be presumptuous. I'll add that in.

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

I'll address this in the refactor mentioned above. I don't think I can
entirely get rid of
or refactor get_block() unfortunately because of the directory
operations, but I'll see
what I can do.

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

Conveniently I had just updated my test VM kernel to 7.3-rc and
verified that the
module still builds unchanged, so everything's in place for that.
Thank you for the
head's up.

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

All simple, all noted.

Thanks,

-j