Re: [PATCH v2 0/4] minix: convert to iomap and add direct I/O

[Jeremy Bingham]

On Wed, Jul 1, 2026 at 11:00 AM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> On Sat, Jun 27, 2026 at 10:15:52PM -0700, Jeremy Bingham wrote:
> > The iomap.c file is #include'd into itree_v1.c and itree_v2.c rather
> > than compiled as a standalone translation unit. This is because the
> > minix filesystem versions (V1 vs V2/V3) have different block_t sizes
> > (16-bit vs 32-bit) and different indirect tree depths. This follows
> > the existing pattern in minix where itree_common.c is included into
> > both itree_v1.c and itree_v2.c. Each version provides a thin wrapper
> > and a corresponding iomap_ops struct.
>
> Yuck.  I guess that's /one/ way to avoid having a geometry struct
> capturing those details... :(

It's all absolutely hideous. I've hated the way the minix module #includes
itree_common.c like that with the separate itree_v1.c and itree_v2.c for
years, but it's also been like that for so long that changing it is
intimidating.
I could address it in this patch, if it's not out of scope. (Also I
thought I had
already responded to this, my apologies.)

> > Changes since v1:
> >
> >   * Added a fourth patch to fix the symlink and truncate issues:
> >     - Replaced page_symlink with a custom __page_symlink that writes
> >       the target directly to a data block via minix_new_block +
>
> Sounds to me like it's time to write iomap_write_symlink.
>
> int
> iomap_symlink_write(struct inode *inode, const char *target, int len,
>                 const struct iomap_ops *ops,
>                 const struct iomap_write_ops *write_ops, void *private)
> {
>         struct kvec vec = {
>                 .iov_base       = target,
>                 .iov_len        = len,
>         };
>         struct iomap_iter iter = {
>                 .inode          = inode,
>                 .pos            = 0,
>                 .len            = len,
>                 .flags          = IOMAP_WRITE,
>                 .private        = private,
>         };
>         struct iov_iter iov;
>         int ret;
>
>         iov_iter_kvec(&iov, ITER_SRC, &vec, 1, iov.iov_len);
>
>         while ((ret = iomap_iter(&iter, ops)) > 0)
>                 iter.status = iomap_write_iter(&iter, &iov, write_ops);
>
>         if (unlikely(iter.pos == 0))
>                 return ret;
>
>         mark_inode_dirty(inode);
>         return 0;
> }
> EXPORT_SYMBOL_GPL(iomap_symlink_write);

Noted. I'll take this and the __page_symlink I originally made, smoosh
them together as or if needed, and put it in fs/iomap.c.

> >       sb_getblk, bypassing the aops write path (which no longer has
> >       write_begin/write_end). Added a matching custom minix_get_link
> >       that reads the target from the data block via sb_bread, similar
> >       to ext4_get_link. No iomap-based filesystem in the kernel uses
> >       page_symlink; XFS, GFS2, and ext4 all handle symlink storage
>
> That's because they embed headers and crcs in the symlink file data
> and/or do fancy things with inline targets.  xfs has its own buffer
> cache, so there's no need to duplicate it with the pagecache and then
> have to interpret ondisk formats.

Also noted.

Thanks again,

-j

> >       directly. The on-disk format is unchanged.
> >     - Fixed a buffer_head/iomap type confusion in truncate:
> >       block_truncate_page attaches buffer_heads to data folios, but
> >       minix_aops now uses iomap which interprets folio->private as
> >       struct iomap_folio_state. truncate() now dispatches between
> >       iomap_truncate_page (for regular files/symlinks) and
> >       block_truncate_page (for directories) based on the inode's aops.
> >     - Added .setattr = minix_setattr to minix_symlink_inode_operations
> >       so symlinks truncate properly through the iomap path.
> >
> >   * Patch 1 (iomap infrastructure): minix_get_block is now exported
> >     (non-static) so the directory aops and iomap writeback path can
> >     use it. Added minix_iomap_ops_ver() inline helper and extern
> >     declarations for minix_aops and the version-specific iomap_ops.
> >     Fixed unsigned -> unsigned int in minix_blocks_needed and
> >     minix_find_first_zero_bit to silence checkpatch warnings.
> >
> >   * Patch 2 (aops conversion): unchanged in approach; minor cleanup
> >     of the writeback callback and minix_bmap conversion.
> >
> >   * Patch 3 (file operations): minix_setattr is now exported for reuse
> >     by the symlink inode operations in patch 4.
> >
> > Testing: the full series has been tested with mkfs.minix V1/V2/V3,
> > exercising file creation, read/write, overwrite, append, binary data,
> > directories, symlinks (full path, relative, directory symlinks), hard
> > links, truncation (shrink/grow), large files (1MB, exercising indirect
> > blocks), deep nesting (20 levels), 100 files in one directory,
> > deletions, remount persistence, and fsck.minix. All pass cleanly. The
> > four syzbot-reported issues are resolved.
> >
> > Jeremy Bingham (4):
> >   minix: add iomap infrastructure
> >   minix: convert address space operations to iomap
> >   minix: convert file operations to iomap and add direct I/O
> >   minix: fix symlilnk and truncate for iomap compatibility
> >
> >  fs/minix/file.c         | 157 ++++++++++++++++++++++++++++++++++++++--
> >  fs/minix/inode.c        |  90 ++++++++++++++++++++---
> >  fs/minix/iomap.c        | 114 +++++++++++++++++++++++++++++
> >  fs/minix/itree_common.c |  11 ++-
> >  fs/minix/itree_v1.c     |  25 ++++++-
> >  fs/minix/itree_v2.c     |  17 ++++-
> >  fs/minix/minix.h        |  30 +++++++-
> >  fs/minix/namei.c        | 137 ++++++++++++++++++++++++++++++++++-
> >  8 files changed, 558 insertions(+), 23 deletions(-)
> >  create mode 100644 fs/minix/iomap.c
> >
> > --
> > 2.47.3
> >
> >