Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 11 Apr 2023 11:19:57 +0200
From:   Jan Kara <jack@suse.cz>
To:     Baokun Li <libaokun1@huawei.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, jack@suse.cz, ritesh.list@gmail.com,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        yangerkun@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH v2 2/2] ext4: use __GFP_NOFAIL if allocating
 extents_status cannot fail
Message-ID: <20230411091957.wuwa7wii7pj35ua7@quack3>
References: <20230406132834.1669710-1-libaokun1@huawei.com>
 <20230406132834.1669710-3-libaokun1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230406132834.1669710-3-libaokun1@huawei.com>
Precedence: bulk

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@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> 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@suse.com>
SUSE Labs, CR