DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C6A33208A3
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [PATCH v1 3/4] NFSD: Refactor the generic write vector fill
 helper
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20181115163246.GB9022@fieldses.org>
Date:   Thu, 15 Nov 2018 16:19:52 -0800
Cc:     linux-rdma@vger.kernel.org,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <A6FEE9E7-0226-460F-AA3E-42D7CEE9A781@oracle.com>
References: <20180727142007.21878.77494.stgit@klimt.1015granger.net>
 <20180727151905.21878.62693.stgit@klimt.1015granger.net>
 <20181115163246.GB9022@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Nov 15, 2018, at 8:32 AM, J. Bruce Fields <bfields@fieldses.org> =
wrote:
>=20
> Um, I somehow managed to overlook a pynfs regression till just now:
>=20
> 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.
>>=20
>> 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.
>>=20
>> 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.
>>=20
> ...
>> @@ -1027,7 +1009,10 @@ static int fill_in_write_vector(struct kvec =
*vec, struct nfsd4_write *write)
>> 	write->wr_how_written =3D write->wr_stable_how;
>> 	gen_boot_verifier(&write->wr_verifier, SVC_NET(rqstp));
>>=20
>> -	nvecs =3D fill_in_write_vector(rqstp->rq_vec, write);
>> +	nvecs =3D svc_fill_write_vector(rqstp, write->wr_pagelist,
>> +				      &write->wr_head, =
write->wr_buflen);
>> +	if (!nvecs)
>> +		return nfserr_io;
>=20
> 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:
>=20
> 	"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.
>=20
>> 	WARN_ON_ONCE(nvecs > ARRAY_SIZE(rqstp->rq_vec));
>>=20
>> 	status =3D 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);
>>=20
>> -	nvecs =3D svc_fill_write_vector(rqstp, &argp->first, cnt);
>> +	nvecs =3D svc_fill_write_vector(rqstp, rqstp->rq_arg.pages,
>> +				      &argp->first, cnt);
>> 	if (!nvecs)
>> 		return nfserr_io;
>> 	nfserr =3D 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 =3D rqstp->rq_vec;
>> -	struct page **pages;
>> 	unsigned int i;
>>=20
>> 	/* 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;
>> 	}
>>=20
>> -	WARN_ON_ONCE(rqstp->rq_arg.page_base !=3D 0);
>> -	pages =3D rqstp->rq_arg.pages;
>> 	while (total) {
>> 		vec[i].iov_base =3D page_address(*pages);
>> 		vec[i].iov_len =3D min_t(size_t, total, PAGE_SIZE);
>> 		total -=3D vec[i].iov_len;
>> 		++i;
>> -
>> 		++pages;
>> 	}
>>=20

--
Chuck Lever