Re: [PATCH v2 2/2] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail

[Jan Kara]

On Thu 06-04-23 21:28:34, Baokun Li wrote:
> If extent status tree update fails, we have inconsistency between what is
> stored in the extent status tree and what is stored on disk. And that can
> cause even data corruption issues in some cases.
> 
> In the extent status tree, we have extents which we can just drop without
> issues and extents we must not drop - this depends on the extent's status
> - currently ext4_es_is_delayed() extents must stay, others may be dropped.
> 
> For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
> A helper function is also added to help determine if the current extent can
> be dropped, although only ext4_es_is_delayed() extents cannot be dropped
> currently. In addition, with the above logic, the undo operation in
> __es_remove_extent that may cause inconsistency if the split extent fails
> is unnecessary, so we remove it as well.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> ---
> V1->V2:
> 	Add the patch 2 as suggested by Jan Kara.
> 
>  fs/ext4/extents_status.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 7bc221038c6c..8eed17f35b11 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -448,12 +448,29 @@ static void ext4_es_list_del(struct inode *inode)
>  	spin_unlock(&sbi->s_es_lock);
>  }
>  
> +/*
> + * Helper function to help determine if memory allocation for this
> + * extent_status is allowed to fail.
> + */
> +static inline bool ext4_es_alloc_should_nofail(struct extent_status *es)

I'd call this function ext4_es_must_keep() and also use it in
es_do_reclaim_extents() instead of ext4_es_is_delayed(). Do this as a
preparatory patch please.

> @@ -792,9 +809,16 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
>  	}
>  
>  	es = ext4_es_alloc_extent(inode, newes->es_lblk, newes->es_len,
> -				  newes->es_pblk);
> -	if (!es)
> -		return -ENOMEM;
> +				  newes->es_pblk, 0);

I would just call this like:

	es = ext4_es_alloc_extent(inode, newes->es_lblk, newes->es_len,
				newes->es_pblk, ext4_es_must_keep(newes));

to save the ifs below.

> +	if (!es) {
> +		/* Use GFP_NOFAIL if the allocation cannot fail. */
> +		if (ext4_es_alloc_should_nofail(newes))
> +			es = ext4_es_alloc_extent(inode, newes->es_lblk,
> +					newes->es_len, newes->es_pblk, 1);
> +		else
> +			return -ENOMEM;
> +	}
> +
>  	rb_link_node(&es->rb_node, parent, p);
>  	rb_insert_color(&es->rb_node, &tree->root);
>  
> @@ -1349,8 +1373,6 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  						    ext4_es_status(&orig_es));
>  			err = __es_insert_extent(inode, &newes);
>  			if (err) {
> -				es->es_lblk = orig_es.es_lblk;
> -				es->es_len = orig_es.es_len;
>  				if ((err == -ENOMEM) &&
>  				    __es_shrink(EXT4_SB(inode->i_sb),
>  							128, EXT4_I(inode)))

Also now __es_remove_extent() cannot fail (it will always remove what it
should, maybe more) so please just make it void function (as a separate
cleanup patch afterwards). Thanks!

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR