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;
Date:   Thu, 13 Apr 2023 12:30:04 +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 v3 3/8] ext4: use __GFP_NOFAIL if allocating
 extents_status cannot fail
Message-ID: <20230413103004.a4hjlxgpfqnhcgtg@quack3>
References: <20230412124126.2286716-1-libaokun1@huawei.com>
 <20230412124126.2286716-4-libaokun1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230412124126.2286716-4-libaokun1@huawei.com>
Precedence: bulk

On Wed 12-04-23 20:41:21, 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.
> 
> For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
> And 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>

When I was looking through this patch, I've realized there's a problem with
my plan :-|. See below...

>  static struct extent_status *
>  ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
> -		     ext4_fsblk_t pblk)
> +		     ext4_fsblk_t pblk, int nofail)
>  {
>  	struct extent_status *es;
> -	es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
> +	gfp_t gfp_flags = GFP_ATOMIC;
> +
> +	if (nofail)
> +		gfp_flags |= __GFP_NOFAIL;
> +
> +	es = kmem_cache_alloc(ext4_es_cachep, gfp_flags);
>  	if (es == NULL)
>  		return NULL;

I have remembered that the combination of GFP_ATOMIC and GFP_NOFAIL is
discouraged because the kernel has no sane way of refilling reserves for
atomic allocations when in atomic context. So this combination can result
in lockups.

So what I think we'll have to do is that we'll just have to return error
from __es_insert_extent() and __es_remove_extent() and in the callers we
drop the i_es_lock, allocate needed status entries (one or two depending on
the desired operation) with GFP_KERNEL | GFP_NOFAIL, get the lock again and
pass the preallocated entries into __es_insert_extent /
__es_remove_extent(). It's a bit ugly but we can at least remove those
__es_shrink() calls which are not pretty either.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR