Date:   Tue, 14 Jun 2022 18:12:22 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        nvdimm@...ts.linux.dev, David Howells <dhowells@...hat.com>
Subject: Re: [RFC][PATCH] fix short copy handling in copy_mc_pipe_to_iter()

On Tue, Jun 14, 2022 at 01:53:50AM +0100, Al Viro wrote:

> FWIW, I've got quite a bit of cleanups in the local tree; reordering and
> cleaning that queue up at the moment, will post tonight or tomorrow.
> 
> I've looked into doing allocations page-by-page (instead of single
> push_pipe(), followed by copying into those).  Doable, but it ends
> up being much messier.

Hmm...  Maybe not - a possible interface would be
	append_pipe(iter, size, &off)

that would either do kmap_local_page() on the last buffer (if it's
anonymous and has space in it) or allocated and mapped a page and
added a new buffer.  Returning the mapped address and offset from it.
Then these loops would looks like this:

	while (left) {
		p = append_pipe(iter, left, &off);
		if (!p)
			break;
		chunk = min(left, PAGE_SIZE - off);
		rem = copy(p + off, whatever, chunk);
		chunk -= rem;
		kunmap_local(p);

		copied += chunk;
		left -= chunk;

		if (unlikely(rem)) {
			pipe_revert(i, rem);
			break;
		}
	}
	return copied;

with no push_pipe() used at all.  For operations that can't fail,
the things are simplified in an obvious way (rem is always 0).

Or we could have append_pipe() return a struct page * and leave
kmap_local_page() to the caller...

struct page *append_pipe(struct iov_iter *i, size_t size, unsigned *off)
{
	struct pipe_inode_info *pipe = i->pipe;
	unsigned offset = i->iov_offset;
	struct page_buffer *buf;
	struct page *page;

	if (offset && offset < PAGE_SIZE) {
		// some space in the last buffer; can we add to it?
		buf = pipe_buf(pipe, pipe->head - 1);
		if (allocated(buf)) {
			size = min(size, PAGE_SIZE - offset);
			buf->len += size;
			i->iov_offset += size;
			i->count -= size;
			*off = offset;
			return buf->page;	// or kmap_local_page(...)
		}
	}
	// OK, we need a new buffer
	size = min(size, PAGE_SIZE);
	if (pipe_full(.....))
		return NULL;
	page = alloc_page(GFP_USER);
	if (!page)
		return NULL;
	// got it...
	buf = pipe_buf(pipe, pipe->head++);
	*buf = (struct pipe_buffer){.ops = &default_pipe_buf_ops,
				    .page = page, .len = size };
	i->head = pipe->head - 1;
	i->iov_offset = size;
	i->count -= size;
	*off = 0;
	return page;	 // or kmap_local_page(...)
}

(matter of fact, the last part could use another helper in my tree - there
the tail would be
	// OK, we need a new buffer
	size = min(size, PAGE_SIZE);
	page = push_anon(pipe, size);
	if (!page)
		return NULL;
	i->head = pipe->head - 1;
	i->iov_offset = size;
	i->count -= size;
	*off = 0;
	return page;
)

Would that be readable enough from your POV?  That way push_pipe()
loses almost all callers and after the "make iov_iter_get_pages()
advancing" part of the series it simply goes away...

It's obviously too intrusive for backports, though - there I'd very much
prefer the variant I posted.

Comments?

PS: re local helpers:

static inline struct pipe_buffer *pipe_buf(const struct pipe_inode_info *pipe,
                                           unsigned int slot)
{
	return &pipe->bufs[slot & (pipe->ring_size - 1)];
}

pretty much all places where we cache pipe->ring_size - 1 had been
absolutely pointless; there are several exceptions, but back in 2019
"pipe: Use head and tail pointers for the ring, not cursor and length"
went overboard with microoptimizations...

Powered by blists - more mailing lists