On Thu, Sep 08, 2016 at 09:44:50PM +0100, Al Viro wrote: > On Thu, Sep 08, 2016 at 11:12:33AM -0700, Linus Torvalds wrote: > > > So the XFS model *requires* that XFS_IOLOCK to be outside all the operations. > > > > But then in iter_file_splice_write we have the other ordering. > > > > Now, xfs could do a wrapper for ->splice_write() like it used to, and > > have that same "take the xfs lock around the call to > > iter_file_splice_write(). That was my first obvious patch. > > > > I threw it out because that's garbage too: then we end up doing > > ->write_iter(), which takes the xfs_rw_ilock() again, and would > > immediately deadlock *there* instead. > > > > So the problem really is that the vfs layer seems to simply not allow > > the filesystem to do any locking around the generic page cache helper > > functions. And XFS really wants to do that. > > Why *have* ->splice_read() there at all? Let's use its ->read_iter(), where > it will take its lock as it always did for read. > > All we need is a variant of __generic_file_splice_read() that would pass > a new kind of iov_iter down to filesystem's own ->read_iter(). And let that > guy do whatever locking it wants. It will end up doing a sequence of > copy_page_to_iter() and, possibly, copy_to_iter() (XFS one would only do the > former). So let's add an iov_iter flavour that would simply grab a reference > to page passed to copy_page_to_iter() and allocate-and-copy for copy_to_iter(). > As the result, you'll get an array of <page, offset, count> triples - same > as you would from the existing __generic_file_splice_read(). Pages already > uptodate, with all readahead logics done as usual, etc. > > What else do we need? Just feed the resulting triples into the pipe and > that's it. Sure, they can get stale by truncate/punchhole/whatnot. So > they can right after we return from xfs_file_splice_read()... > > Moreover, I don't see why we need to hold pipe lock the actual call of > ->read_iter(). Right now we only grab it for "feed into pipe buffers" > part. Objections? PS: take a look at how much of do_generic_file_read() logics is kinda-sorta open-coded in __generic_file_splice_read(); readahead stuff, etc. It also assumes that filesystem needs no extra locking for playing with pagecache, etc. That's exactly why XFS ends up having to do a wrapper for that sucker and why we get all this headache. Why bother, when we already have a method that turns "read that much from this offset in this file" into a sequence of "take that many bytes from that offset in this page" and "take that many bytes from that buffer"? It doesn't even need to be a ->splice_read() instance - just a function called by do_splice_to() if ->read_iter() is present. _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
