2023-05-03 14:33:52

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH v4 06/12] ext4: using nofail preallocation in ext4_es_remove_extent()

On Mon 24-04-23 11:38:40, Baokun Li wrote:
> If __es_remove_extent() returns an error it means that when splitting
> extent, allocating an extent that must be kept failed, where returning
> an error directly would cause the extent tree to be inconsistent. So we
> use GFP_NOFAIL to pre-allocate an extent_status and pass it to
> __es_remove_extent() to avoid this problem.
>
> In addition, since the allocated memory is outside the i_es_lock, the
> extent_status tree may change and the pre-allocated extent_status is
> no longer needed, so we release the pre-allocated extent_status when
> es->es_len is not initialized.
>
> Suggested-by: Jan Kara <[email protected]>
> Signed-off-by: Baokun Li <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/ext4/extents_status.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 7219116e0d68..f4d50cd501fc 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1458,6 +1458,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> ext4_lblk_t end;
> int err = 0;
> int reserved = 0;
> + struct extent_status *es = NULL;
>
> if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
> return 0;
> @@ -1472,17 +1473,25 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> end = lblk + len - 1;
> BUG_ON(end < lblk);
>
> +retry:
> + if (err && !es)
> + es = __es_alloc_extent(true);
> /*
> * ext4_clear_inode() depends on us taking i_es_lock unconditionally
> * so that we are sure __es_shrink() is done with the inode before it
> * is reclaimed.
> */
> write_lock(&EXT4_I(inode)->i_es_lock);
> - err = __es_remove_extent(inode, lblk, end, &reserved, NULL);
> + err = __es_remove_extent(inode, lblk, end, &reserved, es);
> + if (es && !es->es_len)
> + __es_free_extent(es);
> write_unlock(&EXT4_I(inode)->i_es_lock);
> + if (err)
> + goto retry;
> +
> ext4_es_print_tree(inode);
> ext4_da_release_space(inode, reserved);
> - return err;
> + return 0;
> }
>
> static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR