[PATCH v3 0/5] minix: convert to iomap and add direct I/O

[PATCH v3 0/5] minix: convert to iomap and add direct I/O

[Jeremy Bingham]

This is version 3 of the minix iomap conversion patch series. Versions 1
and 2 both had issues that syzbot found while testing, while Darrick J.
Wong gave some critiques of the version 2 patches that I have addressed
here.

This patch series converts the minix filesystem module from using the
buffer_head based path to the iomap API, and adds direct I/O support in
the process.

Patch 1 adds a new iomap_symlink_write function to
fs/iomap/buffered-io.c. This allows minix's symlinks to be written and
read without bypassing iomap entirely. (I considered whether this ought
to be broken out as its own patch since it's outside of fs/minix, but
decided that since it was suggested as part of this and is a necessary
piece that I would include it unless told otherwise.) This was suggested
by Darrick J. Wong, who also provided an initial implementation that I
only had to modify slightly.

Patch 2 adds the base iomap functions in fs/minix/iomap.c. This provides
the iomap begin/end functions which sit at the base of the rest of this.

Patch 3 wires iomap.c into the itree*.c files, adds version specific
helper functions, and moves the old buffer_head based functions over to
new names to support directory operations.

Patch 4 adds the new iomap file operations functions to file.c,
including the direct i/o functions.

Patch 5 updates the inode operations to use iomap, except for directory
operations which work the old way. The minix_symlink function in namei.c
is also updated to use the new iomap_write_symlink function.

Changes since v2:
  * Reorganized the patches entirely.
  * Addressed Darrick Wong's critiques of v2:
    - Added a new iomap_symlink_write function per his suggestion.
    - Removed the minix custom __page_symlink function that replaced the
      usual page_symlink function and the changes to minix_get_link that
      allowed symlinks to be written and read by bypassing iomap
      entirely. This ended up also removing both the issue syzbot found
      in v2 of the patch and the fix for it.
    - Reworked minix_iomap_begin to remove the nest of gotos inherited
      from get_block to use helper functions instead.
    - Pared down some unnecessary comments.
    - Dropped bmap entirely.
    - Dropped IOMAP_F_MERGED in minix_iomap_begin since there aren't any
      merged blocks to worry about.
    - Removed unneeded manual page cache invalidation.

This patch series has been verified to compile cleanly between each
patch and has been tested with a script that exercises mounting,
unmounting, file creation/writing/reading/verification, directory
creation and listing, symlink creation/reading/following, hard links,
file truncation (both shrinking and growing), deep directory nesting,
large files that span multiple blocks and indirect blocks for both size
and content, deleting files and directories, persistence, and that
fsck.minix behaves as expected for all versions of the minix filesystem.

Unfortunately, no version of the minix fs module can support an xfstests
run; all of the tests either fail (seemingly for reasons unrelated to
the module itself) or aren't run because minix doesn't support the
features in question. Getting xfstests to run would be nice, but either
requires or would be greatly helped by making changes to minix's mkfs
and fsck programs. It would definitely be a nice-to-have, but is outside
the scope of this series.

Jeremy Bingham (5):
  iomap: add iomap_symlink_write
  minix: add base iomap begin/end functions
  minix: update itree* files to add iomap functions
  minix: add file operation functions
  minix: iomap inode operations

 fs/iomap/buffered-io.c  |  31 ++++++++
 fs/minix/file.c         | 153 ++++++++++++++++++++++++++++++++++++++--
 fs/minix/inode.c        |  87 +++++++++++++++++++----
 fs/minix/iomap.c        | 122 ++++++++++++++++++++++++++++++++
 fs/minix/itree_common.c |  11 ++-
 fs/minix/itree_v1.c     |  25 ++++++-
 fs/minix/itree_v2.c     |  17 ++++-
 fs/minix/minix.h        |  24 ++++++-
 fs/minix/namei.c        |   8 ++-
 include/linux/iomap.h   |   4 ++
 10 files changed, 457 insertions(+), 25 deletions(-)
 create mode 100644 fs/minix/iomap.c

-- 
2.47.3

[PATCH v3 1/5] iomap: add iomap_symlink_write

[Jeremy Bingham]

Add a new iomap_symlink_write function as an iomap based equivalent to
page_symlink found in fs/namei.c. This implementation was suggested by
Darrick J. Wong, who also provided the initial implementation that I
only needed to change slightly to get working.

Suggested-by: Darrick J. Wong <djwong@kernel.org>

