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

[Dominique Martinet <asmadeus@codewreck.org>]

Al Viro wrote on Sat, Jun 11, 2022 at 03:19:07PM +0000:
> > 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...

Well, yes, but qemu supports .L -- even if we arbitrarily decide .u
won't go through the zero-copy path it's not going to harm the VM case.

If zc had been a thing for e.g. trans_fd there are plenty of servers
that could still be only used with older variants, but I don't think we
have to try supporting 9p2000.u + zc if it's a burden in the code.

> > 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).

Ok, thanks

> > ... 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.


Right, I forgot the string itself has a size, so we're not looking at
the last bytes but something that is no longer there and readf will
fail. Ok.

Sure, I don't see any other problem with this code.
I still think it's complexity we don't really need, but it's better than
what we have -- care to resend it as a real patch? and I'll apply/run
through some basic tests with qemu+2000u a bit.
(well, I don't have to wait to run the tests)

-- 
Dominique