On Tue, Jun 30, 2020 at 11:26:45AM -0700, Darrick J. Wong wrote: > On Tue, Jun 30, 2020 at 06:23:12PM +0300, Amir Goldstein wrote: > > > /me wonders if someone could please check all the *_ops that point to > > > generic helpers to see if we're missing obvious things like lock > > > taking. Particularly someone who wants to learn about xfs' locking > > > strategy; I promise it won't let out a ton of bees. > > > > > > > The list was compiled manually by auditing 'git grep '_operations.*=' fs/xfs' > > structs for non xfs_/iomap_/noop_ functions. > > I am not sure if all iomap_ functions are safe in that respect, but I suppose > > those were done recently with sufficient xfs developers review... > > The iomap functions shouldn't be taking/releasing any locks at all; it's > up to the filesystem to provide the concurrency controls. > > > fs/xfs/xfs_aops.c:const struct address_space_operations > > xfs_address_space_operations = { > > .error_remove_page = generic_error_remove_page, > > > > generic_error_remove_page() calls truncate_inode_page() without MMAPLOCK > > Is that safe? not sure > > /me has a funny feeling it isn't, since this does the same thing to the > pagecache as a holepunch. And I really can't tell, because this comes from the convoluted hardware memory error path. We know that path is completely screwed up w.r.t. memory errors in pmem and DAX filesystems. Hence I think there's a good chance it's completely screwed up for cached file-backed page cache pages, too. Indeed, look at the comment in me_pagecache_clean(): /* * Truncation is a bit tricky. Enable it per file system for now. * * Open: to take i_mutex or not for this? Right now we don't. */ return truncate_error_page(p, pfn, mapping); the call path is: me_pagecache_clean() truncate_error_page() ->error_remove_page() IOWs, the authors of this code did not know what to do, and like the DAX failure stuff, merged the code without having resolving the fundamental issues around interfacing with filesystem owned pages directly... I don't really have the time to look at it in any more depth right now. I also suspect the memory failure code is a path we simply cannot exercise in any useful manner so it's unlikely that we'll ever be able to tell if this stuff works correctly or not.... Cheers, -Dave. -- Dave Chinner david@xxxxxxxxxxxxx