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:     Neil Brown <neil@brown.name>
CC:     Mel Gorman <mgorman@techsingularity.net>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        "kuba@kernel.org" <kuba@kernel.org>
Subject: Re: [PATCH v2 4/4] SUNRPC: Cache pages that were replaced during a
 read splice
Thread-Topic: [PATCH v2 4/4] SUNRPC: Cache pages that were replaced during a
 read splice
Thread-Index: AQHXCS9m88O7Je1z8UCixcmIHApXT6pqDTIAgABzogA=
Date:   Fri, 26 Feb 2021 14:13:49 +0000
Message-ID: <3B2A403F-A64D-4148-B8B2-529F86E0309E@oracle.com>
References: <161400722731.195066.9584156841718557193.stgit@klimt.1015granger.net>
 <161400740732.195066.3792261943053910900.stgit@klimt.1015granger.net>
 <87k0qvi9cz.fsf@notabene.neil.brown.name>
In-Reply-To: <87k0qvi9cz.fsf@notabene.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30D05A887CE3C949A929BE88A8156FAF@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 597c0b7a-20ff-4432-f21a-08d8da60bced
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Feb 2021 14:13:49.3186
 (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: v94uRUdZR//C3t6PFOZ2FCLRO2FDWeJr5xYWqoTodypJ2CckSFJTk8xxPcQ1eil9Z57YtJbGF7Sj0jqJGbA3og==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2568
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9906 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0
 suspectscore=0 mlxlogscore=999 mlxscore=0 spamscore=0 bulkscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2102260110
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9906 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0
 priorityscore=1501 impostorscore=0 bulkscore=0 mlxscore=0 malwarescore=0
 clxscore=1011 phishscore=0 mlxlogscore=999 lowpriorityscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2102260111
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 26, 2021, at 2:19 AM, NeilBrown <neil@brown.name> wrote:
>=20
> On Mon, Feb 22 2021, Chuck Lever wrote:
>=20
>> To avoid extra trips to the page allocator, don't free unused pages
>> in nfsd_splice_actor(), but instead place them in a local cache.
>> That cache is then used first when refilling rq_pages.
>>=20
>> On workloads that perform large NFS READs on splice-capable file
>> systems, this saves a considerable amount of work.
>>=20
>> Suggested-by: NeilBrown <neilb@suse.de>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfsd/vfs.c                   |    4 ++--
>> include/linux/sunrpc/svc.h      |    1 +
>> include/linux/sunrpc/svc_xprt.h |   28 ++++++++++++++++++++++++++++
>> net/sunrpc/svc.c                |    7 +++++++
>> net/sunrpc/svc_xprt.c           |   12 ++++++++++++
>> 5 files changed, 50 insertions(+), 2 deletions(-)
>>=20
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index d316e11923c5..25cf41eaf3c4 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -852,14 +852,14 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, st=
ruct pipe_buffer *buf,
>>=20
>> 	if (rqstp->rq_res.page_len =3D=3D 0) {
>> 		get_page(page);
>> -		put_page(*rqstp->rq_next_page);
>> +		svc_rqst_put_unused_page(rqstp, *rqstp->rq_next_page);
>> 		*(rqstp->rq_next_page++) =3D page;
>> 		rqstp->rq_res.page_base =3D buf->offset;
>> 		rqstp->rq_res.page_len =3D size;
>> 	} else if (page !=3D pp[-1]) {
>> 		get_page(page);
>> 		if (*rqstp->rq_next_page)
>> -			put_page(*rqstp->rq_next_page);
>> +			svc_rqst_put_unused_page(rqstp, *rqstp->rq_next_page);
>> 		*(rqstp->rq_next_page++) =3D page;
>> 		rqstp->rq_res.page_len +=3D size;
>> 	} else
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index 31ee3b6047c3..340f4f3989c0 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -250,6 +250,7 @@ struct svc_rqst {
>> 	struct xdr_stream	rq_arg_stream;
>> 	struct page		*rq_scratch_page;
>> 	struct xdr_buf		rq_res;
>> +	struct list_head	rq_unused_pages;
>> 	struct page		*rq_pages[RPCSVC_MAXPAGES + 1];
>> 	struct page *		*rq_respages;	/* points into rq_pages */
>> 	struct page *		*rq_next_page; /* next reply page to use */
>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_=
xprt.h
>> index 571f605bc91e..49ef86499876 100644
>> --- a/include/linux/sunrpc/svc_xprt.h
>> +++ b/include/linux/sunrpc/svc_xprt.h
>> @@ -150,6 +150,34 @@ static inline void svc_xprt_get(struct svc_xprt *xp=
rt)
>> {
>> 	kref_get(&xprt->xpt_ref);
>> }
>> +
>> +/**
>> + * svc_rqst_get_unused_page - Tap a page from the local cache
>> + * @rqstp: svc_rqst with cached unused pages
>> + *
>> + * To save an allocator round trip, pages can be added to a
>> + * local cache and re-used later by svc_alloc_arg().
>> + *
>> + * Returns an unused page, or NULL if the cache is empty.
>> + */
>> +static inline struct page *svc_rqst_get_unused_page(struct svc_rqst *rq=
stp)
>> +{
>> +	return list_first_entry_or_null(&rqstp->rq_unused_pages,
>> +					struct page, lru);
>> +}
>> +
>> +/**
>> + * svc_rqst_put_unused_page - Stash a page in the local cache
>> + * @rqstp: svc_rqst with cached unused pages
>> + * @page: page to cache
>> + *
>> + */
>> +static inline void svc_rqst_put_unused_page(struct svc_rqst *rqstp,
>> +					    struct page *page)
>> +{
>> +	list_add(&page->lru, &rqstp->rq_unused_pages);
>> +}
>> +
>> static inline void svc_xprt_set_local(struct svc_xprt *xprt,
>> 				      const struct sockaddr *sa,
>> 				      const size_t salen)
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 61fb8a18552c..3920fa8f1146 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -570,6 +570,8 @@ svc_init_buffer(struct svc_rqst *rqstp, unsigned int=
 size, int node)
>> 	if (svc_is_backchannel(rqstp))
>> 		return 1;
>>=20
>> +	INIT_LIST_HEAD(&rqstp->rq_unused_pages);
>> +
>> 	pages =3D size / PAGE_SIZE + 1; /* extra page as we hold both request a=
nd reply.
>> 				       * We assume one is at most one page
>> 				       */
>> @@ -593,8 +595,13 @@ svc_init_buffer(struct svc_rqst *rqstp, unsigned in=
t size, int node)
>> static void
>> svc_release_buffer(struct svc_rqst *rqstp)
>> {
>> +	struct page *page;
>> 	unsigned int i;
>>=20
>> +	while ((page =3D svc_rqst_get_unused_page(rqstp))) {
>> +		list_del(&page->lru);
>> +		put_page(page);
>> +	}
>> 	for (i =3D 0; i < ARRAY_SIZE(rqstp->rq_pages); i++)
>> 		if (rqstp->rq_pages[i])
>> 			put_page(rqstp->rq_pages[i]);
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index 15aacfa5ca21..84210e546a66 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -678,6 +678,18 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
>> 	for (needed =3D 0, i =3D 0; i < pages ; i++)
>> 		if (!rqstp->rq_pages[i])
>> 			needed++;
>> +	if (needed) {
>> +		for (i =3D 0; i < pages; i++) {
>> +			if (!rqstp->rq_pages[i]) {
>> +				page =3D svc_rqst_get_unused_page(rqstp);
>> +				if (!page)
>> +					break;
>> +				list_del(&page->lru);
>> +				rqstp->rq_pages[i] =3D page;
>> +				needed--;
>> +			}
>> +		}
>> +	}
>> 	if (needed) {
>> 		LIST_HEAD(list);
>>=20
> This looks good!  Probably simpler than the way I imagined it :-)
> I would do that last bit of code differently though...
>=20
>  for (needed =3D 0, i =3D 0; i < pages ; i++)
>          if (!rqstp->rq_pages[i]) {
>                  page =3D svc_rqst_get_unused_pages(rqstp);
>                  if (page) {
>                          list_del(&page->lru);
>                          rqstp->rq_pages[i] =3D page;
>                  } else
>                          needed++;
>          }
>=20
> but it is really a minor style difference - I don't object to your
> version.

One trip through the array is better than two. I'll use yours.


> Reviewed-by: NeilBrown <neilb@suse.de>
>=20
> Thanks,
> NeilBrown

--
Chuck Lever