Signed-off-by: Jeremy Bingham <jbingham@gmail.com>
---
 fs/iomap/buffered-io.c | 31 +++++++++++++++++++++++++++++++
 include/linux/iomap.h  |  4 ++++
 2 files changed, 35 insertions(+)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 276720bc18dc..6f3f79fdbd8b 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -2066,3 +2066,34 @@ iomap_writepages(struct iomap_writepage_ctx *wpc)
 	return error;
 }
 EXPORT_SYMBOL_GPL(iomap_writepages);
+
+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	= (void *)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_SOURCE, &vec, 1, 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);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 56b43d594e6e..9274fa256bc9 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -393,6 +393,10 @@ loff_t iomap_seek_data(struct inode *inode, loff_t offset,
 sector_t iomap_bmap(struct address_space *mapping, sector_t bno,
 		const struct iomap_ops *ops);
 
+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);
+
 /*
  * Flags for iomap_ioend->io_flags.
  */
-- 
2.47.3

[PATCH v3 2/5] minix: add base iomap begin/end functions

[Jeremy Bingham]

Adds a new file, iomap.c, to fs/minix. This provides minix_iomap_begin,
which is the iomap-based version of get_block in itree_common.c. It also
provides minix_iomap_end, which is a no-op but required by the iomap_ops
struct. This also undoes the nest of gotos inherited from get_block and
uses a set of helper functions instead.

Suggested-by: Darrick J. Wong <djwong@kernel.org>

Signed-off-by: Jeremy Bingham <jbingham@gmail.com>
---
 fs/minix/iomap.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)
 create mode 100644 fs/minix/iomap.c

