On Tue 10-04-18 05:05:28, Matthew Wilcox wrote: > On Tue, Apr 10, 2018 at 10:26:43AM +0200, Michal Hocko wrote: > > On Mon 09-04-18 12:40:44, Matthew Wilcox wrote: > > > The problem is that the mapping gfp flags are used not only for allocating > > > pages, but also for allocating the page cache data structures that hold > > > the pages. F2FS is the only filesystem that set the __GFP_ZERO bit, > > > so it's the first time anyone's noticed that the page cache passes the > > > __GFP_ZERO bit through to the radix tree allocation routines, which > > > causes the radix tree nodes to be zeroed instead of constructed. > > > > > > I think the right solution to this is: > > > > This just hides the underlying problem that the node is not fully and > > properly initialized. Relying on the previous released state is just too > > subtle. > > That's the fundamental design of slab-with-constructors. The user provides > a constructor, so all newly allocagted objects are initialised to a known > state, then the user will restore the object to that state when it frees > the object to slab. And that is fundamentally subtle semantic and leads to bugs. So we should reconsider whether that is really worth keeping for the radix tree. > > Are you going to blacklist all potential gfp flags that come > > from the mapping? This is just unmaintainable! If anything this should > > be an explicit & with the allowed set of allowed flags. > > Oh, I agree that using the set of flags used to allocate the page > in order to allocate the radix tree nodes is a pretty horrible idea. > > Your suggestion, then, is: > > - error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM); > + error = radix_tree_preload(gfp_mask & GFP_RECLAIM_MASK); > > correct? Something like that, yes. -- Michal Hocko SUSE Labs
