Re: [f2fs-dev] [PATCH v2 3/3] f2fs: fix to zero data after EOF for compressed file correctly

[Jaegeuk Kim via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

Chao, it seems you made a clean up before this? Can you post it first?

On 08/04, Chao Yu wrote:
> generic/091 may fail, then it bisects to the bad commit ba8dac350faf
> ("f2fs: fix to zero post-eof page").
> 
> What will cause generic/091 to fail is something like below Testcase #1:
> 1. write 16k as compressed blocks
> 2. truncate to 12k
> 3. truncate to 20k
> 4. verify data in range of [12k, 16k], however data is not zero as
> expected
> 
> Script of Testcase #1
> mkfs.f2fs -f -O extra_attr,compression /dev/vdb
> mount -t f2fs -o compress_extension=* /dev/vdb /mnt/f2fs
> dd if=/dev/zero of=/mnt/f2fs/file bs=12k count=1
> dd if=/dev/random of=/mnt/f2fs/file bs=4k count=1 seek=3 conv=notrunc
> sync
> truncate -s $((12*1024)) /mnt/f2fs/file
> truncate -s $((20*1024)) /mnt/f2fs/file
> dd if=/mnt/f2fs/file of=/mnt/f2fs/data bs=4k count=1 skip=3
> od /mnt/f2fs/data
> umount /mnt/f2fs
> 
> Analisys:
> in step 2), we will redirty all data pages from #0 to #3 in compressed
> cluster, and zero page #3,
> in step 3), f2fs_setattr() will call f2fs_zero_post_eof_page() to drop
> all page cache post eof, includeing dirtied page #3,
> in step 4) when we read data from page #3, it will decompressed cluster
> and extra random data to page #3, finally, we hit the non-zeroed data
> post eof.
> 
> However, the commit ba8dac350faf ("f2fs: fix to zero post-eof page") just
> let the issue be reproduced easily, w/o the commit, it can reproduce this
> bug w/ below Testcase #2:
> 1. write 16k as compressed blocks
> 2. truncate to 8k
> 3. truncate to 12k
> 4. truncate to 20k
> 5. verify data in range of [12k, 16k], however data is not zero as
> expected
> 
> Script of Testcase #2
> mkfs.f2fs -f -O extra_attr,compression /dev/vdb
> mount -t f2fs -o compress_extension=* /dev/vdb /mnt/f2fs
> dd if=/dev/zero of=/mnt/f2fs/file bs=12k count=1
> dd if=/dev/random of=/mnt/f2fs/file bs=4k count=1 seek=3 conv=notrunc
> sync
> truncate -s $((8*1024)) /mnt/f2fs/file
> truncate -s $((12*1024)) /mnt/f2fs/file
> truncate -s $((20*1024)) /mnt/f2fs/file
> echo 3 > /proc/sys/vm/drop_caches
> dd if=/mnt/f2fs/file of=/mnt/f2fs/data bs=4k count=1 skip=3
> od /mnt/f2fs/data
> umount /mnt/f2fs
> 
> Anlysis:
> in step 2), we will redirty all data pages from #0 to #3 in compressed
> cluster, and zero page #2 and #3,
> in step 3), we will truncate page #3 in page cache,
> in step 4), expand file size,
> in step 5), hit random data post eof w/ the same reason in Testcase #1.
> 
> Root Cause:
> In f2fs_truncate_partial_cluster(), after we truncate partial data block
> on compressed cluster, all pages in cluster including the one post eof
> will be dirtied, after another tuncation, dirty page post eof will be
> dropped, however on-disk compressed cluster is still valid, it includes
> invalid data post eof, result in exposing previous data post eof while
> reading.
> 
> Fix:
> In f2fs_truncate_partial_cluster(), let change as below to fix:
> - call filemap_write_and_wait_range() to flush dirty page
> - call truncate_pagecache() to drop pages or zero partial page post eof
> - call f2fs_do_truncate_blocks() to truncate non-compress cluster to
>   last vali block
> 
> Fixes: 3265d3db1f16 ("f2fs: support partial truncation on compressed inode")
> Reported-by: Jan Prusakowski <jprusakowski@google.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> v2:
> - should dirty & flush all pages in cluster and truncate blocks post eof
> later
>  fs/f2fs/compress.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index e37a7ed801e5..6ad8d3bc6df7 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1245,17 +1245,25 @@ int f2fs_truncate_partial_cluster(struct inode *inode, u64 from, bool lock)
>  	for (i = (1 << log_cluster_size) - 1; i >= 0; i--) {
>  		struct folio *folio = page_folio(rpages[i]);
>  		loff_t start = (loff_t)folio->index << PAGE_SHIFT;
> +		loff_t offset = from > start ? from - start : 0;
>  
> -		if (from > start) {
> -			folio_zero_segment(folio, from - start,
> -					folio_size(folio));
> +		folio_zero_segment(folio, offset, folio_size(folio));
> +
> +		if (from >= start)
>  			break;
> -		}
> -		folio_zero_segment(folio, 0, folio_size(folio));
>  	}
>  
>  	f2fs_compress_write_end(inode, fsdata, start_idx, true);
> -	return 0;
> +
> +	err = filemap_write_and_wait_range(inode->i_mapping,
> +			round_down(from, 1 << log_cluster_size << PAGE_SHIFT),
> +			LLONG_MAX);
> +	if (err)
> +		return err;
> +
> +	truncate_pagecache(inode, from);
> +
> +	return f2fs_do_truncate_blocks(inode, round_up(from, PAGE_SIZE), lock);
>  }
>  
>  static int f2fs_write_compressed_pages(struct compress_ctx *cc,
> -- 
> 2.49.0


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel