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

[Hengyu Liang]

Hi Jan,

On Thu 22-10-26 11:48:35, Jan Kara wrote:
> I agree with the problem but first, Ted has merged rewrite of ext4
> buffered IO path to iomap your fix would very likely need rebasing, and
> second:

Thanks for the review.

In the ext4 dev branch, the iomap buffered path is only used with the
new buffered_iomap mount option, and it faults the source in before
->write_begin(), so it does not have this problem. The default
buffer_head path still does: on 8c4f6e337634, a 100-byte file still
has a size of 1048586 after a failed pwrite() at 1 MiB + 10, sync and
remount. The patch applies to the dev branch without changes, and I
will base v2 on it.

> this is wrong and would leak delayed allocation reservations AFAICT. You
> should just use ext4_truncate_failed_write() like e.g.
> ext4_da_write_begin() does on failure.

I don't think it leaks. ext4_es_remove_extent() gives back the
reservation of the delayed blocks it removes through
ext4_da_release_space(), which ext4_iomap_punch_delalloc() also relies
on in the iomap path.

To check this, I ran my earlier tests on the dev branch with the patch
as sent and with the helper replaced by ext4_truncate_failed_write(),
plus a test on an image with the quota feature that reads the user's
quota usage (which includes reserved space) and the free block count.
After a failed write past EOF, both versions bring both numbers back
to their values before the write, and nothing is logged at umount.
Without the patch, the same case keeps 64 KiB reserved until the inode
is evicted.

I did not use ext4_truncate_failed_write() because since 665575cff098
this path is also taken by ordinary writes. When the source page is
not faulted in yet, the first copy fails, ->write_end() sees
copied == 0, and generic_perform_write() faults the page in and tries
again. ext4_truncate_failed_write() would then run ext4_truncate() for
each such write that starts past EOF, and that frees blocks
preallocated past EOF with FALLOC_FL_KEEP_SIZE, even though the write
succeeds right after. In my test, a 64 KiB write from untouched
anonymous memory into a file with 1 MiB preallocated past EOF leaves
st_blocks at 2056 with the helper, but drops it to 136 with
ext4_truncate_failed_write().

So I would prefer to keep the helper. Is that OK with you, or would
you still rather use ext4_truncate_failed_write()?

Thanks,
Hengyu