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:     Hugh Dickins <hughd@google.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Mark Hemment <markhemm@googlemail.com>,
        Patrice CHOTARD <patrice.chotard@foss.st.com>,
        Mikulas Patocka <mpatocka@redhat.com>,
        Lukas Czerner <lczerner@redhat.com>,
        Christoph Hellwig <hch@lst.de>,
        "Darrick J. Wong" <djwong@kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: Regression in xfstests on tmpfs-backed NFS exports
Thread-Topic: Regression in xfstests on tmpfs-backed NFS exports
Thread-Index: AQHYSdpNL2d3Xru4B0OzHZJ1LCngEazjlm2AgAE//ICAADMPAIABKSsA
Date:   Fri, 8 Apr 2022 16:10:33 +0000
Message-ID: <C7966059-D583-4B20-A838-067BAE86FB3E@oracle.com>
References: <673D708E-2DFA-4812-BB63-6A437E0C72EE@oracle.com>
 <11f319-c9a-4648-bfbb-dc5a83c774@google.com>
 <2B7AF707-67B1-4ED8-A29F-957C26B7F87A@oracle.com>
 <c5ea49a-1a76-8cf9-5c76-4bb31aa3d458@google.com>
In-Reply-To: <c5ea49a-1a76-8cf9-5c76-4bb31aa3d458@google.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?upXrGhe08YHTb8yov1RldEDL5EtZSeRbZiLu/es0UsovQ2rh72NlqpDVr3H3?=
 =?us-ascii?Q?UG6Z7F2MGBovYALbEQVxqF08v+aY7sJ9OJpb3ePJ5QOEBvmgN0T+a18mrRx+?=
 =?us-ascii?Q?3y1zIinObAwtHEXvnvhXFST0d9+IHeLoMSk32raIlka/SSMMZuHUdhUboJGf?=
 =?us-ascii?Q?dxa7nrs5qC+SJb1i0TDz7mnCWaLULZ1R2Em1LUeZ/6dUB2ZkGGXayW2rtly9?=
 =?us-ascii?Q?JCIINYRzmBm3nt+uQ3zpnwS2ywc3/ZDLxmt3UZ11H6OwjGGOqG+TkPOd375+?=
 =?us-ascii?Q?n7hsS81wD9tMUQcdCyhFqPQNWjMcRn9+4mRdEl3Xqt8KOH5X0s1OdzKoMiuw?=
 =?us-ascii?Q?JT65RvroisfdSZgnQ6vzHE+8l0RQY+Zs8tjJSdA1ML3HAI170olDPk/YqcFI?=
 =?us-ascii?Q?OFiK3wpFCUvqREIOxGcT8PwBCvICcEC++YURuYGTTJZFDHOajuZ9kjklnWDh?=
 =?us-ascii?Q?jQF/nxlXPrxDebYxstLwgnfpEp5YZKbNmCsYXtgUfIQMjTXhNu4iw+fGw+cc?=
 =?us-ascii?Q?hy+MLbzoR+qPmZQRiBEgREynJfVbMQJCLMUj6dd3RBtIuV3J+db6KPEj7wWv?=
 =?us-ascii?Q?7/3HzuSIpHZF0VO44MKvY7ElOMR2CA3cFHiKvLi+wB3ghJUopeakBSLh6n1t?=
 =?us-ascii?Q?OOPXC5vs9kBL/L1GlwVG5KDNiCrBCoAs2agCfIOWb1Knnn1MuoAe+mKk5c9j?=
 =?us-ascii?Q?6W7No40meS+gDshHFeJ+sQ7fb4PMVlWiG/5IZkwfMVWwSK2RRhIUbZUFyE4d?=
 =?us-ascii?Q?5bU8Jv2gsiC6iyhRkbPTWEePQF1t/qHr1mXZHgtqOc0PycxuJgGO7rjkZNQA?=
 =?us-ascii?Q?2ckWZG7SqxBpWCMNS1w80CELui9uEU2FBrYRSjvocBKPFX0WDYJnOm4MDCmH?=
 =?us-ascii?Q?Ke/N156aE00EhmHfuhyuVXyApnfwc+oTB196Hyy/fREYtGx8o+U5kV2nuG9P?=
 =?us-ascii?Q?bg9tvLDsGDl607gq6g1Q8CIowxSshVTLIZ03w20NgVpHm/WlifBU6VCC2jUh?=
 =?us-ascii?Q?2JTbKcipzqRnaNSbX6IrcF0Gv5Ybryg4ltj0Koq8kNtmKKjwrXPBPzQBwRvJ?=
 =?us-ascii?Q?KJKagLR8kzjuVZ7JvYqmGGHFgfZQF7m3JlqppEI+INDh2yokrpyFH0rrKvvS?=
 =?us-ascii?Q?StBBKkblnC7O+Neflgaig8vVT9Ze8VwCdmA7fwoQFMOBDLnO2j9UUkm29mev?=
 =?us-ascii?Q?X2uVKshjMapemvO48uymgpiKz5HgcrKI+mpuPtUdTOgNWo8YlOrQDhaqtwLu?=
 =?us-ascii?Q?PL5Zo3rrp7C4WXhLci6sZdKs4WdC/Z1XZ0LP5frfZR9WZi6nLWHd+CT7LIvl?=
 =?us-ascii?Q?oEIgvLMmrJU1wWELqWaBYsiO2tJOiRamcBIfpkWlqrqdciVvuXnOq3bL6uf9?=
 =?us-ascii?Q?cLbTdwNOIDkYv/inWHwwgYiJserqykaIDaTP/7AlaNsXuXmS8gp/uZhi9cCC?=
 =?us-ascii?Q?DqSs6PRV8AxgvEvYK8OcKJ8BTuYqFolIO3NseG8ztVlZQw7PuEpOkG92+KXQ?=
 =?us-ascii?Q?vTwqU70ybtAveGdjztYc8zntJ5l3g2R5DLElysGD8vPx8g2SwgA0fMn6QTRR?=
 =?us-ascii?Q?Y0NH1jMpJ3ODTfsNJMIbyLh91ZrPK3IYTM0h7fEE7vTI90tG9pZ7h0dXcHNw?=
 =?us-ascii?Q?2NjHHJHz8DuTT9RVbVJN8tETA+3WwG/vFUGicW2NoFUrBUEb9moBw8udJ2AQ?=
 =?us-ascii?Q?YF7GsnbZEod9emuyq90n5pZpGQqId/+BxrbjvY3j4lhicBXNus6CoCh65qeG?=
 =?us-ascii?Q?fPtLAWO9DXj3KeVHHWtMWkCpSUjS17Y=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6144219C30CA4543B7BD26DAA07DCCC9@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: 370684c0-299f-499e-5a26-08da197a4f5a
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2022 16:10:33.3591
 (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: 1KP/c8p9uVFd8ozNQ14ruTFd90jm5LPDXFSWi7BWdkZKc8vfBaKy6zEU/KWgFVc3wM8uHa1vixg4sc3dUdXfew==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2981
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.425,18.0.858
 definitions=2022-04-08_05:2022-04-08,2022-04-08 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0
 phishscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204080082
X-Proofpoint-GUID: IoYjSLULMwcU_fEgMwMybaJ79o6ISwQT
X-Proofpoint-ORIG-GUID: IoYjSLULMwcU_fEgMwMybaJ79o6ISwQT
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_H5,RCVD_IN_MSPIKE_WL,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


> On Apr 7, 2022, at 6:26 PM, Hugh Dickins <hughd@google.com> wrote:
>=20
> On Thu, 7 Apr 2022, Chuck Lever III wrote:
>>> On Apr 6, 2022, at 8:18 PM, Hugh Dickins <hughd@google.com> wrote:
>>>=20
>>> But I can sit here and try to guess.  I notice fs/nfsd checks
>>> file->f_op->splice_read, and employs fallback if not available:
>>> if you have time, please try rerunning those xfstests on an -rc1
>>> kernel, but with mm/shmem.c's .splice_read line commented out.
>>> My guess is that will then pass the tests, and we shall know more.
>>=20
>> This seemed like the most probative next step, so I commented
>> out the .splice_read call-out in mm/shmem.c and ran the tests
>> again. Yes, that change enables the fsx-related tests to pass
>> as expected.
>=20
> Great, thank you for trying that.
>=20
>>=20
>>> What could be going wrong there?  I've thought of two possibilities.
>>> A minor, hopefully easily fixed, issue would be if fs/nfsd has
>>> trouble with seeing the same page twice in a row: since tmpfs is
>>> now using the ZERO_PAGE(0) for all pages of a hole, and I think I
>>> caught sight of code which looks to see if the latest page is the
>>> same as the one before.  It's easy to imagine that might go wrong.
>>=20
>> Are you referring to this function in fs/nfsd/vfs.c ?
>=20
> I think that was it, didn't pay much attention.
>=20
>>=20
>> 847 static int
>> 848 nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *=
buf,
>> 849                   struct splice_desc *sd)
>> 850 {
>> 851         struct svc_rqst *rqstp =3D sd->u.data;
>> 852         struct page **pp =3D rqstp->rq_next_page;
>> 853         struct page *page =3D buf->page;
>> 854=20
>> 855         if (rqstp->rq_res.page_len =3D=3D 0) {
>> 856                 svc_rqst_replace_page(rqstp, page);
>> 857                 rqstp->rq_res.page_base =3D buf->offset;
>> 858         } else if (page !=3D pp[-1]) {
>> 859                 svc_rqst_replace_page(rqstp, page);
>> 860         }
>> 861         rqstp->rq_res.page_len +=3D sd->len;
>> 862=20
>> 863         return sd->len;
>> 864 }
>>=20
>> rq_next_page should point to the first unused element of
>> rqstp->rq_pages, so IIUC that check is looking for the
>> final page that is part of the READ payload.
>>=20
>> But that does suggest that if page -> ZERO_PAGE and so does
>> pp[-1], then svc_rqst_replace_page() would not be invoked.