diff --git a/fs/minix/iomap.c b/fs/minix/iomap.c
new file mode 100644
index 000000000000..90e016aaa01e
--- /dev/null
+++ b/fs/minix/iomap.c
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * iomap functions for minix.
+ */
+
+static inline void minix_chain_cleanup(Indirect *chain, Indirect *partial)
+{
+	while (partial > chain) {
+		brelse(partial->bh);
+		partial--;
+	}
+}
+
+static inline void minix_iomap_set_mapped(struct iomap *iomap, sector_t phys,
+		unsigned int blkbits, sector_t iblock)
+{
+	iomap->type = IOMAP_MAPPED;
+	iomap->addr = (u64)phys << blkbits;
+	iomap->length = 1 << blkbits;
+	iomap->offset = (u64)iblock << blkbits;
+}
+
+static inline void minix_iomap_set_hole(struct iomap *iomap,
+		unsigned int blkbits, sector_t iblock)
+{
+	iomap->type = IOMAP_HOLE;
+	iomap->addr = IOMAP_NULL_ADDR;
+	iomap->length = 1 << blkbits;
+	iomap->offset = (u64)iblock << blkbits;
+}
+
+/*
+ * minix_iomap_begin - map a file range to disk blocks. It acts as a replacement
+ * for get_block in itree_common.c, at least in the important ways, and is
+ * adapted from it, but it uses iomap instead of buffer_head.
+ */
+static int minix_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
+	unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
+{
+	struct super_block *sb = inode->i_sb;
+	unsigned int blkbits = sb->s_blocksize_bits;
+	sector_t iblock = offset >> blkbits;
+	int create = flags & IOMAP_WRITE;
+
+	int offsets[DEPTH];
+	Indirect chain[DEPTH];
+	Indirect *partial;
+	int depth = block_to_path(inode, iblock, offsets);
+	int left;
+	int err = -EIO;
+
+	sector_t phys;
+
+	/* block is beyond max file size */
+	if (depth == 0)
+		return -EINVAL;
+
+	iomap->bdev = inode->i_sb->s_bdev;
+
+reread:
+	partial = get_branch(inode, depth, offsets, chain, &err);
+
+	/* Simplest case - block found, no allocation needed */
+	if (!partial) {
+		iomap->flags = 0;
+		phys = block_to_cpu(chain[depth - 1].key);
+		partial = chain+depth-1;
+		minix_iomap_set_mapped(iomap, phys, blkbits, iblock);
+		minix_chain_cleanup(chain, partial);
+		return err;
+	}
+
+	/* Next simple case - plain lookup or failed read of indirect block */
+	if (!create || err == -EIO) {
+		minix_iomap_set_hole(iomap, blkbits, iblock);
+		minix_chain_cleanup(chain, partial);
+		return err;
+	}
+
+	/*
+	 * This is held over from the original get_block logic, where it
+	 * acted as a guard in case truncate() deleted blocks from under that
+	 * function. There should not be a race with iomap operations, but
+	 * we're retaining the defensive coding here to be extra safe just in
+	 * case.
+	 */
+	if (err == -EAGAIN) {
+		minix_chain_cleanup(chain, partial);
+		goto reread;
+	}
+
+	left = (chain + depth) - partial;
+	err = alloc_branch(inode, left, offsets + (partial - chain), partial);
+	if (err) {
+		minix_chain_cleanup(chain, partial);
+		return err;
+	}
+
+	if (splice_branch(inode, chain, partial, left) < 0) {
+		minix_chain_cleanup(chain, partial);
+		goto reread;
+	}
+
+	/* Successful allocation, mapping it. */
+	iomap->flags = IOMAP_F_NEW;
+	phys = block_to_cpu(chain[depth - 1].key);
+	minix_iomap_set_mapped(iomap, phys, blkbits, iblock);
+	minix_chain_cleanup(chain, partial);
+
+	return err;
+}
+
+/*
+ * 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;
+}
-- 
2.47.3

[PATCH v3 3/5] minix: update itree* files to add iomap functions

[Jeremy Bingham]

Now that iomap.c is in place, update itree_v1.c, itree_v2.c, and
itree_common.c to use iomap. This also updates the exports in minix.h,
adds a convenient helper function to get the right version of
minix_iomap_ops, and exports the previously static minix_aops struct
from inode.c.

There are a couple of odd things in here that need to be explicitly
called out and explained:

First, in itree_common.c there is now a check on inode->i_mapping->a_ops
where if it is equal to &minix_aops. If it is, it calls
iomap_truncate_page; otherwise, it calls block_truncate_page. This is
because unlike everything else directory operations still use
buffer_heads and have their own address_space_operations struct.

Second, instead of standing alone iomap.c is #included into itree_v1.c
and itree_v2.c the same way itree_common.c is. This is an existing
pattern that has been present in this module as far back as the git
history goes to accomodate the different block pointer sizes and number
of indirect block levels the different versions of the minix filesystem
has. The way it is now does look very strange, but it avoids massive
changes that might not be appreciated.

Signed-off-by: Jeremy Bingham <jbingham@gmail.com>
---
 fs/minix/inode.c        |  2 +-
 fs/minix/itree_common.c | 11 ++++++++++-
 fs/minix/itree_v1.c     | 25 ++++++++++++++++++++++++-
 fs/minix/itree_v2.c     | 17 ++++++++++++++++-
 fs/minix/minix.h        | 22 ++++++++++++++++++++--
 5 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index c30cc590698d..d5ba72f51049 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -490,7 +490,7 @@ static sector_t minix_bmap(struct address_space *mapping, sector_t block)
 	return generic_block_bmap(mapping,block,minix_get_block);
 }
 
-static const struct address_space_operations minix_aops = {
+const struct address_space_operations minix_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio = minix_read_folio,
diff --git a/fs/minix/itree_common.c b/fs/minix/itree_common.c
index c3cd2c75af9c..5a8b73a7beda 100644
--- a/fs/minix/itree_common.c
+++ b/fs/minix/itree_common.c
@@ -311,7 +311,16 @@ static inline void truncate (struct inode * inode)
 	long iblock;
 
 	iblock = (inode->i_size + sb->s_blocksize -1) >> sb->s_blocksize_bits;
-	block_truncate_page(inode->i_mapping, inode->i_size, get_block);
+
+	/* Depending on what address space operations are being used by the
+	 * inode being truncated, we need to either call iomap_truncate_page or
+	 * block_truncate_page.
+	 */
+	if (inode->i_mapping->a_ops == &minix_aops)
+		iomap_truncate_page(inode, inode->i_size, NULL,
+			minix_iomap_ops_ver(inode), NULL, NULL);
+	else
+		block_truncate_page(inode->i_mapping, inode->i_size, get_block);
 
 	n = block_to_path(inode, iblock, offsets);
 	if (!n)
diff --git a/fs/minix/itree_v1.c b/fs/minix/itree_v1.c
index 1fed906042aa..58c29f4443d3 100644
--- a/fs/minix/itree_v1.c
+++ b/fs/minix/itree_v1.c
@@ -49,6 +49,18 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 }
 
 #include "itree_common.c"
+/* NOTA BENE:
+ *
+ * This is icky to me, but at the same time having it be a standalone C file
+ * that's compiled to object form and linked separately like one would think is
+ * much nastier in minix because of the different versions of the minix fs that
+ * have some very, very different aspects, like the size of block_t. I don't
+ * like it, but since minix already has this pattern where a common itree file
+ * is included in the itree_v1 and itree_v2(and v3) files, I'm including iomap.c
+ * in these files as well. It does at least avoid exporting some currently
+ * static functions that aren't needed anywhere but itree_common.c and iomap.c.
+ */
+#include "iomap.c"
 
 int V1_minix_get_block(struct inode * inode, long block,
 			struct buffer_head *bh_result, int create)
