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:     Trond Myklebust <trondmy@hammerspace.com>
CC:     "trondmy@kernel.org" <trondmy@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Bruce Fields <bfields@redhat.com>
Subject: Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
Thread-Topic: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
Thread-Index: AQHXY9BFZBFmWOpSekG1W92QMIYbjKsesyqAgAATvQCAAAezgA==
Date:   Mon, 21 Jun 2021 18:27:39 +0000
Message-ID: <F48D54BC-24F6-4E92-9C1E-773E5C5E29DA@oracle.com>
References: <20210617232652.264884-1-trondmy@kernel.org>
 <1669C849-D7DC-46C1-B6B6-F2C79C819710@oracle.com>
 <ff7400e35ec0b227de4546c608d231caed921d5b.camel@hammerspace.com>
In-Reply-To: <ff7400e35ec0b227de4546c608d231caed921d5b.camel@hammerspace.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?2VC6TEJdspbe0p/V4hu6nz8Irbzgq014S06XLJSUjmvrHwDZavxmTU7b1VLE?=
 =?us-ascii?Q?UdapFgmkfVfZPbiV2oxqVs+hF8zDAH3MPCxfs2KcVhkxQrbHkXQTI9SFk1pD?=
 =?us-ascii?Q?v9HmeJ+YwD+BPoWZkQpRG0o1PJnODxNWcb5oEB4yB8zXXCFYy5BAR5eitlWD?=
 =?us-ascii?Q?YGFzQTgCBf6TkXKmiKMgvGG5xT7GsQjw/EVFjyJdJDPzO7NvTQdktsZgOhv7?=
 =?us-ascii?Q?47VM9MJK1ihDV4ZYFSKjZGKMTu6dfFNpwfq0qDEzuLwNYwH16E+a0Way8kP8?=
 =?us-ascii?Q?4luOPSY31CNMTxFD2dW3FTSpEMHB8+s0F8oOgmL/GEVlDyyLM2x8RgjkwOj1?=
 =?us-ascii?Q?V6Dwt93WatZcp0ATBXxGbpAYMMOXrPheJ/1vVp6A2MWi/qFuh5iNHTMcWd7w?=
 =?us-ascii?Q?ejLccZDggM7RDkNhy15hORJmGMpoENiLvbR4PyjxJeu/sZjMMogIE5eWkgwP?=
 =?us-ascii?Q?IZbNtv0R+rGAoFdaUguq0VHwDzyxQr3TWmLGl4NX0IxYpqjl1eQ6Xco4K1jC?=
 =?us-ascii?Q?n8aSyGu3u7i++7glUp9TopP88bd4HaODB3ZRvTx0SlqvtgKz53olGBalweZW?=
 =?us-ascii?Q?ttOVOS2p/YH1o8f+R8IAzKFeaG8jFdmYP/QoQQYF1/uszruvDkWDqokivzxH?=
 =?us-ascii?Q?NC/avKn3NF0zvPgE//8Hqnt6ZszfJWEeiXVtiRfTwhCWey/EQblN11tn4CEw?=
 =?us-ascii?Q?sAy/xv8ImHdp+fhz0Kp6dPeRewlGPKE4D9PXKn1fLeYnCTWdaWHxwP7H3VRo?=
 =?us-ascii?Q?JdLp1t5ENj/N4K8SSfzKo9HPw67vk+gxasPJzFXZ1cQlaSSIcoNp6kaa3V+W?=
 =?us-ascii?Q?czLnbM43TX2KzW0ahGsO/Klt/T5vQLFXJC02g8xL7ChNabxneWKlN37bCyll?=
 =?us-ascii?Q?EVo7NsaI82SRi9biTSeovZHu0aY+SA16WvIF1AExOH9nmHR1VXOGMEKsI36l?=
 =?us-ascii?Q?52KlLRw8I5I3099bqZmkwGA0rUzE90jpJHy6KfcG2ucSnl4/ppi2ondQrPxK?=
 =?us-ascii?Q?hzk1jBWds5hkHT8p0orxOvgkQQ98JeYkvP0ommFXkKX3NwiEWpuPcijR61GK?=
 =?us-ascii?Q?z8ApeVk/87Yz5/qNIwZqwl+vkNoo1qE4X7qtlkEjgJO8dLxs8HmmnNUcu9lV?=
 =?us-ascii?Q?1EILTkGB6fWPUfgENrVylGA8PkgDfaQ5ICZZWKxGRJ5SZyymWc9uogyfyJb5?=
 =?us-ascii?Q?V8LjVp7VOEIS3s7G+BGkXQ8kXLrLqSxLPtUgTmYatLo61a3dntqt1Wqibr+w?=
 =?us-ascii?Q?J+6QKqhUttcJR1P0LtVofS5s8gdv26bkrGiFpXqUPs/RkU1JnZwjcFBqstMN?=
 =?us-ascii?Q?2b1IphGGx5N9hbrYiLzA5Hxw?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <DF78DBB69010A84C9547649DF210C0DE@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: 06f201b6-e399-40d9-70b7-08d934e24057
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Jun 2021 18:27:39.4768
 (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: O9U2LRUJXolVxdnjbtfoB3+FmgGH2Y17Fp4Ty+d3DbQtqZK5k6GaEj0mruZPqHahuFe6mDPJ0cTkT135HFqVRg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3029
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10022 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 mlxscore=0 mlxlogscore=999 adultscore=0 phishscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2106210107
X-Proofpoint-ORIG-GUID: mEJXt-yRhrY2ePlNm5HaEc0MOZSowXRZ
X-Proofpoint-GUID: mEJXt-yRhrY2ePlNm5HaEc0MOZSowXRZ
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jun 21, 2021, at 2:00 PM, Trond Myklebust <trondmy@hammerspace.com> wr=
ote:
>=20
> On Mon, 2021-06-21 at 16:49 +0000, Chuck Lever III wrote:
>> Hi-
>>=20
>>> On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
>>>=20
>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>=20
>>> When flushing out the unstable file writes as part of a COMMIT
>>> call, try
>>> to perform most of of the data writes and waits outside the
>>> semaphore.
>>>=20
>>> This means that if the client is sending the COMMIT as part of a
>>> memory
>>> reclaim operation, then it can continue performing I/O, with
>>> contention
>>> for the lock occurring only once the data sync is finished.
>>>=20
>>> Fixes: 5011af4c698a ("nfsd: Fix stable writes")
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>=20
>> The good news is I've found no functional regressions. The bad
>> news is I haven't seen any difference in performance. Is there
>> a particular test that I can run to observe improvement?
>=20
> I'd expect that re-exported NFS would be the best test since fsync() is
> a high latency operation when the page cache is loaded. You also want
> to use a client with relatively limited memory so that it will try to
> reclaim memory by pushing out dirty pages and doing COMMIT.

I thought I was hitting those low-memory cases with direct I/O testing. <sh=
rug>


>> I wonder about adding a Fixes: tag for a change that the patch
>> description describes as an optimization.
>=20
> I've occasionally hit OOM situations in the re-export case when the r/w
> lock contention causes softerr failure to be serialised.
> i.e. if the server is down, and you're essentially hoping that the nfsd
> threads will give up and return EJUKEBOX/NFS4ERR_DELAY to the client,
> then that lock ensures that threads fail one-by-one (grab lock, write,
> timeout, release lock) instead of being able to all fail at once
> (write, timeout).
>=20
>>=20
>>=20
>>> ---
>>> fs/nfsd/vfs.c | 18 ++++++++++++++++--
>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>=20
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index 15adf1f6ab21..46485c04740d 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -1123,6 +1123,19 @@ nfsd_write(struct svc_rqst *rqstp, struct
>>> svc_fh *fhp, loff_t offset,
>>> }
>>>=20
>>> #ifdef CONFIG_NFSD_V3
>>> +static int
>>> +nfsd_filemap_write_and_wait_range(struct nfsd_file *nf, loff_t
>>> offset,
>>> +                                 loff_t end)
>>> +{
>>> +       struct address_space *mapping =3D nf->nf_file->f_mapping;
>>> +       int ret =3D filemap_fdatawrite_range(mapping, offset, end);
>>> +
>>> +       if (ret)
>>> +               return ret;
>>> +       filemap_fdatawait_range_keep_errors(mapping, offset, end);
>>> +       return 0;
>>> +}
>>> +
>>> /*
>>>  * Commit all pending writes to stable storage.
>>>  *
>>> @@ -1153,10 +1166,11 @@ nfsd_commit(struct svc_rqst *rqstp, struct
>>> svc_fh *fhp,
>>>         if (err)
>>>                 goto out;
>>>         if (EX_ISSYNC(fhp->fh_export)) {
>>> -               int err2;
>>> +               int err2 =3D nfsd_filemap_write_and_wait_range(nf,
>>> offset, end);
>>>=20
>>>                 down_write(&nf->nf_rwsem);
>>> -               err2 =3D vfs_fsync_range(nf->nf_file, offset, end,
>>> 0);
>>> +               if (!err2)
>>> +                       err2 =3D vfs_fsync_range(nf->nf_file, offset,
>>> end, 0);
>>>                 switch (err2) {
>>>                 case 0:
>>>                         nfsd_copy_boot_verifier(verf,
>>> net_generic(nf->nf_net,
>>> --=20
>>> 2.31.1
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever