Linux filesystem development
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <djwong@kernel.org>
To: Jeremy Bingham <jbingham@gmail.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	brauner@kernel.org, jkoolstra@xs4all.nl, jack@suse.cz,
	hch@infradead.org, viro@zeniv.linux.org.uk,
	syzkaller@googlegroups.com
Subject: Re: [PATCH v2 0/4] minix: convert to iomap and add direct I/O
Date: Wed, 1 Jul 2026 11:00:35 -0700	[thread overview]
Message-ID: <20260701180035.GA6507@frogsfrogsfrogs> (raw)
In-Reply-To: <cover.1782619718.git.jbingham@gmail.com>

On Sat, Jun 27, 2026 at 10:15:52PM -0700, Jeremy Bingham wrote:
> This is v2 of the minix iomap conversion series. The original v1
> submission (3 patches) was tested by syzbot, which found four issues.
> Three were straightforward; the fourth, a null pointer dereference in
> page_symlink when creating symlinks, required more substantial changes.
> 
> The original description follows:
> 
> This series converts the minix filesystem from the buffer_head-based
> I/O path to the iomap API, and adds direct I/O support in the process.
> 
> The conversion is straightforward: minix's indirect block tree mapping
> logic (from itree_common.c) is reworked into an iomap_begin/iomap_end
> implementation. The iomap_end callback is a no-op since minix has no
> extents or transactions to finalize.
> 
> Patch 1 adds the iomap infrastructure: the new iomap.c file, wrapper
> functions and iomap_ops structs in itree_v1.c and itree_v2.c, and the
> relevant declarations in minix.h.
> 
> 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... :(

> Patch 2 converts the regular file address space operations to iomap:
> read_folio, readahead, writepages (with a writeback callback), bmap,
> and folio lifecycle helpers. Directory inodes continue to use
> buffer_head-based operations via a new minix_dir_aops, since directory
> handling still relies on buffer head chunks for prepare/write_begin.
> 
> Patch 3 converts the file_operations: replacing the generic read/write
> iterators with iomap-aware versions, adding direct I/O read/write paths
> using iomap_dio_rw, and setting FMODE_CAN_ODIRECT in the open handler.
> 
> The minix iomap implementation was adapted from the out-of-tree xiafs
> iomap conversion. The xiafs module itself borrowed heavily from the
> modernized minix kernel module. The exfat iomap changes were an
> additional reference for both conversions.
> 
> 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);


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

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

  parent reply	other threads:[~2026-07-01 18:00 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-28  5:15 [PATCH v2 0/4] minix: convert to iomap and add direct I/O Jeremy Bingham
2026-06-28  5:15 ` [PATCH v2 1/4] minix: add iomap infrastructure Jeremy Bingham
2026-07-01 18:06   ` Darrick J. Wong
2026-07-01 18:32     ` Jeremy Bingham
2026-06-28  5:15 ` [PATCH v2 2/4] minix: convert address space operations to iomap Jeremy Bingham
2026-07-01 18:14   ` Darrick J. Wong
2026-07-01 18:37     ` Jeremy Bingham
2026-07-01 18:47       ` Darrick J. Wong
2026-06-28  5:15 ` [PATCH v2 3/4] minix: convert file operations to iomap and add Jeremy Bingham
2026-07-01 18:21   ` Darrick J. Wong
2026-07-01 18:42     ` Jeremy Bingham
2026-06-28  5:15 ` [PATCH v2 4/4] minix: fix symlink and truncate for iomap Jeremy Bingham
2026-06-29  5:27 ` [syzbot ci] Re: minix: convert to iomap and add direct I/O syzbot ci
2026-06-30  3:05   ` Jeremy Bingham
2026-06-30  4:05     ` syzbot ci
2026-07-01 18:00 ` Darrick J. Wong [this message]
2026-07-02 18:42   ` [PATCH v2 0/4] " Jeremy Bingham

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260701180035.GA6507@frogsfrogsfrogs \
    --to=djwong@kernel.org \
    --cc=brauner@kernel.org \
    --cc=hch@infradead.org \
    --cc=jack@suse.cz \
    --cc=jbingham@gmail.com \
    --cc=jkoolstra@xs4all.nl \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=syzkaller@googlegroups.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox