On Tue, Jan 14, 2014 at 05:22:07AM -0800, Christoph Hellwig wrote: > On Mon, Jan 13, 2014 at 11:56:46PM +0000, Al Viro wrote: > > On Mon, Jan 13, 2014 at 06:14:16AM -0800, Christoph Hellwig wrote: > > > ping? Would be nice to get this into 3.14 > > > > Umm... The reason for pipe_lock outside of ->i_mutex is this: > > default_file_splice_write() calls splice_from_pipe() with > > write_pipe_buf for callback. splice_from_pipe() calls that > > callback under pipe_lock(pipe). And write_pipe_buf() calls > > __kernel_write(), which certainly might want to take ->i_mutex. > > > > Now, this codepath isn't taken for files that have non-NULL > > ->splice_write(), so that's not an issue for XFS and OCFS2, > > but having pipe_lock nest between the ->i_mutex for filesystems > > that do and do not have ->splice_write()... Ouch... > > What would be the alternative? Duplicating the code in even more > filesystems to enforce an non-natural locking order for filesystems > actually implementing splice? There don't actually seem to be a whole > lot of real filesystems not implemting splice_write, the prime use > would be for device drivers or synthetic ones. I'm not even sure > how much that fallback gets used in practice. Actually, I wonder if splice should be allowed just because destination has ->write() - for a lot of synthetic files the effect of writes really depends on boundaries in output stream and I'm not sure if splice to such a beast makes any sense. Going through fs/*: * 9p: messy when we have O_DIRECT on target file. Other than that, probably could use generic_file_splice_write(). * adfs, affs, afs, bfs, fat, hfs, hfs+, minix, sysv: can use generic_file_splice_write() * /proc/fs/afs/*: probably shouldn't allow splice at all * binfmt_misc: shouldn't allow splice * btrfs: not sure, O_DIRECT complicates the picture again. * cachefiles_daemon_write(): probably shouldn't allow splice at all * ceph: uses generic_file_splice_write(), but I'm not sure whether it is correct - there are interesting things done by ceph_aio_write() that do not have any counterparts on that path. * cifs: trouble; might be switchable to generic, but there's an interesting bit in the end of its ->aio_write() to consider... In any case, should be doable as generic_file_splice_write() + filemap_fdatawrite(). * cifs mounted with strict_io - ouch. Would need ->splice_write() of its own with really interesting locking order. * cifs with direct_io - about the same story. Er... Just where is it doing suid removal in that case, BTW? cifs_user_writev() doesn't seem to? * /proc/fs/cifs/*: shouldn't allow splice to it * coda: needs ->splice_write() with that patch, might or might not be tricky * coda misc device: shouldn't allow splice to it * configfs: shouldn't allow splice to it * debugfs default fops: blackhole ->write(), might as well offer blackhole ->splice_write(). Or refuse to allow splice to it, since I'd bet that most of non-default ones are of the "shouldn't allow splice to it" kind... * debugfs bool: shouldn't allow splice to it * dlm misc devices: shouldn't allow splice to it * ecryptfs: probably should use generic_file_splice_write() (incidentally, who the hell has thought it would be a good idea to put ->iterate (aka ->readdir) into file_operations of a non-directory?) * ecryptfs misc device: probably shouldn't allow splice to it at all * eventfd: shouldn't allow splice to it at all * XIP ext2: needs ->splice_write() with that patch And that's less than half of fs/*... I'm not saying that the current situation on the write side is good; hell, just the mess with write/aio_write alone is ugly enough - we have * a bunch of file_operations without ->aio_write(); simple enough. * a bunch with ->write == do_sync_write. Also simple. * several with NULL ->write and non-NULL ->aio_write(); same as do_sync_write() for ->write (socket, android/logger, kmsg, macvtap) * several with ->aio_write being an optimized equivalent of do_sync_write() (blackhole for /dev/null and /dev/zero, error for bad_inode) * 9p cached with its "oh, but if we have O_DIRECT we want ->write() to be different" (why not use a separate file_operations, then? It's not as if ->open() couldn't switch to it if it sees O_DIRECT...) * two infinibad things (ipath and qib), with completely unrelated semantics on write(2) and writev(2) (the latter shared with aio). As in "writev() of a single-element iovec array does things that do not even resemble what write() of the same data would've done". Yes, really - check for yourself. * snd_pcm - hell knows; it might be that it tries to collect the data from iovec and push it in one go, as if it was a single write, but then it might be something as bogus as what ipath is doing... * gadgetfs - hell knows; ep_write() seems to be doing something beyond what ep_aio_write() does, but I haven't traced them down the call chain... That one, BTW, won't be fun for splice - looks like it cares about datagram boundaries a lot, so it's not obvious what the semantics should be. * lustre. I _think_ do_sync_write() would work there, but I'm might be easily missing something in all those layers of obfusca^Wgood software development practices. And ->splice_write() thrown into that mess, defaulting to "just do ->write() or ->aio_write(), everything writable should be able to cope with splice to it" hasn't made it any prettier. Unfortunately, what you are proposing will make it worse - we'll have to grow a bunch of ->splice_write() instances, with non-trivial correspondence between them and ->aio_write() ones... It needs to be cleaned up, but it's nowhere near as simple as "just flip the order of i_mutex and pipe_lock" ;-/ BTW, speaking of ->aio_write() - why the devil do we pass the pos argument (long long, at that) separately, when all call sites provably have it equal to iocb->ki_pos? If nothing else, on a bunch of architectures it makes the difference between passing arguments in registers and spilling them on stack; moreover, if we do something and only then call generic_file_aio_write(), we get to propagate it all way down. And generic_file_aio_write() has had explicit BUG_ON(iocb->ki_pos != pos) since 2.5.55, for crying out loud... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
