Received-SPF: pass (google.com: domain of linux-kernel+bounces-25065-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240111064208.2969599-1-chao@kernel.org> <20240111064208.2969599-4-chao@kernel.org>
In-Reply-To: <20240111064208.2969599-4-chao@kernel.org>
From: Daeho Jeong <daeho43@gmail.com>
Date: Fri, 12 Jan 2024 14:19:44 -0800
Message-ID: <CACOAw_yzAWpHzHerTtm5vCkejt4Xiex5m_xKMTC_WHspOXFmRg@mail.gmail.com>
Subject: Re: [f2fs-dev] [PATCH v4 4/6] f2fs: compress: fix to avoid
 inconsistence bewteen i_blocks and dnode
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:
>
> In reserve_compress_blocks(), we update blkaddrs of dnode in prior to
> inc_valid_block_count(), it may cause inconsistent status bewteen
> i_blocks and blkaddrs once inc_valid_block_count() fails.
>
> To fix this issue, it needs to reverse their invoking order.
>
> Fixes: c75488fb4d82 ("f2fs: introduce F2FS_IOC_RESERVE_COMPRESS_BLOCKS")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/data.c    |  5 +++--
>  fs/f2fs/f2fs.h    |  7 ++++++-
>  fs/f2fs/file.c    | 26 ++++++++++++++------------
>  fs/f2fs/segment.c |  2 +-
>  4 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index b171a9980f6a..8d2ace723310 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1219,7 +1219,8 @@ int f2fs_reserve_new_blocks(struct dnode_of_data *d=
n, blkcnt_t count)
>
>         if (unlikely(is_inode_flag_set(dn->inode, FI_NO_ALLOC)))
>                 return -EPERM;
> -       if (unlikely((err =3D inc_valid_block_count(sbi, dn->inode, &coun=
t))))
> +       err =3D inc_valid_block_count(sbi, dn->inode, &count, true);
> +       if (unlikely(err))
>                 return err;
>
>         trace_f2fs_reserve_new_blocks(dn->inode, dn->nid,
> @@ -1476,7 +1477,7 @@ static int __allocate_data_block(struct dnode_of_da=
ta *dn, int seg_type)
>
>         dn->data_blkaddr =3D f2fs_data_blkaddr(dn);
>         if (dn->data_blkaddr =3D=3D NULL_ADDR) {
> -               err =3D inc_valid_block_count(sbi, dn->inode, &count);
> +               err =3D inc_valid_block_count(sbi, dn->inode, &count, tru=
e);
>                 if (unlikely(err))
>                         return err;
>         }
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 50f3d546ded8..69e71460a950 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2252,7 +2252,7 @@ static inline bool __allow_reserved_blocks(struct f=
2fs_sb_info *sbi,
>
>  static inline void f2fs_i_blocks_write(struct inode *, block_t, bool, bo=
ol);
>  static inline int inc_valid_block_count(struct f2fs_sb_info *sbi,
> -                                struct inode *inode, blkcnt_t *count)
> +                                struct inode *inode, blkcnt_t *count, bo=
ol partial)
>  {
>         blkcnt_t diff =3D 0, release =3D 0;
>         block_t avail_user_block_count;
> @@ -2292,6 +2292,11 @@ static inline int inc_valid_block_count(struct f2f=
s_sb_info *sbi,
>                         avail_user_block_count =3D 0;
>         }
>         if (unlikely(sbi->total_valid_block_count > avail_user_block_coun=
t)) {
> +               if (!partial) {
> +                       spin_unlock(&sbi->stat_lock);
> +                       goto enospc;
> +               }
> +
>                 diff =3D sbi->total_valid_block_count - avail_user_block_=
count;
>                 if (diff > *count)
>                         diff =3D *count;
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 80d9c4c096f0..53c495651789 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3614,14 +3614,16 @@ static int reserve_compress_blocks(struct dnode_o=
f_data *dn, pgoff_t count)
>                 blkcnt_t reserved;
>                 int ret;
>
> -               for (i =3D 0; i < cluster_size; i++, dn->ofs_in_node++) {
> -                       blkaddr =3D f2fs_data_blkaddr(dn);
> +               for (i =3D 0; i < cluster_size; i++) {
> +                       blkaddr =3D data_blkaddr(dn->inode, dn->node_page=
,
> +                                               dn->ofs_in_node + i);
>
>                         if (i =3D=3D 0) {
> -                               if (blkaddr =3D=3D COMPRESS_ADDR)
> -                                       continue;
> -                               dn->ofs_in_node +=3D cluster_size;
> -                               goto next;
> +                               if (blkaddr !=3D COMPRESS_ADDR) {
> +                                       dn->ofs_in_node +=3D cluster_size=
;
> +                                       goto next;
> +                               }
> +                               continue;
>                         }
>
>                         /*
> @@ -3634,20 +3636,20 @@ static int reserve_compress_blocks(struct dnode_o=
f_data *dn, pgoff_t count)
>                                 compr_blocks++;
>                                 continue;
>                         }
> -
> -                       f2fs_set_data_blkaddr(dn, NEW_ADDR);
>                 }
>
>                 reserved =3D cluster_size - compr_blocks;
>                 if (!reserved)
>                         goto next;
>
> -               ret =3D inc_valid_block_count(sbi, dn->inode, &reserved);
> -               if (ret)
> +               ret =3D inc_valid_block_count(sbi, dn->inode, &reserved, =
false);
> +               if (unlikely(ret))
>                         return ret;
>
> -               if (reserved !=3D cluster_size - compr_blocks)
> -                       return -ENOSPC;
> +               for (i =3D 0; i < cluster_size; i++, dn->ofs_in_node++) {
> +                       if (f2fs_data_blkaddr(dn) =3D=3D NULL_ADDR)
> +                               f2fs_set_data_blkaddr(dn, NEW_ADDR);
> +               }
>
>                 f2fs_i_compr_blocks_update(dn->inode, compr_blocks, true)=
;
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 4c8836ded90f..ef5b3848426b 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -248,7 +248,7 @@ static int __replace_atomic_write_block(struct inode =
*inode, pgoff_t index,
>         } else {
>                 blkcnt_t count =3D 1;
>
> -               err =3D inc_valid_block_count(sbi, inode, &count);
> +               err =3D inc_valid_block_count(sbi, inode, &count, true);
>                 if (err) {
>                         f2fs_put_dnode(&dn);
>                         return err;
> --
> 2.40.1
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel