xfs_file_splice_read: possible circular locking dependency detected

Nope, this goes back to the splice rework back around ~3.16, IIRC.

pipe_lock taken below the filesystem IO path, filesystem holds locks to protect against racing hole punch, etc...

pipe_lock taken above the filesystem IO path, filesystem tries to take locks to protect against racing hole punch, etc, lockdep goes boom.

Fundamentally a splice infrastructure problem. If we let splice race with hole punch and other fallocate() based extent manipulations to avoid this lockdep warning, we allow potential for read or write to regions of the file that have been freed. We can live with having lockdep complain about this potential deadlock as it is unlikely to ever occur in practice. The other option is simply not an acceptible solution....

Cheers,

Dave.

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Yeah, I don't really like how we handle the pipe lock.

It *might* be possible to instead just increment the reference counters as we build a kvec[] array of them, and simply do teh write without holding the pipe lock at all.

That has other problems, ie concurrect spices from the same pipe would possibly write the same data multiple times, though.

But yes, the fundamental problem is how splice wants to take the pipe lock both early and late. Very annoying.

Linus

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

We could, in principle, add another flavour of iov_iter, with bvec array attached to it with copy_page_to_iter() sticking an extra ref to that page into array. Then, under pipe lock, feed that thing to ->read_iter() and do an equivalent of splice_to_pipe() that would take bvec array instead of struct page */struct partial_page arrays.

Hell, we could even have copy_to_iter() for these puppies allocate a page, stick it into the next bvec and copy into it. Especially if we have those bvec zeroed, with copy_page_to_iter() leaving ->bvec pointing to the next (unused) bvec and copy_to_iter() doing that only when a page had been completely filled. I.e.

copy_page_to_iter()

	if (!iter->nr_segs)
		return 0;
	if (iter->bvec->bv_page) {
		iter->bvec++;
		if (!--iter->nr_segs)
			return 0;
	}
	stick (page,offset,bytes) into iter->bvec
	iter->bvec++;
	iter->nr_segs--;
	return bytes;

copy_to_iter():

	wanted = bytes;
	while (bytes && iter->nr_segs) {
		if (!iter->bvec->bv_page)
			iter->bvec->bv_page = alloc_page()
		n = min(PAGE_SIZE - iter->bvec->bv_len, bytes);
		memcpy_to_page(addr, iter->bvec->bv_len, n);
		bytes -= n;
		iter->bvec->bv_len += n;
		if (iter->bvec->bv_len == PAGE_SIZE) {
			iter->bvec++;
			iter->nr_segs--;
		}
	}
	return wanted - bytes;

That should suffice for quite a few of read_iter-using file_operations, if not for all of them. pipe lock is on the outside, same as for write side *and* for default_file_splice_read(). And filesystem gets the same locking it would for read(2)/readv(2)/etc...

Comments?

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Not sure I quite follow - where do the pages come from? Do we allocate new pages that get put into the bvec, then run the read which copies data from the page cache page into them, then hand those pages in the bvec to the pipe?

ISTR this read->splice_to_pipe path was once supposed to be a zero-copy path - doesn't this make zero-copy impossible? Or was the zero-copy splice read path done through some other path I've forgotten about?

This has the same "data copy in the splice read path" as the above interface. However, I suspect that this interface could actually be used for zero copy (by stealing pages from the page cache rather than allocating new pages and copying), so it may be a better way to proceed...

Cheers,

Dave.

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Nope. generic_file_read_iter() (do_generic_file_read(), in the end) finds them in page cache, or allocates and sticks them into pagecache,
makes sure that they are uptodate, etc. And passes them to
copy_page_to_iter(), which would, for this iov_iter flavour, just grab a reference to page and stash it into bvec. There's your zero-copy, exactly as it works now. Only __generic_file_splice_read() open-codes everything ->read_iter() would do, sans the locks filesystem would need.

For copy_page_to_iter() we have a page; for copy_to_iter() the data comes from hell knows what - kmalloc'ed array into which we'd decrypted something, results of sprintf() into on-stack array, etc. So the counterparts of copy_to_iter() callers must be non-zerocopy. copy_page_to_iter() is the potential zerocopy path and we do get zerocopy there that way.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

----- Original Message -----

The problem with living with having this lockdep complain that it seems once this lockdep happens, it will prevent other complains from showing up. For example, I have to apply the commit dc3a04d to fix an early rcu lockdep first during the bisecting.
CAI Qian

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Not my problem.

My primary responsibility is to maintain the filesystem integrity and data safety for the hundreds of thousands (millions?) of XFS
users: it's their data, and I will always err on the side of safety and integrity. As such I really don't care if there's collateral damage to developer debug tools - user data integrity requirements always come first...

Cheers,

Dave.

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

----- Original Message -----

Right. FYI, revert the commit below fixes the regression,

8d02076 : ->splice_write() via ->write_iter()

CAI Qian

... and brings back a lot of other crap. The thing is, pipe lock should be on the outside of everything fs might be taking, so that splice IO is the same as normal IO as far as filesystem locking is concerned. For the write side it had been done in that commit, for the read side it's yet to be done.

Al, look at generic_file_splice_read().

The problem is that XFS takes its own

        xfs_rw_ilock(ip, XFS_IOLOCK_SHARED);
        ..
        xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);

around all the generic file accessors. So for example, it doesn't use "generic_file_read_iter()", it does

STATIC ssize_t
xfs_file_buffered_aio_read(
struct kiocb *iocb,
struct iov_iter *to)

{
        struct xfs_inode        *ip = XFS_I(file_inode(iocb->ki_filp));
        ssize_t                 ret;

        trace_xfs_file_buffered_read(ip, iov_iter_count(to), iocb->ki_pos);

        xfs_rw_ilock(ip, XFS_IOLOCK_SHARED);
        ret = generic_file_read_iter(iocb, to);
        xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED);

        return ret;
}

and the exact same pattern holds for generic_file_splice_read().

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.

Linus

Hmm.

I wonder if we could just take the pipe lock *much* earlier at the splice() layer? Do it before any callbacks to the low-level filesystems, not inside the "generic" splice helpers at all?

That would clean up a ton of crap.

The *one* reason that seems impossible right now seems to be that we use "pipe_wait()" in our splice ops. And "pipe_wait()" drops and retakes the pipe lock over the waiting.

BUT.

What if we got rid of all the pipe-wait crap entirely, and just made all the splice routines return EAGAIN instead of waiting? And then do the pipe_wait() at the higher level, outside the filesystem callback code, and outside the low-level generic helpers?

Maybe that pipe_wait() movement doesn't really work for some reason that I didn't look at, but that would really help make the locking enormously simpler. And then the pipe lock would *obviously* be the outermost lock, and we'd get rid of all the issues with filesystem lock ordering.

Linus

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Actually, screw the "array of bvec"; we'd need to mark the ones that are pagecache-backed somehow to tell which methods should be used. Let's add a variant of iov_iter that would be backed by pipe_buffer array; copy_page_to_iter() fills the next slot with an extra reference to the page we'd been given and using page_cache_pipe_buf_ops for ->ops. copy_to_iter() adds to the last slot if it has default_pipe_buf_ops for ->ops and still has space in it or allocates a new page, sticks into the next slot, copies data into it and sets default_pipe_ops for ->ops.

Then all we need is a variant of splice_to_pipe()/splice_{grow,shrink}_spd() that would work with an array of pipe_buffer instead of page/partial_page array pair.

That's what I first tried when this was first reported back in 3.18-rc0, and after a couple of other aborted attempts to work around the pipe_lock I came to the same conclusion:

"That smells like a splice architecture bug. splice write puts the pipe lock outside the inode locks, but splice read puts the pipes locks *inside* the inode locks. "

http://oss.sgi.com/archives/xfs/2014-10/msg00319.html

It's not an XFS specific problem: any filesystem that supports hole punch and it's fallocate() friends needs this high level splice IO exclusion as well.

Cheers,

Dave.

Dave Chinner
[email protected]

How is hole punch different from truncate()? My reading of the situation is that we don't need exclusion between that and insertion into pipe; only for "gather uptodate page references" part. If some page gets evicted afterwards... how is that different from having that happen right after we'd finished with ->splice_read()? Am I missing something subtle in there?

I'm still looking at the O_DIRECT paths in that stuff; we'll probably need iov_iter_get_pages() for these suckers to allocate pages and stick them into slots. The tricky part is to get the semantics of iov_iter_advance() right for them, but it does look feasible.

Again, what I propose is a new iov_iter flavour. Backed by pipe_buffer array, used only for reads (i.e. copy to, not copy from). Three states for element:
pagecache one, copied data, empty. Semantics:
* copy_page_to_iter(): grab a reference to page and stick it into
the next element (making it a pagecache one) with offset and len coming directly from arguments.
* copy_to_iter(): if the last element is a 'copied data' with empty
space remaining - copy to the end. Otherwise allocate a new page and stick it into the next element (making it 'copied data'), then copy into it. If still not all data copied, do the same for the next element, etc. Of course, if there's no elements left, we are done copying.
* zero_iter(): ditto, with s/copy/fill with zeroes/
* iov_iter_get_pages(): allocate pages, stick them into the next
slots (making those 'copied data'). That might need some changes, though - I'm still looking through the users. The tricky part is decision when to update the lengths.
* iov_iter_get_pages_alloc(): not sure, hadn't really looked yet.
* iov_iter_alignment(): probably just returns 0.
* iov_iter_advance(): probably like bvec variant.

generic_file_splice_read() gathers pages into spd.pages[], taking a refernce to them. The pages are not locked.

truncate does things in this order:

move EOF,
invalidate page cache,
free disk space

So if we race iwth a truncate, the pages in spd.pages[] that are beyond the new EOF may or may not have been removed from the page cache. The splice code handles this specific race condition by again checking the uptodate page against the current EOF before updating the spd to include it:

fill_it:

		/*
		 * i_size must be checked after PageUptodate.
		 */
		isize = i_size_read(mapping->host);
		end_index = (isize - 1) >> PAGE_SHIFT;
		if (unlikely(!isize || index > end_index))
			break;

At this point, if the page is inside isize we know it has good data in it, and we can hand it off to whoever.

The problem with hole punch or an extent shift is that the size does not change and so the invalidated page is still within the valid range of the file. Hence if we race with invalidation here, it does not get caught and what we put into the buffer does not reflect the data in the file at the time the pipe buffer is built.

This isn't specific to splice - it's the same issue for all page cache lookup and validation checks. This issue is one of the reasons why XFS has a MMAPLOCK similar to the IOLOCK - we can't take the IOLOCK in the page fault path, but we still need to protect page faults against racing page invalidations within EOF from operations like hole punch.

Sounds reasonable, but the iter stuff makes my head hurt so I haven't thought about it that deeply yet.

Cheers,

Dave.

Dave Chinner
[email protected]

So I'm not sure why we'd need to care?

The thing is, if the splicer and the hole puncher aren't synchronized, then there is by definition no "before/after" point.

The splice data may be "stale" in the sense that we look at the page after the hole punch has happened and the page no longer has a ->mapping associated with it, but it is equally valid to treat that as just a case of "the read happened before the hole punch".

Put another way: it's not wrong to use the ostensibly "stale" data, it just means that the splice acts as if the IO had happened before the data became stale.