To put a little more color on this, I think the idea here
is to prevent releasing the same page twice. It might be
possible that NFSD can add the same page to the rq_pages
array more than once, and we don't want to do a double
put_page().

The only time I can think this might happen is if the
READ payload is partially contained in the page that
contains the NFS header. I'm not sure that can ever
happen these days.


> I still haven't studied the logic there: Mark's input made it clear
> that it's just too risky for tmpfs to pass back ZERO_PAGE repeatedly,
> there could be expectations of uniqueness in other places too.

So far I haven't seen an issue with skb_can_coalesce().
I will keep an eye out for that.


>>> A more difficult issue would be, if fsx is racing writes and reads,
>>> in a way that it can guarantee the correct result, but that correct
>>> result is no longer delivered: because the writes go into freshly
>>> allocated tmpfs cache pages, while reads are still delivering
>>> stale ZERO_PAGEs from the pipe.  I'm hazy on the guarantees there.
>>>=20
>>> But unless someone has time to help out, we're heading for a revert.
>=20
> We might be able to avoid that revert, and go the whole way to using
> iov_iter_zero() instead.  But the significant slowness of clear_user()
> relative to copy to user, on x86 at least, does ask for a hybrid.
>=20
> Suggested patch below, on top of 5.18-rc1, passes my own testing:
> but will it pass yours?  It seems to me safe, and as fast as before,
> but we don't know yet if this iov_iter_zero() works right for you.
> Chuck, please give it a go and let us know.

Applied to stock v5.18-rc1. The tests pass as expected.


> (Don't forget to restore mm/shmem.c's .splice_read first!  And if
> this works, I can revert mm/filemap.c's SetPageUptodate(ZERO_PAGE(0))
> in the same patch, fixing the other regression, without recourse to
> #ifdefs or arch mods.)
>=20
> Thanks!
> Hugh
>=20
> --- 5.18-rc1/mm/shmem.c
> +++ linux/mm/shmem.c
> @@ -2513,7 +2513,6 @@ static ssize_t shmem_file_read_iter(struct kiocb *i=
ocb, struct iov_iter *to)
> 		pgoff_t end_index;
> 		unsigned long nr, ret;
> 		loff_t i_size =3D i_size_read(inode);
> -		bool got_page;
>=20
> 		end_index =3D i_size >> PAGE_SHIFT;
> 		if (index > end_index)
> @@ -2570,24 +2569,34 @@ static ssize_t shmem_file_read_iter(struct kiocb =
*iocb, struct iov_iter *to)
> 			 */
> 			if (!offset)
> 				mark_page_accessed(page);
> -			got_page =3D true;
> +			/*
> +			 * Ok, we have the page, and it's up-to-date, so
> +			 * now we can copy it to user space...
> +			 */
> +			ret =3D copy_page_to_iter(page, offset, nr, to);
> +			put_page(page);
> +
> +		} else if (iter_is_iovec(to)) {
> +			/*
> +			 * Copy to user tends to be so well optimized, but
> +			 * clear_user() not so much, that it is noticeably
> +			 * faster to copy the zero page instead of clearing.
> +			 */
> +			ret =3D copy_page_to_iter(ZERO_PAGE(0), offset, nr, to);
> 		} else {
> -			page =3D ZERO_PAGE(0);
> -			got_page =3D false;
> +			/*
> +			 * But submitting the same page twice in a row to
> +			 * splice() - or others? - can result in confusion:
> +			 * so don't attempt that optimization on pipes etc.
> +			 */
> +			ret =3D iov_iter_zero(nr, to);
> 		}
>=20
> -		/*
> -		 * Ok, we have the page, and it's up-to-date, so
> -		 * now we can copy it to user space...
> -		 */
> -		ret =3D copy_page_to_iter(page, offset, nr, to);
> 		retval +=3D ret;
> 		offset +=3D ret;
> 		index +=3D offset >> PAGE_SHIFT;
> 		offset &=3D ~PAGE_MASK;
>=20
> -		if (got_page)
> -			put_page(page);
> 		if (!iov_iter_count(to))
> 			break;
> 		if (ret < nr) {

--
Chuck Lever