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