@@ -61,7 +73,18 @@ void V1_minix_truncate(struct inode * inode)
 	truncate(inode);
 }
 
-unsigned V1_minix_blocks(loff_t size, struct super_block *sb)
+unsigned int V1_minix_blocks(loff_t size, struct super_block *sb)
 {
 	return nblocks(size, sb);
 }
+
+int V1_minix_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
+	unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
+{
+	return minix_iomap_begin(inode, offset, length, flags, iomap, srcmap);
+}
+
+const struct iomap_ops V1_minix_iomap_ops = {
+	.iomap_begin = V1_minix_iomap_begin,
+	.iomap_end   = minix_iomap_end,
+};
diff --git a/fs/minix/itree_v2.c b/fs/minix/itree_v2.c
index 9d00f31a2d9d..fc7a5ae8fa1c 100644
--- a/fs/minix/itree_v2.c
+++ b/fs/minix/itree_v2.c
@@ -57,6 +57,10 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 }
 
 #include "itree_common.c"
+/* See the note in itree_v1 in a comment that starts "NOTA BENE" for an
+ * explanation for why iomap.c is included here.
+ */
+#include "iomap.c"
 
 int V2_minix_get_block(struct inode * inode, long block,
 			struct buffer_head *bh_result, int create)
@@ -69,7 +73,18 @@ void V2_minix_truncate(struct inode * inode)
 	truncate(inode);
 }
 
-unsigned V2_minix_blocks(loff_t size, struct super_block *sb)
+unsigned int V2_minix_blocks(loff_t size, struct super_block *sb)
 {
 	return nblocks(size, sb);
 }
+
+int V2_minix_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
+	unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
+{
+	return minix_iomap_begin(inode, offset, length, flags, iomap, srcmap);
+}
+
+const struct iomap_ops V2_minix_iomap_ops = {
+	.iomap_begin = V2_minix_iomap_begin,
+	.iomap_end   = minix_iomap_end,
+};
diff --git a/fs/minix/minix.h b/fs/minix/minix.h
index 9e52d4302f0d..cf85edfb5bfa 100644
--- a/fs/minix/minix.h
+++ b/fs/minix/minix.h
@@ -5,6 +5,7 @@
 #include <linux/fs.h>
 #include <linux/pagemap.h>
 #include <linux/minix_fs.h>
+#include <linux/iomap.h>
 
 #define INODE_VERSION(inode)	minix_sb(inode->i_sb)->s_version
 #define MINIX_V1		0x0001		/* original minix fs */
@@ -77,13 +78,24 @@ int minix_make_empty(struct inode*, struct inode*);
 int minix_empty_dir(struct inode*);
 int minix_set_link(struct minix_dir_entry *de, struct folio *folio,
 		struct inode *inode);
+
 struct minix_dir_entry *minix_dotdot(struct inode*, struct folio **);
 ino_t minix_inode_by_name(struct dentry*);
 
+extern int V1_minix_iomap_begin(struct inode *inode, loff_t offset,
+	loff_t length, unsigned int flags, struct iomap *iomap,
+	struct iomap *srcmap);
+extern int V2_minix_iomap_begin(struct inode *inode, loff_t offset,
+	loff_t length, unsigned int flags, struct iomap *iomap,
+	struct iomap *srcmap);
+
+extern const struct address_space_operations minix_aops;
 extern const struct inode_operations minix_file_inode_operations;
 extern const struct inode_operations minix_dir_inode_operations;
 extern const struct file_operations minix_file_operations;
 extern const struct file_operations minix_dir_operations;
+extern const struct iomap_ops V1_minix_iomap_ops;
+extern const struct iomap_ops V2_minix_iomap_ops;
 
 static inline struct minix_sb_info *minix_sb(struct super_block *sb)
 {
@@ -95,11 +107,17 @@ static inline struct minix_inode_info *minix_i(struct inode *inode)
 	return container_of(inode, struct minix_inode_info, vfs_inode);
 }
 
-static inline unsigned minix_blocks_needed(unsigned bits, unsigned blocksize)
+static inline unsigned int minix_blocks_needed(unsigned int bits, unsigned int blocksize)
 {
 	return DIV_ROUND_UP_POW2(bits, blocksize * 8);
 }
 
+static inline const struct iomap_ops *minix_iomap_ops_ver(struct inode *inode)
+{
+	return (INODE_VERSION(inode) == MINIX_V1) ?
+		&V1_minix_iomap_ops : &V2_minix_iomap_ops;
+}
+
 #if defined(CONFIG_MINIX_FS_NATIVE_ENDIAN) && \
 	defined(CONFIG_MINIX_FS_BIG_ENDIAN_16BIT_INDEXED)
 
