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

On 8/14/2026 6:35 PM, [email protected] wrote:
> 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?


This is a false positive. The iomap path doesn't support inline
data at the moment. But for future-proofing, I think we should
move the inline data conversion before the iomap path check,
even though it's harmless for now.

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

This is a false positive. Patch 18 adds a new function
ext4_block_iomap_zero_range() for the iomap path.

Thanks,
Yi.

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