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>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH v3 6/6] NFSD: Repeal and replace the READ_PLUS
 implementation
Thread-Topic: [PATCH v3 6/6] NFSD: Repeal and replace the READ_PLUS
 implementation
Thread-Index: AQHYmHrzYsqP79DW8UWdtY79IK5gxq1/y34AgAOLZACAAqAVgA==
Date:   Tue, 19 Jul 2022 17:21:21 +0000
Message-ID: <5A400446-A6FD-436B-BDE2-DAD61239F98F@oracle.com>
References: <20220715184433.838521-1-anna@kernel.org>
 <20220715184433.838521-7-anna@kernel.org>
 <EC97C20D-A317-49F9-8280-062D1AAEE49A@oracle.com>
 <20220718011552.GK3600936@dread.disaster.area>
In-Reply-To: <20220718011552.GK3600936@dread.disaster.area>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?JddBxqbPAQHtFKH8ZA1q6weSaClQfEIZDbKnIxb5ECl3V9xaW/ylkzYuZCC9?=
 =?us-ascii?Q?msm8GzHpqyOUk5XhvIgMcmKkZ15s8hIqVR2sEVSBVXKydrKEG17y7ddlOeEL?=
 =?us-ascii?Q?/4h9gq7oQwwfVriqwOCjR5nnSOQbcZOo3qmKQnPAMydyrFqbZx54qNxs/SLL?=
 =?us-ascii?Q?cpAHH1v/JfZYOv+72j/QjNgbU9J/WgOFN6YKY+KQDl/G7UGIRjZ1LaOptTOw?=
 =?us-ascii?Q?phQqrHbApZPE8HJbn2DgbFtancM4CtnKXCjNOu3tz7fR8VA+/DZMLCuqyf9G?=
 =?us-ascii?Q?VSeRdmcRu/GPj5t9KfGck0RX/sPQWuDmjWYg9VzFlC3X8drl+iCMfQktJeCi?=
 =?us-ascii?Q?7oGD+jIGbG2N/92noqww06SaZYTMSxyd4yDZkWwszxNigTGxzbsV56v1pXrQ?=
 =?us-ascii?Q?KwwbAHx1NQf6WfrnpOK1wtOUfgLiTAUADKeGiVBSM+Rfdmbo7+1qXXBxrjJc?=
 =?us-ascii?Q?kbPmQBLcVGRKwgJYYfMl0VTJK30kVbFmYCZF9veSmUGlNzjBOjQkHadRjEu2?=
 =?us-ascii?Q?jQLR2o42JOjFxP2UXYwGjWwiGHEWwQLwvajM5U0uY1Q2HSlEnVw9Xd1XOqQs?=
 =?us-ascii?Q?RBkbrnfLEbsuyEjmd70awAAjkmKNI+YhVACYrOKsWKCnPkQkO59Ni9zKz2T/?=
 =?us-ascii?Q?h3XbyHfmkUeOAHnzGDAWH0MsATyHt8+xY4l6sv+MRmrOPReYnNkdk3AA1FXJ?=
 =?us-ascii?Q?COrMNMTkQnaLfiQBHnpXnjf9jWzNsgbFkRy0PHMkkMD3Y7w3Bwje2/Slkbar?=
 =?us-ascii?Q?YM+FS00LjMdCHrULgoc1i1w2JAMg2sxzeel0Qg/0fZu5HlzXZnaCktnTcbDE?=
 =?us-ascii?Q?WMVH1OAWk5brl3SZNohpEY4XOWdYeEUEsVuDnbS2DF8prVDzZeouO4N6xEG9?=
 =?us-ascii?Q?iUBx06feufDEKFdgWPd+UmRXBzuwku3jQY3IDV0xDWXAVJbfsFRIIO8QOo4f?=
 =?us-ascii?Q?J4SRVtApUL4DcvQD1hYcR8IQ3U8JKq++36LDtQRMKK/iBJwhSuYh1gXHFt7F?=
 =?us-ascii?Q?vpTh5dajO14tCdSEcQeEozZWKng0UZEWBV/0m7cGxZApedoz963zkfJd+W5i?=
 =?us-ascii?Q?lpiGvy87/zB6TVBEBe9xsubWA7eEm4f/GBYc+K0Q26Z2x9DcgtGjPTU8QbOH?=
 =?us-ascii?Q?zzhp4FxtAr48zc7Q9BBUbJnJ94SjlfvX36tvgIjMLtZ5I07H84bG2cAL8ODD?=
 =?us-ascii?Q?Wq6dsVau3BWluVna7WYZJpybBcy5zpQPlBY0ct4JzakspT4IYGKFUrynHgDZ?=
 =?us-ascii?Q?vfQdqeQPt4VSOcTkZKgEPSyQZF0fsmuIcZSFpTK3hLH2PJlsNNBGqei3y5ON?=
 =?us-ascii?Q?2+tmgrGXtn720byRpMA4I86+vs76x5ZKbEN0uVt5qNPaaPJ/PuXpiTDqXcNL?=
 =?us-ascii?Q?z/hB/viapu/SiA4AHctzMTqh4gDxwDTVeFVpFKzFRc3xhPaSXkruUMoSBc/p?=
 =?us-ascii?Q?+xPfOf/An8CylS0Cb6JFGyK/gZE7/WFB1nFlseMSSTc/mm0LHhr+3YhYKmrG?=
 =?us-ascii?Q?3KgyIDFENuCX6ipQ9Dgf2vMJd/Sn7AF6PLPFu0VmVDfsBRk50nvqRemH2rB4?=
 =?us-ascii?Q?rFjpeVqUoeOTa6LzR/7eL4fyEMUYtXeiez9pqSUYXkMsdiizmTrFfqVtzCJw?=
 =?us-ascii?Q?4g=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <797188907B156E4D82A33F9FD7DC435F@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: 93018004-fbde-4dc7-11a9-08da69ab19ba
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jul 2022 17:21:21.8138
 (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: SYcLexYhG6pcQDHQF8vWEwN16MxE15e4k29PqNk5XxXMq3/rClP1kdYd7YFukoAU/OZ2WXYu3ynb/C1YzAFhgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR10MB5427
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-19_05,2022-07-19_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 adultscore=0 phishscore=0
 mlxlogscore=999 spamscore=0 malwarescore=0 bulkscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207190073
X-Proofpoint-ORIG-GUID: svGtsTBVdlo0Q2ec8WMNgdvoM3ydpPL3
X-Proofpoint-GUID: svGtsTBVdlo0Q2ec8WMNgdvoM3ydpPL3
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 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


> On Jul 17, 2022, at 9:15 PM, Dave Chinner <david@fromorbit.com> wrote:
>=20
> On Fri, Jul 15, 2022 at 07:08:13PM +0000, Chuck Lever III wrote:
>>> On Jul 15, 2022, at 2:44 PM, Anna Schumaker <anna@kernel.org> wrote:
>>>=20
>>> +nfsd4_encode_read_plus_segments(struct nfsd4_compoundres *resp,
>>> +				struct nfsd4_read *read,
>>> +				unsigned int *segments, 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;
>>> +	struct xdr_stream *xdr =3D resp->xdr;
>>> +	unsigned int bufpos =3D xdr->buf->len;
>>> +	u64 offset =3D read->rd_offset;
>>> +	struct read_plus_segment segment;
>>> +	enum data_content4 pagetype;
>>> +	unsigned long maxcount;
>>> +	unsigned int pagenum =3D 0;
>>> +	unsigned int pagelen;
>>> +	char *vpage, *p;
>>> +	__be32 nfserr;
>>>=20
>>> -	if (data_pos =3D=3D -ENXIO)
>>> -		data_pos =3D f_size;
>>> -	else if (data_pos <=3D read->rd_offset || (data_pos < f_size && data_=
pos % PAGE_SIZE))
>>> -		return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_siz=
e);
>>> -	count =3D data_pos - read->rd_offset;
>>> -
>>> -	/* Content type, offset, byte count */
>>> -	p =3D xdr_reserve_space(resp->xdr, 4 + 8 + 8);
>>> -	if (!p)
>>> +	/* enough space for a HOLE segment before we switch to the pages */
>>> +	if (!xdr_reserve_space(xdr, 5 * XDR_UNIT))
>>> 		return nfserr_resource;
>>> +	xdr_commit_encode(xdr);
>>>=20
>>> -	*p++ =3D htonl(NFS4_CONTENT_HOLE);
>>> -	p =3D xdr_encode_hyper(p, read->rd_offset);
>>> -	p =3D xdr_encode_hyper(p, count);
>>> +	maxcount =3D min_t(unsigned long, read->rd_length,
>>> +			 (xdr->buf->buflen - xdr->buf->len));
>>>=20
>>> -	*eof =3D (read->rd_offset + count) >=3D f_size;
>>> -	*maxcount =3D min_t(unsigned long, count, *maxcount);
>>> +	nfserr =3D nfsd4_read_plus_readv(resp, read, &maxcount, eof);
>>> +	if (nfserr)
>>> +		return nfserr;
>>> +
>>> +	while (maxcount > 0) {
>>> +		vpage =3D xdr_buf_nth_page_address(xdr->buf, pagenum, &pagelen);
>>> +		pagelen =3D min_t(unsigned int, pagelen, maxcount);
>>> +		if (!vpage || pagelen =3D=3D 0)
>>> +			break;
>>> +		p =3D memchr_inv(vpage, 0, pagelen);
>>=20
>> I'm still not happy about touching every byte in each READ_PLUS
>> payload. I think even though the rest of this work is merge-ready,
>> this is a brute-force mechanism that's OK for a proof of concept
>> but not appropriate for production-ready code.
>=20
> Seems like a step backwards as it defeats the benefit zero-copy read
> IO paths on the server side....

Tom Haynes' vision for READ_PLUS was to eventually replace the
legacy READ operation. That means READ_PLUS(CONTENT_DATA) needs
to be as fast and efficient as plain READ. (It would be great
to use splice reads for CONTENT_DATA if we can!)

But I also thought the purpose of READ_PLUS was to help clients
preserve unallocated extents in files during copy operations.
An unallocated extent is not the same as an allocated extent
that has zeroes written into it. IIUC this new logic does not
distinguish between those two cases at all. (And please correct
me if this is really not the goal of READ_PLUS).

I would like to retain precise detection of unallocated extents
in files. Maybe SEEK_HOLE/SEEK_DATA is not how to do that, but
my feeling is that checking for zero bytes is definitely not
the way to do it.


>> I've cc'd linux-fsdevel to see if we can get some more ideas going
>> and move this forward.
>>=20
>> Another thought I had was to support returning only one or two
>> segments per reply. One CONTENT segment, one HOLE segment, or one
>> of each. Would that be enough to prevent the issues around file
>> updates racing with the construction of the reply?
>=20
> Before I can make any sort of useful suggestion, I need to have it
> explained to me why we care if the underlying file mapping has
> changed between the read of the data and the SEEK_HOLE trim check,
> because it's not at all clear to me what problem this change is
> actually solving.

The cover letter for this series calls out generic/091 and
generic/263 -- it mentioned both are failing with NFSv4.2. I've
tried to reproduce failures here, but both passed.

Anna, can you provide a root-cause analysis of what is failing
in your testing? Maybe a reproducer for us to kick around?
I'm guessing you might be encountering races because your
usual test environment is virtual, so we need to understand
how timing effects the results.


--
Chuck Lever