Re: [PATCH -next v5 17/32] ext4: implement mmap path using iomap

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] File operations that zero partial blocks will crash on iomap-enabled files due to legacy buffer_head dereferencing.
- [Medium] Bypass of inline data conversion for iomap-enabled inodes causes failures and kernel warnings.
--

--- Patch [17]: [PATCH -next v5 17/32] ext4: implement mmap path using iomap ---
commit 355e5d2bcead3197b1566acfa637f19866c3b52c
Author: Zhang Yi <[email protected]>
Subject: ext4: implement mmap path using iomap

This commit introduces ext4_iomap_page_mkwrite() to implement the mmap
iomap path for ext4. It delegates the heavy lifting to iomap_page_mkwrite(),
using ext4_iomap_buffered_write_ops and ext4_iomap_buffered_da_write_ops.
The lock ordering documentation in super.c is also updated accordingly.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 0b3e54e12b78f..a054456258953 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c

[ ... ]

> @@ -7318,6 +7343,11 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>  
>  	filemap_invalidate_lock_shared(mapping);
>  
> +	if (ext4_inode_buffered_iomap(inode)) {
> +		ret = ext4_iomap_page_mkwrite(vmf);
> +		goto out;
> +	}
> +

[Severity: Medium]
Will this bypass inline data conversion for iomap-enabled inodes?

If ext4_inode_buffered_iomap(inode) is true, we route to
ext4_iomap_page_mkwrite() and skip ext4_convert_inline_data(inode).

However, the iomap implementation explicitly forbids inline data, triggering
a warning and returning -ERANGE:

fs/ext4/inode.c:ext4_iomap_buffered_do_write_begin() {
    ...
    /* Inline data and non-extent are not supported. */
    if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
        return -ERANGE;
    ...
}

Should the inline data conversion happen before checking for the iomap path?

[Severity: Medium]
Will file operations that zero partial blocks crash on iomap-enabled files?

When an application performs an extending write or truncate up on a file
with EXT4_STATE_BUFFERED_IOMAP enabled, ext4_write_checks() unconditionally
calls ext4_block_zero_eof() without checking if the inode uses iomap:

fs/ext4/file.c:ext4_write_checks() {
    ...
        ret = ext4_block_zero_eof(inode, old_size, iocb->ki_pos);
    ...
}

This proceeds to ext4_block_do_zero_range() and ext4_load_tail_bh(), which
calls folio_buffers(). For an iomap folio, folio->private points to an
iomap_folio_state, not a buffer_head:

fs/ext4/inode.c:ext4_load_tail_bh() {
    ...
    bh = folio_buffers(folio);
    if (!bh)
        bh = create_empty_buffers(folio, blocksize, 0);

    /* Find the buffer that contains "offset" */
    offset = offset_in_folio(folio, from);
    pos = blocksize;
    while (offset >= pos) {
        bh = bh->b_this_page;
    ...
}

Treating iomap_folio_state as a buffer_head leads to a crash when iterating
bh->b_this_page. Since there are no callers setting EXT4_STATE_BUFFERED_IOMAP
in the current codebase, this is a latent regression. How should partial block
zeroing be handled for iomap-enabled files?

>  	err = ext4_convert_inline_data(inode);
>  	if (err)
>  		goto out_ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17