DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6DC7E20831
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: <20181119195442.GA22976@fieldses.org>
Date:   Mon, 19 Nov 2018 14:58:17 -0500
Cc:     linux-rdma <linux-rdma@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <A6299E15-DDDB-4BF4-8F54-62BC799215AF@oracle.com>
References: <20180727142007.21878.77494.stgit@klimt.1015granger.net>
 <20180727151905.21878.62693.stgit@klimt.1015granger.net>
 <20181115163246.GB9022@fieldses.org>
 <A6FEE9E7-0226-460F-AA3E-42D7CEE9A781@oracle.com>
 <20181119195442.GA22976@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Nov 19, 2018, at 2:54 PM, Bruce Fields <bfields@fieldses.org> =
wrote:
>=20
> On Thu, Nov 15, 2018 at 04:19:52PM -0800, Chuck Lever wrote:
>>=20
>>=20
>>> 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?
>>=20
>> 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.
>>=20
>>=20
>>> 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."
>>=20
>> RFC 7530, Section 16.36.4.
>>=20
>>=20
>>> I'm not seeing a reason why it wouldn't be safe just to remove that
>>> check.
>>=20
>> Or, make the check more specific:
>>=20
>> 	if (!nvecs && write->wr_buflen)
>> 		return nfserr_io;
>>=20
>> This is a little more bullet proof, in case of bugs in the
>> transport layer.
>=20
> 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