Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Tom Talpey <tom@talpey.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH v2 1/6] xprtrdma: Remove FMR support in
 rpcrdma_convert_iovs()
Thread-Topic: [PATCH v2 1/6] xprtrdma: Remove FMR support in
 rpcrdma_convert_iovs()
Thread-Index: AQHW+kkkB0ViIEM5jEWO7hNw7EyLY6pGuhyAgAAA0QA=
Date:   Wed, 3 Feb 2021 18:09:45 +0000
Message-ID: <13F6C716-AF73-4B45-A248-EB4BD71E7C38@oracle.com>
References: <161236925476.1030487.10407536259816633879.stgit@manet.1015granger.net>
 <161236943446.1030487.4542967452464402073.stgit@manet.1015granger.net>
 <a2a7d8a1-9372-04ac-7ad6-f29c34ef3804@talpey.com>
In-Reply-To: <a2a7d8a1-9372-04ac-7ad6-f29c34ef3804@talpey.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <FFB4BFDA6B6DCC4BAD9E03253E9FC9B7@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: a45fe552-a4fb-459a-f49e-08d8c86ee311
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Feb 2021 18:09:45.2297
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Omac/V59VgFhTx1awyLdtu8LwCbE81dlYGIioCrAjSFkbwcy66OwlNDa+U6K1nj/TI0mXyByoy/HTOiva9pGjQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4388
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9884 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0
 suspectscore=0 spamscore=0 mlxlogscore=999 phishscore=0 adultscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2102030106
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9884 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxlogscore=999
 mlxscore=0 priorityscore=1501 spamscore=0 impostorscore=0 clxscore=1015
 suspectscore=0 lowpriorityscore=0 phishscore=0 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2102030106
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 3, 2021, at 1:06 PM, Tom Talpey <tom@talpey.com> wrote:
>=20
> On 2/3/2021 11:23 AM, Chuck Lever wrote:
>> Support for FMR was removed by commit ba69cd122ece ("xprtrdma:
>> Remove support for FMR memory registration") [Dec 2018]. That means
>> the buffer-splitting behavior of rpcrdma_convert_kvec(), added by
>> commit 821c791a0bde ("xprtrdma: Segment head and tail XDR buffers
>> on page boundaries") [Mar 2016], is no longer necessary. FRWR
>> memory registration handles this case with aplomb.
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  net/sunrpc/xprtrdma/rpc_rdma.c |   19 ++++---------------
>>  1 file changed, 4 insertions(+), 15 deletions(-)
>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rd=
ma.c
>> index 8f5d0cb68360..832765f3ebba 100644
>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>> @@ -204,9 +204,7 @@ rpcrdma_alloc_sparse_pages(struct xdr_buf *buf)
>>  	return 0;
>>  }
>>  -/* Split @vec on page boundaries into SGEs. FMR registers pages, not
>> - * a byte range. Other modes coalesce these SGEs into a single MR
>> - * when they can.
>> +/* Convert @vec to a single SGL element.
>>   *
>>   * Returns pointer to next available SGE, and bumps the total number
>>   * of SGEs consumed.
>> @@ -215,21 +213,12 @@ static struct rpcrdma_mr_seg *
>>  rpcrdma_convert_kvec(struct kvec *vec, struct rpcrdma_mr_seg *seg,
>>  		     unsigned int *n)
>>  {
>> -	u32 remaining, page_offset;
>> -	char *base;
>> -
>> -	base =3D vec->iov_base;
>> -	page_offset =3D offset_in_page(base);
>> -	remaining =3D vec->iov_len;
>> -	while (remaining) {
>> +	if (vec->iov_len) {
>=20
> This is weird. Is it ever possible for a zero-length segment to be
> passed? If so, it's obviously wrong to return an uninitialized "seg"
> to the caller. I'd suggest simply asserting that iov_len is !=3D 0.
>=20
> I guess this was an issue in the existing code, but there, it would
> only trigger if *all* the segs were zero.

It would be a bug if head.iov_len was zero.

tail.iov_len is checked before the call. I think that means this
check is superfluous and can be removed.


>=20
>>  		seg->mr_page =3D NULL;
>> -		seg->mr_offset =3D base;
>> -		seg->mr_len =3D min_t(u32, PAGE_SIZE - page_offset, remaining);
>> -		remaining -=3D seg->mr_len;
>> -		base +=3D seg->mr_len;
>> +		seg->mr_offset =3D vec->iov_base;
>=20
> I thought the previous discussion was that this should be offset_in_page
> not the (virtual) iov_base.

Addressed in 3/6?


>=20
> Tom.
>=20
>> +		seg->mr_len =3D vec->iov_len;
>>  		++seg;
>>  		++(*n);
>> -		page_offset =3D 0;
>>  	}
>>  	return seg;
>>  }

--
Chuck Lever