Re: [PATCH] ext4: do not advance i_size on a buffered write that copied nothing

[Hengyu Liang]

Hi Yi,

On 10/23/2026 10:00 AM, Zhang Yi wrote:
> As Jan mentioned, this issue actually also exists on the iomap path,
> although in my view the current approach can fix the problem on the
> buffer_head path.

Thanks for the review.

You are right, what I wrote to Jan was too strong. The iomap path
faults the source in first, so the reproducer in the patch does not
show the problem there. But iomap_write_iter() still sets i_size to pos
when written is 0 and pos is beyond i_size, so the same thing happens
if the source becomes unreadable after the fault-in, e.g. through a
racing munmap(). I think that part needs a fix as well.

> IIUC, 665575cff098 only increased the probability of the issue
> occurring. Before it, copied could also end up being 0 in some
> concurrent scenarios, because in block_write_end(), if the folio is not
> uptodate, copied will be set to 0 as long as it is less than len.
> Right?

Yes. Before that commit, ->write_end() could also see copied == 0,
after a short copy into a folio that is not uptodate, or when the
source was unmapped between the fault-in and the copy (commit
ea51d132dbf9 ("ext4: avoid hangs in ext4_da_should_update_i_disksize()")
dealt with that case for i_disksize). ext4 has moved i_size up to pos
in that case since commit bfc1af650a8f ("ext4: convert to new aops"),
and the delalloc path since commit 64769240bd07 ("ext4: Add delayed
allocation support in data=writeback mode"). The wrong size then
stayed only if the retry could not fault the source in either, so it
needed a race. Since 665575cff098 a single unreadable buffer is enough.
I will say this in the v2 commit message and keep the Fixes tag on
665575cff098, unless you prefer otherwise.

>> +	if (!copied && pos > old_size)
>
> I think it should be if (pos + len > old_size) here?

Agreed. Then a failed write that starts at or before EOF also gives
back what it reserved past EOF, as ext4_write_end() and the error path
of ext4_da_write_begin() already do with the same check. I ran the
earlier tests on the dev branch again with this change, plus new cases
where the write starts at or before EOF (4k and 1k blocks, bigalloc,
quota, large folios). File sizes and contents are the same as with the
patch as sent, e2fsck is clean, and the blocks reserved past EOF are
given back right after the failed write instead of at eviction.

There is one cost. Since 665575cff098, an append from a source page
that is not faulted in yet takes this path once before the retry, so
it now trims as well. On a kernel without debug options, such a 4 KiB
append went from about 2.3 us to 4.1 us, and a 64 KiB one from about
18 us to 26 us. Writes from memory that is already faulted in are not
affected, and with nodelalloc the same appends take about 10 us and
50 us, because that path already truncates in this case. So I think
the cost is fine, and v2 will use pos + len > old_size.

Jan suggested ext4_truncate_failed_write() instead of the new helper,
and my answer with test results is in this thread. I will wait for his
reply before sending v2.

Thanks,
Hengyu