> 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.
> --b.
>
>> WARN_ON_ONCE(nvecs > ARRAY_SIZE(rqstp->rq_vec));
>>
>> status = nfsd_vfs_write(rqstp, &cstate->current_fh, filp,
>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
>> index f107f9f..a6faee5 100644
>> --- a/fs/nfsd/nfsproc.c
>> +++ b/fs/nfsd/nfsproc.c
>> @@ -218,7 +218,8 @@
>> SVCFH_fmt(&argp->fh),
>> argp->len, argp->offset);
>>
>> - nvecs = svc_fill_write_vector(rqstp, &argp->first, cnt);
>> + nvecs = svc_fill_write_vector(rqstp, rqstp->rq_arg.pages,
>> + &argp->first, cnt);
>> if (!nvecs)
>> return nfserr_io;
>> nfserr = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh),
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 574368e..43f88bd 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -496,6 +496,7 @@ int svc_register(const struct svc_serv *, struct net *, const int,
>> struct svc_pool * svc_pool_for_cpu(struct svc_serv *serv, int cpu);
>> char * svc_print_addr(struct svc_rqst *, char *, size_t);
>> unsigned int svc_fill_write_vector(struct svc_rqst *rqstp,
>> + struct page **pages,
>> struct kvec *first, size_t total);
>> char *svc_fill_symlink_pathname(struct svc_rqst *rqstp,
>> struct kvec *first, size_t total);
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 30a4226..2194ed5 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -1537,16 +1537,16 @@ u32 svc_max_payload(const struct svc_rqst *rqstp)
>> /**
>> * svc_fill_write_vector - Construct data argument for VFS write call
>> * @rqstp: svc_rqst to operate on
>> + * @pages: list of pages containing data payload
>> * @first: buffer containing first section of write payload
>> * @total: total number of bytes of write payload
>> *
>> - * Returns the number of elements populated in the data argument array.
>> + * Fills in rqstp::rq_vec, and returns the number of elements.
>> */
>> -unsigned int svc_fill_write_vector(struct svc_rqst *rqstp, struct kvec *first,
>> - size_t total)
>> +unsigned int svc_fill_write_vector(struct svc_rqst *rqstp, struct page **pages,
>> + struct kvec *first, size_t total)
>> {
>> struct kvec *vec = rqstp->rq_vec;
>> - struct page **pages;
>> unsigned int i;
>>
>> /* Some types of transport can present the write payload
>> @@ -1560,14 +1560,11 @@ unsigned int svc_fill_write_vector(struct svc_rqst *rqstp, struct kvec *first,
>> ++i;
>> }
>>
>> - WARN_ON_ONCE(rqstp->rq_arg.page_base != 0);
>> - pages = rqstp->rq_arg.pages;
>> while (total) {
>> vec[i].iov_base = page_address(*pages);
>> vec[i].iov_len = min_t(size_t, total, PAGE_SIZE);
>> total -= vec[i].iov_len;
>> ++i;
>> -
>> ++pages;
>> }
>>
--
Chuck Lever
