Re: [PATCH v4 1/3] btrfs: add might_sleep() to some places in update_qgroup_limit_item()

[Qu Wenruo]

On 2022/11/16 20:24, David Sterba wrote:
> 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@xxxxxxxx>
> > > >
> > > > 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.

OK, then it makes sense now for btrfs_alloc_path().

But I still believe this looks like a bug in gfpflags_allow_blocking()...

Thanks,
Qu