The whole point of "splice" is for the pipe to act as a in-kernel buffer. So a splice does not *synchronize* the two end-points, quite the reverse: it is meant to act as a "read + write" with the pipe itself being the buffer in between (and because it's a in-kernel buffer rather than a user space buffer like a real read()+write() pair would be, it means that we then *can* do things like zero-copy, but realistically it really aims for "one-copy" rather than "two-copy".

So if the splice buffer contains stale values, then that's exactly similar to a user space application having done a "read()" of old data, then the file is truncated (or hole punched), and then the application does a "write()" on that data. The target clearly sees *different* data than is on the filesystem at that point, but since "complete synchronization" has never been a guarantee of splice() in the first place, that's just not a downside.

If an application expects to have "splice()" give some kind of data consistency guarantees wrt people writing to the file (or with truncate or hole punching), then the application would have to implement that serialization itself. Splice in itself does not do serialization, it does data copying.

Linus

Side note, just to clarify: I'm not actually convinced that turning things into page/offset/len tuples is the right thing to do.

I still suspect that the reference count updates on each page may not be a good idea. I suspect we'd easily be better off trying to do everything under the pipe lock exactly so that we can *avoid* having to do per-page "increment ref-count, then decrement it again". But the locking would have to be changed radically for us to be able to do that (and the only sane model ios, I think, to make pipe_lock be the outermost lock, and outside *every* downcall)

Linus

IDGI. Suppose we do splice from file to pipe. Everything had been in page cache, so we want to end up with pipe_buffers containing references to those page cache pages. How do you propose to do that without having grabbed references to them? What's to keep them from being freed by the time we get to reading from the pipe?

So that's obviously what we already do. That is, after all, why splice doesn't actually keep track of "pages", it keeps track of "struct pipe_buffer". So each page has not just offset/len associated with it, but also a get/release/verify operation block and some flags with them (it might not be a page-cache page, so in some cases it might be a skb or something that needs different release semantics).

And if you can make the iterator basically interface with turning the page/offset/len directly into a "struct pipe_buffer" and not do any extra reference operations, then it actually would work very well.

But the way I read your description of what you'd do I just expected you to have an extra "get/put" ref at the iterator level.

Maybe I misunderstood. I'd love to see a rough patch.

Linus

Umm... Looks like I misunderstood you, then. Yes, it ends up with get/get/put, the last two close in time. Do you expect that to be a serious overhead? atomic_inc + atomic_dec_and_test + branch not taken shouldn't be _that_ hot, and I would rather avoid complicating do_generic_file_read() and friends with "iov_iter (somehow) told us not to put this page as we normally would". Can be done that way, but let's not bother until it really shows in profiles.

Cooking it... The thing I really hate about the use of pipe_buffer is that we want to keep the "use on-stack array for default pipe size" trick, and pipe_buffer is fatter than I'd like. Instead of pointer + two numbers + something to indicate whether it's picked from page cache or something we'd allocated we get get pointer + int + int + pointer + int + long, which turns into 5 words on 64bit. With 16-element array of those on stack frame, it's not nice - more than half kilobyte of stack space with ->read_iter() yet to be called... bvec would be better (60% savings boils down to 384 bytes shaved off that thing), but we'd need to play games with encoding the "is it page cache or not" bit somewhere in it.

BTW, AFAICS that thing can be used to replace _all_ non-default filesystem instances - lustre, nfs, gfs2, ocfs2, xfs, shmem, even coda. What remains:
* fuse_dev_splice_read()
* relay_file_splice_read()
* tracing_splice_read_pipe()
* tracing_buffers_splice_read()
* sock_splice_read()
TBH, the last one makes me nervious -
/* Drop the socket lock, otherwise we have reverse
* locking dependencies between sk_lock and i_mutex
* here as compared to sendfile(). We enter here
* with the socket lock held, and splice_to_pipe() will
* grab the pipe inode lock. For sendfile() emulation,
* we call into ->sendpage() with the i_mutex lock held
* and networking will grab the socket lock.
*/
release_sock(sk);
ret = splice_to_pipe(pipe, spd);
lock_sock(sk);
in skb_socket_splice() and

        mutex_unlock(&u->readlock);
        ret = splice_to_pipe(pipe, spd);
        mutex_lock(&u->readlock);

in skb_unix_socket_splice() smell like yet another indication that we are taking the locks in wrong order. OTOH, lifting the pipe lock all way out of that, especially the last one, really smells like asking for deadlocks. It is a separate issue, but it'll also need looking into...

I wonder if relay_file_read() would be better off converted to ->read_iter() and unified with relay_file_splice_read() - we do copy_to_user() from vmap'ed area, but we have the array of underlying struct page *, so it could switch to copy_page_to_iter(), at which point ->splice_read() would _probably_ be OK with switch to ->read_iter() use. The tricky part is their use games with relay_consume_bytes() in relay_pipe_buf_release(). Who maintains that thing (kernel/relay.c) these days? git log for it looks like it's been pretty much abandoned...

No, please don't play games like that.

I think you'd be better off with just a really small on-stack case (like maybe 2-3 entries), and just allocate anything bigger dynamically. Or you could even see how bad it is if you just force-limit it to max 4 entries or something like that and just do partial writes.

buffer sizes for things like read/write system calls were *very* skewed.

There's a lot of small stuff, then there is the stuff that actually honors st.st_blksize (normally one page), and then there is the big buffers stuff.

And the thing is, the big buffers are almost never worth it. It's often better to have a tight loop over smaller data than bouncing lots of data into buffers and then out of buffers.

So I suspect all the "let's do many pages in one go" stuff is actually not worth it. Especially since the pipes will basically force a wait event when the pipe buffers fill up anyway.

So feel free to try maxing out using only a small handful of pipe_buffer entries. Returning partial IO from splice() is fine.

Linus

[Jens and Nick Cc'd]

Umm... Right now it tries to allocate as much as the output pipe could possibly hold. With default being 16 buffers, you'll end up with doing dynamic allocation in all cases (it doesn't even look at the amount of data we want to transfer).

The situation with splice_pipe_desc looks very odd:

* all but one instance are on stack frames of some ->splice_read()
or something called by it (exception is in vmsplice)

* all but one instance (a different one - see below) go through
splice_grow_spd / splice_to_pipe / splice_shrink_spd sequence and nothing else sees them. The exception is skb_splice_bits() and there we have MAX_SKB_FRAGS for size, don't bother with grow/shrink and the only thing done to that spd is splice_to_pipe() (from the callback passed to skb_splice_bits()).

* only one ->splice_read() instance does _not_ create
splice_pipe_descriptor. It's fuse_dev_splice_read(), and it pays for that by open-coding splice_to_pipe(). The only reason for open-coding is that we don't have a "stronger SPLICE_F_NONBLOCK" that would fail if the data wouldn't fit. SPLICE_F_NONBLOCK stuffs as much as possible and buggers off without waiting, fuse_dev_splice_read() wants all or nothing (and no waiting).

* incidentally, we *can't* add new flags - splice(2)/tee(2)/vmsplice(2)
quietly ignore all bits they do not recognize. In fact, splice(2) ends up passing them (unsanitized) to ->splice_read and ->splice_write instances.

* for splice(2) the IO size is limited by nominal capacity of output
pipe. Looks fairly arbitrary (the limit is the same whether the pipe is full or empty), but I wouldn't be surprised if userland programmers would get unhappy if they have to take more iterations through their loops.

* the other caller of ->splice_read() is splice_direct_to_actor() and
that can be called on a fairly deep stack. However, there we loop ourselves and smaller chunk size is not a problem.

* in case of skb_splice_bits(), we probably want a grow/shrink pair
as well, with well below MAX_SKB_FRAGS for a default - what's the typical number of fragments per skb?

Are you sure that nobody's growing the output pipe buffer before
doing splice() into it as a way to reduce the amount of iterations?

FWIW, I would love to replace these array of page * + array of
<offset,len,private> triples with array of pipe_buffer; for one thing, this ridiculous ->sbd_release() goes away (we simply call ->ops->release() on all unwanted buffers), which gets rid of wonders like
static void buffer_spd_release(struct splice_pipe_desc *spd, unsigned int i)

{
        struct buffer_ref *ref =
                (struct buffer_ref *)spd->partial[i].private;

        if (--ref->ref)
                return;

        ring_buffer_free_read_page(ref->buffer, ref->page);
        kfree(ref);
        spd->partial[i].private = 0;
}
static void buffer_pipe_buf_release(struct pipe_inode_info *pipe,
                                    struct pipe_buffer *buf)
{
        struct buffer_ref *ref = (struct buffer_ref *)buf->private;

        if (--ref->ref)
                return;

        ring_buffer_free_read_page(ref->buffer, ref->page);
        kfree(ref);
        buf->private = 0;
}

pairs that need to be kept in sync, etc.

One inconvenience created by that is stuff like

        spd.nr_pages = find_get_pages_contig(mapping, index, nr_pages, spd.pages);

in there; granted, this one will go away with __generic_file_splice_read(), but e.g. get_iovec_page_array() is using get_user_pages_fast(), which wants to put pages next to each other. That one is from vmsplice_to_pipe() guts, and I've no idea what the normal use patterns are. OTOH, how much overhead would we get from repeated calls of get_user_pages_fast() for e.g. 16 pages or so, compared to larger chunks? It is on a shallow stack, so it's not as if we couldn't afford a 16-element array of struct page * in there...

Should not be so bad, but I don't have hard numbers for you. PAGEVEC_SIZE is 14, and that's conceptually rather similar operation (walk radix tree; grab pages). OTOH many archs are heavier and do locking and vmas walking etc.

Documentation/features/vm/pte_special/arch-support.txt

But even for those, at 16 entries, the bulk of the cost *should* be hitting struct page cachelines and refcounting. The rest should mostly stay in cache.

Yes. And those costs will be exactly the same whether we do 16 entries at a time or 4 loops of 4 entries.

There's something to be said for small temp buffers. They often have better cache behavior thanks to re-use than having larger arrays.

But I still think that the biggest win could be from just trying to cut down on code, if we can just say "we'll limit splice to N entries" (where "N" is small enough that we really can do everything in a simple stack allocation - I suspect 16 is already too big, and we really should look at 4 or 8).

And if we actually get a report of a performance regression, we'd at least hear who actually *uses* splice and notices.

I'm (sadly) still not at all convinced that "splice()" was ever a good idea. I think it was a clever idea, and it is definitely much more powerful conceptually than sendfile(), but I also suspect that it's simply not used enough to be really worth the pain.

You can get great benchmark numbers with it. But whether it actually matters in real life? I really don't know. But if we screw it up, and make the buffers too small, and people actually complain and tell us about what they are doing, that in itself would be a good datapoint.

So I wouldn't be too worried about just trying things out. We certainly don't want to *break* anything, but at the same time I really don't think we should be too nervous about it either.

Which is why I'd be more than happy to say "Just try limiting things to a pretty small buffer and see if anybody even notices!"

Linus

[finally Cc'd to fsdevel - should've done that several iterations upthread]

OK... That's actually important only for vmsplice_to_pipe() and 16-page array seems to be doing fine there.

Another question, now that you've finally resurfaced: could you reconstruct the story with page-stealing and breakage(s) thereof that had lead to commit 485ddb4b9741bafb70b22e5c1f9b4f37dc3e85bd

Author: Nick Piggin <[email protected]>
Date:   Tue Mar 27 08:55:08 2007 +0200

1/2 splice: dont steal

I realize that it had been 9 years ago, but anything resembling a braindump would be very welcome. Note that there is a couple of ->splice_write() instances that _do_ use ->steal() (fuse_dev_splice_write() and virtio_console port_fops_splice_write()) and I wonder if they suffer from the same problems; your commit message is rather short on details, unfortunately. FUSE one is especially interesting...

Without looking through all the patches again, I believe the issue was just that filesystems were not expecting (or at least, not audited to expect) pages being added to their pagecache in that particular state (they'd expect to go through ->readpage or see !uptodate in prepare_write).

If some wanted to attach metadata to uptodate pages for example, this may have caused a problem. It wasn't some big fundamental problem, just a mechanical one.

Thanks,
Nick'

Umm... Why not make it non-uptodate/locked, try to replace the original with it in pagecache and then do full-page ->write_begin immediately followed by full-page ->write_end? Looks like that ought to work in all in-tree cases...

That sounds like it probably should work for that case. IIRC, I was looking at using a write_begin flag to notify the case of of replacing the page, so the fs could also handle the case of replacing existing pagecache.

Do we care?

There's a lot of people who use large buffers. That doesn't necessarily mean that it is the right thing to do. A small buffer that we can allocate on-stack might well be better even if it causes more iterations.

I'd also like to simplify the splice code if at all possible. Particularly as there really aren't necessarily all that many actual users of it. So if we can say "screw that" and just allocate a small buffer on stack, and people end up iterating a bit more, so what? The point of splice is to avoid the data copies and VM games, not to make big buffers.

Linus

Then pipe_buffer it is; it will take a bit of surgery, but I'd expect the end result to be much simpler. OK, so splice_pipe_desc switches from the pages/partial_pages/ops/spd_release to pipe_bufs, and I'm actually tempted to replace nr_pages with "the rest of ->pipe_bufs[] has NULL ->page". Then it becomes simply

struct splice_pipe_desc {
	struct pipe_buffer *bufs;
	int nbufs;
	unsigned flags;
}, perhaps with struct pipe_buffer _bufs[INLINE_SPLICE_BUFS]; in the end.

struct partial_page simply dies...

Next question: what to do with sanitizing flags in splice(2)/vmsplice(2)/tee(2)? Right now we accept anything, and quietly ignore everything outside of lower 4 bits. Should we start masking everything else out and/or warning about anything unexpected?

What I definitely want for splice_to_pipe() is an additional flag for
"fail unless there's enough space to copy everything". Having fuse open-code splice_to_pipe() with all its guts is just plain wrong. I'm not saying that it should be possible to set in splice(2) arguments; it's obviously an ABI breakage, since currently we ignore all unknown bits. The question is whether we mask the unknown bits quietly; doing that with yelling might allow to make them eventually available.

Actually, we can do even better, and kill the sodding splice_pipe_desc entirely, along with skb_splice_bits() callback.

1) make splice_to_pipe() return on pipe overflow, flags be damned. And lift pipe_lock()/looping/waking the readers up into callers. Basically, what you've suggested earlier in the thread. There are 2 kinds of callers - vmsplice_to_pipe() and assorted ->splice_read(), called from do_splice_to(). pipe_lock and loop is lifted into vmsplice_to_pipe() and into do_splice(); another caller of do_splice_to() already has a loop *and* couldn't wait on the pipe anyway - it uses an internal one.

2) fuse_dev_splice_read() checks the amount of space in the pipe and either buggers off or calls splice_to_pipe().

3) since the pipe is locked, skb_splice_bits() callbacks don't need to unlock/relock any socket locks. All those callbacks are simply splice_to_pipe() and can be replaced with direct call of that sucker.

4) since the pipe is locked, there's no point feeding the bits in one go; we can as well send them one by one. That kills splice_to_pipe(), splice_pipe_desc and these on-stack arrays, along with the questions about their size.

5) that iov_iter flavour is backed by pipe. {__,}generic_file_splice_read() is gone - we simply set an iov_iter over our locked pipe and pass it to ->read_iter(). That serves as ->splice_read() where generic_file_splice_read() used to be used, as well as nfs/ocfs2/gfs2/shmem instances.

6) The same happens to coda and lustre instances, taking a bunch of crud out in case of lustre (IO_SPLICE handling parallel to IO_NORMAL and ->vui_io_subtype in general). Moreover, skb_splice_bits() becomes very similar to skb_copy_datagram_iter(), possibly allowing to replace at least AF_UNIX ->splice_read() with the same generic ->read_iter()-based one - or doing the same to _all_ socket ones. Even more interesting is that fuse_dev_splice_read() just might become replacable with that, at the price of some massage (and simplifications) of fuse_copy_page(). If _that_ works out, we are in a situation where that thing is universal for everything that has ->read_iter() in the first place. Most of the stuff that has only ->read() uses default_file_splice_read(); the only irregular instances left are kernel/relay.c and kernel/trace/trace.c ones. Incidentally, these irregulars are precisely the ones that make use of buf->private.

So that all sounds very much like a big improvement. Not that I think removing splice_pipe_desc is such a big deal per se, but on the whole the less the actual low-level iterator does, and the more we do at a higher level, the happier I am. The fact that you say you can remove it does make it sound like you got rid of the right amount of complexity, though. The reason that whole thing exists is exactly because otherwise the splice callbacks would look too damn hair for words.

If we get rid of all the SPLICE_F_NONBLOCK, sighandling, pipe_wait() and fasync crap at the low level, I'll already be much happier. I hate how complex that code is, and how the filesystems call into it as a helper etc. Doing just the iterator in the deep corners of splice sounds like absolutely the right thing to do.

So reading your outline I say "wonderful".

Of course, maybe I'll change my mind when I actually see your patches ;^p

Linus

FWIW, I'm not sure if skb_splice_bits() can't land us in trouble; fragments might come from compound pages and I'm not entirely convinced that we won't end up with coalesced fragments putting more than PAGE_SIZE into a single pipe_buffer. And that could badly confuse a bunch of code.

Can that legitimately happen? If so, we'll need to audit quite a few ->splice_write()-related codepaths; FUSE, in particular, is very likely to be unhappy with that kind of stuff, and it's not the only place where we might count upon never seeing e.g. longer than PAGE_SIZE chunks in bio_vec. It shouldn't be all that hard to fix, but if the whole thing is simply impossible, I would rather avoid that round of RTFS at the moment...

Comments?

The pipe buffer code is actually *supposed* to handle any size allocations at all. They should *not* be limited by pages, exactly because the data can come from huge-pages or just multi-page allocations. It's definitely possible with networking, and networking is one of the *primary* targets of splice in many ways.

So if the splice code ends up being confused by "this is not just inside a single page", then the splice code is buggy, I think.

Why would splice_write() cases be confused anyway? A filesystem needs to be able to handle the case of "this needs to be split" regardless, since even if the source buffer were to fit in a page, the offset might obviously mean that the target won't fit in a page.

Now, if you decide that you want to make the iterator always split those possibly big cases and never have big iovec entries, I guess that would potentially be ok. But my initial reaction is that they are perfectly normal and should be handled normally, and any code that depends on a splice buffer fitting in one page is just buggy and should be fixed.

