> On Nov 19, 2018, at 2:54 PM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > 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. Should I send you a patch? -- Chuck Lever
