Messages in this thread | | | Date | Wed, 16 Nov 2022 20:26:46 +0800 | Subject | Re: [PATCH v4 1/3] btrfs: add might_sleep() to some places in update_qgroup_limit_item() | From | 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@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.
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
| |