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:     Neil Brown <neilb@suse.de>
CC:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Mel Gorman <mgorman@suse.com>, Linux-MM <linux-mm@kvack.org>
Subject: Re: [PATCH] SUNRPC: use congestion_wait() in svc_alloc_args()
Thread-Topic: [PATCH] SUNRPC: use congestion_wait() in svc_alloc_args()
Thread-Index: AQHXotncpaMymYUO4UWd0a8FIk5gVauXJwiA
Date:   Mon, 6 Sep 2021 15:46:34 +0000
Message-ID: <848A6498-CFF3-4C66-AE83-959F8221E930@oracle.com>
References: <163090344807.19339.10071205771966144716@noble.neil.brown.name>
In-Reply-To: <163090344807.19339.10071205771966144716@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?hu1JpcRNQkujFWeDbHC4s0IW5J/Jq5MRMQbPFD+wvfy6xReQxQsPxv1bNT0k?=
 =?us-ascii?Q?pyr2+HDM1a5ZYYVunbWu0CWz7fhtF2rnEZab+F/5UtDdCXR5Banw8MsLItZQ?=
 =?us-ascii?Q?/RSwfJJ+Ze5VdosYWTbIAi5K7746ERqOO8oqbeLCvTUesZHte461UjggkdJo?=
 =?us-ascii?Q?QDcR2oq7XWmdmWURdIR5WbzF3Ip37p4Z6StAHQdXk1h5MSIwtw8jGWVfTfbj?=
 =?us-ascii?Q?KjC1+oh+wFfp45CfWw3c0brvVzNtjBG0ftxLofPHzVN2zMgzRdk+w5Tu0BGb?=
 =?us-ascii?Q?fEaMJOLIBAD7udtoTWzZeIz5gNBXfNprYn+Ds9EH1lYt71KbThKqrgEIvRJC?=
 =?us-ascii?Q?+l7brmaNFykqfiRcQRscMl+sJbD5wxFn2EtFEy+wPDe5rRtu5MBpsWPROb8t?=
 =?us-ascii?Q?v816YSR62wMJFcz6dLd6oiMOU4709tCtPIMjoAMA/HZNNvH4N/wFG6hgZjvc?=
 =?us-ascii?Q?0eTcEjqxOOPrrm3e2H6uMBFMeyrlEmNuHE9VOsqMx2AOdcBhlxT7FsQpu9IT?=
 =?us-ascii?Q?TIBe/uCyTnZzYrAYpR62gEIJ27AHbctg0vW7ky9m18klRA1lcobz+fwgpLM8?=
 =?us-ascii?Q?LaEruQ5bBOF0dzPQTm1lMDyLOalmCkNlPQhW7BVgCaz96o14AmK+xldJLsdN?=
 =?us-ascii?Q?3X4KpkKPhcCWuO6bzEhhNLKGUWiUYlvmuVsYmIHHR/ka1VvYHxACUf1WRkUU?=
 =?us-ascii?Q?4j4ymZrre0qrMa8gB03cC7+TPsq/AMkKalJgeA0nnBZ0wJO+mwiHE3Tb4iTR?=
 =?us-ascii?Q?bKp+aqZKFWkprERO+LNwIkbhAsIO3zLHGfTTB9TZYZXsiav8gXivfVB0WHRz?=
 =?us-ascii?Q?m82guOCuy4Z8DFqO53vvn7Lg4GTCzvbsOCWIinkaqZQx7TWPhv29wn9EvRZh?=
 =?us-ascii?Q?AENfKiNEClyBoZjenE86GvERk5vrN6SqVVKNwnf1Joa96l9pyvvAS82lzS/P?=
 =?us-ascii?Q?3L/ZVhcrwW4LjgdAtErZFoIXjvl4nnrXEXRMuGfejouEsWUqLNhZZyDsptpw?=
 =?us-ascii?Q?N3w8DEqE9bw1s4DjathjzZYI7QBibTUD63LeXBeyNpdeHfY5ymMz4HB3Yj8N?=
 =?us-ascii?Q?KCiX35pA5ZIws6IcCZdwRUP8AZQF7MTkB7Csy+p28OEU8dLRpRgUm8A8+9VJ?=
 =?us-ascii?Q?gkSCiyX53UW8NfEU6Crxn+EU+4C4/WiBq+V/Q6pm0MGwgvRTTR2Ndfzm1dsF?=
 =?us-ascii?Q?4bvrEyHGY5Uh1loh+9+qOrEiwxbr6xn+6492Vs/uJJJgBHXX5gxStBTjKMgi?=
 =?us-ascii?Q?wBJtwsy5f6dEV386v1c8NlNh5Y0XM11uwF7t5IXWxJ97suH2wlSe1VTHpAr6?=
 =?us-ascii?Q?o9Z0RQREEFP1+p5kbxvtMuxI?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <AE64E98148E20545BB8596662EF7D998@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: 102c4454-9120-4391-cd2e-08d9714d811c
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Sep 2021 15:46:34.1398
 (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: GbdGmZ8qTQ+vlljhI5bCYv1HA9PmOBJ4ezvZQSXO4MygBpnEhJ5GuZ5sG4fGJSB8DEfE/8Te1uSauQl4TqT0ig==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB3825
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10099 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 spamscore=0 mlxlogscore=999
 adultscore=0 bulkscore=0 suspectscore=0 malwarescore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2108310000
 definitions=main-2109060100
X-Proofpoint-GUID: wxzPMwDnDaZQCPMPxImD0K5ftr-V2oCn
X-Proofpoint-ORIG-GUID: wxzPMwDnDaZQCPMPxImD0K5ftr-V2oCn
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

Hi Neil-

> On Sep 6, 2021, at 12:44 AM, NeilBrown <neilb@suse.de> wrote:
>=20
>=20
> Many places that need to wait before retrying a memory allocation use
> congestion_wait().  xfs_buf_alloc_pages() is a good example which
> follows a similar pattern to that in svc_alloc_args().
>=20
> It make sense to do the same thing in svc_alloc_args(); This will allow
> the allocation to be retried sooner if some backing device becomes
> non-congested before the timeout.
>=20
> Every call to congestion_wait() in the entire kernel passes BLK_RW_ASYNC
> as the first argument, so we should so.
>=20
> The second argument - an upper limit for waiting - seem fairly
> arbitrary.  Many places use "HZ/50" or "HZ/10".  As there is no obvious
> choice, it seems reasonable to leave the maximum time unchanged.
>=20
> If a service using svc_alloc_args() is terminated, it may now have to
> wait up to the full 500ms before termination completes as
> congestion_wait() cannot be interrupted.  I don't believe this will be a
> problem in practice, though it might be justification for using a
> smaller timeout.
>=20
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>=20
> I happened to notice this inconsistency between svc_alloc_args() and
> xfs_buf_alloc_pages() despite them doing very similar things, so thought
> I'd send a patch.
>=20
> NeilBrown

When we first considered the alloc_pages_bulk() API, the SUNRPC
patch in that series replaced this schedule_timeout(). Mel
suggested we postpone that to a separate patch. Now is an ideal
time to consider this change again. I've added the MM folks for
expert commentary.

I would rather see a shorter timeout, since that will be less
disruptive in practice and today's systems shouldn't have to wait
that long for free memory to become available. DEFAULT_IO_TIMEOUT
might be a defensible choice -- it will slow down this loop
effectively without adding a significant delay.


> net/sunrpc/svc_xprt.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>=20
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 796eebf1787d..161433ae0fab 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -10,6 +10,7 @@
> #include <linux/freezer.h>
> #include <linux/kthread.h>
> #include <linux/slab.h>
> +#include <linux/backing-dev.h>
> #include <net/sock.h>
> #include <linux/sunrpc/addr.h>
> #include <linux/sunrpc/stats.h>
> @@ -682,12 +683,10 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
> 			/* Made progress, don't sleep yet */
> 			continue;
>=20
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		if (signalled() || kthread_should_stop()) {
> -			set_current_state(TASK_RUNNING);
> +		if (signalled() || kthread_should_stop())
> 			return -EINTR;
> -		}
> -		schedule_timeout(msecs_to_jiffies(500));
> +
> +		congestion_wait(BLK_RW_ASYNC, msecs_to_jiffies(500));
> 	}
> 	rqstp->rq_page_end =3D &rqstp->rq_pages[pages];
> 	rqstp->rq_pages[pages] =3D NULL; /* this might be seen in nfsd_splice_ac=
tor() */
> --=20
> 2.33.0
>=20

--
Chuck Lever