Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Anna Schumaker <anna@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 1/1] NFSD: Simplify READ_PLUS
Thread-Topic: [PATCH v2 1/1] NFSD: Simplify READ_PLUS
Thread-Index: AQHYwvN0H50fO0bVeUq5hyL/2HYwe63UazcA
Date:   Wed, 7 Sep 2022 20:29:43 +0000
Message-ID: <06EA863F-8C11-4342-8D88-954E99A07598@oracle.com>
References: <20220907195259.926736-1-anna@kernel.org>
 <20220907195259.926736-2-anna@kernel.org>
In-Reply-To: <20220907195259.926736-2-anna@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?w/8YfhaD1UvqUMEoBxTvNRwVJ8NmgKMlRPE2oiIAormihq8Yn6B2cocCIq90?=
 =?us-ascii?Q?bfilJZfIs9BfOB3/7AMP6x7uz1ueyBJiSTPenRxY7YQ0i5CJ1wtoqyKTWPBq?=
 =?us-ascii?Q?Cjzx15ftEdBuXjdWndjq+Z5VnSxx448Ep8JzadjLX3V4H9Ydx2ZC8HCHLM9j?=
 =?us-ascii?Q?TjFFhWoEgC0ZoyvwGVD9N+C2jnZmC2zZ8kuod7QjfEgJCuMv2lXaAyUl+U0k?=
 =?us-ascii?Q?C4pEg0ZxZO7zzvXsLy2q9PDpNHIsqCPTrQLpaD7aQC8h4EM02h1TUaaRdEHd?=
 =?us-ascii?Q?SbMA71ECqpduyk1pKUXPvlyO6TnarJhbCDEHMe/Gw17+xFEnCqa1171ITDLi?=
 =?us-ascii?Q?XH+r/O3KX6poYiJe0HYVUIEMeK7FZVwWxZJAG1jYtIPjcSgB19E6+LEvRZk9?=
 =?us-ascii?Q?SpkQGNHyObyEfEiGXj5CRwFl4DH1zIa+OMGAYR+F9YM2RzgQBclNY3L74I+2?=
 =?us-ascii?Q?Xc5x8b91EFHxbEWDGQ5NGBRdteBFnkMdDoaCzUNiZgapnO4HkVbtkcS8YJcZ?=
 =?us-ascii?Q?xxIAN19Pqtv2l6fbt9kk043k1i6oh03rXVLxHngKeTIzX802EIddgi7meSAh?=
 =?us-ascii?Q?jqkD1mjC6W1OYeGQxpnB0dy6ARU8491EFgYgvIDUaNnHeueFn0tEpzsltgIm?=
 =?us-ascii?Q?Kr2X2idnYHEH+nz7W+eu9LAMMJL3Wy1mjf1T9B7a4S/Y7mtjhLzBrP5f8xDR?=
 =?us-ascii?Q?tmOjCvzIiKsm57sR891R58Q41ubt0VvjS5+TRAmVKRdLCY0GHPYr9uome+Qc?=
 =?us-ascii?Q?glA/GB3DtEfKwMz6MErMHlQG5X9qCeksd3SWM0g4E/T41Befi/Hiby8NljM6?=
 =?us-ascii?Q?k1piBZ49tMJm7V28Ov7Oo2zZSjmVsIbLsAvqZIXUAhRtXuSIpgqLKW4EWL4k?=
 =?us-ascii?Q?BNo67gUwM8BIbPWEG/WDiotzFPlNrMIg0UjzWqavbHGdw+Zvvkm6tBvkwfDI?=
 =?us-ascii?Q?4UQvoSR81/TEDzsXA2kz0gYXyGi8zhmAMlJvMy0gHput6s/BSQCI3IeKvYCU?=
 =?us-ascii?Q?oBi60gEHQLl3rravsmytZ6MckfEyUrWb6Opvh6TAeSIKTVx5ZtT+sXGHyGjg?=
 =?us-ascii?Q?Mo0YRjJLy4xLX+Oed8pqF0/zUdq5MKU08GVQvHHsBG7XP2g/JDLPs+vaHBFm?=
 =?us-ascii?Q?n3hzrmv+iYUXK2srKi9keBL7tDZT+jOZrIWIheEUlcf/X8XP2dFuaPbqY24m?=
 =?us-ascii?Q?Lkavof1c5xS3V7+kSb71ImVTvS1NBEefWFSIVnPNMp67zTV4zP569XZjbnt+?=
 =?us-ascii?Q?NbG3qgdREnHMhAbOE2OvWnoNrsuQUTklRWai6/90aayd98d8xP8/y3lUVXrE?=
 =?us-ascii?Q?bet/yfUYoXOwHTwSMu3/vn/3bK46Bgcj/FBkr5p9d4vSlPbZ4MgSA7Y5rQnI?=
 =?us-ascii?Q?LtW0Fs68ravR8cEhDDW9pj9QZT7PHWSyKfhx0EG6QvUJr7GDbk+RUxDG6GB+?=
 =?us-ascii?Q?4PDYXOPCIRW0x8PvCLZDnt6iTgT5bMO2FPi7drcO0FlvjwqqgZMZC7eqEcqo?=
 =?us-ascii?Q?ykiPHloYFrvGMuUu4ei90DhPTX/w20UvbjEwr4JoHi/fNLQnm7+nEKUZ+vnv?=
 =?us-ascii?Q?aZ3SF0KaTWWNt8sx6aKBk3ruBcMGivTRRncvN+dE4arWSngG0CyP/ulI8iAi?=
 =?us-ascii?Q?uw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <114E4DB9B931944899AF85FD0DB57CDC@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: faf6020e-7d2f-4eb3-2d22-08da910fb298
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Sep 2022 20:29:43.3044
 (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: hH7Lrl/ikAxOMvN8SGz3UirTFIruK2+uWYjIthgceNdcUTd3/mecGMFG3fUpgNk2+0u93OZVNi2l51/h3SMrTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB4983
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-07_10,2022-09-07_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 spamscore=0
 mlxlogscore=999 mlxscore=0 bulkscore=0 malwarescore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000
 definitions=main-2209070076
X-Proofpoint-ORIG-GUID: blTUJ9GvGgAFsdJGqlz9Q7V5YFPsoQWu
X-Proofpoint-GUID: blTUJ9GvGgAFsdJGqlz9Q7V5YFPsoQWu
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

Be sure to Cc: Jeff on these. Thanks!


> On Sep 7, 2022, at 3:52 PM, Anna Schumaker <anna@kernel.org> wrote:
>=20
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>=20
> Chuck had suggested reverting READ_PLUS so it returns a single DATA
> segment covering the requested read range. This prepares the server for
> a future "sparse read" function so support can easily be added without
> needing to rip out the old READ_PLUS code at the same time.
>=20
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> fs/nfsd/nfs4xdr.c | 139 +++++++++++-----------------------------------
> 1 file changed, 32 insertions(+), 107 deletions(-)
>=20
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 1e9690a061ec..bcc8c385faf2 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4731,79 +4731,37 @@ nfsd4_encode_offload_status(struct nfsd4_compound=
res *resp, __be32 nfserr,
>=20
> static __be32
> nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp,
> -			    struct nfsd4_read *read,
> -			    unsigned long *maxcount, u32 *eof,
> -			    loff_t *pos)
> +			    struct nfsd4_read *read)
> {
> -	struct xdr_stream *xdr =3D resp->xdr;
> +	bool splice_ok =3D test_bit(RQ_SPLICE_OK, &resp->rqstp->rq_flags);
> 	struct file *file =3D read->rd_nf->nf_file;
> -	int starting_len =3D xdr->buf->len;
> -	loff_t hole_pos;
> -	__be32 nfserr;
> -	__be32 *p, tmp;
> -	__be64 tmp64;
> -
> -	hole_pos =3D pos ? *pos : vfs_llseek(file, read->rd_offset, SEEK_HOLE);
> -	if (hole_pos > read->rd_offset)
> -		*maxcount =3D min_t(unsigned long, *maxcount, hole_pos - read->rd_offs=
et);
> -	*maxcount =3D min_t(unsigned long, *maxcount, (xdr->buf->buflen - xdr->=
buf->len));
> +	struct xdr_stream *xdr =3D resp->xdr;
> +	unsigned long maxcount;
> +	__be32 nfserr, *p;
>=20
> 	/* Content type, offset, byte count */
> 	p =3D xdr_reserve_space(xdr, 4 + 8 + 4);
> 	if (!p)
> -		return nfserr_resource;
> +		return nfserr_io;

Wouldn't nfserr_rep_too_big be a more appropriate status for running
off the end of the send buffer? I'm not 100% sure, but I would expect
that exhausting send buffer space would imply the reply has grown too
large.


> +	if (resp->xdr->buf->page_len && splice_ok) {
> +		WARN_ON_ONCE(splice_ok);
> +		return nfserr_io;
> +	}

I wish I understood why this test was needed. It seems to have been
copied and pasted from historic code into nfsd4_encode_read(), and
there have been recent mechanical changes to it, but there's no
comment explaining it there...

In any event, this seems to be checking for a server software bug,
so maybe this should return nfserr_serverfault. Oddly that status
code isn't defined yet.


Do you have some performance results for v2?


> -	read->rd_vlen =3D xdr_reserve_space_vec(xdr, resp->rqstp->rq_vec, *maxc=
ount);
> -	if (read->rd_vlen < 0)
> -		return nfserr_resource;
> +	maxcount =3D min_t(unsigned long, read->rd_length,
> +			 (xdr->buf->buflen - xdr->buf->len));
>=20
> -	nfserr =3D nfsd_readv(resp->rqstp, read->rd_fhp, file, read->rd_offset,
> -			    resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof);
> +	if (file->f_op->splice_read && splice_ok)
> +		nfserr =3D nfsd4_encode_splice_read(resp, read, file, maxcount);
> +	else
> +		nfserr =3D nfsd4_encode_readv(resp, read, file, maxcount);
> 	if (nfserr)
> 		return nfserr;
> -	xdr_truncate_encode(xdr, starting_len + 16 + xdr_align_size(*maxcount))=
;
>=20
> -	tmp =3D htonl(NFS4_CONTENT_DATA);
> -	write_bytes_to_xdr_buf(xdr->buf, starting_len,      &tmp,   4);
> -	tmp64 =3D cpu_to_be64(read->rd_offset);
> -	write_bytes_to_xdr_buf(xdr->buf, starting_len + 4,  &tmp64, 8);
> -	tmp =3D htonl(*maxcount);
> -	write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp,   4);
> -
> -	tmp =3D xdr_zero;
> -	write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 + *maxcount, &tmp,
> -			       xdr_pad_size(*maxcount));
> -	return nfs_ok;
> -}
> -
> -static __be32
> -nfsd4_encode_read_plus_hole(struct nfsd4_compoundres *resp,
> -			    struct nfsd4_read *read,
> -			    unsigned long *maxcount, u32 *eof)
> -{
> -	struct file *file =3D read->rd_nf->nf_file;
> -	loff_t data_pos =3D vfs_llseek(file, read->rd_offset, SEEK_DATA);
> -	loff_t f_size =3D i_size_read(file_inode(file));
> -	unsigned long count;
> -	__be32 *p;
> -
> -	if (data_pos =3D=3D -ENXIO)
> -		data_pos =3D f_size;
> -	else if (data_pos <=3D read->rd_offset || (data_pos < f_size && data_po=
s % PAGE_SIZE))
> -		return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_size)=
;
> -	count =3D data_pos - read->rd_offset;
> -
> -	/* Content type, offset, byte count */
> -	p =3D xdr_reserve_space(resp->xdr, 4 + 8 + 8);
> -	if (!p)
> -		return nfserr_resource;
> -
> -	*p++ =3D htonl(NFS4_CONTENT_HOLE);
> +	*p++ =3D cpu_to_be32(NFS4_CONTENT_DATA);
> 	p =3D xdr_encode_hyper(p, read->rd_offset);
> -	p =3D xdr_encode_hyper(p, count);
> +	*p =3D cpu_to_be32(read->rd_length);
>=20
> -	*eof =3D (read->rd_offset + count) >=3D f_size;
> -	*maxcount =3D min_t(unsigned long, count, *maxcount);
> 	return nfs_ok;
> }
>=20
> @@ -4811,69 +4769,36 @@ static __be32
> nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr,
> 		       struct nfsd4_read *read)
> {
> -	unsigned long maxcount, count;
> +	struct file *file =3D read->rd_nf->nf_file;
> 	struct xdr_stream *xdr =3D resp->xdr;
> -	struct file *file;
> 	int starting_len =3D xdr->buf->len;
> -	int last_segment =3D xdr->buf->len;
> -	int segments =3D 0;
> -	__be32 *p, tmp;
> -	bool is_data;
> -	loff_t pos;
> -	u32 eof;
> +	u32 segments =3D 0;
> +	__be32 *p;
>=20
> 	if (nfserr)
> 		return nfserr;
> -	file =3D read->rd_nf->nf_file;
>=20
> 	/* eof flag, segment count */
> 	p =3D xdr_reserve_space(xdr, 4 + 4);
> 	if (!p)
> -		return nfserr_resource;
> +		return nfserr_io;
> 	xdr_commit_encode(xdr);
>=20
> -	maxcount =3D min_t(unsigned long, read->rd_length,
> -			 (xdr->buf->buflen - xdr->buf->len));
> -	count    =3D maxcount;
> -
> -	eof =3D read->rd_offset >=3D i_size_read(file_inode(file));
> -	if (eof)
> +	read->rd_eof =3D read->rd_offset >=3D i_size_read(file_inode(file));
> +	if (read->rd_eof)
> 		goto out;
>=20
> -	pos =3D vfs_llseek(file, read->rd_offset, SEEK_HOLE);
> -	is_data =3D pos > read->rd_offset;
> -
> -	while (count > 0 && !eof) {
> -		maxcount =3D count;
> -		if (is_data)
> -			nfserr =3D nfsd4_encode_read_plus_data(resp, read, &maxcount, &eof,
> -						segments =3D=3D 0 ? &pos : NULL);
> -		else
> -			nfserr =3D nfsd4_encode_read_plus_hole(resp, read, &maxcount, &eof);
> -		if (nfserr)
> -			goto out;
> -		count -=3D maxcount;
> -		read->rd_offset +=3D maxcount;
> -		is_data =3D !is_data;
> -		last_segment =3D xdr->buf->len;
> -		segments++;
> -	}
> -
> -out:
> -	if (nfserr && segments =3D=3D 0)
> +	nfserr =3D nfsd4_encode_read_plus_data(resp, read);
> +	if (nfserr) {
> 		xdr_truncate_encode(xdr, starting_len);
> -	else {
> -		if (nfserr) {
> -			xdr_truncate_encode(xdr, last_segment);
> -			nfserr =3D nfs_ok;
> -			eof =3D 0;
> -		}
> -		tmp =3D htonl(eof);
> -		write_bytes_to_xdr_buf(xdr->buf, starting_len,     &tmp, 4);
> -		tmp =3D htonl(segments);
> -		write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4);
> +		return nfserr;
> 	}
>=20
> +	segments++;
> +
> +out:
> +	p =3D xdr_encode_bool(p, read->rd_eof);
> +	*p =3D cpu_to_be32(segments);
> 	return nfserr;
> }
>=20
> --=20
> 2.37.2
>=20

--
Chuck Lever