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 III <chuck.lever@oracle.com>
To:     Dave Wysochanski <dwysocha@redhat.com>
CC:     Trond Myklebust <trondmy@hammerspace.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Matthew Wilcox <willy@infradead.org>,
        David Howells <dhowells@redhat.com>,
        "linux-cachefs@redhat.com" <linux-cachefs@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] NFS: Convert from readpages() to readahead()
Thread-Topic: [PATCH 1/1] NFS: Convert from readpages() to readahead()
Thread-Index: AQHXu9PCTxWz7e0uNku4Se2Jl0pkd6vJJJuA
Date:   Fri, 8 Oct 2021 14:09:40 +0000
Message-ID: <3F1E7B93-EB8D-4744-8143-D44654CA6451@oracle.com>
References: <1633649528-1321-1-git-send-email-dwysocha@redhat.com>
 <1633649528-1321-2-git-send-email-dwysocha@redhat.com>
In-Reply-To: <1633649528-1321-2-git-send-email-dwysocha@redhat.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?U7CYwaRphin7MIP1gGG0fT5dSxc8OF9AcNj6hGXLKMWaUfPACeMOGUQLdnun?=
 =?us-ascii?Q?phR1+uPLr3duMa+yBDvoOxfsCQ0e1QDPrd4ZuQz56M8qJ7/r1i8ZAySsNDO0?=
 =?us-ascii?Q?FYxhg+l70cYZYjwzYtjX69BYiL8u0q/i0mj9IkgbalcajRsOIM0TojTzBilv?=
 =?us-ascii?Q?D4KXI+66Bn3iVwzO8GhAVesd7EBqmY+eB0nctXVLtGgVsQ6IPX0XFutF4UMc?=
 =?us-ascii?Q?L51BuWMGS1ko1DydVkM1+02PwB1FPmZhF5xPJnW6+jP/7PpmpeTz2pt4BYAz?=
 =?us-ascii?Q?gh2oD9Ke9Ald/FNU4C2vhIxidivgo9oz4GCnuHRKK7aRQPZjuocyYUlU0gLD?=
 =?us-ascii?Q?oPSfQD/c9zUFDchZf9zXQbxqLl0uhb/CDBXTm3hkDWnjgl4qBduNsB+X2n//?=
 =?us-ascii?Q?GLMT0KV+p0jQyy6+eKtfvBHVLAbCyIHuhiJcwFt+fhgLwvSKEZYlZVdgBs9a?=
 =?us-ascii?Q?0vUvtcxKHalAzl5FOPmVrIH2j4UMexAgZYu2sPFATSc0T59vUIxCrqBRcIxq?=
 =?us-ascii?Q?X7+zDGAxuUJGkbKVlYqZBL4donzGfJ8TXT8D4e+bHIDhJ568RITqw3p+mDqH?=
 =?us-ascii?Q?Qf6dh4IbI3BhXg5DUbEfDlOCTUwcpYynwKb4pX1GCvrbKuXXNS0fcAsg1mXO?=
 =?us-ascii?Q?HI+FtdGulNtISPN2ouvm3KKTWVzRx95ecwBryUth+ifiL2inwffQ+6bTaWtT?=
 =?us-ascii?Q?qWMeMRrkdEbVmjJw0PQ5DGNZ6phVg2P+82irgS9uj72AhwQ3BIHoDP0tsp6M?=
 =?us-ascii?Q?GUl0mT4fAgoIbe0nWDAs0zk/6z0NseRMgRO6MGRK20jCMy0s1UByqxIs0QLk?=
 =?us-ascii?Q?BhA8OOnN7eUUFVHkdPoRQb+a244gmV0wWPGq/cZU+Hk75LxSZt9Fla29prYd?=
 =?us-ascii?Q?lZuRD8v1DW+uNb6zyzjJTwsRV5md6eXcgCiOg7zebVogbxg0XyRGeP7/PIEh?=
 =?us-ascii?Q?n+OQqDRaafETQxViumtP2umq54Wv6d3SJcnedJdKRz/45z5jxsssErs58RKz?=
 =?us-ascii?Q?7dXivuPJg3JvNaNujImgujJ/f5gZtmBSbdRZfU5VqJZPd87YTVXdWtMbqwbA?=
 =?us-ascii?Q?CeSzk69uCXUdchGuO4LqKwFSPVEE3ubJdk6kwnaRiPMDpTaqi/39mL8A8R/b?=
 =?us-ascii?Q?A0+HOuC/aAoYRguOvRhCyrq7uK79Q2x804JskU/zd7Y9TgDA6JeghbP7FjHU?=
 =?us-ascii?Q?pHK73zU8cKB3cfSSlZ9PMcppYbsV2qtK2zHpY8RlIu+Z9DPhs+XLgLUVroVD?=
 =?us-ascii?Q?V/0cRWpgPi5Fct/oDo9yI5tp18HZVKuVHAwwTrSdmj3Z7k2u2XdWBrYivzyj?=
 =?us-ascii?Q?KiJ7HR743gzuJ3nzFE/SxTLd?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <76B9179E6D11D244AAFF5891AB08D910@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: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c95f3064-3907-4150-a026-08d98a654557
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Oct 2021 14:09:40.8579
 (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: gq3bdS1t/n5Xfi4VVL3HQsrrs06w6IjBBiqtOsz8qtKm7p1ixcC/aYS/DBezWynNh0VlfyWvi6oAYNk/L+U9wg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4147
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10130 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0
 phishscore=0 malwarescore=0 bulkscore=0 mlxlogscore=999 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109230001 definitions=main-2110080085
X-Proofpoint-GUID: yC7vW5EJfEGDGEbFjHSa4whIBVoA-awy
X-Proofpoint-ORIG-GUID: yC7vW5EJfEGDGEbFjHSa4whIBVoA-awy
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Oct 7, 2021, at 7:32 PM, Dave Wysochanski <dwysocha@redhat.com> wrote:
>=20
> Convert to the new VM readahead() API which is the preferred API
> to read multiple pages, and rename the NFSIOS_* counters and the
> tracepoint as needed.
>=20
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
> fs/nfs/file.c              |  2 +-
> fs/nfs/nfstrace.h          |  2 +-
> fs/nfs/read.c              | 21 +++++++++++++++------
> include/linux/nfs_fs.h     |  3 +--
> include/linux/nfs_iostat.h |  6 +++---
> 5 files changed, 21 insertions(+), 13 deletions(-)
>=20
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 209dac208477..cc76d17fa97f 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -519,7 +519,7 @@ static void nfs_swap_deactivate(struct file *file)
>=20
> const struct address_space_operations nfs_file_aops =3D {
> 	.readpage =3D nfs_readpage,
> -	.readpages =3D nfs_readpages,
> +	.readahead =3D nfs_readahead,
> 	.set_page_dirty =3D __set_page_dirty_nobuffers,
> 	.writepage =3D nfs_writepage,
> 	.writepages =3D nfs_writepages,
> diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
> index 78b0f649dd09..d2b2080765a6 100644
> --- a/fs/nfs/nfstrace.h
> +++ b/fs/nfs/nfstrace.h
> @@ -915,7 +915,7 @@
> 		)
> );
>=20
> -TRACE_EVENT(nfs_aops_readpages,
> +TRACE_EVENT(nfs_aops_readahead,

In v2 and v3 of my patch, this tracepoint has already been
renamed to nfs_aop_readahead.


> 		TP_PROTO(
> 			const struct inode *inode,
> 			unsigned int nr_pages
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index 927504605e0f..5c2aab47cf1d 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -395,15 +395,19 @@ int nfs_readpage(struct file *file, struct page *pa=
ge)
> 	return ret;
> }
>=20
> -int nfs_readpages(struct file *file, struct address_space *mapping,
> -		struct list_head *pages, unsigned nr_pages)
> +void nfs_readahead(struct readahead_control *ractl)
> {
> +	struct file *file =3D ractl->file;
> +	struct address_space *mapping =3D ractl->mapping;
> +	struct page *page;
> +	unsigned int nr_pages =3D readahead_count(ractl);
> +
> 	struct nfs_readdesc desc;
> 	struct inode *inode =3D mapping->host;
> 	int ret;
>=20
> -	trace_nfs_aops_readpages(inode, nr_pages);
> -	nfs_inc_stats(inode, NFSIOS_VFSREADPAGES);
> +	trace_nfs_aops_readahead(inode, nr_pages);
> +	nfs_inc_stats(inode, NFSIOS_VFSREADAHEAD);
>=20
> 	ret =3D -ESTALE;
> 	if (NFS_STALE(inode))
> @@ -420,13 +424,18 @@ int nfs_readpages(struct file *file, struct address=
_space *mapping,
> 	nfs_pageio_init_read(&desc.pgio, inode, false,
> 			     &nfs_async_read_completion_ops);
>=20
> -	ret =3D read_cache_pages(mapping, pages, readpage_async_filler, &desc);
> +	ret =3D 0;
> +	while (!ret && (page =3D readahead_page(ractl))) {
> +		prefetchw(&page->flags);
> +		ret =3D readpage_async_filler(&desc, page);
> +		put_page(page);
> +	}
>=20
> 	nfs_pageio_complete_read(&desc.pgio);
>=20
> 	put_nfs_open_context(desc.ctx);
> out:
> -	return ret;
> +	return;
> }
>=20
> int __init nfs_init_readpagecache(void)
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index b9a8b925db43..6cbe3f2c5669 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -580,8 +580,7 @@ extern int nfs_access_get_cached(struct inode *inode,=
 const struct cred *cred, s
>  * linux/fs/nfs/read.c
>  */
> extern int  nfs_readpage(struct file *, struct page *);
> -extern int  nfs_readpages(struct file *, struct address_space *,
> -		struct list_head *, unsigned);
> +extern void nfs_readahead(struct readahead_control *);
>=20
> /*
>  * inline functions
> diff --git a/include/linux/nfs_iostat.h b/include/linux/nfs_iostat.h
> index 027874c36c88..418145f23700 100644
> --- a/include/linux/nfs_iostat.h
> +++ b/include/linux/nfs_iostat.h
> @@ -22,7 +22,7 @@
> #ifndef _LINUX_NFS_IOSTAT
> #define _LINUX_NFS_IOSTAT
>=20
> -#define NFS_IOSTAT_VERS		"1.1"
> +#define NFS_IOSTAT_VERS		"1.2"
>=20
> /*
>  * NFS byte counters
> @@ -53,7 +53,7 @@
>  * NFS page counters
>  *
>  * These count the number of pages read or written via nfs_readpage(),
> - * nfs_readpages(), or their write equivalents.
> + * nfs_readahead(), or their write equivalents.
>  *
>  * NB: When adding new byte counters, please include the measured
>  * units in the name of each byte counter to help users of this
> @@ -98,7 +98,7 @@ enum nfs_stat_eventcounters {
> 	NFSIOS_VFSACCESS,
> 	NFSIOS_VFSUPDATEPAGE,
> 	NFSIOS_VFSREADPAGE,
> -	NFSIOS_VFSREADPAGES,
> +	NFSIOS_VFSREADAHEAD,

I'm wondering if you should add NFSIOS_VFSREADAHEAD
but leave NFSIOS_VFSREADPAGES? I don't remember exactly
how the mountstats API versioning is supposed to work.


> 	NFSIOS_VFSWRITEPAGE,
> 	NFSIOS_VFSWRITEPAGES,
> 	NFSIOS_VFSGETDENTS,
> --=20
> 1.8.3.1
>=20

--
Chuck Lever