@@ -129,7 +147,7 @@ static inline unsigned minix_blocks_needed(unsigned bits, unsigned blocksize)
  * big-endian 16bit indexed bitmaps
  */
 
-static inline int minix_find_first_zero_bit(const void *vaddr, unsigned size)
+static inline int minix_find_first_zero_bit(const void *vaddr, unsigned int size)
 {
 	const unsigned short *p = vaddr, *addr = vaddr;
 	unsigned short num;
-- 
2.47.3

[PATCH v3 4/5] minix: add file operation functions

[Jeremy Bingham]

This adds the minix iomap file operation functions, including direct I/O
functions, and updates the minix_file_operations struct to use them.
Previously, most of the function pointers in minix_file_operations were
the defaults. With the iomap changes, though, the defaults are no longer
sufficient and minix-specific functions are required.

The previously static minix_setattr function is also now exported; it will
be used by minix_symlink_inode_operations.

Signed-off-by: Jeremy Bingham <jbingham@gmail.com>
---
 fs/minix/file.c  | 153 +++++++++++++++++++++++++++++++++++++++++++++--
 fs/minix/minix.h |   2 +
 2 files changed, 149 insertions(+), 6 deletions(-)

diff --git a/fs/minix/file.c b/fs/minix/file.c
index 86e5943cd2ff..88117c0e9f11 100644
--- a/fs/minix/file.c
+++ b/fs/minix/file.c
@@ -17,21 +17,162 @@ int minix_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 			start, end, datasync);
 }
 
+static ssize_t minix_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	struct inode *inode = iocb->ki_filp->f_mapping->host;
+	ssize_t ret;
+
+	inode_lock_shared(inode);
+
+	const struct iomap_ops *ops = minix_iomap_ops_ver(inode);
+
+	ret = iomap_dio_rw(iocb, to, ops, NULL, 0, NULL, 0);
+	inode_unlock_shared(inode);
+	return ret;
+}
+
+static int minix_dio_write_end_io(struct kiocb *iocb, ssize_t size, int error,
+		unsigned int flags)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+	loff_t pos = iocb->ki_pos;
+
+	if (error)
+		return error;
+
+	pos += size;
+	if (size && pos > i_size_read(inode)) {
+		i_size_write(inode, pos);
+		mark_inode_dirty(inode);
+	}
+	return 0;
+}
+
+static const struct iomap_dio_ops minix_dio_write_ops = {
+	.end_io = minix_dio_write_end_io,
+};
+
+static ssize_t minix_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
+{
+	struct inode *inode = iocb->ki_filp->f_mapping->host;
+	ssize_t ret;
+	unsigned int flags = 0;
+	unsigned long blocksize = inode->i_sb->s_blocksize;
+
+	inode_lock(inode);
+	ret = generic_write_checks(iocb, from);
+	if (ret <= 0)
+		goto out_unlock;
+
+	ret = kiocb_modified(iocb);
+	if (ret)
+		goto out_unlock;
+
+	if (iocb->ki_pos + iov_iter_count(from) > i_size_read(inode) ||
+		!IS_ALIGNED(iocb->ki_pos | iov_iter_alignment(from), blocksize))
+		flags |= IOMAP_DIO_FORCE_WAIT;
+
+	const struct iomap_ops *ops = minix_iomap_ops_ver(inode);
+
+	ret = iomap_dio_rw(iocb, from, ops,
+		&minix_dio_write_ops, flags, NULL, 0);
+	if (ret == -ENOTBLK)
+		ret = 0; /* fallback to buffered */
+
+	if (ret >= 0 && iov_iter_count(from)) {
+		loff_t pos;
+		loff_t endbyte;
+		ssize_t status;
+
+		iocb->ki_flags &= ~IOCB_DIRECT;
+		iocb->ki_flags |= IOCB_DSYNC;
+		pos = iocb->ki_pos;
+		status = iomap_file_buffered_write(iocb, from, ops,
+			NULL, NULL);
+		if (unlikely(status < 0)) {
+			ret = status;
+			goto out_unlock;
+		}
+
+		ret += status;
+		endbyte = pos + status - 1;
+		invalidate_mapping_pages(inode->i_mapping,
+			pos >> PAGE_SHIFT,
+			endbyte >> PAGE_SHIFT);
+		if (ret > 0)
+			ret = generic_write_sync(iocb, ret);
+	}
+
+out_unlock:
+	inode_unlock(inode);
+	return ret;
+}
+
+static ssize_t minix_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+	if (iocb->ki_flags & IOCB_DIRECT)
+		return minix_dio_read_iter(iocb, to);
+
+	return generic_file_read_iter(iocb, to);
+}
+
+static ssize_t minix_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
+{
+	struct inode *inode = iocb->ki_filp->f_mapping->host;
+	ssize_t ret;
+
+	/* minix_dio_write_iter also locks the inode and appears to do the same
+	 * general sorts of checks as this, so just return directly from there.
+	 */
+	if (iocb->ki_flags & IOCB_DIRECT)
+		return minix_dio_write_iter(iocb, from);
+
+	inode_lock(inode);
+	ret = generic_write_checks(iocb, from);
+	if (ret <= 0)
+		goto unlock;
+
+	ret = file_modified(iocb->ki_filp);
+	if (ret)
+		goto unlock;
+
+	const struct iomap_ops *ops = minix_iomap_ops_ver(inode);
+
+	ret = iomap_file_buffered_write(iocb, from, ops,
+			NULL, NULL);
+
+	if (ret > 0)
+		ret = generic_write_sync(iocb, ret);
+
+unlock:
+	inode_unlock(inode);
+	return ret;
+}
+
+static int minix_file_open(struct inode *inode, struct file *filp)
+{
+	filp->f_mode |= FMODE_CAN_ODIRECT;
+	return generic_file_open(inode, filp);
+}
+
 /*
- * We have mostly NULLs here: the current defaults are OK for
- * the minix filesystem.
+ * We still have some NULLs here, but not as many of the current defaults are
+ * still OK for the minix filesystem.
  */
+
 const struct file_operations minix_file_operations = {
 	.llseek		= generic_file_llseek,
-	.read_iter	= generic_file_read_iter,
-	.write_iter	= generic_file_write_iter,
+	.read_iter	= minix_file_read_iter,
+	.write_iter	= minix_file_write_iter,
 	.mmap_prepare	= generic_file_mmap_prepare,
+	.open		= minix_file_open,
 	.fsync		= minix_fsync,
 	.splice_read	= filemap_splice_read,
+	.splice_write	= iter_file_splice_write,
 };
 
-static int minix_setattr(struct mnt_idmap *idmap,
-			 struct dentry *dentry, struct iattr *attr)
+int minix_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+	struct iattr *attr)
 {
 	struct inode *inode = d_inode(dentry);
 	int error;
diff --git a/fs/minix/minix.h b/fs/minix/minix.h
index cf85edfb5bfa..45beadef0d8c 100644
--- a/fs/minix/minix.h
+++ b/fs/minix/minix.h
@@ -58,6 +58,8 @@ void minix_free_block(struct inode *inode, unsigned long block);
 unsigned long minix_count_free_blocks(struct super_block *sb);
 int minix_getattr(struct mnt_idmap *, const struct path *,
 		struct kstat *, u32, unsigned int);
+int minix_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+	struct iattr *attr);
 int minix_prepare_chunk(struct folio *folio, loff_t pos, unsigned len);
 struct mapping_metadata_bhs *minix_get_metadata_bhs(struct inode *inode);
 int minix_fsync(struct file *file, loff_t start, loff_t end, int datasync);
-- 
2.47.3

[PATCH v3 5/5] minix: iomap inode operations

[Jeremy Bingham]

Wire the new iomap functionality into the inode operations. Entirely new
functions for writeback and a minix_writeback_ops struct have been
added, and the address space operation functions have been updated to
use iomap. Since directory operations still work the old way, the old
functions have been preserved with new names. Directories also get their
own address operation struct.

Also, bmap support is dropped entirely per Darrick Wong's suggestion.

This patch also updates minix_symlink to use the new iomap_symlink_write
function, removing the need to bypass iomap to read and write symlinks
directly. This was also suggested by Darrick Wong.

Suggested-by: Darrick J. Wong <djwong@kernel.org>

Signed-off-by: Jeremy Bingham <jbingham@gmail.com>
---
 fs/minix/inode.c | 85 +++++++++++++++++++++++++++++++++++++++++-------
 fs/minix/namei.c |  8 ++++-
 2 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index d5ba72f51049..f6747bacdb14 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -436,6 +436,31 @@ static int minix_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
+static ssize_t minix_writeback_range(struct iomap_writepage_ctx *wpc,
+	struct folio *folio, u64 pos, unsigned int len, u64 end_pos)
+{
+	int error;
+
+	if (pos < wpc->iomap.offset ||
+			pos >= wpc->iomap.offset + wpc->iomap.length) {
+		if (INODE_VERSION(wpc->inode) == MINIX_V1)
+			error = V1_minix_iomap_begin(wpc->inode, pos, len, IOMAP_WRITE,
+				&wpc->iomap, NULL);
+		else
+			error = V2_minix_iomap_begin(wpc->inode, pos, len, IOMAP_WRITE,
+				&wpc->iomap, NULL);
+		if (error)
+			return error;
+	}
+
+	return iomap_add_to_ioend(wpc, folio, pos, end_pos, len);
+}
+
+static const struct iomap_writeback_ops minix_writeback_ops = {
+	.writeback_range = minix_writeback_range,
+	.writeback_submit = iomap_ioend_writeback_submit,
+};
+
 static int minix_get_block(struct inode *inode, sector_t block,
 		    struct buffer_head *bh_result, int create)
 {
@@ -445,17 +470,45 @@ static int minix_get_block(struct inode *inode, sector_t block,
 		return V2_minix_get_block(inode, block, bh_result, create);
 }
 
-static int minix_writepages(struct address_space *mapping,
+/* The old minix_writepages, preserved for directory operations. */
+static int minix_block_writepages(struct address_space *mapping,
 		struct writeback_control *wbc)
 {
 	return mpage_writepages(mapping, wbc, minix_get_block);
 }
 
+static int minix_writepages(struct address_space *mapping,
+		struct writeback_control *wbc)
+{
+	struct iomap_writepage_ctx wpc = {
+		.inode = mapping->host,
+		.wbc = wbc,
+		.ops = &minix_writeback_ops,
+	};
+	return iomap_writepages(&wpc);
+}
+
 static int minix_read_folio(struct file *file, struct folio *folio)
+{
+	const struct iomap_ops *ops = minix_iomap_ops_ver(folio->mapping->host);
+
+	iomap_bio_read_folio(folio, ops);
+	return 0;
+}
+
+/* The old minix_read_folio, preserved for directory operations. */
+static int minix_block_read_folio(struct file *file, struct folio *folio)
 {
 	return block_read_full_folio(folio, minix_get_block);
 }
 
+static void minix_readahead(struct readahead_control *rac)
+{
+	const struct iomap_ops *ops = minix_iomap_ops_ver(rac->mapping->host);
+
+	iomap_bio_readahead(rac, ops);
+}
+
 int minix_prepare_chunk(struct folio *folio, loff_t pos, unsigned len)
 {
 	return __block_write_begin(folio, pos, len, minix_get_block);
@@ -485,26 +538,35 @@ static int minix_write_begin(const struct kiocb *iocb,
 	return ret;
 }
 
-static sector_t minix_bmap(struct address_space *mapping, sector_t block)
-{
-	return generic_block_bmap(mapping,block,minix_get_block);
-}
-
 const struct address_space_operations minix_aops = {
-	.dirty_folio	= block_dirty_folio,
-	.invalidate_folio = block_invalidate_folio,
+	.dirty_folio	= iomap_dirty_folio,
+	.invalidate_folio = iomap_invalidate_folio,
 	.read_folio = minix_read_folio,
+	.readahead = minix_readahead,
 	.writepages = minix_writepages,
+	.migrate_folio = filemap_migrate_folio,
+	.is_partially_uptodate = iomap_is_partially_uptodate,
+	.release_folio = iomap_release_folio,
+	.error_remove_folio = generic_error_remove_folio,
+};
+
+/* A special aops for directories that keeps using the buffer head chunks, at
+ * least for the time being.
+ */
+static const struct address_space_operations minix_dir_aops = {
+	.dirty_folio = block_dirty_folio,
+	.invalidate_folio = block_invalidate_folio,
+	.read_folio = minix_block_read_folio,
 	.write_begin = minix_write_begin,
 	.write_end = generic_write_end,
 	.migrate_folio = buffer_migrate_folio,
-	.bmap = minix_bmap,
-	.direct_IO = noop_direct_IO
+	.writepages = minix_block_writepages,
 };
 
 static const struct inode_operations minix_symlink_inode_operations = {
 	.get_link	= page_get_link,
 	.getattr	= minix_getattr,
+	.setattr	= minix_setattr,
 };
 
 void minix_set_inode(struct inode *inode, dev_t rdev)
@@ -516,7 +578,7 @@ void minix_set_inode(struct inode *inode, dev_t rdev)
 	} else if (S_ISDIR(inode->i_mode)) {
 		inode->i_op = &minix_dir_inode_operations;
 		inode->i_fop = &minix_dir_operations;
-		inode->i_mapping->a_ops = &minix_aops;
+		inode->i_mapping->a_ops = &minix_dir_aops;
 	} else if (S_ISLNK(inode->i_mode)) {
 		inode->i_op = &minix_symlink_inode_operations;
 		inode_nohighmem(inode);
@@ -768,4 +830,3 @@ module_init(init_minix_fs)
 module_exit(exit_minix_fs)
 MODULE_DESCRIPTION("Minix file system");
 MODULE_LICENSE("GPL");
-
diff --git a/fs/minix/namei.c b/fs/minix/namei.c
index 263e4ba8b1c8..bb5892f9fb2a 100644
--- a/fs/minix/namei.c
+++ b/fs/minix/namei.c
@@ -6,6 +6,7 @@
  */
 
 #include "minix.h"
+#include <linux/iomap.h>
 
 static int add_nondir(struct dentry *dentry, struct inode *inode)
 {
@@ -84,12 +85,17 @@ static int minix_symlink(struct mnt_idmap *idmap, struct inode *dir,
 		return PTR_ERR(inode);
 
 	minix_set_inode(inode, 0);
-	err = page_symlink(inode, symname, i);
+	err = iomap_symlink_write(inode, symname, i, minix_iomap_ops_ver(inode), NULL, NULL);
+
+	if (!err)
+		i_size_write(inode, i - 1);
+
 	if (unlikely(err)) {
 		inode_dec_link_count(inode);
 		iput(inode);
 		return err;
 	}
+
 	return add_nondir(dentry, inode);
 }
 
-- 
2.47.3

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

[Christoph Hellwig]

On Sat, Jul 11, 2026 at 01:59:04PM -0700, Jeremy Bingham wrote:
> This is version 3 of the minix iomap conversion patch series. Versions 1
> and 2 both had issues that syzbot found while testing, while Darrick J.
> Wong gave some critiques of the version 2 patches that I have addressed
> here.
> 
> This patch series converts the minix filesystem module from using the
> buffer_head based path to the iomap API, and adds direct I/O support in
> the process.
> 
> Patch 1 adds a new iomap_symlink_write function to

There's no point explaining each patch in the over letter, that's
what the commit log for each patch are for.

How did you test this?  How do the xfstests auto group results look
with an without the series for minix?

Re: [PATCH v3 1/5] iomap: add iomap_symlink_write

[Christoph Hellwig]

On Sat, Jul 11, 2026 at 01:59:05PM -0700, Jeremy Bingham wrote:
> Add a new iomap_symlink_write function as an iomap based equivalent to
> page_symlink found in fs/namei.c. This implementation was suggested by
> Darrick J. Wong, who also provided the initial implementation that I
> only needed to change slightly to get working.
> 
> Suggested-by: Darrick J. Wong <djwong@kernel.org>
> 
> Signed-off-by: Jeremy Bingham <jbingham@gmail.com>

No empty line between tags pleaase.

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

There is nothign symlink-specific in this code.  It's really just a
low-level write that doesn't need an iocb.

> +	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);

And given that a normal write doesn't mark the inode dirty we should
probably leave this to the caller for consistency.

> +	return 0;

And maybe also return the length writtenn for consistency?  Or if we
avoid partial writes for some reason, return an error when they happen?

Re: [PATCH v3 2/5] minix: add base iomap begin/end functions

[Christoph Hellwig]

On Sat, Jul 11, 2026 at 01:59:06PM -0700, Jeremy Bingham wrote:
> Adds a new file, iomap.c, to fs/minix. This provides minix_iomap_begin,
> which is the iomap-based version of get_block in itree_common.c. It also
> provides minix_iomap_end, which is a no-op but required by the iomap_ops
> struct. This also undoes the nest of gotos inherited from get_block and
> uses a set of helper functions instead.

You don't need to describe what you do, you need to decribe why you
do it.  Bonus points for explaining things like design tradeoffs.

Re: [PATCH v3 4/5] minix: add file operation functions

[Christoph Hellwig]

On Sat, Jul 11, 2026 at 01:59:08PM -0700, Jeremy Bingham wrote:
> This adds the minix iomap file operation functions, including direct I/O
> functions, and updates the minix_file_operations struct to use them.
> Previously, most of the function pointers in minix_file_operations were
> the defaults. With the iomap changes, though, the defaults are no longer
> sufficient and minix-specific functions are required.
> 
> The previously static minix_setattr function is also now exported; it will
> be used by minix_symlink_inode_operations.

How does this work without the next patch?

> +	struct inode *inode = iocb->ki_filp->f_mapping->host;
> +	ssize_t ret;
> +
> +	inode_lock_shared(inode);
> +
> +	const struct iomap_ops *ops = minix_iomap_ops_ver(inode);

Please keep all variable declarations at the top of the file.

> +
> +	ret = iomap_dio_rw(iocb, to, ops, NULL, 0, NULL, 0);
> +	inode_unlock_shared(inode);
> +	return ret;
> +}

But more importantly please explain the use case for adding direct I/O
support to minix, as adding new barely tested code to a legacy file
needs a very good justification.  And it really needs to be split into
a separate patch.