Re: [RFC][CFT] handling Rerror without copy_from_iter_full()

[Al Viro <viro@zeniv.linux.org.uk>]

On Sat, Jun 11, 2022 at 10:27:54PM +0900, Dominique Martinet wrote:

> That makes me wonder just how much use we get for the legacy
> protocols -- I guess we do have some but all the filesystem-y
> implementations that I would expect to be main users for large
> IOs/zc are 9P2000.L as far as I know -- especially considering
> virtio is pretty much limited to qemu? Are there other 9p virtio
> servers?

Client can trivially force its use - -o version=9p2000.u and there
you go...

> So would it make sense to just say "not .L? tough luck, no zc",
> or am I just being lazy?
> 
> > Had that been in fixed-sized buffer (which is actually 4K), we would've
> > dealt with that the same way we handle non-zerocopy case.  However,
> > for zerocopy it doesn't end up there, so we need to copy it from
> > those pages.
> > 
> > The trouble is, by the time we get around to that, the references to
> > pages in question are already dropped.  As the result, p9_zc_check_errors()
> > tries to get the data using copy_from_iter_full().  Unfortunately, the
> > iov_iter it's trying to read from might *NOT* be capable of that.
> > It is, after all, a data destination, not data source.  In particular,
> > if it's an ITER_PIPE one, copy_from_iter_full() will simply fail.
> 
> Silly question, in case of a pipe we'll have written something we
> shouldn't have, or is it not gone yet until we actually finish the IO
> with iov_iter_advance?
> (my understanding is that reader won't have anything to read until
> someone else does the advance, and that someone else will have
> overwritten the data with their own content so no garbage wlll be read)

More than that, pipe is locked through ->read_iter(), so transient garbage
in it doesn't matter - we will either advance it by zero (it's an error)
or, with iov_iter_get_pages_alloc() switching to advancing variant,
we'll revert by the amount it had reserved there (error is the same as
extremely short read).

> ... With that said though your approach here definitely looks better
> than what we currently have -- my main issue is that truncating is fine
> for the original 9p2000 but for .U you'd be losing the ecode that we
> currently trust for being errno-compatible.

No, we wouldn't - this
                len = req->rc.size - req->rc.offset;
		if (len > (P9_ZC_HDR_SZ - 7)) {
			err = -EFAULT;
			goto out_err;
		}
in p9_check_zc_errors() means that mainline won't even look at that value.
And we'll get the same -EFAULT when we get to
                err = p9pdu_readf(&req->rc, c->proto_version, "s?d",
				  &ename, &ecode);
in p9_check_errors() and see that the length of string is too large to
fit into (truncated) reply.