Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230402013000.73713-1-sunjunchao2870@gmail.com>
In-Reply-To: <20230402013000.73713-1-sunjunchao2870@gmail.com>
From:   JunChao Sun <sunjunchao2870@gmail.com>
Date:   Fri, 7 Apr 2023 20:06:44 +0800
Message-ID: <CAHB1NahWmNX7ADXn0a3SVhz5+4Cdg5neZ9jHpqnERFVC-v0Ftw@mail.gmail.com>
Subject: Re: [PATCH v2] ext4: fix performance issue of xattr when expanding inode
To:     linux-ext4@vger.kernel.org, linux-doc@vger.kernel.org
Cc:     tytso@mit.edu, jun.nie@linaro.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Friendly ping...

JunChao Sun <sunjunchao2870@gmail.com> =E4=BA=8E2023=E5=B9=B44=E6=9C=882=E6=
=97=A5=E5=91=A8=E6=97=A5 09:30=E5=86=99=E9=81=93=EF=BC=9A
>
> Currently ext4 will delete ea entry from ibody and recreate ea entry
> which store the same value when expanding inode. The main performance
> issue is caused by the fact that ext4 will destroy and recreate the
> ea inode, and such behavior is redundant.
>
> The patch is a bit ugly, because ext4_xattr_set_entry() contains the
> creating,deleting,replacing of xattr without external intervention,
> this looks good. But the movement of ea entry from ibody to block
> breaks this, so add an argument for ext4_xattr_set_entry() for this
> break, and then ext4_xattr_block_set() will reuse the ea_inode instead
> of recreating an ea_inode which store the same value.
>
> Signed-off-by: JunChao Sun <sunjunchao2870@gmail.com>
> ---
>  fs/ext4/xattr.c | 99 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 81 insertions(+), 18 deletions(-)
>
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 767454d74cd6..439581e630d4 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1634,7 +1634,7 @@ static int ext4_xattr_inode_lookup_create(handle_t =
*handle, struct inode *inode,
>  static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
>                                 struct ext4_xattr_search *s,
>                                 handle_t *handle, struct inode *inode,
> -                               bool is_block)
> +                               bool is_block, struct inode *mv_ea_inode)
>  {
>         struct ext4_xattr_entry *last, *next;
>         struct ext4_xattr_entry *here =3D s->here;
> @@ -1727,7 +1727,7 @@ static int ext4_xattr_set_entry(struct ext4_xattr_i=
nfo *i,
>                         goto out;
>                 }
>         }
> -       if (i->value && in_inode) {
> +       if (i->value && in_inode && !mv_ea_inode) {
>                 WARN_ON_ONCE(!i->value_len);
>
>                 ret =3D ext4_xattr_inode_alloc_quota(inode, i->value_len)=
;
> @@ -1819,7 +1819,9 @@ static int ext4_xattr_set_entry(struct ext4_xattr_i=
nfo *i,
>
>         if (i->value) {
>                 /* Insert new value. */
> -               if (in_inode) {
> +               if (in_inode && mv_ea_inode) {
> +                       here->e_value_inum =3D cpu_to_le32(mv_ea_inode->i=
_ino);
> +               } else if (in_inode) {
>                         here->e_value_inum =3D cpu_to_le32(new_ea_inode->=
i_ino);
>                 } else if (i->value_len) {
>                         void *val =3D s->base + min_offs - new_size;
> @@ -1838,7 +1840,7 @@ static int ext4_xattr_set_entry(struct ext4_xattr_i=
nfo *i,
>         }
>
>  update_hash:
> -       if (i->value) {
> +       if (i->value && !mv_ea_inode) {
>                 __le32 hash =3D 0;
>
>                 /* Entry hash calculation. */
> @@ -1922,7 +1924,7 @@ ext4_xattr_block_find(struct inode *inode, struct e=
xt4_xattr_info *i,
>  static int
>  ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>                      struct ext4_xattr_info *i,
> -                    struct ext4_xattr_block_find *bs)
> +                    struct ext4_xattr_block_find *bs, struct inode *mv_e=
a_inode)
>  {
>         struct super_block *sb =3D inode->i_sb;
>         struct buffer_head *new_bh =3D NULL;
> @@ -1972,7 +1974,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode=
 *inode,
>                         }
>                         ea_bdebug(bs->bh, "modifying in-place");
>                         error =3D ext4_xattr_set_entry(i, s, handle, inod=
e,
> -                                                    true /* is_block */)=
;
> +                                                    true /* is_block */,=
 NULL);
>                         ext4_xattr_block_csum_set(inode, bs->bh);
>                         unlock_buffer(bs->bh);
>                         if (error =3D=3D -EFSCORRUPTED)
> @@ -2040,7 +2042,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode=
 *inode,
>                 s->end =3D s->base + sb->s_blocksize;
>         }
>
> -       error =3D ext4_xattr_set_entry(i, s, handle, inode, true /* is_bl=
ock */);
> +       error =3D ext4_xattr_set_entry(i, s, handle, inode, true /* is_bl=
ock */, mv_ea_inode);
>         if (error =3D=3D -EFSCORRUPTED)
>                 goto bad_block;
>         if (error)
> @@ -2286,7 +2288,7 @@ int ext4_xattr_ibody_set(handle_t *handle, struct i=
node *inode,
>         if (!EXT4_INODE_HAS_XATTR_SPACE(inode))
>                 return -ENOSPC;
>
> -       error =3D ext4_xattr_set_entry(i, s, handle, inode, false /* is_b=
lock */);
> +       error =3D ext4_xattr_set_entry(i, s, handle, inode, false /* is_b=
lock */, NULL);
>         if (error)
>                 return error;
>         header =3D IHDR(inode, ext4_raw_inode(&is->iloc));
> @@ -2429,7 +2431,7 @@ ext4_xattr_set_handle(handle_t *handle, struct inod=
e *inode, int name_index,
>                 if (!is.s.not_found)
>                         error =3D ext4_xattr_ibody_set(handle, inode, &i,=
 &is);
>                 else if (!bs.s.not_found)
> -                       error =3D ext4_xattr_block_set(handle, inode, &i,=
 &bs);
> +                       error =3D ext4_xattr_block_set(handle, inode, &i,=
 &bs, NULL);
>         } else {
>                 error =3D 0;
>                 /* Xattr value did not change? Save us some work and bail=
 out */
> @@ -2446,7 +2448,7 @@ ext4_xattr_set_handle(handle_t *handle, struct inod=
e *inode, int name_index,
>                 error =3D ext4_xattr_ibody_set(handle, inode, &i, &is);
>                 if (!error && !bs.s.not_found) {
>                         i.value =3D NULL;
> -                       error =3D ext4_xattr_block_set(handle, inode, &i,=
 &bs);
> +                       error =3D ext4_xattr_block_set(handle, inode, &i,=
 &bs, NULL);
>                 } else if (error =3D=3D -ENOSPC) {
>                         if (EXT4_I(inode)->i_file_acl && !bs.s.base) {
>                                 brelse(bs.bh);
> @@ -2455,7 +2457,7 @@ ext4_xattr_set_handle(handle_t *handle, struct inod=
e *inode, int name_index,
>                                 if (error)
>                                         goto cleanup;
>                         }
> -                       error =3D ext4_xattr_block_set(handle, inode, &i,=
 &bs);
> +                       error =3D ext4_xattr_block_set(handle, inode, &i,=
 &bs, NULL);
>                         if (!error && !is.s.not_found) {
>                                 i.value =3D NULL;
>                                 error =3D ext4_xattr_ibody_set(handle, in=
ode, &i,
> @@ -2615,6 +2617,10 @@ static int ext4_xattr_move_to_block(handle_t *hand=
le, struct inode *inode,
>                 .in_inode =3D !!entry->e_value_inum,
>         };
>         struct ext4_xattr_ibody_header *header =3D IHDR(inode, raw_inode)=
;
> +       struct ext4_xattr_entry *here =3D NULL, *last =3D NULL, *next =3D=
 NULL;
> +       struct inode *old_ea_inode =3D NULL;
> +       size_t name_size =3D EXT4_XATTR_LEN(entry->e_name_len);
> +       size_t min_offs;
>         int error;
>
>         is =3D kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
> @@ -2660,20 +2666,76 @@ static int ext4_xattr_move_to_block(handle_t *han=
dle, struct inode *inode,
>
>         i.value =3D buffer;
>         i.value_len =3D value_size;
> +       here =3D is->s.here;
> +       last =3D is->s.first;
> +       min_offs =3D is->s.end - is->s.base;
> +       /* Compute min_offs and last entry */
> +       for (; !IS_LAST_ENTRY(last); last =3D next) {
> +               next =3D EXT4_XATTR_NEXT(last);
> +               if ((void *)next >=3D is->s.end) {
> +                       EXT4_ERROR_INODE(inode, "corrupted xattr entries"=
);
> +                       error =3D -EFSCORRUPTED;
> +                       goto out;
> +               }
> +               if (!last->e_value_inum && last->e_value_size) {
> +                       size_t offs =3D le16_to_cpu(last->e_value_offs);
> +
> +                       if (offs < min_offs)
> +                               min_offs =3D offs;
> +               }
> +       }
> +
> +       /* Remove the name in ibody */
> +       last =3D ENTRY((void *)last - name_size);
> +       memmove(here, (void *)here + name_size,
> +               (void *)last - (void *)here + sizeof(__u32));
> +       memset(last, 0, name_size);
> +
> +       /* Get the ea_inode which store the old value */
> +       if (here->e_value_inum) {
> +               error =3D ext4_xattr_inode_iget(inode,
> +                                           le32_to_cpu(here->e_value_inu=
m),
> +                                           le32_to_cpu(here->e_hash),
> +                                           &old_ea_inode);
> +               if (error) {
> +                       old_ea_inode =3D NULL;
> +                       goto out;
> +               }
> +       } else if (here->e_value_size) {
> +               /* Remove the old value in ibody */
> +               void *first_val =3D is->s.base + min_offs;
> +               void *rm_val =3D is->s.base + le16_to_cpu(here->e_value_o=
ffs);
> +               size_t rm_size =3D EXT4_XATTR_SIZE(le32_to_cpu(here->e_va=
lue_size));
> +               size_t offs =3D le16_to_cpu(here->e_value_offs);
> +
> +               memmove(first_val + rm_size, first_val, rm_val - first_va=
l);
> +               memset(first_val, 0, rm_size);
> +               min_offs +=3D rm_size;
> +
> +               /* Adjust all value offsets */
> +               last =3D is->s.first;
> +               while (!IS_LAST_ENTRY(last)) {
> +                       size_t o =3D le16_to_cpu(last->e_value_offs);
> +
> +                       if (!last->e_value_inum &&
> +                           last->e_value_size && o < offs)
> +                               last->e_value_offs =3D cpu_to_le16(o + rm=
_size);
> +                       last =3D EXT4_XATTR_NEXT(last);
> +               }
> +       }
> +
>         error =3D ext4_xattr_block_find(inode, &i, bs);
>         if (error)
>                 goto out;
>
> -       /* Move ea entry from the inode into the block */
> -       error =3D ext4_xattr_block_set(handle, inode, &i, bs);
> +       /*
> +        * Move ea entry from the inode into the block, and do not need t=
o
> +        * recreate an ea_inode that store the same value.
> +        */
> +       error =3D ext4_xattr_block_set(handle, inode, &i, bs, old_ea_inod=
e);
>         if (error)
>                 goto out;
>
> -       /* Remove the chosen entry from the inode */
> -       i.value =3D NULL;
> -       i.value_len =3D 0;
> -       error =3D ext4_xattr_ibody_set(handle, inode, &i, is);
> -
>  out:
>         kfree(b_entry_name);
>         if (entry->e_value_inum && buffer)
> @@ -2684,6 +2746,7 @@ static int ext4_xattr_move_to_block(handle_t *handl=
e, struct inode *inode,
>                 brelse(bs->bh);
>         kfree(is);
>         kfree(bs);
> +       iput(old_ea_inode);
>
>         return error;
>  }
> --
> 2.17.1
>