Re: [PATCH v4 3/3] minix: finish wiring in iomap functions

[Christoph Hellwig]

On Wed, Aug 26, 2026 at 02:41:57PM -0700, Jeremy Bingham wrote:
> Wire in the new iomap functionality in one pass. Per Christoph Hellwig's
> feedback, there are no longer direct I/O operations. Without that, only
> write_iter in minix_file_operations needs a custom function. That and
> exporting minix_setattr for minix_symlink_inode_operations are the only
> changes in file.c.

Commit history just goes into the cover letter.  In genral you don't
need to enumerate all low-level changes either.  Explain the high-level
change, what motivated it, and anything that looks a bit unusual and
unexpected for that high-level change.

Note that you probably want to merge this into the previous patch
adding the actual iomap ops instead of leaving them dangling between
thet two patches.

> +	ret = iomap_file_buffered_write(iocb, from, ops,
> +			NULL, NULL);

The two NULL still fit onto the previous line:

	ret = iomap_file_buffered_write(iocb, from, ops, NULL, NULL);

> +
> +	if (ret > 0)
> +		ret = generic_write_sync(iocb, ret);
> +
> +unlock:
> +	inode_unlock(inode);

For most file systems we try to have the generic_write_sync outside
the inode lock to not do the expensive sync with the inode locked.
generic_file_write_iter also doesn't have the inode locked, so you
should probably sync after dropping the lock here as swell.

> +int minix_setattr(struct mnt_idmap *idmap,
>  			 struct dentry *dentry, struct iattr *attr)

Odd formatting again.  The typical style would be:

int minix_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
		struct iattr *attr)

> +static ssize_t minix_writeback_range(struct iomap_writepage_ctx *wpc,
> +	struct folio *folio, u64 pos, unsigned int len, u64 end_pos)

Two-tab indents please:

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

Overly long lines.  These should hopefully go away for free with the
common iomap_ops.

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

minix_dir_writepages?

If you're looking for another project, we could probably also have
a iomap version of the directories in pagecache used by minix, ext2
and co eventually.

>  static 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,
> +};

Maybe use tabs to align the initializers if you touch most of them
anyway?

> +	/* Depending on whether the inode being truncated is a directory or not,
> +	 * we need to either call iomap_truncate_page or block_truncate_page.
> +	 */

The kernel coding style would be:

	/*
	 * Depending on whether the inode being truncated is a directory or not,
	 * we need to either call iomap_truncate_page or block_truncate_page.
	 */

> +	err = iomap_symlink_write(inode, symname, i, minix_iomap_ops_ver(inode), NULL, NULL);

Overly long line.