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

[Jan Kara]

On Thu 22-10-26 10:29:11, Hengyu Liang wrote:
> 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.

You are right. I'm sorry for my confusion.

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

OK, that's a good observation. Let's keep your special function then.
Thanks!

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR