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:   Wed, 16 Nov 2022 13:24:40 +0100
From:   David Sterba <dsterba@suse.cz>
To:     Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc:     ChenXiaoSong <chenxiaosong2@huawei.com>, clm@fb.com,
        josef@toxicpanda.com, dsterba@suse.com,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhangxiaoxu5@huawei.com, yanaijie@huawei.com, wqu@suse.com
Subject: Re: [PATCH v4 1/3] btrfs: add might_sleep() to some places in
 update_qgroup_limit_item()
Message-ID: <20221116122440.GN5824@suse.cz>
Reply-To: dsterba@suse.cz
References: <20221115171709.3774614-1-chenxiaosong2@huawei.com>
 <20221115171709.3774614-2-chenxiaosong2@huawei.com>
 <9b47b291-b1a0-ac0c-2049-b7de6545c26b@gmx.com>
 <e058c1b9-7f57-11da-6ad1-6387604813c5@huawei.com>
 <3918175e-dddd-2a55-32c4-c07de78ff4cb@gmx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3918175e-dddd-2a55-32c4-c07de78ff4cb@gmx.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Wed, Nov 16, 2022 at 04:43:50PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/11/16 16:09, ChenXiaoSong wrote:
> > 在 2022/11/16 6:48, Qu Wenruo 写道:
> >> Looks good.
> >>
> >> We may want to add more in other locations, but this is really a good 
> >> start.
> >>
> >> Reviewed-by: Qu Wenruo <wqu@suse.com>
> >>
> >> Thanks,
> >> Qu
> > 
> > If I just add might_sleep() in btrfs_alloc_path() and 
> > btrfs_search_slot(), is it reasonable?
> 
> Adding it to btrfs_search_slot() is definitely correct.
> 
> But why for btrfs_alloc_path()? Wouldn't kmem_cache_zalloc() itself 
> already do the might_sleep_if() somewhere?
> 
> I just looked the call chain, and indeed it is doing the check already:
> 
> btrfs_alloc_path()
> |- kmem_cache_zalloc()
>     |- kmem_cache_alloc()
>        |- __kmem_cache_alloc_lru()
>           |- slab_alloc()
>              |- slab_alloc_node()
>                 |- slab_pre_alloc_hook()
>                    |- might_alloc()
>                       |- might_sleep_if()

The call chaing is unconditional so the check will always happen but the
condition itself in might_sleep_if does not recognize GFP_NOFS:

 34 static inline bool gfpflags_allow_blocking(const gfp_t gfp_flags)
 35 {
 36         return !!(gfp_flags & __GFP_DIRECT_RECLAIM);
 37 }

#define GFP_NOFS        (__GFP_RECLAIM | __GFP_IO)

And I think the qgroup limit was exactly a spin lock over btrfs_path_alloc so
it did not help. An might_sleep() inside btrfs_path_alloc() is a very minimal
but reliable check we could add, the paths are used in many places so it would
increase the coverage.