Re: [PATCH v1 3/4] NFSD: Refactor the generic write vector fill helper

[Bruce Fields <bfields@fieldses.org>]

On Thu, Nov 15, 2018 at 04:19:52PM -0800, Chuck Lever wrote:
> 
> 
> > On Nov 15, 2018, at 8:32 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > Um, I somehow managed to overlook a pynfs regression till just now:
> > 
> > On Fri, Jul 27, 2018 at 11:19:05AM -0400, Chuck Lever wrote:
> >> fill_in_write_vector() is nearly the same logic as
> >> svc_fill_write_vector(), but there are a few differences so that
> >> the former can handle multiple WRITE payloads in a single COMPOUND.
> >> 
> >> svc_fill_write_vector() can be adjusted so that it can be used in
> >> the NFSv4 WRITE code path too. Instead of assuming the pages are
> >> coming from rq_args.pages, have the caller pass in the page list.
> >> 
> >> The immediate benefit is a reduction of code duplication. It also
> >> prevents the NFSv4 WRITE decoder from passing an empty vector
> >> element when the transport has provided the payload in the xdr_buf's
> >> page array.
> >> 
> > ...
> >> @@ -1027,7 +1009,10 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
> >> 	write->wr_how_written = write->wr_stable_how;
> >> 	gen_boot_verifier(&write->wr_verifier, SVC_NET(rqstp));
> >> 
> >> -	nvecs = fill_in_write_vector(rqstp->rq_vec, write);
> >> +	nvecs = svc_fill_write_vector(rqstp, write->wr_pagelist,
> >> +				      &write->wr_head, write->wr_buflen);
> >> +	if (!nvecs)
> >> +		return nfserr_io;
> > 
> > Do you remember why you added this check?
> 
> Yes, at one point svc_fill_write_vector() called the transport
> layer to do some processing, and I needed a way for it to
> indicate a graceful failure.
> 
> 
> > It's causing zero-length writes to fail, in violation of the spec:
> > 
> > 	"If the count is zero, the WRITE will succeed and return a count
> > 	of zero subject to permissions checking."
> 
> RFC 7530, Section 16.36.4.
> 
> 
> > I'm not seeing a reason why it wouldn't be safe just to remove that
> > check.
> 
> Or, make the check more specific:
> 
> 	if (!nvecs && write->wr_buflen)
> 		return nfserr_io;
> 
> This is a little more bullet proof, in case of bugs in the
> transport layer.

The current svc_fill_write_vector is pretty short and obviously can't
hit that case, so I'd just find that confusing.  We can re-add that
check when if and when it's needed.

--b.