[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221116122440.GN5824@suse.cz>
Date: Wed, 16 Nov 2022 13:24:40 +0100
From: David Sterba <dsterba@...e.cz>
To: Qu Wenruo <quwenruo.btrfs@....com>
Cc: ChenXiaoSong <chenxiaosong2@...wei.com>, clm@...com,
josef@...icpanda.com, dsterba@...e.com,
linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
zhangxiaoxu5@...wei.com, yanaijie@...wei.com, wqu@...e.com
Subject: Re: [PATCH v4 1/3] btrfs: add might_sleep() to some places in
update_qgroup_limit_item()
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@...e.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.
Powered by blists - more mailing lists