* [f2fs-dev] [PATCH] f2fs: compress: don't redirty sparse cluster during {, de}compress [not found] <CGME20240813061105epcas1p4898159104ce2ee12ef6c203c4ab828ff@epcas1p4.samsung.com> @ 2024-08-13 6:11 ` Yeongjin Gil 0 siblings, 0 replies; 6+ messages in thread From: Yeongjin Gil @ 2024-08-13 6:11 UTC (permalink / raw) To: jaegeuk, chao; +Cc: linux-f2fs-devel, linux-kernel, Jaewook Kim, Sungjong Seo In f2fs_do_write_data_page, when the data block is NULL_ADDR, it skips writepage considering that it has been already truncated. This results in an infinite loop as the PAGECACHE_TAG_TOWRITE tag is not cleared during the writeback process for a compressed file including NULL_ADDR in compress_mode=user. This is the reproduction process: 1. dd if=/dev/zero bs=4096 count=1024 seek=1024 of=testfile 2. f2fs_io compress testfile 3. dd if=/dev/zero bs=4096 count=1 conv=notrunc of=testfile 4. f2fs_io decompress testfile To prevent the problem, let's check whether the cluster is fully allocated before redirty its pages. Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com> Tested-by: Jaewook Kim <jw5454.kim@samsung.com> Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com> --- fs/f2fs/compress.c | 34 ++++++++++++++++++++++++++-------- fs/f2fs/f2fs.h | 12 ++++++++++++ fs/f2fs/file.c | 39 +++++++++++++++++++++------------------ 3 files changed, 59 insertions(+), 26 deletions(-) diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index 990b93689b46..09b91d725807 100644 --- a/fs/f2fs/compress.c +++ b/fs/f2fs/compress.c @@ -945,7 +945,7 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, unsigned int cluster_size = F2FS_I(inode)->i_cluster_size; int count, i; - for (i = 1, count = 1; i < cluster_size; i++) { + for (i = 0, count = 0; i < cluster_size; i++) { block_t blkaddr = data_blkaddr(dn->inode, dn->node_page, dn->ofs_in_node + i); @@ -956,8 +956,8 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, return count; } -static int __f2fs_cluster_blocks(struct inode *inode, - unsigned int cluster_idx, bool compr_blks) +static int __f2fs_cluster_blocks(struct inode *inode, unsigned int cluster_idx, + enum cluster_check_type type) { struct dnode_of_data dn; unsigned int start_idx = cluster_idx << @@ -978,10 +978,12 @@ static int __f2fs_cluster_blocks(struct inode *inode, } if (dn.data_blkaddr == COMPRESS_ADDR) { - if (compr_blks) - ret = __f2fs_get_cluster_blocks(inode, &dn); - else + if (type == COMPR_BLKS) + ret = 1 + __f2fs_get_cluster_blocks(inode, &dn); + else if (type == COMPR_CLUSTER) ret = 1; + } else if (type == DECOMPR_BLKS) { + ret = __f2fs_get_cluster_blocks(inode, &dn); } fail: f2fs_put_dnode(&dn); @@ -991,7 +993,14 @@ static int __f2fs_cluster_blocks(struct inode *inode, /* return # of compressed blocks in compressed cluster */ static int f2fs_compressed_blocks(struct compress_ctx *cc) { - return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, true); + return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, COMPR_BLKS); +} + +/* return # of raw blocks in non-compressed cluster */ +static int f2fs_decompressed_blocks(struct inode *inode, + unsigned int cluster_idx) +{ + return __f2fs_cluster_blocks(inode, cluster_idx, DECOMPR_BLKS); } /* return whether cluster is compressed one or not */ @@ -999,7 +1008,16 @@ int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index) { return __f2fs_cluster_blocks(inode, index >> F2FS_I(inode)->i_log_cluster_size, - false); + COMPR_CLUSTER); +} + +/* return whether the cluster is filled with raw blocks */ +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index) +{ + unsigned int cluster_idx = index >> F2FS_I(inode)->i_log_cluster_size; + + return f2fs_decompressed_blocks(inode, cluster_idx) == + F2FS_I(inode)->i_cluster_size; } static bool cluster_may_compress(struct compress_ctx *cc) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 51fd5063a69c..6f26c8cb8857 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -4302,6 +4302,11 @@ static inline bool f2fs_meta_inode_gc_required(struct inode *inode) * compress.c */ #ifdef CONFIG_F2FS_FS_COMPRESSION +enum cluster_check_type { + COMPR_CLUSTER, + COMPR_BLKS, + DECOMPR_BLKS +}; bool f2fs_is_compressed_page(struct page *page); struct page *f2fs_compress_control_page(struct page *page); int f2fs_prepare_compress_overwrite(struct inode *inode, @@ -4328,6 +4333,7 @@ int f2fs_write_multi_pages(struct compress_ctx *cc, struct writeback_control *wbc, enum iostat_type io_type); int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index); +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index); void f2fs_update_read_extent_tree_range_compressed(struct inode *inode, pgoff_t fofs, block_t blkaddr, unsigned int llen, unsigned int c_len); @@ -4414,6 +4420,12 @@ static inline bool f2fs_load_compressed_page(struct f2fs_sb_info *sbi, static inline void f2fs_invalidate_compress_pages(struct f2fs_sb_info *sbi, nid_t ino) { } #define inc_compr_inode_stat(inode) do { } while (0) +static inline int f2fs_is_compressed_cluster( + struct inode *inode, + pgoff_t index) { return 0; } +static inline bool f2fs_is_non_sparse_cluster( + struct inode *inode, + pgoff_t index) { return false; } static inline void f2fs_update_read_extent_tree_range_compressed( struct inode *inode, pgoff_t fofs, block_t blkaddr, diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 270c32e3385f..fba8b5f216f9 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -4220,9 +4220,8 @@ static int f2fs_ioc_decompress_file(struct file *filp) struct inode *inode = file_inode(filp); struct f2fs_sb_info *sbi = F2FS_I_SB(inode); struct f2fs_inode_info *fi = F2FS_I(inode); - pgoff_t page_idx = 0, last_idx; - int cluster_size = fi->i_cluster_size; - int count, ret; + pgoff_t page_idx = 0, last_idx, cluster_idx; + int ret; if (!f2fs_sb_has_compression(sbi) || F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ -4257,10 +4256,15 @@ static int f2fs_ioc_decompress_file(struct file *filp) goto out; last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); + last_idx >>= fi->i_log_cluster_size; + + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { + page_idx = cluster_idx << fi->i_log_cluster_size; + + if (!f2fs_is_compressed_cluster(inode, page_idx)) + continue; - count = last_idx - page_idx; - while (count && count >= cluster_size) { - ret = redirty_blocks(inode, page_idx, cluster_size); + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); if (ret < 0) break; @@ -4270,9 +4274,6 @@ static int f2fs_ioc_decompress_file(struct file *filp) break; } - count -= cluster_size; - page_idx += cluster_size; - cond_resched(); if (fatal_signal_pending(current)) { ret = -EINTR; @@ -4299,9 +4300,9 @@ static int f2fs_ioc_compress_file(struct file *filp) { struct inode *inode = file_inode(filp); struct f2fs_sb_info *sbi = F2FS_I_SB(inode); - pgoff_t page_idx = 0, last_idx; - int cluster_size = F2FS_I(inode)->i_cluster_size; - int count, ret; + struct f2fs_inode_info *fi = F2FS_I(inode); + pgoff_t page_idx = 0, last_idx, cluster_idx; + int ret; if (!f2fs_sb_has_compression(sbi) || F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ -4335,10 +4336,15 @@ static int f2fs_ioc_compress_file(struct file *filp) set_inode_flag(inode, FI_ENABLE_COMPRESS); last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); + last_idx >>= fi->i_log_cluster_size; - count = last_idx - page_idx; - while (count && count >= cluster_size) { - ret = redirty_blocks(inode, page_idx, cluster_size); + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { + page_idx = cluster_idx << fi->i_log_cluster_size; + + if (!f2fs_is_non_sparse_cluster(inode, page_idx)) + continue; + + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); if (ret < 0) break; @@ -4348,9 +4354,6 @@ static int f2fs_ioc_compress_file(struct file *filp) break; } - count -= cluster_size; - page_idx += cluster_size; - cond_resched(); if (fatal_signal_pending(current)) { ret = -EINTR; -- 2.40.1 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress @ 2024-08-13 6:11 ` Yeongjin Gil 0 siblings, 0 replies; 6+ messages in thread From: Yeongjin Gil @ 2024-08-13 6:11 UTC (permalink / raw) To: jaegeuk, chao Cc: linux-f2fs-devel, linux-kernel, Yeongjin Gil, Sungjong Seo, Sunmin Jeong, Jaewook Kim In f2fs_do_write_data_page, when the data block is NULL_ADDR, it skips writepage considering that it has been already truncated. This results in an infinite loop as the PAGECACHE_TAG_TOWRITE tag is not cleared during the writeback process for a compressed file including NULL_ADDR in compress_mode=user. This is the reproduction process: 1. dd if=/dev/zero bs=4096 count=1024 seek=1024 of=testfile 2. f2fs_io compress testfile 3. dd if=/dev/zero bs=4096 count=1 conv=notrunc of=testfile 4. f2fs_io decompress testfile To prevent the problem, let's check whether the cluster is fully allocated before redirty its pages. Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com> Tested-by: Jaewook Kim <jw5454.kim@samsung.com> Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com> --- fs/f2fs/compress.c | 34 ++++++++++++++++++++++++++-------- fs/f2fs/f2fs.h | 12 ++++++++++++ fs/f2fs/file.c | 39 +++++++++++++++++++++------------------ 3 files changed, 59 insertions(+), 26 deletions(-) diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index 990b93689b46..09b91d725807 100644 --- a/fs/f2fs/compress.c +++ b/fs/f2fs/compress.c @@ -945,7 +945,7 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, unsigned int cluster_size = F2FS_I(inode)->i_cluster_size; int count, i; - for (i = 1, count = 1; i < cluster_size; i++) { + for (i = 0, count = 0; i < cluster_size; i++) { block_t blkaddr = data_blkaddr(dn->inode, dn->node_page, dn->ofs_in_node + i); @@ -956,8 +956,8 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, return count; } -static int __f2fs_cluster_blocks(struct inode *inode, - unsigned int cluster_idx, bool compr_blks) +static int __f2fs_cluster_blocks(struct inode *inode, unsigned int cluster_idx, + enum cluster_check_type type) { struct dnode_of_data dn; unsigned int start_idx = cluster_idx << @@ -978,10 +978,12 @@ static int __f2fs_cluster_blocks(struct inode *inode, } if (dn.data_blkaddr == COMPRESS_ADDR) { - if (compr_blks) - ret = __f2fs_get_cluster_blocks(inode, &dn); - else + if (type == COMPR_BLKS) + ret = 1 + __f2fs_get_cluster_blocks(inode, &dn); + else if (type == COMPR_CLUSTER) ret = 1; + } else if (type == DECOMPR_BLKS) { + ret = __f2fs_get_cluster_blocks(inode, &dn); } fail: f2fs_put_dnode(&dn); @@ -991,7 +993,14 @@ static int __f2fs_cluster_blocks(struct inode *inode, /* return # of compressed blocks in compressed cluster */ static int f2fs_compressed_blocks(struct compress_ctx *cc) { - return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, true); + return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, COMPR_BLKS); +} + +/* return # of raw blocks in non-compressed cluster */ +static int f2fs_decompressed_blocks(struct inode *inode, + unsigned int cluster_idx) +{ + return __f2fs_cluster_blocks(inode, cluster_idx, DECOMPR_BLKS); } /* return whether cluster is compressed one or not */ @@ -999,7 +1008,16 @@ int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index) { return __f2fs_cluster_blocks(inode, index >> F2FS_I(inode)->i_log_cluster_size, - false); + COMPR_CLUSTER); +} + +/* return whether the cluster is filled with raw blocks */ +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index) +{ + unsigned int cluster_idx = index >> F2FS_I(inode)->i_log_cluster_size; + + return f2fs_decompressed_blocks(inode, cluster_idx) == + F2FS_I(inode)->i_cluster_size; } static bool cluster_may_compress(struct compress_ctx *cc) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 51fd5063a69c..6f26c8cb8857 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -4302,6 +4302,11 @@ static inline bool f2fs_meta_inode_gc_required(struct inode *inode) * compress.c */ #ifdef CONFIG_F2FS_FS_COMPRESSION +enum cluster_check_type { + COMPR_CLUSTER, + COMPR_BLKS, + DECOMPR_BLKS +}; bool f2fs_is_compressed_page(struct page *page); struct page *f2fs_compress_control_page(struct page *page); int f2fs_prepare_compress_overwrite(struct inode *inode, @@ -4328,6 +4333,7 @@ int f2fs_write_multi_pages(struct compress_ctx *cc, struct writeback_control *wbc, enum iostat_type io_type); int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index); +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index); void f2fs_update_read_extent_tree_range_compressed(struct inode *inode, pgoff_t fofs, block_t blkaddr, unsigned int llen, unsigned int c_len); @@ -4414,6 +4420,12 @@ static inline bool f2fs_load_compressed_page(struct f2fs_sb_info *sbi, static inline void f2fs_invalidate_compress_pages(struct f2fs_sb_info *sbi, nid_t ino) { } #define inc_compr_inode_stat(inode) do { } while (0) +static inline int f2fs_is_compressed_cluster( + struct inode *inode, + pgoff_t index) { return 0; } +static inline bool f2fs_is_non_sparse_cluster( + struct inode *inode, + pgoff_t index) { return false; } static inline void f2fs_update_read_extent_tree_range_compressed( struct inode *inode, pgoff_t fofs, block_t blkaddr, diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 270c32e3385f..fba8b5f216f9 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -4220,9 +4220,8 @@ static int f2fs_ioc_decompress_file(struct file *filp) struct inode *inode = file_inode(filp); struct f2fs_sb_info *sbi = F2FS_I_SB(inode); struct f2fs_inode_info *fi = F2FS_I(inode); - pgoff_t page_idx = 0, last_idx; - int cluster_size = fi->i_cluster_size; - int count, ret; + pgoff_t page_idx = 0, last_idx, cluster_idx; + int ret; if (!f2fs_sb_has_compression(sbi) || F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ -4257,10 +4256,15 @@ static int f2fs_ioc_decompress_file(struct file *filp) goto out; last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); + last_idx >>= fi->i_log_cluster_size; + + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { + page_idx = cluster_idx << fi->i_log_cluster_size; + + if (!f2fs_is_compressed_cluster(inode, page_idx)) + continue; - count = last_idx - page_idx; - while (count && count >= cluster_size) { - ret = redirty_blocks(inode, page_idx, cluster_size); + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); if (ret < 0) break; @@ -4270,9 +4274,6 @@ static int f2fs_ioc_decompress_file(struct file *filp) break; } - count -= cluster_size; - page_idx += cluster_size; - cond_resched(); if (fatal_signal_pending(current)) { ret = -EINTR; @@ -4299,9 +4300,9 @@ static int f2fs_ioc_compress_file(struct file *filp) { struct inode *inode = file_inode(filp); struct f2fs_sb_info *sbi = F2FS_I_SB(inode); - pgoff_t page_idx = 0, last_idx; - int cluster_size = F2FS_I(inode)->i_cluster_size; - int count, ret; + struct f2fs_inode_info *fi = F2FS_I(inode); + pgoff_t page_idx = 0, last_idx, cluster_idx; + int ret; if (!f2fs_sb_has_compression(sbi) || F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ -4335,10 +4336,15 @@ static int f2fs_ioc_compress_file(struct file *filp) set_inode_flag(inode, FI_ENABLE_COMPRESS); last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); + last_idx >>= fi->i_log_cluster_size; - count = last_idx - page_idx; - while (count && count >= cluster_size) { - ret = redirty_blocks(inode, page_idx, cluster_size); + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { + page_idx = cluster_idx << fi->i_log_cluster_size; + + if (!f2fs_is_non_sparse_cluster(inode, page_idx)) + continue; + + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); if (ret < 0) break; @@ -4348,9 +4354,6 @@ static int f2fs_ioc_compress_file(struct file *filp) break; } - count -= cluster_size; - page_idx += cluster_size; - cond_resched(); if (fatal_signal_pending(current)) { ret = -EINTR; -- 2.40.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: compress: don't redirty sparse cluster during {, de}compress 2024-08-13 6:11 ` [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress Yeongjin Gil @ 2024-08-15 10:12 ` Chao Yu -1 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2024-08-15 10:12 UTC (permalink / raw) To: Yeongjin Gil, jaegeuk Cc: Sungjong Seo, Jaewook Kim, linux-kernel, linux-f2fs-devel On 2024/8/13 14:11, Yeongjin Gil wrote: > In f2fs_do_write_data_page, when the data block is NULL_ADDR, it skips > writepage considering that it has been already truncated. > This results in an infinite loop as the PAGECACHE_TAG_TOWRITE tag is not > cleared during the writeback process for a compressed file including > NULL_ADDR in compress_mode=user. > > This is the reproduction process: > > 1. dd if=/dev/zero bs=4096 count=1024 seek=1024 of=testfile > 2. f2fs_io compress testfile > 3. dd if=/dev/zero bs=4096 count=1 conv=notrunc of=testfile > 4. f2fs_io decompress testfile Good catch! > > To prevent the problem, let's check whether the cluster is fully > allocated before redirty its pages. > > Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> > Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com> > Tested-by: Jaewook Kim <jw5454.kim@samsung.com> > Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com> Need a fixes line? > --- > fs/f2fs/compress.c | 34 ++++++++++++++++++++++++++-------- > fs/f2fs/f2fs.h | 12 ++++++++++++ > fs/f2fs/file.c | 39 +++++++++++++++++++++------------------ > 3 files changed, 59 insertions(+), 26 deletions(-) > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > index 990b93689b46..09b91d725807 100644 > --- a/fs/f2fs/compress.c > +++ b/fs/f2fs/compress.c > @@ -945,7 +945,7 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, > unsigned int cluster_size = F2FS_I(inode)->i_cluster_size; > int count, i; > > - for (i = 1, count = 1; i < cluster_size; i++) { > + for (i = 0, count = 0; i < cluster_size; i++) { > block_t blkaddr = data_blkaddr(dn->inode, dn->node_page, > dn->ofs_in_node + i); > > @@ -956,8 +956,8 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, > return count; > } > > -static int __f2fs_cluster_blocks(struct inode *inode, > - unsigned int cluster_idx, bool compr_blks) > +static int __f2fs_cluster_blocks(struct inode *inode, unsigned int cluster_idx, > + enum cluster_check_type type) > { > struct dnode_of_data dn; > unsigned int start_idx = cluster_idx << > @@ -978,10 +978,12 @@ static int __f2fs_cluster_blocks(struct inode *inode, > } > > if (dn.data_blkaddr == COMPRESS_ADDR) { > - if (compr_blks) > - ret = __f2fs_get_cluster_blocks(inode, &dn); > - else > + if (type == COMPR_BLKS) > + ret = 1 + __f2fs_get_cluster_blocks(inode, &dn); > + else if (type == COMPR_CLUSTER) > ret = 1; > + } else if (type == DECOMPR_BLKS) { > + ret = __f2fs_get_cluster_blocks(inode, &dn); > } > fail: > f2fs_put_dnode(&dn); > @@ -991,7 +993,14 @@ static int __f2fs_cluster_blocks(struct inode *inode, > /* return # of compressed blocks in compressed cluster */ > static int f2fs_compressed_blocks(struct compress_ctx *cc) > { > - return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, true); > + return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, COMPR_BLKS); > +} > + > +/* return # of raw blocks in non-compressed cluster */ > +static int f2fs_decompressed_blocks(struct inode *inode, > + unsigned int cluster_idx) > +{ > + return __f2fs_cluster_blocks(inode, cluster_idx, DECOMPR_BLKS); > } > > /* return whether cluster is compressed one or not */ > @@ -999,7 +1008,16 @@ int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index) > { > return __f2fs_cluster_blocks(inode, > index >> F2FS_I(inode)->i_log_cluster_size, > - false); > + COMPR_CLUSTER); > +} > + > +/* return whether the cluster is filled with raw blocks */ > +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index) f2fs_is_sparse_cluster()? > +{ > + unsigned int cluster_idx = index >> F2FS_I(inode)->i_log_cluster_size; > + > + return f2fs_decompressed_blocks(inode, cluster_idx) == > + F2FS_I(inode)->i_cluster_size; > } > > static bool cluster_may_compress(struct compress_ctx *cc) > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 51fd5063a69c..6f26c8cb8857 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -4302,6 +4302,11 @@ static inline bool f2fs_meta_inode_gc_required(struct inode *inode) > * compress.c > */ > #ifdef CONFIG_F2FS_FS_COMPRESSION > +enum cluster_check_type { > + COMPR_CLUSTER, > + COMPR_BLKS, > + DECOMPR_BLKS How about? CLUSTER_IS_COMPR, CLUSTER_COMPR_BLKS, CLUSTER_RAW_BLKS > +}; Can you please add some comments for these enums? > bool f2fs_is_compressed_page(struct page *page); > struct page *f2fs_compress_control_page(struct page *page); > int f2fs_prepare_compress_overwrite(struct inode *inode, > @@ -4328,6 +4333,7 @@ int f2fs_write_multi_pages(struct compress_ctx *cc, > struct writeback_control *wbc, > enum iostat_type io_type); > int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index); > +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index); > void f2fs_update_read_extent_tree_range_compressed(struct inode *inode, > pgoff_t fofs, block_t blkaddr, > unsigned int llen, unsigned int c_len); > @@ -4414,6 +4420,12 @@ static inline bool f2fs_load_compressed_page(struct f2fs_sb_info *sbi, > static inline void f2fs_invalidate_compress_pages(struct f2fs_sb_info *sbi, > nid_t ino) { } > #define inc_compr_inode_stat(inode) do { } while (0) > +static inline int f2fs_is_compressed_cluster( > + struct inode *inode, > + pgoff_t index) { return 0; } > +static inline bool f2fs_is_non_sparse_cluster( > + struct inode *inode, > + pgoff_t index) { return false; } > static inline void f2fs_update_read_extent_tree_range_compressed( > struct inode *inode, > pgoff_t fofs, block_t blkaddr, > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 270c32e3385f..fba8b5f216f9 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -4220,9 +4220,8 @@ static int f2fs_ioc_decompress_file(struct file *filp) > struct inode *inode = file_inode(filp); > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > struct f2fs_inode_info *fi = F2FS_I(inode); > - pgoff_t page_idx = 0, last_idx; > - int cluster_size = fi->i_cluster_size; > - int count, ret; > + pgoff_t page_idx = 0, last_idx, cluster_idx; > + int ret; > > if (!f2fs_sb_has_compression(sbi) || > F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) > @@ -4257,10 +4256,15 @@ static int f2fs_ioc_decompress_file(struct file *filp) > goto out; > > last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); > + last_idx >>= fi->i_log_cluster_size; > + > + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { > + page_idx = cluster_idx << fi->i_log_cluster_size; > + > + if (!f2fs_is_compressed_cluster(inode, page_idx)) > + continue; > > - count = last_idx - page_idx; > - while (count && count >= cluster_size) { > - ret = redirty_blocks(inode, page_idx, cluster_size); > + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); > if (ret < 0) > break; > > @@ -4270,9 +4274,6 @@ static int f2fs_ioc_decompress_file(struct file *filp) > break; > } > > - count -= cluster_size; > - page_idx += cluster_size; > - > cond_resched(); > if (fatal_signal_pending(current)) { > ret = -EINTR; > @@ -4299,9 +4300,9 @@ static int f2fs_ioc_compress_file(struct file *filp) > { > struct inode *inode = file_inode(filp); > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > - pgoff_t page_idx = 0, last_idx; > - int cluster_size = F2FS_I(inode)->i_cluster_size; > - int count, ret; > + struct f2fs_inode_info *fi = F2FS_I(inode); > + pgoff_t page_idx = 0, last_idx, cluster_idx; > + int ret; > > if (!f2fs_sb_has_compression(sbi) || > F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) > @@ -4335,10 +4336,15 @@ static int f2fs_ioc_compress_file(struct file *filp) > set_inode_flag(inode, FI_ENABLE_COMPRESS); > > last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); > + last_idx >>= fi->i_log_cluster_size; > > - count = last_idx - page_idx; > - while (count && count >= cluster_size) { > - ret = redirty_blocks(inode, page_idx, cluster_size); > + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { > + page_idx = cluster_idx << fi->i_log_cluster_size; > + > + if (!f2fs_is_non_sparse_cluster(inode, page_idx)) if (f2fs_is_sparse_cluster()) continue; Thanks, > + continue; > + > + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); > if (ret < 0) > break; > > @@ -4348,9 +4354,6 @@ static int f2fs_ioc_compress_file(struct file *filp) > break; > } > > - count -= cluster_size; > - page_idx += cluster_size; > - > cond_resched(); > if (fatal_signal_pending(current)) { > ret = -EINTR; _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress @ 2024-08-15 10:12 ` Chao Yu 0 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2024-08-15 10:12 UTC (permalink / raw) To: Yeongjin Gil, jaegeuk Cc: linux-f2fs-devel, linux-kernel, Sungjong Seo, Sunmin Jeong, Jaewook Kim On 2024/8/13 14:11, Yeongjin Gil wrote: > In f2fs_do_write_data_page, when the data block is NULL_ADDR, it skips > writepage considering that it has been already truncated. > This results in an infinite loop as the PAGECACHE_TAG_TOWRITE tag is not > cleared during the writeback process for a compressed file including > NULL_ADDR in compress_mode=user. > > This is the reproduction process: > > 1. dd if=/dev/zero bs=4096 count=1024 seek=1024 of=testfile > 2. f2fs_io compress testfile > 3. dd if=/dev/zero bs=4096 count=1 conv=notrunc of=testfile > 4. f2fs_io decompress testfile Good catch! > > To prevent the problem, let's check whether the cluster is fully > allocated before redirty its pages. > > Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> > Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com> > Tested-by: Jaewook Kim <jw5454.kim@samsung.com> > Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com> Need a fixes line? > --- > fs/f2fs/compress.c | 34 ++++++++++++++++++++++++++-------- > fs/f2fs/f2fs.h | 12 ++++++++++++ > fs/f2fs/file.c | 39 +++++++++++++++++++++------------------ > 3 files changed, 59 insertions(+), 26 deletions(-) > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > index 990b93689b46..09b91d725807 100644 > --- a/fs/f2fs/compress.c > +++ b/fs/f2fs/compress.c > @@ -945,7 +945,7 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, > unsigned int cluster_size = F2FS_I(inode)->i_cluster_size; > int count, i; > > - for (i = 1, count = 1; i < cluster_size; i++) { > + for (i = 0, count = 0; i < cluster_size; i++) { > block_t blkaddr = data_blkaddr(dn->inode, dn->node_page, > dn->ofs_in_node + i); > > @@ -956,8 +956,8 @@ static int __f2fs_get_cluster_blocks(struct inode *inode, > return count; > } > > -static int __f2fs_cluster_blocks(struct inode *inode, > - unsigned int cluster_idx, bool compr_blks) > +static int __f2fs_cluster_blocks(struct inode *inode, unsigned int cluster_idx, > + enum cluster_check_type type) > { > struct dnode_of_data dn; > unsigned int start_idx = cluster_idx << > @@ -978,10 +978,12 @@ static int __f2fs_cluster_blocks(struct inode *inode, > } > > if (dn.data_blkaddr == COMPRESS_ADDR) { > - if (compr_blks) > - ret = __f2fs_get_cluster_blocks(inode, &dn); > - else > + if (type == COMPR_BLKS) > + ret = 1 + __f2fs_get_cluster_blocks(inode, &dn); > + else if (type == COMPR_CLUSTER) > ret = 1; > + } else if (type == DECOMPR_BLKS) { > + ret = __f2fs_get_cluster_blocks(inode, &dn); > } > fail: > f2fs_put_dnode(&dn); > @@ -991,7 +993,14 @@ static int __f2fs_cluster_blocks(struct inode *inode, > /* return # of compressed blocks in compressed cluster */ > static int f2fs_compressed_blocks(struct compress_ctx *cc) > { > - return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, true); > + return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, COMPR_BLKS); > +} > + > +/* return # of raw blocks in non-compressed cluster */ > +static int f2fs_decompressed_blocks(struct inode *inode, > + unsigned int cluster_idx) > +{ > + return __f2fs_cluster_blocks(inode, cluster_idx, DECOMPR_BLKS); > } > > /* return whether cluster is compressed one or not */ > @@ -999,7 +1008,16 @@ int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index) > { > return __f2fs_cluster_blocks(inode, > index >> F2FS_I(inode)->i_log_cluster_size, > - false); > + COMPR_CLUSTER); > +} > + > +/* return whether the cluster is filled with raw blocks */ > +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index) f2fs_is_sparse_cluster()? > +{ > + unsigned int cluster_idx = index >> F2FS_I(inode)->i_log_cluster_size; > + > + return f2fs_decompressed_blocks(inode, cluster_idx) == > + F2FS_I(inode)->i_cluster_size; > } > > static bool cluster_may_compress(struct compress_ctx *cc) > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 51fd5063a69c..6f26c8cb8857 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -4302,6 +4302,11 @@ static inline bool f2fs_meta_inode_gc_required(struct inode *inode) > * compress.c > */ > #ifdef CONFIG_F2FS_FS_COMPRESSION > +enum cluster_check_type { > + COMPR_CLUSTER, > + COMPR_BLKS, > + DECOMPR_BLKS How about? CLUSTER_IS_COMPR, CLUSTER_COMPR_BLKS, CLUSTER_RAW_BLKS > +}; Can you please add some comments for these enums? > bool f2fs_is_compressed_page(struct page *page); > struct page *f2fs_compress_control_page(struct page *page); > int f2fs_prepare_compress_overwrite(struct inode *inode, > @@ -4328,6 +4333,7 @@ int f2fs_write_multi_pages(struct compress_ctx *cc, > struct writeback_control *wbc, > enum iostat_type io_type); > int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index); > +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index); > void f2fs_update_read_extent_tree_range_compressed(struct inode *inode, > pgoff_t fofs, block_t blkaddr, > unsigned int llen, unsigned int c_len); > @@ -4414,6 +4420,12 @@ static inline bool f2fs_load_compressed_page(struct f2fs_sb_info *sbi, > static inline void f2fs_invalidate_compress_pages(struct f2fs_sb_info *sbi, > nid_t ino) { } > #define inc_compr_inode_stat(inode) do { } while (0) > +static inline int f2fs_is_compressed_cluster( > + struct inode *inode, > + pgoff_t index) { return 0; } > +static inline bool f2fs_is_non_sparse_cluster( > + struct inode *inode, > + pgoff_t index) { return false; } > static inline void f2fs_update_read_extent_tree_range_compressed( > struct inode *inode, > pgoff_t fofs, block_t blkaddr, > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 270c32e3385f..fba8b5f216f9 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -4220,9 +4220,8 @@ static int f2fs_ioc_decompress_file(struct file *filp) > struct inode *inode = file_inode(filp); > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > struct f2fs_inode_info *fi = F2FS_I(inode); > - pgoff_t page_idx = 0, last_idx; > - int cluster_size = fi->i_cluster_size; > - int count, ret; > + pgoff_t page_idx = 0, last_idx, cluster_idx; > + int ret; > > if (!f2fs_sb_has_compression(sbi) || > F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) > @@ -4257,10 +4256,15 @@ static int f2fs_ioc_decompress_file(struct file *filp) > goto out; > > last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); > + last_idx >>= fi->i_log_cluster_size; > + > + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { > + page_idx = cluster_idx << fi->i_log_cluster_size; > + > + if (!f2fs_is_compressed_cluster(inode, page_idx)) > + continue; > > - count = last_idx - page_idx; > - while (count && count >= cluster_size) { > - ret = redirty_blocks(inode, page_idx, cluster_size); > + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); > if (ret < 0) > break; > > @@ -4270,9 +4274,6 @@ static int f2fs_ioc_decompress_file(struct file *filp) > break; > } > > - count -= cluster_size; > - page_idx += cluster_size; > - > cond_resched(); > if (fatal_signal_pending(current)) { > ret = -EINTR; > @@ -4299,9 +4300,9 @@ static int f2fs_ioc_compress_file(struct file *filp) > { > struct inode *inode = file_inode(filp); > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > - pgoff_t page_idx = 0, last_idx; > - int cluster_size = F2FS_I(inode)->i_cluster_size; > - int count, ret; > + struct f2fs_inode_info *fi = F2FS_I(inode); > + pgoff_t page_idx = 0, last_idx, cluster_idx; > + int ret; > > if (!f2fs_sb_has_compression(sbi) || > F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) > @@ -4335,10 +4336,15 @@ static int f2fs_ioc_compress_file(struct file *filp) > set_inode_flag(inode, FI_ENABLE_COMPRESS); > > last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); > + last_idx >>= fi->i_log_cluster_size; > > - count = last_idx - page_idx; > - while (count && count >= cluster_size) { > - ret = redirty_blocks(inode, page_idx, cluster_size); > + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { > + page_idx = cluster_idx << fi->i_log_cluster_size; > + > + if (!f2fs_is_non_sparse_cluster(inode, page_idx)) if (f2fs_is_sparse_cluster()) continue; Thanks, > + continue; > + > + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); > if (ret < 0) > break; > > @@ -4348,9 +4354,6 @@ static int f2fs_ioc_compress_file(struct file *filp) > break; > } > > - count -= cluster_size; > - page_idx += cluster_size; > - > cond_resched(); > if (fatal_signal_pending(current)) { > ret = -EINTR; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: compress: don't redirty sparse cluster during {, de}compress 2024-08-15 10:12 ` [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress Chao Yu @ 2024-08-15 11:10 ` Yeongjin Gil -1 siblings, 0 replies; 6+ messages in thread From: Yeongjin Gil @ 2024-08-15 11:10 UTC (permalink / raw) To: 'Chao Yu', jaegeuk Cc: 'Sungjong Seo', 'Jaewook Kim', linux-kernel, linux-f2fs-devel Thanks for your review. I will send patch v2. >On 2024/8/13 14:11, Yeongjin Gil wrote: >> In f2fs_do_write_data_page, when the data block is NULL_ADDR, it skips >> writepage considering that it has been already truncated. >> This results in an infinite loop as the PAGECACHE_TAG_TOWRITE tag is >> not cleared during the writeback process for a compressed file >> including NULL_ADDR in compress_mode=user. >> >> This is the reproduction process: >> >> 1. dd if=/dev/zero bs=4096 count=1024 seek=1024 of=testfile 2. f2fs_io >> compress testfile 3. dd if=/dev/zero bs=4096 count=1 conv=notrunc >> of=testfile 4. f2fs_io decompress testfile > >Good catch! > >> >> To prevent the problem, let's check whether the cluster is fully >> allocated before redirty its pages. >> >> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> >> Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com> >> Tested-by: Jaewook Kim <jw5454.kim@samsung.com> >> Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com> > >Need a fixes line? > >> --- >> fs/f2fs/compress.c | 34 ++++++++++++++++++++++++++-------- >> fs/f2fs/f2fs.h | 12 ++++++++++++ >> fs/f2fs/file.c | 39 +++++++++++++++++++++------------------ >> 3 files changed, 59 insertions(+), 26 deletions(-) >> >> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index >> 990b93689b46..09b91d725807 100644 >> --- a/fs/f2fs/compress.c >> +++ b/fs/f2fs/compress.c >> @@ -945,7 +945,7 @@ static int __f2fs_get_cluster_blocks(struct inode >*inode, >> unsigned int cluster_size = F2FS_I(inode)->i_cluster_size; >> int count, i; >> >> - for (i = 1, count = 1; i < cluster_size; i++) { >> + for (i = 0, count = 0; i < cluster_size; i++) { >> block_t blkaddr = data_blkaddr(dn->inode, dn->node_page, >> dn->ofs_in_node + i); >> >> @@ -956,8 +956,8 @@ static int __f2fs_get_cluster_blocks(struct inode >*inode, >> return count; >> } >> >> -static int __f2fs_cluster_blocks(struct inode *inode, >> - unsigned int cluster_idx, bool compr_blks) >> +static int __f2fs_cluster_blocks(struct inode *inode, unsigned int >cluster_idx, >> + enum cluster_check_type type) >> { >> struct dnode_of_data dn; >> unsigned int start_idx = cluster_idx << @@ -978,10 +978,12 @@ >> static int __f2fs_cluster_blocks(struct inode *inode, >> } >> >> if (dn.data_blkaddr == COMPRESS_ADDR) { >> - if (compr_blks) >> - ret = __f2fs_get_cluster_blocks(inode, &dn); >> - else >> + if (type == COMPR_BLKS) >> + ret = 1 + __f2fs_get_cluster_blocks(inode, &dn); >> + else if (type == COMPR_CLUSTER) >> ret = 1; >> + } else if (type == DECOMPR_BLKS) { >> + ret = __f2fs_get_cluster_blocks(inode, &dn); >> } >> fail: >> f2fs_put_dnode(&dn); >> @@ -991,7 +993,14 @@ static int __f2fs_cluster_blocks(struct inode *inode, >> /* return # of compressed blocks in compressed cluster */ >> static int f2fs_compressed_blocks(struct compress_ctx *cc) >> { >> - return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, true); >> + return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, >> +COMPR_BLKS); } >> + >> +/* return # of raw blocks in non-compressed cluster */ static int >> +f2fs_decompressed_blocks(struct inode *inode, >> + unsigned int cluster_idx) >> +{ >> + return __f2fs_cluster_blocks(inode, cluster_idx, DECOMPR_BLKS); >> } >> >> /* return whether cluster is compressed one or not */ @@ -999,7 >> +1008,16 @@ int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t >index) >> { >> return __f2fs_cluster_blocks(inode, >> index >> F2FS_I(inode)->i_log_cluster_size, >> - false); >> + COMPR_CLUSTER); >> +} >> + >> +/* return whether the cluster is filled with raw blocks */ bool >> +f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index) > >f2fs_is_sparse_cluster()? > >> +{ >> + unsigned int cluster_idx = index >> >> +F2FS_I(inode)->i_log_cluster_size; >> + >> + return f2fs_decompressed_blocks(inode, cluster_idx) == >> + F2FS_I(inode)->i_cluster_size; >> } >> >> static bool cluster_may_compress(struct compress_ctx *cc) diff --git >> a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 51fd5063a69c..6f26c8cb8857 >> 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -4302,6 +4302,11 @@ static inline bool >f2fs_meta_inode_gc_required(struct inode *inode) >> * compress.c >> */ >> #ifdef CONFIG_F2FS_FS_COMPRESSION >> +enum cluster_check_type { >> + COMPR_CLUSTER, >> + COMPR_BLKS, >> + DECOMPR_BLKS > >How about? > >CLUSTER_IS_COMPR, >CLUSTER_COMPR_BLKS, >CLUSTER_RAW_BLKS > >> +}; > >Can you please add some comments for these enums? > >> bool f2fs_is_compressed_page(struct page *page); >> struct page *f2fs_compress_control_page(struct page *page); >> int f2fs_prepare_compress_overwrite(struct inode *inode, @@ -4328,6 >> +4333,7 @@ int f2fs_write_multi_pages(struct compress_ctx *cc, >> struct writeback_control *wbc, >> enum iostat_type io_type); >> int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index); >> +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index); >> void f2fs_update_read_extent_tree_range_compressed(struct inode *inode, >> pgoff_t fofs, block_t blkaddr, >> unsigned int llen, unsigned int c_len); @@ - >4414,6 +4420,12 @@ >> static inline bool f2fs_load_compressed_page(struct f2fs_sb_info *sbi, >> static inline void f2fs_invalidate_compress_pages(struct f2fs_sb_info >*sbi, >> nid_t ino) { } >> #define inc_compr_inode_stat(inode) do { } while (0) >> +static inline int f2fs_is_compressed_cluster( >> + struct inode *inode, >> + pgoff_t index) { return 0; } >> +static inline bool f2fs_is_non_sparse_cluster( >> + struct inode *inode, >> + pgoff_t index) { return false; } >> static inline void f2fs_update_read_extent_tree_range_compressed( >> struct inode *inode, >> pgoff_t fofs, block_t blkaddr, >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index >> 270c32e3385f..fba8b5f216f9 100644 >> --- a/fs/f2fs/file.c >> +++ b/fs/f2fs/file.c >> @@ -4220,9 +4220,8 @@ static int f2fs_ioc_decompress_file(struct file >*filp) >> struct inode *inode = file_inode(filp); >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> struct f2fs_inode_info *fi = F2FS_I(inode); >> - pgoff_t page_idx = 0, last_idx; >> - int cluster_size = fi->i_cluster_size; >> - int count, ret; >> + pgoff_t page_idx = 0, last_idx, cluster_idx; >> + int ret; >> >> if (!f2fs_sb_has_compression(sbi) || >> F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ - >4257,10 >> +4256,15 @@ static int f2fs_ioc_decompress_file(struct file *filp) >> goto out; >> >> last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); >> + last_idx >>= fi->i_log_cluster_size; >> + >> + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { >> + page_idx = cluster_idx << fi->i_log_cluster_size; >> + >> + if (!f2fs_is_compressed_cluster(inode, page_idx)) >> + continue; >> >> - count = last_idx - page_idx; >> - while (count && count >= cluster_size) { >> - ret = redirty_blocks(inode, page_idx, cluster_size); >> + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); >> if (ret < 0) >> break; >> >> @@ -4270,9 +4274,6 @@ static int f2fs_ioc_decompress_file(struct file >*filp) >> break; >> } >> >> - count -= cluster_size; >> - page_idx += cluster_size; >> - >> cond_resched(); >> if (fatal_signal_pending(current)) { >> ret = -EINTR; >> @@ -4299,9 +4300,9 @@ static int f2fs_ioc_compress_file(struct file *filp) >> { >> struct inode *inode = file_inode(filp); >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> - pgoff_t page_idx = 0, last_idx; >> - int cluster_size = F2FS_I(inode)->i_cluster_size; >> - int count, ret; >> + struct f2fs_inode_info *fi = F2FS_I(inode); >> + pgoff_t page_idx = 0, last_idx, cluster_idx; >> + int ret; >> >> if (!f2fs_sb_has_compression(sbi) || >> F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ - >4335,10 >> +4336,15 @@ static int f2fs_ioc_compress_file(struct file *filp) >> set_inode_flag(inode, FI_ENABLE_COMPRESS); >> >> last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); >> + last_idx >>= fi->i_log_cluster_size; >> >> - count = last_idx - page_idx; >> - while (count && count >= cluster_size) { >> - ret = redirty_blocks(inode, page_idx, cluster_size); >> + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { >> + page_idx = cluster_idx << fi->i_log_cluster_size; >> + >> + if (!f2fs_is_non_sparse_cluster(inode, page_idx)) > >if (f2fs_is_sparse_cluster()) > continue; > >Thanks, > >> + continue; >> + >> + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); >> if (ret < 0) >> break; >> >> @@ -4348,9 +4354,6 @@ static int f2fs_ioc_compress_file(struct file *filp) >> break; >> } >> >> - count -= cluster_size; >> - page_idx += cluster_size; >> - >> cond_resched(); >> if (fatal_signal_pending(current)) { >> ret = -EINTR; _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress @ 2024-08-15 11:10 ` Yeongjin Gil 0 siblings, 0 replies; 6+ messages in thread From: Yeongjin Gil @ 2024-08-15 11:10 UTC (permalink / raw) To: 'Chao Yu', jaegeuk Cc: linux-f2fs-devel, linux-kernel, 'Sungjong Seo', 'Sunmin Jeong', 'Jaewook Kim' Thanks for your review. I will send patch v2. >On 2024/8/13 14:11, Yeongjin Gil wrote: >> In f2fs_do_write_data_page, when the data block is NULL_ADDR, it skips >> writepage considering that it has been already truncated. >> This results in an infinite loop as the PAGECACHE_TAG_TOWRITE tag is >> not cleared during the writeback process for a compressed file >> including NULL_ADDR in compress_mode=user. >> >> This is the reproduction process: >> >> 1. dd if=/dev/zero bs=4096 count=1024 seek=1024 of=testfile 2. f2fs_io >> compress testfile 3. dd if=/dev/zero bs=4096 count=1 conv=notrunc >> of=testfile 4. f2fs_io decompress testfile > >Good catch! > >> >> To prevent the problem, let's check whether the cluster is fully >> allocated before redirty its pages. >> >> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> >> Reviewed-by: Sunmin Jeong <s_min.jeong@samsung.com> >> Tested-by: Jaewook Kim <jw5454.kim@samsung.com> >> Signed-off-by: Yeongjin Gil <youngjin.gil@samsung.com> > >Need a fixes line? > >> --- >> fs/f2fs/compress.c | 34 ++++++++++++++++++++++++++-------- >> fs/f2fs/f2fs.h | 12 ++++++++++++ >> fs/f2fs/file.c | 39 +++++++++++++++++++++------------------ >> 3 files changed, 59 insertions(+), 26 deletions(-) >> >> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index >> 990b93689b46..09b91d725807 100644 >> --- a/fs/f2fs/compress.c >> +++ b/fs/f2fs/compress.c >> @@ -945,7 +945,7 @@ static int __f2fs_get_cluster_blocks(struct inode >*inode, >> unsigned int cluster_size = F2FS_I(inode)->i_cluster_size; >> int count, i; >> >> - for (i = 1, count = 1; i < cluster_size; i++) { >> + for (i = 0, count = 0; i < cluster_size; i++) { >> block_t blkaddr = data_blkaddr(dn->inode, dn->node_page, >> dn->ofs_in_node + i); >> >> @@ -956,8 +956,8 @@ static int __f2fs_get_cluster_blocks(struct inode >*inode, >> return count; >> } >> >> -static int __f2fs_cluster_blocks(struct inode *inode, >> - unsigned int cluster_idx, bool compr_blks) >> +static int __f2fs_cluster_blocks(struct inode *inode, unsigned int >cluster_idx, >> + enum cluster_check_type type) >> { >> struct dnode_of_data dn; >> unsigned int start_idx = cluster_idx << @@ -978,10 +978,12 @@ >> static int __f2fs_cluster_blocks(struct inode *inode, >> } >> >> if (dn.data_blkaddr == COMPRESS_ADDR) { >> - if (compr_blks) >> - ret = __f2fs_get_cluster_blocks(inode, &dn); >> - else >> + if (type == COMPR_BLKS) >> + ret = 1 + __f2fs_get_cluster_blocks(inode, &dn); >> + else if (type == COMPR_CLUSTER) >> ret = 1; >> + } else if (type == DECOMPR_BLKS) { >> + ret = __f2fs_get_cluster_blocks(inode, &dn); >> } >> fail: >> f2fs_put_dnode(&dn); >> @@ -991,7 +993,14 @@ static int __f2fs_cluster_blocks(struct inode *inode, >> /* return # of compressed blocks in compressed cluster */ >> static int f2fs_compressed_blocks(struct compress_ctx *cc) >> { >> - return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, true); >> + return __f2fs_cluster_blocks(cc->inode, cc->cluster_idx, >> +COMPR_BLKS); } >> + >> +/* return # of raw blocks in non-compressed cluster */ static int >> +f2fs_decompressed_blocks(struct inode *inode, >> + unsigned int cluster_idx) >> +{ >> + return __f2fs_cluster_blocks(inode, cluster_idx, DECOMPR_BLKS); >> } >> >> /* return whether cluster is compressed one or not */ @@ -999,7 >> +1008,16 @@ int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t >index) >> { >> return __f2fs_cluster_blocks(inode, >> index >> F2FS_I(inode)->i_log_cluster_size, >> - false); >> + COMPR_CLUSTER); >> +} >> + >> +/* return whether the cluster is filled with raw blocks */ bool >> +f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index) > >f2fs_is_sparse_cluster()? > >> +{ >> + unsigned int cluster_idx = index >> >> +F2FS_I(inode)->i_log_cluster_size; >> + >> + return f2fs_decompressed_blocks(inode, cluster_idx) == >> + F2FS_I(inode)->i_cluster_size; >> } >> >> static bool cluster_may_compress(struct compress_ctx *cc) diff --git >> a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 51fd5063a69c..6f26c8cb8857 >> 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -4302,6 +4302,11 @@ static inline bool >f2fs_meta_inode_gc_required(struct inode *inode) >> * compress.c >> */ >> #ifdef CONFIG_F2FS_FS_COMPRESSION >> +enum cluster_check_type { >> + COMPR_CLUSTER, >> + COMPR_BLKS, >> + DECOMPR_BLKS > >How about? > >CLUSTER_IS_COMPR, >CLUSTER_COMPR_BLKS, >CLUSTER_RAW_BLKS > >> +}; > >Can you please add some comments for these enums? > >> bool f2fs_is_compressed_page(struct page *page); >> struct page *f2fs_compress_control_page(struct page *page); >> int f2fs_prepare_compress_overwrite(struct inode *inode, @@ -4328,6 >> +4333,7 @@ int f2fs_write_multi_pages(struct compress_ctx *cc, >> struct writeback_control *wbc, >> enum iostat_type io_type); >> int f2fs_is_compressed_cluster(struct inode *inode, pgoff_t index); >> +bool f2fs_is_non_sparse_cluster(struct inode *inode, pgoff_t index); >> void f2fs_update_read_extent_tree_range_compressed(struct inode *inode, >> pgoff_t fofs, block_t blkaddr, >> unsigned int llen, unsigned int c_len); @@ - >4414,6 +4420,12 @@ >> static inline bool f2fs_load_compressed_page(struct f2fs_sb_info *sbi, >> static inline void f2fs_invalidate_compress_pages(struct f2fs_sb_info >*sbi, >> nid_t ino) { } >> #define inc_compr_inode_stat(inode) do { } while (0) >> +static inline int f2fs_is_compressed_cluster( >> + struct inode *inode, >> + pgoff_t index) { return 0; } >> +static inline bool f2fs_is_non_sparse_cluster( >> + struct inode *inode, >> + pgoff_t index) { return false; } >> static inline void f2fs_update_read_extent_tree_range_compressed( >> struct inode *inode, >> pgoff_t fofs, block_t blkaddr, >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index >> 270c32e3385f..fba8b5f216f9 100644 >> --- a/fs/f2fs/file.c >> +++ b/fs/f2fs/file.c >> @@ -4220,9 +4220,8 @@ static int f2fs_ioc_decompress_file(struct file >*filp) >> struct inode *inode = file_inode(filp); >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> struct f2fs_inode_info *fi = F2FS_I(inode); >> - pgoff_t page_idx = 0, last_idx; >> - int cluster_size = fi->i_cluster_size; >> - int count, ret; >> + pgoff_t page_idx = 0, last_idx, cluster_idx; >> + int ret; >> >> if (!f2fs_sb_has_compression(sbi) || >> F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ - >4257,10 >> +4256,15 @@ static int f2fs_ioc_decompress_file(struct file *filp) >> goto out; >> >> last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); >> + last_idx >>= fi->i_log_cluster_size; >> + >> + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { >> + page_idx = cluster_idx << fi->i_log_cluster_size; >> + >> + if (!f2fs_is_compressed_cluster(inode, page_idx)) >> + continue; >> >> - count = last_idx - page_idx; >> - while (count && count >= cluster_size) { >> - ret = redirty_blocks(inode, page_idx, cluster_size); >> + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); >> if (ret < 0) >> break; >> >> @@ -4270,9 +4274,6 @@ static int f2fs_ioc_decompress_file(struct file >*filp) >> break; >> } >> >> - count -= cluster_size; >> - page_idx += cluster_size; >> - >> cond_resched(); >> if (fatal_signal_pending(current)) { >> ret = -EINTR; >> @@ -4299,9 +4300,9 @@ static int f2fs_ioc_compress_file(struct file *filp) >> { >> struct inode *inode = file_inode(filp); >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> - pgoff_t page_idx = 0, last_idx; >> - int cluster_size = F2FS_I(inode)->i_cluster_size; >> - int count, ret; >> + struct f2fs_inode_info *fi = F2FS_I(inode); >> + pgoff_t page_idx = 0, last_idx, cluster_idx; >> + int ret; >> >> if (!f2fs_sb_has_compression(sbi) || >> F2FS_OPTION(sbi).compress_mode != COMPR_MODE_USER) @@ - >4335,10 >> +4336,15 @@ static int f2fs_ioc_compress_file(struct file *filp) >> set_inode_flag(inode, FI_ENABLE_COMPRESS); >> >> last_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); >> + last_idx >>= fi->i_log_cluster_size; >> >> - count = last_idx - page_idx; >> - while (count && count >= cluster_size) { >> - ret = redirty_blocks(inode, page_idx, cluster_size); >> + for (cluster_idx = 0; cluster_idx < last_idx; cluster_idx++) { >> + page_idx = cluster_idx << fi->i_log_cluster_size; >> + >> + if (!f2fs_is_non_sparse_cluster(inode, page_idx)) > >if (f2fs_is_sparse_cluster()) > continue; > >Thanks, > >> + continue; >> + >> + ret = redirty_blocks(inode, page_idx, fi->i_cluster_size); >> if (ret < 0) >> break; >> >> @@ -4348,9 +4354,6 @@ static int f2fs_ioc_compress_file(struct file *filp) >> break; >> } >> >> - count -= cluster_size; >> - page_idx += cluster_size; >> - >> cond_resched(); >> if (fatal_signal_pending(current)) { >> ret = -EINTR; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-08-15 11:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20240813061105epcas1p4898159104ce2ee12ef6c203c4ab828ff@epcas1p4.samsung.com>
2024-08-13 6:11 ` [f2fs-dev] [PATCH] f2fs: compress: don't redirty sparse cluster during {, de}compress Yeongjin Gil
2024-08-13 6:11 ` [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress Yeongjin Gil
2024-08-15 10:12 ` [f2fs-dev] [PATCH] f2fs: compress: don't redirty sparse cluster during {, de}compress Chao Yu
2024-08-15 10:12 ` [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress Chao Yu
2024-08-15 11:10 ` [f2fs-dev] [PATCH] f2fs: compress: don't redirty sparse cluster during {, de}compress Yeongjin Gil
2024-08-15 11:10 ` [PATCH] f2fs: compress: don't redirty sparse cluster during {,de}compress Yeongjin Gil
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.