Received-SPF: pass (google.com: domain of linux-kernel+bounces-22918-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231228143152.1543509-1-chao@kernel.org>
In-Reply-To: <20231228143152.1543509-1-chao@kernel.org>
From: Daeho Jeong <daeho43@gmail.com>
Date: Wed, 10 Jan 2024 16:55:07 -0800
Message-ID: <CACOAw_xXGUt=2JJgq5yfW5ZG7EfZMm1iNTVxvfA9FoJVDwbH_A@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH v3 1/6] f2fs: compress: fix to guarantee
 persisting compressed blocks by CP
To: Chao Yu <chao@kernel.org>
Cc: jaegeuk@kernel.org, linux-kernel@vger.kernel.org, 
	linux-f2fs-devel@lists.sourceforge.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Dec 28, 2023 at 6:33=E2=80=AFAM Chao Yu <chao@kernel.org> 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@kernel.org>
> ---
> v3:
> - treat compressed page as CP guaranteed data explictly.
>  fs/f2fs/compress.c |  4 +++-
>  fs/f2fs/data.c     | 17 +++++++++--------
>  fs/f2fs/f2fs.h     |  4 +++-
>  3 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index c5a4364c4482..9940b7886e5d 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1418,6 +1418,8 @@ void f2fs_compress_write_end_io(struct bio *bio, st=
ruct page *page)
>         struct f2fs_sb_info *sbi =3D bio->bi_private;
>         struct compress_io_ctx *cic =3D
>                         (struct compress_io_ctx *)page_private(page);
> +       enum count_type type =3D WB_DATA_TYPE(page,
> +                               f2fs_is_compressed_page(page));
>         int i;
>
>         if (unlikely(bio->bi_status))
> @@ -1425,7 +1427,7 @@ void f2fs_compress_write_end_io(struct bio *bio, st=
ruct 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 dce8defdf4c7..81f9e2cc49e2 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 =3D page->mapping;
>         struct inode *inode;
> @@ -65,8 +65,6 @@ static bool __is_cp_guaranteed(struct page *page)
>                         S_ISDIR(inode->i_mode))
>                 return true;
>
> -       if (f2fs_is_compressed_page(page))
> -               return false;

Out of curiosity, why don't we simply change the above to "return true"?

>         if ((S_ISREG(inode->i_mode) && IS_NOQUOTA(inode)) ||
>                         page_private_gcing(page))
>                 return true;
> @@ -338,7 +336,7 @@ static void f2fs_write_end_io(struct bio *bio)
>
>         bio_for_each_segment_all(bvec, bio, iter_all) {
>                 struct page *page =3D bvec->bv_page;
> -               enum count_type type =3D WB_DATA_TYPE(page);
> +               enum count_type type =3D WB_DATA_TYPE(page, false);
>
>                 if (page_private_dummy(page)) {
>                         clear_page_private_dummy(page);
> @@ -762,7 +760,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>                 wbc_account_cgroup_owner(fio->io_wbc, fio->page, PAGE_SIZ=
E);
>
>         inc_page_count(fio->sbi, is_read_io(fio->op) ?
> -                       __read_io_type(page) : WB_DATA_TYPE(fio->page));
> +                       __read_io_type(page) : WB_DATA_TYPE(fio->page, fa=
lse));
>
>         if (is_read_io(bio_op(bio)))
>                 f2fs_submit_read_bio(fio->sbi, bio, fio->type);
> @@ -973,7 +971,7 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
>         if (fio->io_wbc)
>                 wbc_account_cgroup_owner(fio->io_wbc, fio->page, PAGE_SIZ=
E);
>
> -       inc_page_count(fio->sbi, WB_DATA_TYPE(page));
> +       inc_page_count(fio->sbi, WB_DATA_TYPE(page, false));
>
>         *fio->last_block =3D fio->new_blkaddr;
>         *fio->bio =3D bio;
> @@ -1007,6 +1005,7 @@ void f2fs_submit_page_write(struct f2fs_io_info *fi=
o)
>         enum page_type btype =3D PAGE_TYPE_OF_BIO(fio->type);
>         struct f2fs_bio_info *io =3D 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 +1045,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fi=
o)
>         /* set submitted =3D true as a return value */
>         fio->submitted =3D 1;
>
> -       inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> +       type =3D WB_DATA_TYPE(bio_page, fio->compressed_page);
> +       inc_page_count(sbi, type);
>
>         if (io->bio &&
>             (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bi=
o,
> @@ -1059,7 +1059,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fi=
o)
>                 if (F2FS_IO_ALIGNED(sbi) &&
>                                 (fio->type =3D=3D DATA || fio->type =3D=
=3D NODE) &&
>                                 fio->new_blkaddr & F2FS_IO_SIZE_MASK(sbi)=
) {
> -                       dec_page_count(sbi, WB_DATA_TYPE(bio_page));
> +                       dec_page_count(sbi, WB_DATA_TYPE(bio_page,
> +                                               fio->compressed_page));
>                         fio->retry =3D 1;
>                         goto skip;
>                 }
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 65294e3b0bef..50f3d546ded8 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1080,7 +1080,8 @@ 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, f)                     \
> +       (f || f2fs_is_cp_guaranteed(p) ? F2FS_WB_CP_DATA : F2FS_WB_DATA)
>  enum count_type {
>         F2FS_DIRTY_DENTS,
>         F2FS_DIRTY_DATA,
> @@ -3804,6 +3805,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
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel