Re: [PATCH 1/6] f2fs: compress: fix to guarantee persisting compressed blocks by CP

[Jaegeuk Kim]

On 12/27, Chao Yu wrote:
> On 2023/12/27 5:02, Jaegeuk Kim wrote:
> > On 12/20, Chao Yu wrote:
> > > If data block in compressed cluster is not persisted with metadata
> > > during checkpoint, after SPOR, the data may be corrupted, let's
> > > guarantee to write compressed page by checkpoint.
> > > 
> > > Fixes: 4c8ff7095bef ("f2fs: support data compression")
> > > Signed-off-by: Chao Yu <chao@xxxxxxxxxx>
> > > ---
> > >   fs/f2fs/compress.c |  3 ++-
> > >   fs/f2fs/data.c     | 12 +++++++++---
> > >   fs/f2fs/f2fs.h     |  3 ++-
> > >   3 files changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > > index 5b076329e9bf..1122db8cc0b0 100644
> > > --- a/fs/f2fs/compress.c
> > > +++ b/fs/f2fs/compress.c
> > > @@ -1442,6 +1442,7 @@ void f2fs_compress_write_end_io(struct bio *bio, struct page *page)
> > >   	struct f2fs_sb_info *sbi = bio->bi_private;
> > >   	struct compress_io_ctx *cic =
> > >   			(struct compress_io_ctx *)page_private(page);
> > > +	enum count_type type = WB_DATA_TYPE(page);
> > >   	int i;
> > >   	if (unlikely(bio->bi_status))
> > > @@ -1449,7 +1450,7 @@ void f2fs_compress_write_end_io(struct bio *bio, struct page *page)
> > >   	f2fs_compress_free_page(page);
> > > -	dec_page_count(sbi, F2FS_WB_DATA);
> > > +	dec_page_count(sbi, type);
> > >   	if (atomic_dec_return(&cic->pending_pages))
> > >   		return;
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index d28c97282e68..6c72a6e86ba8 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -48,7 +48,7 @@ void f2fs_destroy_bioset(void)
> > >   	bioset_exit(&f2fs_bioset);
> > >   }
> > > -static bool __is_cp_guaranteed(struct page *page)
> > > +bool f2fs_is_cp_guaranteed(struct page *page)
> > >   {
> > >   	struct address_space *mapping = page->mapping;
> > >   	struct inode *inode;
> > > @@ -66,7 +66,7 @@ static bool __is_cp_guaranteed(struct page *page)
> > >   		return true;
> > >   	if (f2fs_is_compressed_page(page))
> > > -		return false;
> > > +		return true;
> > >   	if ((S_ISREG(inode->i_mode) && IS_NOQUOTA(inode)) ||
> > >   			page_private_gcing(page))
> > >   		return true;
> > > @@ -1007,6 +1007,7 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> > >   	enum page_type btype = PAGE_TYPE_OF_BIO(fio->type);
> > >   	struct f2fs_bio_info *io = sbi->write_io[btype] + fio->temp;
> > >   	struct page *bio_page;
> > > +	enum count_type type;
> > >   	f2fs_bug_on(sbi, is_read_io(fio->op));
> > > @@ -1046,7 +1047,12 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> > >   	/* set submitted = true as a return value */
> > >   	fio->submitted = 1;
> > > -	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> > > +	type = WB_DATA_TYPE(bio_page);
> > > +	/* override count type if page is compressed one */
> > > +	if (fio->compressed_page)
> > > +		type = WB_DATA_TYPE(fio->compressed_page);
> > 
> > Doesn't bio_page already point fio->compressed_page?
> 
> Please check below codes, bio_page will point to fio->encrypted_page if
> both software encryption feature and compression feature are on, for this
> case, we still need to account F2FS_WB_CP_DATA.

So, it seems you want to make F2FS_WB_CP_DATA regardless of conditions. Then,
how about making this explictly instead of implicit condition check of the page?

#define WB_DATA_TYPE(p, f) (f || __is_cp_guaranteed(p) ? F2FS_WB_CP_DATA : F2FS_WB_DATA)

	inc_page_count(sbi, WB_DATA_TYPE(bio_page, bio_page == fio->compressed_page));

	dec_page_count(sbi, WB_DATA_TYPE(page, f2fs_is_compressed_page(page)));

> 
> 	if (fio->encrypted_page)
> 		bio_page = fio->encrypted_page;
> 	else if (fio->compressed_page)
> 		bio_page = fio->compressed_page;
> 	else
> 		bio_page = fio->page;
> 
> Thanks,
> 
> > 
> > > +
> > > +	inc_page_count(sbi, type);
> > >   	if (io->bio &&
> > >   	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 76e9a8682e38..bcb3940ab5ba 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -1092,7 +1092,7 @@ struct f2fs_sm_info {
> > >    * f2fs monitors the number of several block types such as on-writeback,
> > >    * dirty dentry blocks, dirty node blocks, and dirty meta blocks.
> > >    */
> > > -#define WB_DATA_TYPE(p)	(__is_cp_guaranteed(p) ? F2FS_WB_CP_DATA : F2FS_WB_DATA)
> > > +#define WB_DATA_TYPE(p)	(f2fs_is_cp_guaranteed(p) ? F2FS_WB_CP_DATA : F2FS_WB_DATA)
> > >   enum count_type {
> > >   	F2FS_DIRTY_DENTS,
> > >   	F2FS_DIRTY_DATA,
> > > @@ -3824,6 +3824,7 @@ void f2fs_init_ckpt_req_control(struct f2fs_sb_info *sbi);
> > >    */
> > >   int __init f2fs_init_bioset(void);
> > >   void f2fs_destroy_bioset(void);
> > > +bool f2fs_is_cp_guaranteed(struct page *page);
> > >   int f2fs_init_bio_entry_cache(void);
> > >   void f2fs_destroy_bio_entry_cache(void);
> > >   void f2fs_submit_read_bio(struct f2fs_sb_info *sbi, struct bio *bio,
> > > -- 
> > > 2.40.1