Linus

What worries me is iov_iter_get_pages() and friends. The calling conventions are

	size = iov_iter_get_pages(iter, pages, maxlen, maxpages, &start);

They are convenient enough for most of the callers - we fill an array of pages, the first (and only in bvec case) one having start bytes skipped.

The thing is, the calculation of the number of pages returned is broken in this case; normally it's ROUND_DIV_UP(start + n, PAGE_SIZE). That, of course, gets broken even by the offset being large enough. We don't have that many users of that thing (and iov_iter_get_pages_alloc()), but it'll need careful review. What's more, looking at those shows other fun issues:

        sg_init_table(sgl->sg, npages + 1);

        for (i = 0, len = n; i < npages; i++) {
                int plen = min_t(int, len, PAGE_SIZE - off);

                sg_set_page(sgl->sg + i, sgl->pages[i], plen, off);

and that'll instantly blow up, due to PAGE_SIZE - off possibly becoming negative. That's af_alg_make_sg(), and it shouldn't see anything coming from pipe buffers (right now the only way for that to happen is iter_file_splice_write()), but the things like e.g. dio_refill_pages() might, and they also get seriously confused by that. Worse, some of those callers have calling conventions that have similar problems of their own.

At the moment there are 11 callers (10 in mainline; one more added in conversion of vmsplice_to_pipe() to new pipe locking, but it's irrelevant anyway - it gets fed an iovec-backed iov_iter). I'm looking through those right now, hopefully will come up with something sane...

So honestly, if it worries you, I'm not going to complain at all if you decide that you'd rather translate the pipe_buffer[] array into a kvec by always splitting at page boundaries.

Even with large packets in networking, it's not going t be a huge deal. And maybe we *should* make it a rule that a "kvec" is always composed of individual entries that fit entirely within a page.

In this code, being safe rather than clever would be a welcome and surprising change, I guess.

Linus

FWIW, I wonder how many of those users are ready to cope with compound pages in the first place; they end up passed to
* skb_fill_page_desc(). Probably OK (as in all of them, modulo
calculating the number of pages and ranges for them).
* shoved into scatterlist, which gets passed to virtqueue_add_sgs().
Need to check virtio to see what happens there.
* shoved into nfs ->wb_page and fed into nfs_pageio_add_request() and
machinery behind it. These, BTW, are reachable by pipe_buffer-derived ones at the moment (splice to O_DIRECT nfs file). The code looks like it's playing fast and loose with ->wb_page - in some cases it's an NFS pagecache one, in some - anything from userland, and there are places like

	inode = page_file_mapping(req->wb_page)->host;

which will do nasty things if they are ever reached by the second kind. nfs_pgio_rpcsetup() looks like it won't be happy with compound pages, but again, I'm not familiar enough with that code to tell if it's reachable from nfs_pageio_add_request().
* shoved into scatterlist, which gets fed into crypto/*.c machinery.
No way for a pipe_buffer stuff to get there, fortunately, because I would be very surprised if it works correctly with compound pages and large ranges in those.
* shoved into lustre ->ldp_pages; almost certainly not ready for
compound pages.
* fed to ceph_osd_data_pages_init(); again, practically certain not
to be ready.
* put into dio_submit ->pages[], eventually fed to bio_add_page();
that might be fixable, but it would take some massage in fs/direct-io.c
*�fuse - probably OK, but that's only on a fairly cursory look.

It certainly won't be easy to verify in details ;-/

FWIW the crypto API has always been supposed to handle SG entries that cross page boundaries. There were a couple of bugs in this area but AFAIK they've all been fixed.

Of course I cannot guarantee that every crypto driver also handles it correctly, but at least we have a few test vectors which test the page-crossing case specifically.

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

The series is supposed to solve the locking order problems for
->splice_read() and get rid of code duplication between the read-side methods.

	pipe_lock is lifted out of ->splice_read() instances, along with

waiting for empty space in pipe, etc. - we do that stuff in callers.
A new variant of iov_iter is introduced - it's backed by a pipe,
copy_to_iter() results in allocating pages and copying into those, copy_page_to_iter() just sticks a reference to that page into pipe. Running out of space in pipe yields a short read, as a fault in iovec-backed iov_iter would have. Enough primitives are implemented for normal ->read_iter() instances to work.
generic_file_splice_read() switched to feeding such iov_iter to
->read_iter() instance. That turns out to be enough to kill almost all ->splice_read() instances; the only ones _not_ using generic_file_splice_read() or default_file_splice_read() (== no zero-copy fallback) are fuse_dev_splice_read(), 3 instances in kernel/{relay.c,trace/trace.c} and sock_splice_read(). It's almost certainly possible to convert fuse one and the same might be possible to do to socket one. relay and tracing stuff is just plain weird; might or might not be doable.

Something hopefully working is in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.splice_read

Several commits in that pipe (#1, #8 and #9) are trivial cleanups and fixes for crap caught while doing the rest, probably ought to be separated.

Shortlog:
Al Viro (11):
fix memory leaks in tracing_buffers_splice_read() splice_to_pipe(): don't open-code wakeup_pipe_readers()
splice: switch get_iovec_page_array() to iov_iter
splice: lift pipe_lock out of splice_to_pipe() skb_splice_bits(): get rid of callback
new helper: add_to_pipe()
fuse_dev_splice_read(): switch to add_to_pipe()
cifs: don't use memcpy() to copy struct iov_iter fuse_ioctl_copy_user(): don't open-code copy_page_{to,from}_iter() new iov_iter flavour: pipe-backed
switch generic_file_splice_read() to use of ->read_iter()

Diffstat:

 drivers/staging/lustre/lustre/llite/file.c         |  70 +--
 .../staging/lustre/lustre/llite/llite_internal.h   |  15 +-
 drivers/staging/lustre/lustre/llite/vvp_internal.h |  14 -
 drivers/staging/lustre/lustre/llite/vvp_io.c       |  45 +-
 fs/cifs/file.c                                     |  14 +-
 fs/coda/file.c                                     |  23 +-
 fs/fuse/dev.c                                      |  48 +-
 fs/fuse/file.c                                     |  30 +-
 fs/gfs2/file.c                                     |  28 +-
 fs/nfs/file.c                                      |  25 +-
 fs/nfs/internal.h                                  |   2 -
 fs/nfs/nfs4file.c                                  |   2 +-
 fs/ocfs2/file.c                                    |  34 +-
 fs/ocfs2/ocfs2_trace.h                             |   2 -
 fs/splice.c                                        | 578 +++++++--------------
 fs/xfs/xfs_file.c                                  |  41 +-
 fs/xfs/xfs_trace.h                                 |   1 -
 include/linux/fs.h                                 |   2 -
 include/linux/skbuff.h                             |   8 +-
 include/linux/splice.h                             |   3 +
 include/linux/uio.h                                |  14 +-
 kernel/trace/trace.c                               |  14 +-
 lib/iov_iter.c                                     | 390 +++++++++++++-
 mm/shmem.c                                         | 115 +---
 net/core/skbuff.c                                  |  28 +-
 net/ipv4/tcp.c                                     |   3 +-
 net/kcm/kcmsock.c                                  |  16 +-
 net/unix/af_unix.c                                 |  17 +-
 28 files changed, 648 insertions(+), 934 deletions(-)

It's not all I would like to do there (in particular, I hadn't
done fuse splice_read conversion to read_iter, even though it does appear to be doable; that'll take copy_page_to_iter_nosteal() as a new primitive
+ considerable amount of massage in fs/fuse/dev.c), but it should at least
* make pipe lock the outermost
* switch generic_file_splice_read() to ->read_iter(), making
it suitable for lustre/coda/gfs2/ocfs2/xfs/shmem without any wrappers
* somewhat simplify socket ->splice_read() guts (not by much - to
start doing that right we'd need the same new primitive)
* remove a considerable pile of code.
* get rid of a bunch of splice_{grow,shrink}_spd/splice_to_pipe
callers; remaining ones are in default_file_splice_read() (trivially killable by conversion to iov_iter_get_pages_alloc(), followed by the same build iovec array + use kernel_readv as we do now + iov_iter_advance to the length returned by kernel_readv), kernel/relay and kernel/trace/trace.c ones (should switch to add_to_pipe(), AFAICS) and skb_splice_bits() (again, a matter of copy_page_to_iter_nosteal(), which will take out spd_can_coalesce/spd_fill_page in there as well). Once the remaining ones are taken care of, splice_pipe_desc and friends will go away.

	In its current form it survives LTP, xfstests and overlayfs testsuite;

if anybody has additional tests for splice and friends, I would like to hear about such. It really needs more beating, though.

Please, help with review and testing.

---
 kernel/trace/trace.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

index dade4c9..9016f98 100644
@@ -6163,9 +6163,6 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 		return -EBUSY;
 #endif
 
-	if (splice_grow_spd(pipe, &spd))
-		return -ENOMEM;
-
 	if (*ppos & (PAGE_SIZE - 1))
 		return -EINVAL;
 

@@ -6175,6 +6172,9 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 		len &= PAGE_MASK;
 	}
 
+	if (splice_grow_spd(pipe, &spd))
+		return -ENOMEM;
+
  again:
 	trace_access_lock(iter->cpu_file);
 	entries = ring_buffer_entries_cpu(iter->trace_buffer->buffer, iter->cpu_file);

@@ -6232,19 +6232,21 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 	/* did we read anything? */
 	if (!spd.nr_pages) {
 		if (ret)
-			return ret;
+			goto out;
 
+		ret = -EAGAIN;
 		if ((file->f_flags & O_NONBLOCK) || (flags & SPLICE_F_NONBLOCK))
-			return -EAGAIN;
+			goto out;
 
 		ret = wait_on_pipe(iter, true);
 		if (ret)
-			return ret;
+			goto out;
 
 		goto again;
 	}
 
 	ret = splice_to_pipe(pipe, &spd);
+out:
 	splice_shrink_spd(&spd);
 
 	return ret;

2.9.3

---
 fs/splice.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index dd9bf7e..36e9353 100644
@@ -242,10 +242,7 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
 		}
 
 		if (do_wakeup) {
-			smp_mb();
-			if (waitqueue_active(&pipe->wait))
-				wake_up_interruptible_sync(&pipe->wait);
-			kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
+			wakeup_pipe_readers(pipe);
 			do_wakeup = 0;
 		}
 

2.9.3

---
 fs/splice.c | 135 ++++++++++++++++--------------------------------------------
 1 file changed, 36 insertions(+), 99 deletions(-)

index 36e9353..31c52e0 100644
@@ -1434,106 +1434,32 @@ static long do_splice(struct file *in, loff_t __user *off_in,
 	return -EINVAL;
 }
 
-/*
- * Map an iov into an array of pages and offset/length tupples. With the
- * partial_page structure, we can map several non-contiguous ranges into
- * our ones pages[] map instead of splitting that operation into pieces.
- * Could easily be exported as a generic helper for other users, in which
- * case one would probably want to add a 'max_nr_pages' parameter as well.
- */
-static int get_iovec_page_array(const struct iovec __user *iov,
-				unsigned int nr_vecs, struct page **pages,
-				struct partial_page *partial, bool aligned,
+static int get_iovec_page_array(struct iov_iter *from,
+				struct page **pages,
+				struct partial_page *partial,
 				unsigned int pipe_buffers)
 {
-	int buffers = 0, error = 0;
-
-	while (nr_vecs) {
-		unsigned long off, npages;
-		struct iovec entry;
-		void __user *base;
-		size_t len;
-		int i;
-
-		error = -EFAULT;
-		if (copy_from_user(&entry, iov, sizeof(entry)))
-			break;
-
-		base = entry.iov_base;
-		len = entry.iov_len;
-
-		/*
-		 * Sanity check this iovec. 0 read succeeds.
-		 */
-		error = 0;
-		if (unlikely(!len))
-			break;
-		error = -EFAULT;
-		if (!access_ok(VERIFY_READ, base, len))
-			break;
-
-		/*
-		 * Get this base offset and number of pages, then map
-		 * in the user pages.
-		 */
-		off = (unsigned long) base & ~PAGE_MASK;
-
-		/*
-		 * If asked for alignment, the offset must be zero and the
-		 * length a multiple of the PAGE_SIZE.
-		 */
-		error = -EINVAL;
-		if (aligned && (off || len & ~PAGE_MASK))
-			break;
-
-		npages = (off + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
-		if (npages > pipe_buffers - buffers)
-			npages = pipe_buffers - buffers;
-
-		error = get_user_pages_fast((unsigned long)base, npages,
-					0, &pages[buffers]);
-
-		if (unlikely(error <= 0))
-			break;
-
-		/*
-		 * Fill this contiguous range into the partial page map.
-		 */
-		for (i = 0; i < error; i++) {
-			const int plen = min_t(size_t, len, PAGE_SIZE - off);
-
-			partial[buffers].offset = off;
-			partial[buffers].len = plen;
-
-			off = 0;
-			len -= plen;
+	int buffers = 0;
+	while (iov_iter_count(from)) {
+		ssize_t copied;
+		size_t start;
+
+		copied = iov_iter_get_pages(from, pages + buffers, ~0UL,
+					pipe_buffers - buffers, &start);
+		if (copied <= 0)
+			return buffers ? buffers : copied;
+
+		iov_iter_advance(from, copied);
+		while (copied) {
+			int size = min_t(int, copied, PAGE_SIZE - start);
+			partial[buffers].offset = start;
+			partial[buffers].len = size;
+			copied -= size;
+			start = 0;
 			buffers++;
 		}
-
-		/*
-		 * We didn't complete this iov, stop here since it probably
-		 * means we have to move some of this into a pipe to
-		 * be able to continue.
-		 */
-		if (len)
-			break;
-
-		/*
-		 * Don't continue if we mapped fewer pages than we asked for,
-		 * or if we mapped the max number of pages that we have
-		 * room for.
-		 */
-		if (error < npages || buffers == pipe_buffers)
-			break;
-
-		nr_vecs--;
-		iov++;
 	}
-
-	if (buffers)
-		return buffers;
-
-	return error;
+	return buffers;
 }
 
 static int pipe_to_user(struct pipe_inode_info *pipe, struct pipe_buffer *buf,

@@ -1587,10 +1513,13 @@ static long vmsplice_to_user(struct file *file, const struct iovec __user *uiov,
  * as splice-from-memory, where the regular splice is splice-from-file (or
  * to file). In both cases the output is a pipe, naturally.
  */
-static long vmsplice_to_pipe(struct file *file, const struct iovec __user *iov,
+static long vmsplice_to_pipe(struct file *file, const struct iovec __user *uiov,
 			     unsigned long nr_segs, unsigned int flags)
 {
 	struct pipe_inode_info *pipe;
+	struct iovec iovstack[UIO_FASTIOV];
+	struct iovec *iov = iovstack;
+	struct iov_iter from;
 	struct page *pages[PIPE_DEF_BUFFERS];
 	struct partial_page partial[PIPE_DEF_BUFFERS];
 	struct splice_pipe_desc spd = {

@@ -1607,11 +1536,18 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *iov,
 	if (!pipe)
 		return -EBADF;
 
-	if (splice_grow_spd(pipe, &spd))
+	ret = import_iovec(WRITE, uiov, nr_segs,
+			   ARRAY_SIZE(iovstack), &iov, &from);
+	if (ret < 0)
+		return ret;
+
+	if (splice_grow_spd(pipe, &spd)) {
+		kfree(iov);
 		return -ENOMEM;
+	}
 
-	spd.nr_pages = get_iovec_page_array(iov, nr_segs, spd.pages,
-					    spd.partial, false,
+	spd.nr_pages = get_iovec_page_array(&from, spd.pages,
+					    spd.partial,
 					    spd.nr_pages_max);
 	if (spd.nr_pages <= 0)
 		ret = spd.nr_pages;

@@ -1619,6 +1555,7 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *iov,
 		ret = splice_to_pipe(pipe, &spd);
 
 	splice_shrink_spd(&spd);
+	kfree(iov);
 	return ret;
 }
 

2.9.3

* splice_to_pipe() stops at pipe overflow and does *not* take pipe_lock
* ->splice_read() instances do the same
* vmsplice_to_pipe() and do_splice() (ultimate callers of splice_to_pipe()) arrange for waiting, looping, etc. themselves.

That should make pipe_lock the outermost one.

Unfortunately, existing rules for the amount passed by vmsplice_to_pipe() and do_splice() are quite ugly _and_ userland code can be easily broken by changing those. It's not even "no more than the maximal capacity of this pipe" - it's "once we'd fed pipe->nr_buffers pages into the pipe, leave instead of waiting". I would like to change it to something saner, but that's for later.

---
 fs/fuse/dev.c |   2 -
 fs/splice.c   | 171 ++++++++++++++++++++++++++++++++--------------------------
 2 files changed, 96 insertions(+), 77 deletions(-)

index a94d2ed..eaf56c6 100644
@@ -1364,7 +1364,6 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 		goto out;
 
 	ret = 0;
-	pipe_lock(pipe);
 
 	if (!pipe->readers) {
 		send_sig(SIGPIPE, current, 0);

@@ -1400,7 +1399,6 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	}
 
 out_unlock:
-	pipe_unlock(pipe);
 
 	if (do_wakeup) {
 		smp_mb();

2.9.3

The thing is, I'm not sure what to do with it; it was brought by the LTP vmsplice test, which asks to feed 128Kb into a pipe. With the caller itself on the other end of that pipe, SPLICE_F_NONBLOCK *not* given and the pipe capacity being 64Kb. Unfortunately, "quietly truncate the length down to 64Kb" does *not* suffice - the damn thing starts not at the page boundary, so we only copy about 62Kb until hitting the pipe overflow (the pipe is initially empty). The reason why it doesn't go to sleep indefinitely on the mainline kernel is that mainline collects up to page->buffers *pages*, before feeding them into the pipe. And these ~62Kb are just that. Note that had there been anything already in the pipe, the same call would've gone to sleep (and in the end transferred the same ~62Kb worth of data).

All of that is completely undocumented in vmsplice(2) (or anywhere else that I'd been able to find) ;-/

OTOH, considering the quality of documentation, I'm somewhat tempted to go for "sleep only if it had been completely full when we entered; once there's some space feed as much as fits and be done with that". OTTH, I'm not sure that no userland cr^Hode will manage to be hurt by that variant...

Let's just try it.

If that then doesn't work, we can introduce your odd code (with a *big* comment). Ok?

Linus

FWIW, updated (with fixes) and force-pushed. Added piece:
default_file_splice_read() converted to iov_iter. Seems to work, after fixing a braino in __pipe_get_pages(). Changed: #4 (sleep only in the beginning, as described above), #6 (context changes from #4), #10 (missing get_page() added in __pipe_get_pages()), #11 (removed pointless truncation of len - ->read_iter() can bloody well handle that on its own) and added #12. Stands at 28 files changed, 657 insertions(+), 1009 deletions(-) now...

I think I see how to get full zero-copy (including the write side
of things). Just add a "from" side for ITER_PIPE iov_iter (advance, get_pages, get_pages_alloc, npages and alignment will need to behave differently for "to" and "from" ones) and pull the following trick:
have fault_in_readable return NULL instead of 0, ERR_PTR(-EFAULT) instead of -EFAULT *and* return a struct page if it was asked for a full-page range on a page that could be successfully stolen (only "from pipe" iov_iter would go for the last one, of course). Then we make generic_perform_write() shove the return value of fault-in into 'page'. ->write_begin() is given &page as an argument, to return the resulting page via that. All instances currently just store into that pointer, completely ignoring the prior value. And they'll keep working just fine.

Let's make sure that all method call sites outside of
generic_perform_write() (there's only one such, actually) have NULL stored in there prior to the call. Now we can start switching the instances to zero-copy support - all it takes is replacing grab_cache_page_write_begin() with "if *page is non-NULL, try to shove it (locked, non-uptodate) into pagecache; if that succeeds grab a reference to our page and we are done, if it fails - fall back to grab_cache_page_write_begin()". Then do get_block, etc., or whatever that ->write_begin() instance would normally do, just remember not to zero anything if the page had been passed to us by caller.

Now all we need is to make sure that iov_iter_copy_from_user_atomic()
for those guys recongnizes the case of full-page copy when source and target are the same page and quietly returns PAGE_SIZE. Voila - we can make iter_file_splice_write() pass pipe-backed iov_iter instead of bvec-backed one *and* get write-side zero-copy for all filesystems with ->write_begin() taught to handle that (see above). Since the filesystems with unmodified ->write_begin() will act correctly (just do the copying), we don't have to make that a flagday change; ->write_begin() instances can be switched one by one. Similar treatment of iomap_write_begin()/iomap_write_actor() would cover iomap-using ->write_iter() instances.

It's clearly not something I want to touch until -rc1, but it looks
feasible for the next cycle, and if done right it promises to unify the plain and splice sides of fuse_dev_...() stuff, simplifying the hell out of them without losing zero-copy there. And if everything really goes right, we might be able to get rid of net/* ->splice_read() and ->sendpage() methods as well...

Interesting stuff. It should also be possible for a filesystem to replace existing pagecache as a zero-copy overwrite with the migration APIs and just a little bit of work.

Kernel NFS server already uses splice for its read path, but the write path appears to require a full data copy of incoming payloads. Would be awesome to see write-side support for zero-copy.

--
Chuck Lever

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

* splice_to_pipe() stops at pipe overflow and does *not* take pipe_lock
* ->splice_read() instances do the same
* vmsplice_to_pipe() and do_splice() (ultimate callers of splice_to_pipe()) arrange for waiting, looping, etc. themselves.

That should make pipe_lock the outermost one.

Unfortunately, existing rules for the amount passed by vmsplice_to_pipe() and do_splice() are quite ugly _and_ userland code can be easily broken by changing those. It's not even "no more than the maximal capacity of this pipe" - it's "once we'd fed pipe->nr_buffers pages into the pipe, leave instead of waiting".

Considering how poorly these rules are documented, let's try "wait for some space to appear, unless given SPLICE_F_NONBLOCK, then push into pipe and if we run into overflow, we are done".

---
 fs/fuse/dev.c |   2 -
 fs/splice.c   | 138 +++++++++++++++++++++++++++-------------------------------
 2 files changed, 63 insertions(+), 77 deletions(-)

index a94d2ed..eaf56c6 100644
@@ -1364,7 +1364,6 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 		goto out;
 
 	ret = 0;
-	pipe_lock(pipe);
 
 	if (!pipe->readers) {
 		send_sig(SIGPIPE, current, 0);

@@ -1400,7 +1399,6 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	}
 
 out_unlock:
-	pipe_unlock(pipe);
 
 	if (do_wakeup) {
 		smp_mb();

2.9.3

[...]

		    ^^^^^^^^^^^^^^^^

Reordering braindamage; fixed.

Rudiment of earlier variant, when we did a non-trivial loop in the caller. Not needed anymore, removed.

Fixed variant force-pushed to the same branch

This break EPIPE handling inside splice when SIGPIPE is ignored:

Before:

$ { sleep 1; strace -e splice pv -q /dev/zero; } | :
splice(3, NULL, 1, NULL, 131072, SPLICE_F_MORE) = -1 EPIPE (Broken pipe)
--- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=23750, si_uid=17005} ---
--- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=23750, si_uid=17005} ---
+++ exited with 0 +++

After:

$ { sleep 1; strace -e splice pv -q /dev/zero; } | :
splice(3, NULL, 1, NULL, 131072, SPLICE_F_MORE) = 65536
splice(3, NULL, 1, NULL, 131072, SPLICE_F_MORE
[hangs]

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Where is that "splice" program from? Google isn't helpful, and fedora doesn't seem to have it. I'm assuming it was posted in one of the threads, but if so I've long since lost sight of it..

Linus

It's running pv (splice is the argument of strace -e).

http://ivarch.com/programs/pv.shtml

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

It's pv(1), actually. I'm looking into that - debian-packaged pv reproduced that crap.

OK, I see what's going on - it's wait_for_space() lifted past the checks for lack of readers. The fix, AFAICS, is simply

index 6a2b0db5..aeba2b7 100644
@@ -1082,6 +1082,10 @@ EXPORT_SYMBOL(do_splice_direct);
 
 static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
 {
+	if (unlikely(!pipe->readers)) {
+		send_sig(SIGPIPE, current, 0);
+		return -EPIPE;
+	}
 	while (pipe->nrbufs == pipe->buffers) {
 		if (flags & SPLICE_F_NONBLOCK)
 			return -EAGAIN;

@@ -1090,6 +1094,10 @@ static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
 		pipe->waiting_writers++;
 		pipe_wait(pipe);
 		pipe->waiting_writers--;
+		if (unlikely(!pipe->readers)) {
+			send_sig(SIGPIPE, current, 0);
+			return -EPIPE;
+		}
 	}
 	return 0;
 }

Ugh. Does it have to be duplicated?

How about just making the wait_for_space() loop be a for-loop, and writing it as

   for (;;) {
        if (unlikely(!pipe->readers)) {
                send_sig(SIGPIPE, current, 0);
                return -EPIPE;
        }
        if (pipe->nrbufs == pipe->buffers)
                return 0;
        if (flags & SPLICE_F_NONBLOCK)
                return -EAGAIN;
        if (signal_pending(current))
                return -ERESTARTSYS;
        pipe->waiting_writers++;
        pipe_wait(pipe);
        pipe->waiting_writers--;
   }

and just having it once?

Regardless - Andreas, can you verify that that fixes your issues? I'm assuming you had some real load that made you notice this, not just he dummy example..

Linus

ITYM "!="...

Right. A bit too much cut-and-pasting going on in my email ;)

Linus

This is from the testsuite of pv, I only noticed because it was hanging.

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

FWIW, I have verified that the testsuite of pv succeeds with this patch:

index 5a7750bd2e..63b8f54485 100644
@@ -1086,7 +1086,13 @@ EXPORT_SYMBOL(do_splice_direct);
 
 static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
 {
-	while (pipe->nrbufs == pipe->buffers) {
+	for (;;) {
+		if (unlikely(!pipe->readers)) {
+			send_sig(SIGPIPE, current, 0);
+			return -EPIPE;
+		}
+		if (pipe->nrbufs != pipe->buffers)
+			return 0;
 		if (flags & SPLICE_F_NONBLOCK)
 			return -EAGAIN;
 		if (signal_pending(current))

@@ -1095,7 +1101,6 @@ static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
 		pipe_wait(pipe);
 		pipe->waiting_writers--;
 	}
-	return 0;
 }
 
 static int splice_pipe_to_pipe(struct pipe_inode_info *ipipe,

2.11.0


Andreas.

-- 
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Ok, thanks, committed.

Al, looking at this area, I think there's some room for cleanups. In particular, isn't the loop in opipe_prep() now just "wait_for_space()"? I'm also thinking that we could perhaps remove the SIGPIPE/EPIPE handling from splice_to_pipe()..

Hmm?

Linus

single-buffer analogue of splice_to_pipe(); vmsplice_to_pipe() switched to that, leaving splice_to_pipe() only for ->splice_read() instances (and that only until they are converted as well).

---
 fs/splice.c            | 113 ++++++++++++++++++++++++++++---------------------
 include/linux/splice.h |   2 +
 2 files changed, 67 insertions(+), 48 deletions(-)

index 02daa61..e13d935 100644
@@ -203,8 +203,6 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
 		buf->len = spd->partial[page_nr].len;
 		buf->private = spd->partial[page_nr].private;
 		buf->ops = spd->ops;
-		if (spd->flags & SPLICE_F_GIFT)
-			buf->flags |= PIPE_BUF_FLAG_GIFT;
 
 		pipe->nrbufs++;
 		page_nr++;

@@ -225,6 +223,27 @@ out:
 }
 EXPORT_SYMBOL_GPL(splice_to_pipe);
 
+ssize_t add_to_pipe(struct pipe_inode_info *pipe, struct pipe_buffer *buf)
+{
+	int ret;
+
+	if (unlikely(!pipe->readers)) {
+		send_sig(SIGPIPE, current, 0);
+		ret = -EPIPE;
+	} else if (pipe->nrbufs == pipe->buffers) {
+		ret = -EAGAIN;
+	} else {
+		int newbuf = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers - 1);
+		pipe->bufs[newbuf] = *buf;
+		pipe->nrbufs++;
+		return buf->len;
+	}
+	buf->ops->release(pipe, buf);
+	buf->ops = NULL;
+	return ret;
+}
+EXPORT_SYMBOL(add_to_pipe);
+
 void spd_release_page(struct splice_pipe_desc *spd, unsigned int i)
 {
 	put_page(spd->pages[i]);

@@ -1415,33 +1434,50 @@ static long do_splice(struct file *in, loff_t __user *off_in,
 	return -EINVAL;
 }
 
-static int get_iovec_page_array(const struct iov_iter *from,
-				struct page **pages,
-				struct partial_page *partial,
-				unsigned int pipe_buffers)
+static int iter_to_pipe(struct iov_iter *from,
+			struct pipe_inode_info *pipe,
+			unsigned flags)
 {
-	struct iov_iter i = *from;
-	int buffers = 0;
-	while (iov_iter_count(&i)) {
+	struct pipe_buffer buf = {
+		.ops = &user_page_pipe_buf_ops,
+		.flags = flags
+	};
+	size_t total = 0;
+	int ret = 0;
+	bool failed = false;
+
+	while (iov_iter_count(from) && !failed) {
+		struct page *pages[16];
 		ssize_t copied;
 		size_t start;
+		int n;
 
-		copied = iov_iter_get_pages(&i, pages + buffers, ~0UL,
-					pipe_buffers - buffers, &start);
-		if (copied <= 0)
-			return buffers ? buffers : copied;
+		copied = iov_iter_get_pages(from, pages, ~0UL, 16, &start);
+		if (copied <= 0) {
+			ret = copied;
+			break;
+		}
 
-		iov_iter_advance(&i, copied);
-		while (copied) {
+		for (n = 0; copied; n++, start = 0) {
 			int size = min_t(int, copied, PAGE_SIZE - start);
-			partial[buffers].offset = start;
-			partial[buffers].len = size;
+			if (!failed) {
+				buf.page = pages[n];
+				buf.offset = start;
+				buf.len = size;
+				ret = add_to_pipe(pipe, &buf);
+				if (unlikely(ret < 0)) {
+					failed = true;
+				} else {
+					iov_iter_advance(from, ret);
+					total += ret;
+				}
+			} else {
+				put_page(pages[n]);
+			}
 			copied -= size;
-			start = 0;
-			buffers++;
 		}
 	}
-	return buffers;
+	return total ? total : ret;
 }
 
 static int pipe_to_user(struct pipe_inode_info *pipe, struct pipe_buffer *buf,

@@ -1502,17 +1538,11 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *uiov,
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
 	struct iov_iter from;
-	struct page *pages[PIPE_DEF_BUFFERS];
-	struct partial_page partial[PIPE_DEF_BUFFERS];
-	struct splice_pipe_desc spd = {
-		.pages = pages,
-		.partial = partial,
-		.nr_pages_max = PIPE_DEF_BUFFERS,
-		.flags = flags,
-		.ops = &user_page_pipe_buf_ops,
-		.spd_release = spd_release_page,
-	};
 	long ret;
+	unsigned buf_flag = 0;
+
+	if (flags & SPLICE_F_GIFT)
+		buf_flag = PIPE_BUF_FLAG_GIFT;
 
 	pipe = get_pipe_info(file);
 	if (!pipe)

@@ -1523,26 +1553,13 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *uiov,
 	if (ret < 0)
 		return ret;
 
-	if (splice_grow_spd(pipe, &spd)) {
-		kfree(iov);
-		return -ENOMEM;
-	}
-
 	pipe_lock(pipe);
 	ret = wait_for_space(pipe, flags);
-	if (!ret) {
-		spd.nr_pages = get_iovec_page_array(&from, spd.pages,
-						    spd.partial,
-						    spd.nr_pages_max);
-		if (spd.nr_pages <= 0)
-			ret = spd.nr_pages;
-		else
-			ret = splice_to_pipe(pipe, &spd);
-		pipe_unlock(pipe);
-		if (ret > 0)
-			wakeup_pipe_readers(pipe);
-	}
-	splice_shrink_spd(&spd);
+	if (!ret)
+		ret = iter_to_pipe(&from, pipe, buf_flag);
+	pipe_unlock(pipe);
+	if (ret > 0)
+		wakeup_pipe_readers(pipe);
 	kfree(iov);
 	return ret;
 }

index da2751d..58b300f 100644
@@ -72,6 +72,8 @@ extern ssize_t __splice_from_pipe(struct pipe_inode_info *,
 				  struct splice_desc *, splice_actor *);
 extern ssize_t splice_to_pipe(struct pipe_inode_info *,
 			      struct splice_pipe_desc *);
+extern ssize_t add_to_pipe(struct pipe_inode_info *,
+			      struct pipe_buffer *);
 extern ssize_t splice_direct_to_actor(struct file *, struct splice_desc *,
 				      splice_direct_actor *);
 

2.9.3

[...]

iov_iter variant for passing data into pipe. copy_to_iter() copies data into page(s) it has allocated and stuffs them into the pipe; copy_page_to_iter() stuffs there a reference to the page given to it. Both will try to coalesce if possible. iov_iter_zero() is similar to copy_to_iter(); iov_iter_get_pages() and friends will do as copy_to_iter() would have and return the pages where the data would've been copied. iov_iter_advance() will truncate everything past the spot it has advanced to.

New primitive: iov_iter_pipe(), used for initializing those. pipe should be locked all along.

Running out of space acts as fault would for iovec-backed ones; in other words, giving it to ->read_iter() may result in short read if the pipe overflows, or -EFAULT if it happens with nothing copied there.

In other words, ->read_iter() on those acts pretty much like ->splice_read(). Moreover, all generic_file_splice_read() users, as well as many other ->splice_read() instances can be switched to that scheme - that'll happen in the next commit.

---
 fs/splice.c            |   2 +-
 include/linux/splice.h |   1 +
 include/linux/uio.h    |  14 +-
 lib/iov_iter.c         | 390 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 401 insertions(+), 6 deletions(-)

index e13d935..589a1d5 100644
@@ -524,7 +524,7 @@ ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
 }
 EXPORT_SYMBOL(generic_file_splice_read);
 
-static const struct pipe_buf_operations default_pipe_buf_ops = {
+const struct pipe_buf_operations default_pipe_buf_ops = {
 	.can_merge = 0,
 	.confirm = generic_pipe_buf_confirm,
 	.release = generic_pipe_buf_release,

index 58b300f..00a2116 100644
@@ -85,4 +85,5 @@ extern void splice_shrink_spd(struct splice_pipe_desc *);
 extern void spd_release_page(struct splice_pipe_desc *, unsigned int);
 
 extern const struct pipe_buf_operations page_cache_pipe_buf_ops;
+extern const struct pipe_buf_operations default_pipe_buf_ops;
 #endif

index 1b5d1cd..c4fe1ab 100644
@@ -13,6 +13,7 @@
 #include <uapi/linux/uio.h>
 
 struct page;
+struct pipe_inode_info;
 
 struct kvec {
 	void *iov_base; /* and that should *never* hold a userland pointer */

@@ -23,6 +24,7 @@ enum {
 	ITER_IOVEC = 0,
 	ITER_KVEC = 2,
 	ITER_BVEC = 4,
+	ITER_PIPE = 8,
 };
 
 struct iov_iter {

@@ -33,8 +35,12 @@ struct iov_iter {
 		const struct iovec *iov;
 		const struct kvec *kvec;
 		const struct bio_vec *bvec;
+		struct pipe_inode_info *pipe;
+	};
+	union {
+		unsigned long nr_segs;
+		int idx;
 	};
-	unsigned long nr_segs;
 };
 
 /*

@@ -64,7 +70,7 @@ static inline struct iovec iov_iter_iovec(const struct iov_iter *iter)
 }
 
 #define iov_for_each(iov, iter, start)				\
-	if (!((start).type & ITER_BVEC))			\
+	if (!((start).type & (ITER_BVEC | ITER_PIPE)))		\
 	for (iter = (start);					\
 	     (iter).count &&					\
 	     ((iov = iov_iter_iovec(&(iter))), 1);		\

@@ -94,6 +100,8 @@ void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,
 			unsigned long nr_segs, size_t count);
 void iov_iter_bvec(struct iov_iter *i, int direction, const struct bio_vec *bvec,
 			unsigned long nr_segs, size_t count);
+void iov_iter_pipe(struct iov_iter *i, int direction, struct pipe_inode_info *pipe,
+			size_t count);
 ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
 			size_t maxsize, unsigned maxpages, size_t *start);
 ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages,

@@ -109,7 +117,7 @@ static inline size_t iov_iter_count(struct iov_iter *i)
 
 static inline bool iter_is_iovec(struct iov_iter *i)
 {
-	return !(i->type & (ITER_BVEC | ITER_KVEC));
+	return !(i->type & (ITER_BVEC | ITER_KVEC | ITER_PIPE));
 }
 
 /*

2.9.3

This is the hardest part of the whole set. I've been trying to understand it, but the modular arithmetic makes it really tricky to read. Couldn't we have more small inline helpers like next_idx()?

Specific comments inline.

[...]

Nope. copy_page_to_iter() *never* returns -EFAULT. Including the iovec one - check copy_page_to_iter_iovec(). Any caller that does not expect a zero return value from that primitive is a bug, triggerable as soon as you feed it an iovec with NULL ->iov_base.

For copy_to_iter() and zero_iter() it's definitely "return zero". For get_pages... Hell knows; those probably ought to return -EFAULT, but I'll need to look some more at the callers. It should end up triggering a short read as the end result (or, as usual, EFAULT on zero-length read).

True.

It should look at i->count, in principle. OTOH, overestimating the amount is not really a problem for possible users of such iov_iter. I'll look into that.

Right.

I was actually looking at iov_iter_get_pages() callers...

Thanks,
Miklos

FWIW, that's interesting - O_DIRECT readv()/writev() reacts to fault anywhere as "nothing done, return -EFAULT now", rather than a short read/write. Despite that some IO is actually done. Note, BTW, that we are not even consistent between the filesystems - local block ones do IO and give -EFAULT, while NFS, Lustre and FUSE do short read/write, reporting -EFAULT only upon shortening to nothing. So does ceph, except that shortening might be for more than one page.

Considering how weak POSIX is in that area, we are probably not violating anything, but... it would be more convenient if we treated those as short read/write, same way for all filesystems.

Linus, do you have any objections against such behaviour change? AFAICS, all it takes is this:

index 7c3ce73..3a8ebda 100644
@@ -246,6 +246,8 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async)
 		if ((dio->op == REQ_OP_READ) &&
 		    ((offset + transferred) > dio->i_size))
 			transferred = dio->i_size - offset;
+		if (ret == -EFAULT)
+			ret = 0;
 	}
 
 	if (ret == 0)

There's this in do_blockdev_direct_IO():

        /* Once we sampled i_size check for reads beyond EOF */
        dio->i_size = i_size_read(inode);
        if (iov_iter_rw(iter) == READ && offset >= dio->i_size) {
                if (dio->flags & DIO_LOCKING)
                        mutex_unlock(&inode->i_mutex);
                kmem_cache_free(dio_cache, dio);
                retval = 0;
                goto out;
        }

so that shouldn't happen. Said that,

... it's certainly less brittle that way. I'd probably still put it under the same if (dio->result) and write it as

	if (unlikely(ret == -EFAULT) && transferred)

though.

... and kill the ->splice_read() instances that can be switched to it

---
 drivers/staging/lustre/lustre/llite/file.c         |  70 ++----
 .../staging/lustre/lustre/llite/llite_internal.h   |  15 +-
 drivers/staging/lustre/lustre/llite/vvp_internal.h |  14 --
 drivers/staging/lustre/lustre/llite/vvp_io.c       |  45 +---
 fs/coda/file.c                                     |  23 +-
 fs/gfs2/file.c                                     |  28 +--
 fs/nfs/file.c                                      |  25 +--
 fs/nfs/internal.h                                  |   2 -
 fs/nfs/nfs4file.c                                  |   2 +-
 fs/ocfs2/file.c                                    |  34 +--
 fs/ocfs2/ocfs2_trace.h                             |   2 -
 fs/splice.c                                        | 244 +++------------------
 fs/xfs/xfs_file.c                                  |  41 +---
 fs/xfs/xfs_trace.h                                 |   1 -
 include/linux/fs.h                                 |   2 -
 mm/shmem.c                                         | 115 +---------
 16 files changed, 58 insertions(+), 605 deletions(-)

index 57281b9..2567b09 100644
@@ -1153,36 +1153,21 @@ restart:
 		int write_mutex_locked = 0;
 
 		vio->vui_fd  = LUSTRE_FPRIVATE(file);
-		vio->vui_io_subtype = args->via_io_subtype;
-
-		switch (vio->vui_io_subtype) {
-		case IO_NORMAL:
-			vio->vui_iter = args->u.normal.via_iter;
-			vio->vui_iocb = args->u.normal.via_iocb;
-			if ((iot == CIT_WRITE) &&
-			    !(vio->vui_fd->fd_flags & LL_FILE_GROUP_LOCKED)) {
-				if (mutex_lock_interruptible(&lli->
-							       lli_write_mutex)) {
-					result = -ERESTARTSYS;
-					goto out;
-				}
-				write_mutex_locked = 1;
+		vio->vui_iter = args->u.normal.via_iter;
+		vio->vui_iocb = args->u.normal.via_iocb;
+		if ((iot == CIT_WRITE) &&
+		    !(vio->vui_fd->fd_flags & LL_FILE_GROUP_LOCKED)) {
+			if (mutex_lock_interruptible(&lli->lli_write_mutex)) {
+				result = -ERESTARTSYS;
+				goto out;
 			}
-			down_read(&lli->lli_trunc_sem);
-			break;
-		case IO_SPLICE:
-			vio->u.splice.vui_pipe = args->u.splice.via_pipe;
-			vio->u.splice.vui_flags = args->u.splice.via_flags;
-			break;
-		default:
-			CERROR("Unknown IO type - %u\n", vio->vui_io_subtype);
-			LBUG();
+			write_mutex_locked = 1;
 		}
+		down_read(&lli->lli_trunc_sem);
 		ll_cl_add(file, env, io);
 		result = cl_io_loop(env, io);
 		ll_cl_remove(file, env);
-		if (args->via_io_subtype == IO_NORMAL)
-			up_read(&lli->lli_trunc_sem);
+		up_read(&lli->lli_trunc_sem);
 		if (write_mutex_locked)
 			mutex_unlock(&lli->lli_write_mutex);
 	} else {

@@ -1237,7 +1222,7 @@ static ssize_t ll_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	if (IS_ERR(env))
 		return PTR_ERR(env);
 
-	args = ll_env_args(env, IO_NORMAL);
+	args = ll_env_args(env);
 	args->u.normal.via_iter = to;
 	args->u.normal.via_iocb = iocb;
 

@@ -1261,7 +1246,7 @@ static ssize_t ll_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (IS_ERR(env))
 		return PTR_ERR(env);
 
-	args = ll_env_args(env, IO_NORMAL);
+	args = ll_env_args(env);
 	args->u.normal.via_iter = from;
 	args->u.normal.via_iocb = iocb;
 

@@ -1271,31 +1256,6 @@ static ssize_t ll_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	return result;
 }
 
-/*
- * Send file content (through pagecache) somewhere with helper
- */
-static ssize_t ll_file_splice_read(struct file *in_file, loff_t *ppos,
-				   struct pipe_inode_info *pipe, size_t count,
-				   unsigned int flags)
-{
-	struct lu_env      *env;
-	struct vvp_io_args *args;
-	ssize_t	     result;
-	int		 refcheck;
-
-	env = cl_env_get(&refcheck);
-	if (IS_ERR(env))
-		return PTR_ERR(env);
-
-	args = ll_env_args(env, IO_SPLICE);
-	args->u.splice.via_pipe = pipe;
-	args->u.splice.via_flags = flags;
-
-	result = ll_file_io_generic(env, args, in_file, CIT_READ, ppos, count);
-	cl_env_put(env, &refcheck);
-	return result;
-}
-
 static int ll_lov_recreate(struct inode *inode, struct ost_id *oi, u32 ost_idx)
 {
 	struct obd_export *exp = ll_i2dtexp(inode);

@@ -3173,7 +3133,7 @@ struct file_operations ll_file_operations = {
 	.release	= ll_file_release,
 	.mmap	   = ll_file_mmap,
 	.llseek	 = ll_file_seek,
-	.splice_read    = ll_file_splice_read,
+	.splice_read    = generic_file_splice_read,
 	.fsync	  = ll_fsync,
 	.flush	  = ll_flush
 };

@@ -3186,7 +3146,7 @@ struct file_operations ll_file_operations_flock = {
 	.release	= ll_file_release,
 	.mmap	   = ll_file_mmap,
 	.llseek	 = ll_file_seek,
-	.splice_read    = ll_file_splice_read,
+	.splice_read    = generic_file_splice_read,
 	.fsync	  = ll_fsync,
 	.flush	  = ll_flush,
 	.flock	  = ll_file_flock,

@@ -3202,7 +3162,7 @@ struct file_operations ll_file_operations_noflock = {
 	.release	= ll_file_release,
 	.mmap	   = ll_file_mmap,
 	.llseek	 = ll_file_seek,
-	.splice_read    = ll_file_splice_read,
+	.splice_read    = generic_file_splice_read,
 	.fsync	  = ll_fsync,
 	.flush	  = ll_flush,
 	.flock	  = ll_file_noflock,

index 4d6d589..0e738c8 100644
@@ -800,17 +800,11 @@ void vvp_write_complete(struct vvp_object *club, struct vvp_page *page);
  */
 struct vvp_io_args {
 	/** normal/splice */
-	enum vvp_io_subtype via_io_subtype;
-
 	union {
 		struct {
 			struct kiocb      *via_iocb;
 			struct iov_iter   *via_iter;
 		} normal;
-		struct {
-			struct pipe_inode_info  *via_pipe;
-			unsigned int       via_flags;
-		} splice;
 	} u;
 };
 

@@ -838,14 +832,9 @@ static inline struct ll_thread_info *ll_env_info(const struct lu_env *env)
 	return lti;
 }
 
-static inline struct vvp_io_args *ll_env_args(const struct lu_env *env,
-					      enum vvp_io_subtype type)
+static inline struct vvp_io_args *ll_env_args(const struct lu_env *env)
 {
-	struct vvp_io_args *via = &ll_env_info(env)->lti_args;
-
-	via->via_io_subtype = type;
-
-	return via;
+	return &ll_env_info(env)->lti_args;
 }
 
 void ll_queue_done_writing(struct inode *inode, unsigned long flags);

index 79fc428..2fa49cc 100644
@@ -49,14 +49,6 @@ struct obd_device;
 struct obd_export;
 struct page;
 
-/* specific architecture can implement only part of this list */
-enum vvp_io_subtype {
-	/** normal IO */
-	IO_NORMAL,
-	/** io started from splice_{read|write} */
-	IO_SPLICE
-};
-
 /**
  * IO state private to IO state private to VVP layer.
  */

@@ -99,10 +91,6 @@ struct vvp_io {
 			bool		ft_flags_valid;
 		} fault;
 		struct {
-			struct pipe_inode_info	*vui_pipe;
-			unsigned int		 vui_flags;
-		} splice;
-		struct {
 			struct cl_page_list vui_queue;
 			unsigned long vui_written;
 			int vui_from;

@@ -110,8 +98,6 @@ struct vvp_io {
 		} write;
 	} u;
 
-	enum vvp_io_subtype	vui_io_subtype;
-
 	/**
 	 * Layout version when this IO is initialized
 	 */

index f47c748..8415d4f 100644
@@ -38,27 +38,6 @@ coda_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 }
 
 static ssize_t
-coda_file_splice_read(struct file *coda_file, loff_t *ppos,
-		      struct pipe_inode_info *pipe, size_t count,
-		      unsigned int flags)
-{
-	ssize_t (*splice_read)(struct file *, loff_t *,
-			       struct pipe_inode_info *, size_t, unsigned int);
-	struct coda_file_info *cfi;
-	struct file *host_file;
-
-	cfi = CODA_FTOC(coda_file);
-	BUG_ON(!cfi || cfi->cfi_magic != CODA_MAGIC);
-	host_file = cfi->cfi_container;
-
-	splice_read = host_file->f_op->splice_read;
-	if (!splice_read)
-		splice_read = default_file_splice_read;
-
-	return splice_read(host_file, ppos, pipe, count, flags);
-}
-
-static ssize_t
 coda_file_write_iter(struct kiocb *iocb, struct iov_iter *to)
 {
 	struct file *coda_file = iocb->ki_filp;

@@ -225,6 +204,6 @@ const struct file_operations coda_file_operations = {
 	.open		= coda_open,
 	.release	= coda_release,
 	.fsync		= coda_fsync,
-	.splice_read	= coda_file_splice_read,
+	.splice_read	= generic_file_splice_read,
 };
 

index 320e65e..7016a6a7 100644
@@ -954,30 +954,6 @@ out_uninit:
 	return ret;
 }
 
-static ssize_t gfs2_file_splice_read(struct file *in, loff_t *ppos,
-				     struct pipe_inode_info *pipe, size_t len,
-				     unsigned int flags)
-{
-	struct inode *inode = in->f_mapping->host;
-	struct gfs2_inode *ip = GFS2_I(inode);
-	struct gfs2_holder gh;
-	int ret;
-
-	inode_lock(inode);
-
-	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
-	if (ret) {
-		inode_unlock(inode);
-		return ret;
-	}
-
-	gfs2_glock_dq_uninit(&gh);
-	inode_unlock(inode);
-
-	return generic_file_splice_read(in, ppos, pipe, len, flags);
-}
-
-
 static ssize_t gfs2_file_splice_write(struct pipe_inode_info *pipe,
 				      struct file *out, loff_t *ppos,
 				      size_t len, unsigned int flags)

@@ -1140,7 +1116,7 @@ const struct file_operations gfs2_file_fops = {
 	.fsync		= gfs2_fsync,
 	.lock		= gfs2_lock,
 	.flock		= gfs2_flock,
-	.splice_read	= gfs2_file_splice_read,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= gfs2_file_splice_write,
 	.setlease	= simple_nosetlease,
 	.fallocate	= gfs2_fallocate,

@@ -1168,7 +1144,7 @@ const struct file_operations gfs2_file_fops_nolock = {
 	.open		= gfs2_open,
 	.release	= gfs2_release,
 	.fsync		= gfs2_fsync,
-	.splice_read	= gfs2_file_splice_read,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= gfs2_file_splice_write,
 	.setlease	= generic_setlease,
 	.fallocate	= gfs2_fallocate,

index 7d62097..5048585 100644
@@ -182,29 +182,6 @@ nfs_file_read(struct kiocb *iocb, struct iov_iter *to)
 }
 EXPORT_SYMBOL_GPL(nfs_file_read);
 
-ssize_t
-nfs_file_splice_read(struct file *filp, loff_t *ppos,
-		     struct pipe_inode_info *pipe, size_t count,
-		     unsigned int flags)
-{
-	struct inode *inode = file_inode(filp);
-	ssize_t res;
-
-	dprintk("NFS: splice_read(%pD2, %lu@%Lu)\n",
-		filp, (unsigned long) count, (unsigned long long) *ppos);
-
-	nfs_start_io_read(inode);
-	res = nfs_revalidate_mapping(inode, filp->f_mapping);
-	if (!res) {
-		res = generic_file_splice_read(filp, ppos, pipe, count, flags);
-		if (res > 0)
-			nfs_add_stats(inode, NFSIOS_NORMALREADBYTES, res);
-	}
-	nfs_end_io_read(inode);
-	return res;
-}
-EXPORT_SYMBOL_GPL(nfs_file_splice_read);
-
 int
 nfs_file_mmap(struct file * file, struct vm_area_struct * vma)
 {

@@ -868,7 +845,7 @@ const struct file_operations nfs_file_operations = {
 	.fsync		= nfs_file_fsync,
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
-	.splice_read	= nfs_file_splice_read,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= simple_nosetlease,

index 74935a1..d7b062b 100644
@@ -365,8 +365,6 @@ int nfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *)
 int nfs_file_fsync(struct file *file, loff_t start, loff_t end, int datasync);
 loff_t nfs_file_llseek(struct file *, loff_t, int);
 ssize_t nfs_file_read(struct kiocb *, struct iov_iter *);
-ssize_t nfs_file_splice_read(struct file *, loff_t *, struct pipe_inode_info *,
-			     size_t, unsigned int);
 int nfs_file_mmap(struct file *, struct vm_area_struct *);
 ssize_t nfs_file_write(struct kiocb *, struct iov_iter *);
 int nfs_file_release(struct inode *, struct file *);

index d085ad7..89a7795 100644
@@ -248,7 +248,7 @@ const struct file_operations nfs4_file_operations = {
 	.fsync		= nfs_file_fsync,
 	.lock		= nfs_lock,
 	.flock		= nfs_flock,
-	.splice_read	= nfs_file_splice_read,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.check_flags	= nfs_check_flags,
 	.setlease	= simple_nosetlease,

index 4e7b0dc..6596e41 100644
@@ -2307,36 +2307,6 @@ out_mutex:
 	return ret;
 }
 
-static ssize_t ocfs2_file_splice_read(struct file *in,
-				      loff_t *ppos,
-				      struct pipe_inode_info *pipe,
-				      size_t len,
-				      unsigned int flags)
-{
-	int ret = 0, lock_level = 0;
-	struct inode *inode = file_inode(in);
-
-	trace_ocfs2_file_splice_read(inode, in, in->f_path.dentry,
-			(unsigned long long)OCFS2_I(inode)->ip_blkno,
-			in->f_path.dentry->d_name.len,
-			in->f_path.dentry->d_name.name, len);
-
-	/*
-	 * See the comment in ocfs2_file_read_iter()
-	 */
-	ret = ocfs2_inode_lock_atime(inode, in->f_path.mnt, &lock_level);
-	if (ret < 0) {
-		mlog_errno(ret);
-		goto bail;
-	}
-	ocfs2_inode_unlock(inode, lock_level);
-
-	ret = generic_file_splice_read(in, ppos, pipe, len, flags);
-
-bail:
-	return ret;
-}
-
 static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
 				   struct iov_iter *to)
 {

@@ -2495,7 +2465,7 @@ const struct file_operations ocfs2_fops = {
 #endif
 	.lock		= ocfs2_lock,
 	.flock		= ocfs2_flock,
-	.splice_read	= ocfs2_file_splice_read,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ocfs2_fallocate,
 };

@@ -2540,7 +2510,7 @@ const struct file_operations ocfs2_fops_no_plocks = {
 	.compat_ioctl   = ocfs2_compat_ioctl,
 #endif
 	.flock		= ocfs2_flock,
-	.splice_read	= ocfs2_file_splice_read,
+	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ocfs2_fallocate,
 };

index f8f5fc5..0b58abc 100644
@@ -1314,8 +1314,6 @@ DEFINE_OCFS2_FILE_OPS(ocfs2_file_aio_write);
 
 DEFINE_OCFS2_FILE_OPS(ocfs2_file_splice_write);
 
-DEFINE_OCFS2_FILE_OPS(ocfs2_file_splice_read);
-
 DEFINE_OCFS2_FILE_OPS(ocfs2_file_aio_read);
 
 DEFINE_OCFS2_ULL_ULL_ULL_EVENT(ocfs2_truncate_file);

index d303a66..f31db44 100644
@@ -1170,7 +1170,6 @@ DEFINE_RW_EVENT(xfs_file_dax_read);
 DEFINE_RW_EVENT(xfs_file_buffered_write);
 DEFINE_RW_EVENT(xfs_file_direct_write);
 DEFINE_RW_EVENT(xfs_file_dax_write);
-DEFINE_RW_EVENT(xfs_file_splice_read);
 
 DECLARE_EVENT_CLASS(xfs_page_class,
 	TP_PROTO(struct inode *inode, struct page *page, unsigned long off,

index 901e25d..b04883e 100644
@@ -2794,8 +2794,6 @@ extern void block_sync_page(struct page *page);
 /* fs/splice.c */
 extern ssize_t generic_file_splice_read(struct file *, loff_t *,
 		struct pipe_inode_info *, size_t, unsigned int);
-extern ssize_t default_file_splice_read(struct file *, loff_t *,
-		struct pipe_inode_info *, size_t, unsigned int);
 extern ssize_t iter_file_splice_write(struct pipe_inode_info *,
 		struct file *, loff_t *, size_t, unsigned int);
 extern ssize_t generic_splice_sendpage(struct pipe_inode_info *pipe,

2.9.3

we only use iov_iter_get_pages_alloc() and iov_iter_advance() - pages are filled by kernel_readv() via a kvec array (as we used to do all along), so iov_iter here is used only as a way of arranging for those pages to be in pipe.

---
 fs/splice.c | 111 ++++++++++++++++++++++--------------------------------------
 1 file changed, 40 insertions(+), 71 deletions(-)

index 58c322a..0df907b 100644
@@ -361,7 +361,7 @@ const struct pipe_buf_operations nosteal_pipe_buf_ops = {
 };
 EXPORT_SYMBOL(nosteal_pipe_buf_ops);
 
-static ssize_t kernel_readv(struct file *file, const struct iovec *vec,
+static ssize_t kernel_readv(struct file *file, const struct kvec *vec,
 			    unsigned long vlen, loff_t offset)
 {
 	mm_segment_t old_fs;

@@ -397,96 +397,65 @@ static ssize_t default_file_splice_read(struct file *in, loff_t *ppos,
 				 struct pipe_inode_info *pipe, size_t len,
 				 unsigned int flags)
 {
+	struct kvec *vec, __vec[PIPE_DEF_BUFFERS];
+	struct iov_iter to;
+	struct page **pages;
 	unsigned int nr_pages;
-	unsigned int nr_freed;
-	size_t offset;
-	struct page *pages[PIPE_DEF_BUFFERS];
-	struct partial_page partial[PIPE_DEF_BUFFERS];
-	struct iovec *vec, __vec[PIPE_DEF_BUFFERS];
+	size_t offset, dummy, copied = 0;
 	ssize_t res;
-	size_t this_len;
-	int error;
 	int i;
-	struct splice_pipe_desc spd = {
-		.pages = pages,
-		.partial = partial,
-		.nr_pages_max = PIPE_DEF_BUFFERS,
-		.flags = flags,
-		.ops = &default_pipe_buf_ops,
-		.spd_release = spd_release_page,
-	};
 
-	if (splice_grow_spd(pipe, &spd))
+	if (pipe->nrbufs == pipe->buffers)
+		return -EAGAIN;
+
+	/*
+	 * Try to keep page boundaries matching to source pagecache ones -
+	 * it probably won't be much help, but...
+	 */
+	offset = *ppos & ~PAGE_MASK;
+
+	iov_iter_pipe(&to, ITER_PIPE | READ, pipe, len + offset);
+
+	res = iov_iter_get_pages_alloc(&to, &pages, len + offset, &dummy);
+	if (res <= 0)
 		return -ENOMEM;
 
-	res = -ENOMEM;
+	nr_pages = res / PAGE_SIZE;
+
 	vec = __vec;
-	if (spd.nr_pages_max > PIPE_DEF_BUFFERS) {
-		vec = kmalloc(spd.nr_pages_max * sizeof(struct iovec), GFP_KERNEL);
-		if (!vec)
-			goto shrink_ret;
+	if (nr_pages > PIPE_DEF_BUFFERS) {
+		vec = kmalloc(nr_pages * sizeof(struct kvec), GFP_KERNEL);
+		if (unlikely(!vec)) {
+			res = -ENOMEM;
+			goto out;
+		}
 	}
 
-	offset = *ppos & ~PAGE_MASK;
-	nr_pages = (len + offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
-
-	for (i = 0; i < nr_pages && i < spd.nr_pages_max && len; i++) {
-		struct page *page;
-
-		page = alloc_page(GFP_USER);
-		error = -ENOMEM;
-		if (!page)
-			goto err;
+	pipe->bufs[to.idx].offset = offset;
+	pipe->bufs[to.idx].len -= offset;
 
-		this_len = min_t(size_t, len, PAGE_SIZE - offset);
-		vec[i].iov_base = (void __user *) page_address(page);
+	for (i = 0; i < nr_pages; i++) {
+		size_t this_len = min_t(size_t, len, PAGE_SIZE - offset);
+		vec[i].iov_base = page_address(pages[i]) + offset;
 		vec[i].iov_len = this_len;
-		spd.pages[i] = page;
-		spd.nr_pages++;
 		len -= this_len;
 		offset = 0;
 	}
 
-	res = kernel_readv(in, vec, spd.nr_pages, *ppos);
-	if (res < 0) {
-		error = res;
-		goto err;
-	}
-
-	error = 0;
-	if (!res)
-		goto err;
-
-	nr_freed = 0;
-	for (i = 0; i < spd.nr_pages; i++) {
-		this_len = min_t(size_t, vec[i].iov_len, res);
-		spd.partial[i].offset = 0;
-		spd.partial[i].len = this_len;
-		if (!this_len) {
-			__free_page(spd.pages[i]);
-			spd.pages[i] = NULL;
-			nr_freed++;
-		}
-		res -= this_len;
-	}
-	spd.nr_pages -= nr_freed;
-
-	res = splice_to_pipe(pipe, &spd);
-	if (res > 0)
+	res = kernel_readv(in, vec, nr_pages, *ppos);
+	if (res > 0) {
+		copied = res;
 		*ppos += res;
+	}
 
-shrink_ret:
 	if (vec != __vec)
 		kfree(vec);
-	splice_shrink_spd(&spd);
+out:
+	for (i = 0; i < nr_pages; i++)
+		put_page(pages[i]);
+	kvfree(pages);
+	iov_iter_advance(&to, copied);	/* truncates and discards */
 	return res;
-
-err:
-	for (i = 0; i < spd.nr_pages; i++)
-		__free_page(spd.pages[i]);
-
-	res = error;
-	goto shrink_ret;
 }
 
 /*

2.9.3

since pipe_lock is the outermost now, we don't need to drop/regain socket locks around the call of splice_to_pipe() from skb_splice_bits(), which kills the need to have a socket-specific callback; we can just call splice_to_pipe() and be done with that.

---
 include/linux/skbuff.h |  8 +-------
 net/core/skbuff.c      | 28 ++--------------------------
 net/ipv4/tcp.c         |  3 +--
 net/kcm/kcmsock.c      | 16 +---------------
 net/unix/af_unix.c     | 17 +----------------
 5 files changed, 6 insertions(+), 66 deletions(-)

index 0f665cb..f520251 100644
@@ -3021,15 +3021,9 @@ int skb_copy_bits(const struct sk_buff *skb, int offset, void *to, int len);
 int skb_store_bits(struct sk_buff *skb, int offset, const void *from, int len);
 __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset, u8 *to,
 			      int len, __wsum csum);
-ssize_t skb_socket_splice(struct sock *sk,
-			  struct pipe_inode_info *pipe,
-			  struct splice_pipe_desc *spd);
 int skb_splice_bits(struct sk_buff *skb, struct sock *sk, unsigned int offset,
 		    struct pipe_inode_info *pipe, unsigned int len,
-		    unsigned int flags,
-		    ssize_t (*splice_cb)(struct sock *,
-					 struct pipe_inode_info *,
-					 struct splice_pipe_desc *));
+		    unsigned int flags);
 void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to);
 unsigned int skb_zerocopy_headlen(const struct sk_buff *from);
 int skb_zerocopy(struct sk_buff *to, struct sk_buff *from,

index 3864b4b6..208a9bc 100644
@@ -1962,37 +1962,13 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
 	return false;
 }
 
-ssize_t skb_socket_splice(struct sock *sk,
-			  struct pipe_inode_info *pipe,
-			  struct splice_pipe_desc *spd)
-{
-	int ret;
-
-	/* Drop the socket lock, otherwise we have reverse
-	 * locking dependencies between sk_lock and i_mutex
-	 * here as compared to sendfile(). We enter here
-	 * with the socket lock held, and splice_to_pipe() will
-	 * grab the pipe inode lock. For sendfile() emulation,
-	 * we call into ->sendpage() with the i_mutex lock held
-	 * and networking will grab the socket lock.
-	 */
-	release_sock(sk);
-	ret = splice_to_pipe(pipe, spd);
-	lock_sock(sk);
-
-	return ret;
-}
-
 /*
  * Map data from the skb to a pipe. Should handle both the linear part,
  * the fragments, and the frag list.
  */
 int skb_splice_bits(struct sk_buff *skb, struct sock *sk, unsigned int offset,
 		    struct pipe_inode_info *pipe, unsigned int tlen,
-		    unsigned int flags,
-		    ssize_t (*splice_cb)(struct sock *,
-					 struct pipe_inode_info *,
-					 struct splice_pipe_desc *))
+		    unsigned int flags)
 {
 	struct partial_page partial[MAX_SKB_FRAGS];
 	struct page *pages[MAX_SKB_FRAGS];

@@ -2009,7 +1985,7 @@ int skb_splice_bits(struct sk_buff *skb, struct sock *sk, unsigned int offset,
 	__skb_splice_bits(skb, pipe, &offset, &tlen, &spd, sk);
 
 	if (spd.nr_pages)
-		ret = splice_cb(sk, pipe, &spd);
+		ret = splice_to_pipe(pipe, &spd);
 
 	return ret;
 }

index ffbb218..ddd2179 100644
@@ -688,8 +688,7 @@ static int tcp_splice_data_recv(read_descriptor_t *rd_desc, struct sk_buff *skb,
 	int ret;
 
 	ret = skb_splice_bits(skb, skb->sk, offset, tss->pipe,
-			      min(rd_desc->count, len), tss->flags,
-			      skb_socket_splice);
+			      min(rd_desc->count, len), tss->flags);
 	if (ret > 0)
 		rd_desc->count -= ret;
 	return ret;

index cb39e05..994baae 100644
@@ -1461,19 +1461,6 @@ out:
 	return copied ? : err;
 }
 
-static ssize_t kcm_sock_splice(struct sock *sk,
-			       struct pipe_inode_info *pipe,
-			       struct splice_pipe_desc *spd)
-{
-	int ret;
-
-	release_sock(sk);
-	ret = splice_to_pipe(pipe, spd);
-	lock_sock(sk);
-
-	return ret;
-}
-
 static ssize_t kcm_splice_read(struct socket *sock, loff_t *ppos,
 			       struct pipe_inode_info *pipe, size_t len,
 			       unsigned int flags)

@@ -1503,8 +1490,7 @@ static ssize_t kcm_splice_read(struct socket *sock, loff_t *ppos,
 	if (len > rxm->full_len)
 		len = rxm->full_len;
 
-	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, len, flags,
-				 kcm_sock_splice);
+	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, len, flags);
 	if (copied < 0) {
 		err = copied;
 		goto err_out;

index f1dffe8..e7707ca 100644
@@ -2488,28 +2488,13 @@ static int unix_stream_recvmsg(struct socket *sock, struct msghdr *msg,
 	return unix_stream_read_generic(&state);
 }
 
-static ssize_t skb_unix_socket_splice(struct sock *sk,
-				      struct pipe_inode_info *pipe,
-				      struct splice_pipe_desc *spd)
-{
-	int ret;
-	struct unix_sock *u = unix_sk(sk);
-
-	mutex_unlock(&u->readlock);
-	ret = splice_to_pipe(pipe, spd);
-	mutex_lock(&u->readlock);
-
-	return ret;
-}
-
 static int unix_stream_splice_actor(struct sk_buff *skb,
 				    int skip, int chunk,
 				    struct unix_stream_read_state *state)
 {
 	return skb_splice_bits(skb, state->socket->sk,
 			       UNIXCB(skb).consumed + skip,
-			       state->pipe, chunk, state->splice_flags,
-			       skb_unix_socket_splice);
+			       state->pipe, chunk, state->splice_flags);
 }
 
 static ssize_t unix_stream_splice_read(struct socket *sock,  loff_t *ppos,

2.9.3

single-buffer analogue of splice_to_pipe(); vmsplice_to_pipe() switched to that, leaving splice_to_pipe() only for ->splice_read() instances (and that only until they are converted as well).

---
 fs/splice.c            | 109 ++++++++++++++++++++++++++++---------------------
 include/linux/splice.h |   2 +
 2 files changed, 64 insertions(+), 47 deletions(-)

index 9ce6e62..085ad37 100644
@@ -203,8 +203,6 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
 		buf->len = spd->partial[page_nr].len;
 		buf->private = spd->partial[page_nr].private;
 		buf->ops = spd->ops;
-		if (spd->flags & SPLICE_F_GIFT)
-			buf->flags |= PIPE_BUF_FLAG_GIFT;
 
 		pipe->nrbufs++;
 		page_nr++;

@@ -225,6 +223,27 @@ out:
 }
 EXPORT_SYMBOL_GPL(splice_to_pipe);
 
+ssize_t add_to_pipe(struct pipe_inode_info *pipe, struct pipe_buffer *buf)
+{
+	int ret;
+
+	if (unlikely(!pipe->readers)) {
+		send_sig(SIGPIPE, current, 0);
+		ret = -EPIPE;
+	} else if (pipe->nrbufs == pipe->buffers) {
+		ret = -EAGAIN;
+	} else {
+		int newbuf = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers - 1);
+		pipe->bufs[newbuf] = *buf;
+		pipe->nrbufs++;
+		return buf->len;
+	}
+	buf->ops->release(pipe, buf);
+	buf->ops = NULL;
+	return ret;
+}
+EXPORT_SYMBOL(add_to_pipe);
+
 void spd_release_page(struct splice_pipe_desc *spd, unsigned int i)
 {
 	put_page(spd->pages[i]);

@@ -1436,33 +1455,50 @@ static long do_splice(struct file *in, loff_t __user *off_in,
 	return -EINVAL;
 }
 
-static int get_iovec_page_array(const struct iov_iter *from,
-				struct page **pages,
-				struct partial_page *partial,
-				unsigned int pipe_buffers)
+static int iter_to_pipe(struct iov_iter *from,
+			struct pipe_inode_info *pipe,
+			unsigned flags)
 {
-	struct iov_iter i = *from;
-	int buffers = 0;
-	while (iov_iter_count(&i)) {
+	struct pipe_buffer buf = {
+		.ops = &user_page_pipe_buf_ops,
+		.flags = flags
+	};
+	size_t total = 0;
+	int ret = 0;
+	bool failed = false;
+
+	while (iov_iter_count(from) && !failed) {
+		struct page *pages[16];
 		ssize_t copied;
 		size_t start;
+		int n;
 
-		copied = iov_iter_get_pages(&i, pages + buffers, ~0UL,
-					pipe_buffers - buffers, &start);
-		if (copied <= 0)
-			return buffers ? buffers : copied;
+		copied = iov_iter_get_pages(from, pages, ~0UL, 16, &start);
+		if (copied <= 0) {
+			ret = copied;
+			break;
+		}
 
-		iov_iter_advance(&i, copied);
-		while (copied) {
+		for (n = 0; copied; n++, start = 0) {
 			int size = min_t(int, copied, PAGE_SIZE - start);
-			partial[buffers].offset = start;
-			partial[buffers].len = size;
+			if (!failed) {
+				buf.page = pages[n];
+				buf.offset = start;
+				buf.len = size;
+				ret = add_to_pipe(pipe, &buf);
+				if (unlikely(ret < 0)) {
+					failed = true;
+				} else {
+					iov_iter_advance(from, ret);
+					total += ret;
+				}
+			} else {
+				put_page(pages[n]);
+			}
 			copied -= size;
-			start = 0;
-			buffers++;
 		}
 	}
-	return buffers;
+	return total ? total : ret;
 }
 
 static int pipe_to_user(struct pipe_inode_info *pipe, struct pipe_buffer *buf,

@@ -1523,19 +1559,13 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *uiov,
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
 	struct iov_iter from;
-	struct page *pages[PIPE_DEF_BUFFERS];
-	struct partial_page partial[PIPE_DEF_BUFFERS];
-	struct splice_pipe_desc spd = {
-		.pages = pages,
-		.partial = partial,
-		.nr_pages_max = PIPE_DEF_BUFFERS,
-		.flags = flags,
-		.ops = &user_page_pipe_buf_ops,
-		.spd_release = spd_release_page,
-	};
 	long ret, total = 0;
+	unsigned buf_flag = 0;
 	int bogus_count;
 
+	if (flags & SPLICE_F_GIFT)
+		buf_flag = PIPE_BUF_FLAG_GIFT;
+
 	pipe = get_pipe_info(file);
 	if (!pipe)
 		return -EBADF;

@@ -1545,27 +1575,13 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *uiov,
 	if (ret < 0)
 		return ret;
 
-	if (splice_grow_spd(pipe, &spd)) {
-		kfree(iov);
-		return -ENOMEM;
-	}
-
 	pipe_lock(pipe);
 	bogus_count = pipe->buffers;
 	do {
 		bogus_count += pipe->nrbufs;
-		spd.nr_pages = get_iovec_page_array(&from, spd.pages,
-						    spd.partial,
-						    spd.nr_pages_max);
-		if (spd.nr_pages <= 0) {
-			ret = spd.nr_pages;
-			break;
-		}
-		ret = splice_to_pipe(pipe, &spd);
-		if (ret > 0) {
+		ret = iter_to_pipe(&from, pipe, buf_flag);
+		if (ret > 0)
 			total += ret;
-			iov_iter_advance(&from, ret);
-		}
 		bogus_count -= pipe->nrbufs;
 		if (bogus_count <= 0)
 			break;

@@ -1575,7 +1591,6 @@ static long vmsplice_to_pipe(struct file *file, const struct iovec __user *uiov,
 		wakeup_pipe_readers(pipe);
 		ret = total;
 	}
-	splice_shrink_spd(&spd);
 	kfree(iov);
 	return ret;
 }

index da2751d..58b300f 100644
@@ -72,6 +72,8 @@ extern ssize_t __splice_from_pipe(struct pipe_inode_info *,
 				  struct splice_desc *, splice_actor *);
 extern ssize_t splice_to_pipe(struct pipe_inode_info *,
 			      struct splice_pipe_desc *);
+extern ssize_t add_to_pipe(struct pipe_inode_info *,
+			      struct pipe_buffer *);
 extern ssize_t splice_direct_to_actor(struct file *, struct splice_desc *,
 				      splice_direct_actor *);
 

2.9.3

---
 fs/fuse/dev.c | 46 +++++++++-------------------------------------
 1 file changed, 9 insertions(+), 37 deletions(-)

index eaf56c6..0a6a808 100644
@@ -1342,9 +1342,8 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 				    struct pipe_inode_info *pipe,
 				    size_t len, unsigned int flags)
 {
-	int ret;
+	int total, ret;
 	int page_nr = 0;
-	int do_wakeup = 0;
 	struct pipe_buffer *bufs;
 	struct fuse_copy_state cs;
 	struct fuse_dev *fud = fuse_get_dev(in);

@@ -1363,50 +1362,23 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	if (ret < 0)
 		goto out;
 
-	ret = 0;
-
-	if (!pipe->readers) {
-		send_sig(SIGPIPE, current, 0);
-		if (!ret)
-			ret = -EPIPE;
-		goto out_unlock;
-	}
-
 	if (pipe->nrbufs + cs.nr_segs > pipe->buffers) {
 		ret = -EIO;
-		goto out_unlock;
+		goto out;
 	}
 
-	while (page_nr < cs.nr_segs) {
-		int newbuf = (pipe->curbuf + pipe->nrbufs) & (pipe->buffers - 1);
-		struct pipe_buffer *buf = pipe->bufs + newbuf;
-
-		buf->page = bufs[page_nr].page;
-		buf->offset = bufs[page_nr].offset;
-		buf->len = bufs[page_nr].len;
+	for (ret = total = 0; page_nr < cs.nr_segs; total += ret) {
 		/*
 		 * Need to be careful about this.  Having buf->ops in module
 		 * code can Oops if the buffer persists after module unload.
 		 */
-		buf->ops = &nosteal_pipe_buf_ops;
-
-		pipe->nrbufs++;
-		page_nr++;
-		ret += buf->len;
-
-		if (pipe->files)
-			do_wakeup = 1;
-	}
-
-out_unlock:
-
-	if (do_wakeup) {
-		smp_mb();
-		if (waitqueue_active(&pipe->wait))
-			wake_up_interruptible(&pipe->wait);
-		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
+		bufs[page_nr].ops = &nosteal_pipe_buf_ops;
+		ret = add_to_pipe(pipe, &bufs[page_nr++]);
+		if (unlikely(ret < 0))
+			break;
 	}
-
+	if (total)
+		ret = total;
 out:
 	for (; page_nr < cs.nr_segs; page_nr++)
 		put_page(bufs[page_nr].page);

2.9.3

it's not 70s anymore.

---
[obviously should be separated; trivial cleanup almost unrelated to series]

 fs/cifs/file.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

index 579e41b..42b99af 100644
@@ -2478,7 +2478,7 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
 	size_t cur_len;
 	unsigned long nr_pages, num_pages, i;
 	struct cifs_writedata *wdata;
-	struct iov_iter saved_from;
+	struct iov_iter saved_from = *from;
 	loff_t saved_offset = offset;
 	pid_t pid;
 	struct TCP_Server_Info *server;

@@ -2489,7 +2489,6 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
 		pid = current->tgid;
 
 	server = tlink_tcon(open_file->tlink)->ses->server;
-	memcpy(&saved_from, from, sizeof(struct iov_iter));
 
 	do {
 		unsigned int wsize, credits;

@@ -2551,8 +2550,7 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
 			kref_put(&wdata->refcount,
 				 cifs_uncached_writedata_release);
 			if (rc == -EAGAIN) {
-				memcpy(from, &saved_from,
-				       sizeof(struct iov_iter));
+				*from = saved_from;
 				iov_iter_advance(from, offset - saved_offset);
 				continue;
 			}

@@ -2576,7 +2574,7 @@ ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from)
 	struct cifs_sb_info *cifs_sb;
 	struct cifs_writedata *wdata, *tmp;
 	struct list_head wdata_list;
-	struct iov_iter saved_from;
+	struct iov_iter saved_from = *from;
 	int rc;
 
 	/*

@@ -2597,8 +2595,6 @@ ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from)
 	if (!tcon->ses->server->ops->async_writev)
 		return -ENOSYS;
 
-	memcpy(&saved_from, from, sizeof(struct iov_iter));
-
 	rc = cifs_write_from_iter(iocb->ki_pos, iov_iter_count(from), from,
 				  open_file, cifs_sb, &wdata_list);
 

@@ -2631,13 +2627,11 @@ restart_loop:
 			/* resend call if it's a retryable error */
 			if (rc == -EAGAIN) {
 				struct list_head tmp_list;
-				struct iov_iter tmp_from;
+				struct iov_iter tmp_from = saved_from;
 
 				INIT_LIST_HEAD(&tmp_list);
 				list_del_init(&wdata->list);
 
-				memcpy(&tmp_from, &saved_from,
-				       sizeof(struct iov_iter));
 				iov_iter_advance(&tmp_from,
 						 wdata->offset - iocb->ki_pos);
 

2.9.3

---
[another cleanup, will be moved out of that branch]

 fs/fuse/file.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

index 3988b43..4c1db6c 100644
@@ -2339,31 +2339,15 @@ static int fuse_ioctl_copy_user(struct page **pages, struct iovec *iov,
 
 	while (iov_iter_count(&ii)) {
 		struct page *page = pages[page_idx++];
-		size_t todo = min_t(size_t, PAGE_SIZE, iov_iter_count(&ii));
-		void *kaddr;
+		size_t copied;
 
-		kaddr = kmap(page);
-
-		while (todo) {
-			char __user *uaddr = ii.iov->iov_base + ii.iov_offset;
-			size_t iov_len = ii.iov->iov_len - ii.iov_offset;
-			size_t copy = min(todo, iov_len);
-			size_t left;
-
-			if (!to_user)
-				left = copy_from_user(kaddr, uaddr, copy);
-			else
-				left = copy_to_user(uaddr, kaddr, copy);
-
-			if (unlikely(left))
-				return -EFAULT;
-
-			iov_iter_advance(&ii, copy);
-			todo -= copy;
-			kaddr += copy;
-		}
+		if (!to_user)
+			copied = copy_page_from_iter(page, 0, PAGE_SIZE, &ii);
+		else
+			copied = copy_page_to_iter(page, 0, PAGE_SIZE, &ii);
 
-		kunmap(page);
+		if (unlikely(copied != PAGE_SIZE && iov_iter_count(&ii)))
+			return -EFAULT;
 	}
 
 	return 0;

2.9.3

Picked up and pushed to fuse.git #for-next

Thanks,
Miklos

iov_iter variant for passing data into pipe. copy_to_iter() copies data into page(s) it has allocated and stuffs them into the pipe; copy_page_to_iter() stuffs there a reference to the page given to it. Both will try to coalesce if possible. iov_iter_zero() is similar to copy_to_iter(); iov_iter_get_pages() and friends will do as copy_to_iter() would have and return the pages where the data would've been copied. iov_iter_advance() will truncate everything past the spot it has advanced to.

New primitive: iov_iter_pipe(), used for initializing those. pipe should be locked all along.

Running out of space acts as fault would for iovec-backed ones; in other words, giving it to ->read_iter() may result in short read if the pipe overflows, or -EFAULT if it happens with nothing copied there.

In other words, ->read_iter() on those acts pretty much like ->splice_read(). Moreover, all generic_file_splice_read() users, as well as many other ->splice_read() instances can be switched to that scheme - that'll happen in the next commit.

---
[this certainly needs to be documented in more details]

 fs/splice.c            |   2 +-
 include/linux/splice.h |   1 +
 include/linux/uio.h    |  14 +-
 lib/iov_iter.c         | 390 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 401 insertions(+), 6 deletions(-)

index 085ad37..0daa7d1 100644
@@ -524,7 +524,7 @@ ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
 }
 EXPORT_SYMBOL(generic_file_splice_read);
 
-static const struct pipe_buf_operations default_pipe_buf_ops = {
+const struct pipe_buf_operations default_pipe_buf_ops = {
 	.can_merge = 0,
 	.confirm = generic_pipe_buf_confirm,
 	.release = generic_pipe_buf_release,

index 58b300f..00a2116 100644
@@ -85,4 +85,5 @@ extern void splice_shrink_spd(struct splice_pipe_desc *);
 extern void spd_release_page(struct splice_pipe_desc *, unsigned int);
 
 extern const struct pipe_buf_operations page_cache_pipe_buf_ops;
+extern const struct pipe_buf_operations default_pipe_buf_ops;
 #endif

index 1b5d1cd..c4fe1ab 100644
@@ -13,6 +13,7 @@
 #include <uapi/linux/uio.h>
 
 struct page;
+struct pipe_inode_info;
 
 struct kvec {
 	void *iov_base; /* and that should *never* hold a userland pointer */

@@ -23,6 +24,7 @@ enum {
 	ITER_IOVEC = 0,
 	ITER_KVEC = 2,
 	ITER_BVEC = 4,
+	ITER_PIPE = 8,
 };
 
 struct iov_iter {

@@ -33,8 +35,12 @@ struct iov_iter {
 		const struct iovec *iov;
 		const struct kvec *kvec;
 		const struct bio_vec *bvec;
+		struct pipe_inode_info *pipe;
+	};
+	union {
+		unsigned long nr_segs;
+		int idx;
 	};
-	unsigned long nr_segs;
 };
 
 /*

@@ -64,7 +70,7 @@ static inline struct iovec iov_iter_iovec(const struct iov_iter *iter)
 }
 
 #define iov_for_each(iov, iter, start)				\
-	if (!((start).type & ITER_BVEC))			\
+	if (!((start).type & (ITER_BVEC | ITER_PIPE)))		\
 	for (iter = (start);					\
 	     (iter).count &&					\
 	     ((iov = iov_iter_iovec(&(iter))), 1);		\

@@ -94,6 +100,8 @@ void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,
 			unsigned long nr_segs, size_t count);
 void iov_iter_bvec(struct iov_iter *i, int direction, const struct bio_vec *bvec,
 			unsigned long nr_segs, size_t count);
+void iov_iter_pipe(struct iov_iter *i, int direction, struct pipe_inode_info *pipe,
+			size_t count);
 ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
 			size_t maxsize, unsigned maxpages, size_t *start);
 ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages,

@@ -109,7 +117,7 @@ static inline size_t iov_iter_count(struct iov_iter *i)
 
 static inline bool iter_is_iovec(struct iov_iter *i)
 {
-	return !(i->type & (ITER_BVEC | ITER_KVEC));
+	return !(i->type & (ITER_BVEC | ITER_KVEC | ITER_PIPE));
 }
 
 /*

2.9.3

... and kill the ->splice_read() instances that can be switched to it

---
 drivers/staging/lustre/lustre/llite/file.c         |  70 ++----
 .../staging/lustre/lustre/llite/llite_internal.h   |  15 +-
 drivers/staging/lustre/lustre/llite/vvp_internal.h |  14 --
 drivers/staging/lustre/lustre/llite/vvp_io.c       |  45 +---
 fs/coda/file.c                                     |  23 +-
 fs/gfs2/file.c                                     |  28 +--
 fs/nfs/file.c                                      |  25 +--
 fs/nfs/internal.h                                  |   2 -
 fs/nfs/nfs4file.c                                  |   2 +-
 fs/ocfs2/file.c                                    |  34 +--
 fs/ocfs2/ocfs2_trace.h                             |   2 -
 fs/splice.c                                        | 238 +++------------------
 fs/xfs/xfs_file.c                                  |  41 +---
 fs/xfs/xfs_trace.h                                 |   1 -
 include/linux/fs.h                                 |   2 -
 mm/shmem.c                                         | 115 +---------
 16 files changed, 57 insertions(+), 600 deletions(-)

index 57281b9..2567b09 100644
@@ -1153,36 +1153,21 @@ restart:
 		int write_mutex_locked = 0;
 
 		vio->vui_fd  = LUSTRE_FPRIVATE(file);
-		vio->vui_io_subtype = args->via_io_subtype;
-
-		switch (vio->vui_io_subtype) {
-		case IO_NORMAL:
-			vio->vui_iter = args->u.normal.via_iter;
-			vio->vui_iocb = args->u.normal.via_iocb;
-			if ((iot == CIT_WRITE) &&
-			    !(vio->vui_fd->fd_flags & LL_FILE_GROUP_LOCKED)) {
-				if (mutex_lock_interruptible(&lli->
-							       lli_write_mutex)) {
-					result = -ERESTARTSYS;
-					goto out;
-				}
-				write_mutex_locked = 1;
+		vio->vui_iter = args->u.normal.via_iter;
+		vio->vui_iocb = args->u.normal.via_iocb;
+		if ((iot == CIT_WRITE) &&
+		    !(vio->vui_fd->fd_flags & LL_FILE_GROUP_LOCKED)) {
+			if (mutex_lock_interruptible(&lli->lli_write_mutex)) {
+				result = -ERESTARTSYS;
+				goto out;
 			}
-			down_read(&lli->lli_trunc_sem);
-			break;
-		case IO_SPLICE:
-			vio->u.splice.vui_pipe = args->u.splice.via_pipe;
-			vio->u.splice.vui_flags = args->u.splice.via_flags;
-			break;
-		default:
-			CERROR("Unknown IO type - %u\n", vio->vui_io_subtype);
-			LBUG();
+			write_mutex_locked = 1;
 		}
+		down_read(&lli->lli_trunc_sem);
 		ll_cl_add(file, env, io);
 		result = cl_io_loop(env, io);
 		ll_cl_remove(file, env);
-		if (args->via_io_subtype == IO_NORMAL)
-			up_read(&lli->lli_trunc_sem);
+		up_read(&lli->lli_trunc_sem);
 		if (write_mutex_locked)
 			mutex_unlock(&lli->lli_write_mutex);
 	} else {