Received-SPF: pass (google.com: domain of linux-kernel+bounces-25059-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240111064208.2969599-1-chao@kernel.org> <20240111064208.2969599-2-chao@kernel.org>
In-Reply-To: <20240111064208.2969599-2-chao@kernel.org>
From: Daeho Jeong <daeho43@gmail.com>
Date: Fri, 12 Jan 2024 14:15:21 -0800
Message-ID: <CACOAw_xnOKPeFQ7u5o17coYkpVepYT8DBQPoi1okuR8VvjfMvg@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH v4 2/6] f2fs: compress: fix to cover normal
 cluster write with cp_rwsem
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

Reviewed-by: Daeho Jeong <daehojeong@google.com>

On Wed, Jan 10, 2024 at 10:43=E2=80=AFPM Chao Yu <chao@kernel.org> wrote:
>
> When we overwrite compressed cluster w/ normal cluster, we should
> not unlock cp_rwsem during f2fs_write_raw_pages(), otherwise data
> will be corrupted if partial blocks were persisted before CP & SPOR,
> due to cluster metadata wasn't updated atomically.
>
> Fixes: 4c8ff7095bef ("f2fs: support data compression")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/compress.c | 20 ++++++++++++++------
>  fs/f2fs/data.c     |  3 ++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 9940b7886e5d..bf4cfab67aec 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1448,7 +1448,8 @@ static int f2fs_write_raw_pages(struct compress_ctx=
 *cc,
>                                         enum iostat_type io_type)
>  {
>         struct address_space *mapping =3D cc->inode->i_mapping;
> -       int _submitted, compr_blocks, ret, i;
> +       struct f2fs_sb_info *sbi =3D F2FS_M_SB(mapping);
> +       int _submitted, compr_blocks, ret =3D 0, i;
>
>         compr_blocks =3D f2fs_compressed_blocks(cc);
>
> @@ -1463,6 +1464,10 @@ static int f2fs_write_raw_pages(struct compress_ct=
x *cc,
>         if (compr_blocks < 0)
>                 return compr_blocks;
>
> +       /* overwrite compressed cluster w/ normal cluster */
> +       if (compr_blocks > 0)
> +               f2fs_lock_op(sbi);
> +
>         for (i =3D 0; i < cc->cluster_size; i++) {
>                 if (!cc->rpages[i])
>                         continue;
> @@ -1495,26 +1500,29 @@ static int f2fs_write_raw_pages(struct compress_c=
tx *cc,
>                                 unlock_page(cc->rpages[i]);
>                                 ret =3D 0;
>                         } else if (ret =3D=3D -EAGAIN) {
> +                               ret =3D 0;
>                                 /*
>                                  * for quota file, just redirty left page=
s to
>                                  * avoid deadlock caused by cluster updat=
e race
>                                  * from foreground operation.
>                                  */
>                                 if (IS_NOQUOTA(cc->inode))
> -                                       return 0;
> -                               ret =3D 0;
> +                                       goto out;
>                                 f2fs_io_schedule_timeout(DEFAULT_IO_TIMEO=
UT);
>                                 goto retry_write;
>                         }
> -                       return ret;
> +                       goto out;
>                 }
>
>                 *submitted +=3D _submitted;
>         }
>
> -       f2fs_balance_fs(F2FS_M_SB(mapping), true);
> +out:
> +       if (compr_blocks > 0)
> +               f2fs_unlock_op(sbi);
>
> -       return 0;
> +       f2fs_balance_fs(sbi, true);
> +       return ret;
>  }
>
>  int f2fs_write_multi_pages(struct compress_ctx *cc,
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 81f9e2cc49e2..b171a9980f6a 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2839,7 +2839,7 @@ int f2fs_write_single_data_page(struct page *page, =
int *submitted,
>                 .encrypted_page =3D NULL,
>                 .submitted =3D 0,
>                 .compr_blocks =3D compr_blocks,
> -               .need_lock =3D LOCK_RETRY,
> +               .need_lock =3D compr_blocks ? LOCK_DONE : LOCK_RETRY,
>                 .post_read =3D f2fs_post_read_required(inode) ? 1 : 0,
>                 .io_type =3D io_type,
>                 .io_wbc =3D wbc,
> @@ -2920,6 +2920,7 @@ int f2fs_write_single_data_page(struct page *page, =
int *submitted,
>         if (err =3D=3D -EAGAIN) {
>                 err =3D f2fs_do_write_data_page(&fio);
>                 if (err =3D=3D -EAGAIN) {
> +                       f2fs_bug_on(sbi, compr_blocks);
>                         fio.need_lock =3D LOCK_REQ;
>                         err =3D f2fs_do_write_data_page(&fio);
>                 }
> --
> 2.40.1
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel