From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Trond Myklebust <trondmy@kernel.org>
CC:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: allow reaping files that are still under writeback
Thread-Topic: [PATCH] nfsd: allow reaping files that are still under writeback
Thread-Index: AQHZP+kYZqo+lvM+xk+OVBsJow3AIK7OhuYAgAADi4CAAAEzAA==
Date:   Tue, 14 Feb 2023 15:05:10 +0000
Message-ID: <21FDFADF-FAA8-4526-80CB-9ECCB2F937CF@oracle.com>
References: <20230213202346.291008-1-jlayton@kernel.org>
 <E1A055FD-45C3-47A0-A6CB-296C84985D43@oracle.com>
 <d7eea987852cffb5fc719310ed01f771390d60d2.camel@kernel.org>
In-Reply-To: <d7eea987852cffb5fc719310ed01f771390d60d2.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?TR9o5L5f/OTHDkTXvBSSJED2Vtg1RlP7sx+PE8DRku+D5ulQJ9hBUPYooG6Z?=
 =?us-ascii?Q?gkUFTyL8oILkJvP68JTYaqEYQwptCcQqOHjCiKI5+bJkIInr3qtwgu3ihT/c?=
 =?us-ascii?Q?J0IwKHyh8ei+RN90+lqNVagi4Ob7LtFgr92wzpzRiBNa6hWJUEUJQ6v48zEL?=
 =?us-ascii?Q?yTa+qQb9FlX1AHkdMeXx6bmmxALB17z6t5eL613tZVS/3f6ZedbycoA+PSTw?=
 =?us-ascii?Q?bttNCo5+YHbZYeRBgCU31+Li2Z2EP+FOJT1NmrwI5ayvFMuOhwQhw2FZRsZ1?=
 =?us-ascii?Q?VW1nkeOxtKLWcmPh6HyBjXM4LH+EnTl8o3jk81ySAVK/EbeYq8owgAFlZcnv?=
 =?us-ascii?Q?p7s1xnY5aPVEwtRVi3epnXLVPMBdC+usrp+NUBKiiFdi/sFdGH6JUtMgvg6N?=
 =?us-ascii?Q?kHUZ8S0407RvGxC6LqVMQLiTCTe86Bh7nJCZL+N2CHCT9wjaiRzwrgmtiw90?=
 =?us-ascii?Q?4qkLaYmXNmbTF9fjM0UeXsCEqzaAN4/dbLiOA+iWH1Z5H6oFhUVF289vNULj?=
 =?us-ascii?Q?iGM8+kXjLMF1Oo8UizFtMVkJktEUw03mwkdeXK7Erh2t3FBW7ZNt6GyVyqOR?=
 =?us-ascii?Q?9O5lRrhASNyA3ORL+ZX69aCpSAbbb9NEGdHt6WK2/X1fKEhh7fcx0Sb8oXbD?=
 =?us-ascii?Q?fsg/SFnbE1U3t6bWQBqI0pqa7Z5dCbN2/p1g7JQlxnOY6i0CnmFmUPvkZlHd?=
 =?us-ascii?Q?46zk4ms/5z5wTI3C5t4pz2GfVzv7nt7210Ebsmkqe1d6TFuAVCFcHngTmvWg?=
 =?us-ascii?Q?ihoxYcVvviw7TE3brWJ/DIIDla8x1cTxsWYKzdt5073EnXJQUYnUZLjXv2m1?=
 =?us-ascii?Q?Lwhk4fPNQBTSsfjmRdni+w9j9ekCMs93AJewHs8oDr3qrcaQ+R9VIRfG8RMx?=
 =?us-ascii?Q?V76FAu+l4+P0AbN6RM8LFk3vF8oRSMgO08eLiHZ2F7mCSfiDrS5BksWFl7Nt?=
 =?us-ascii?Q?knt4qRiFdVd8SxKj6JFBJq28CURv0icdEvZ68yrBxOUJpDJwJjxbgmenelRn?=
 =?us-ascii?Q?90+9kZGczI+/nzdyqr/wCqLUr5SYoN7vYgw7iNWmViKkkSNlMnErBpM5wNIw?=
 =?us-ascii?Q?sjgpbijv8zRZKMwrZut6t7rhi6x/owM5HxDEvjyzUnfAke3XgXrWk6Q5sMtK?=
 =?us-ascii?Q?W2BSqmviQzjdTp1Oity5/g7Yj/C3YHgQWtBcHCrYYutZ8EktKF2NPzgWSz4L?=
 =?us-ascii?Q?XbQOrtgek22YFPG3H/6rJITDq+DyDvneEwecYw/WjmTt9QH07PW/dSFraw+K?=
 =?us-ascii?Q?VV0zudgjQcYe5SlTzZP7KRCD+WTWb4RK8zn6SNcf6olxIacvwS0fQOaigZkO?=
 =?us-ascii?Q?1zf4JHRJo93fnAmLtCJ27zbUzP6jcklQaCiydbt1+BVbiBHU0V4KNR/zdX8Y?=
 =?us-ascii?Q?vDB88Q2MqhlpB/9OE4NZfPRKiuG9ohOFt8nZCL2cA8qwAhQPGU/c+4WzYegM?=
 =?us-ascii?Q?p1mjH/ZJkRU1u4clIMheyHfz0fsj7SjrAfStQWjAcnSEvE4CoUa5zoKtH8tn?=
 =?us-ascii?Q?9WKhDew00HyNHVB9TVoJ/v5yATX8C3hsif5grTmdfBeisAHrVaKzjeg9q34/?=
 =?us-ascii?Q?jPI6GTcMohQh+8t8IptrBOaV6RBtYATo979PIX6F29y0a6X0WKVv0c6ReFVK?=
 =?us-ascii?Q?PA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <EBB7E97A844E0C42881D2761FE295F3C@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: cIAjUWHL9qKX7rPl3T/2Bypqjr1IJBANqAQ/3mXxAuEp8F2JlOIiWca+ZYCZBQJjbZ/F/6dQqkAV7Kbh9vg5B2l4TqHmOA5qwunzGNM98t/T+9ySDScseXRD5lZ8aAdlVMrTVdUfbVfcf7vyCtFa9w5zKjySpq9z4HrhNPSHnf+LXX/k+wY8vM7/2STGDtHT2be66ZMJdTCqdJrpaGJpJQar4kNoNOoXyG1xWqJo1OtQeiQwuW8VOvqH96xPI4RzJAh6bGKVoW4qBMD2bxxDVID7Jt+CHX7sP3GQoqrkNgiyVmvEnNvn8tF6Ljuj2huBZe6+dzl3mlAZv0sK+6hzfDhA+nawWW2nbJ95uw8W/8VKQLw9gkYVsq0aVlFa4h59ttTBPwPsP8n98wWoRvvY9hhD8z9SDzswKJ5oDLqf+Kx2elNybyemOxu1e6SctB+Y1uGS1DIUkqq77i9H6Ma833ATDMzPpy1t3kd+B+zehMhIXWYt7I3x3+e+5ZyH5s5/xmIUVZW2+2Mwz4o0bwcFCgi4TMHPmAovx4VHz4z3susGMzHbtZr/mUysb3bnATg/tEjXmbs6fazvcKN+foqSMf1mzSpS/oJmvVZ6D5TK/oWjK3HC3J/ltM17VI7w1iwoo/SO1OMWEgc3UgCvzfwMC5u0N3kPt5XaxOtE01JSudxuK9GG0f6oAvlvue5TMosmZbBXuqj562ztYFh4CJVtApBWd9NX38RwkOuP23Yq9WW4+vQNG4ONCt9QPRtnShWhC6I3raGAu7/TAawKNGbIoU9FoOOxMXaAcnuYdiUw82Q=
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: e45d4c36-8310-4689-9cc2-08db0e9cde2d
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Feb 2023 15:05:10.7962
 (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: vNX2Yzv8syk/yvuGE4FBWG1wN6w7+8ACSkC60mGWK3XOHkraIUbYuT3CEu1FantwyDbs1PykTimLM6IXDF6VZw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR10MB6214
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.170.22
 definitions=2023-02-14_10,2023-02-14_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0 adultscore=0
 mlxlogscore=999 phishscore=0 spamscore=0 mlxscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2302140129
X-Proofpoint-ORIG-GUID: jGy5WV7GlT5ZN3cQo0x2seHToPdbkEfG
X-Proofpoint-GUID: jGy5WV7GlT5ZN3cQo0x2seHToPdbkEfG
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 14, 2023, at 10:00 AM, Trond Myklebust <trondmy@kernel.org> wrote:
>=20
> On Tue, 2023-02-14 at 14:48 +0000, Chuck Lever III wrote:
>>=20
>>=20
>>> On Feb 13, 2023, at 3:23 PM, Jeff Layton <jlayton@kernel.org>
>>> wrote:
>>>=20
>>> There's no reason to delay reaping an nfsd_file just because its
>>> underlying inode is still under writeback. nfsd just relies on
>>> client
>>> activity or the local flusher threads to do writeback.
>>>=20
>>> Holding the file open does nothing to facilitate that, nor does it
>>> help
>>> with tracking errors. Just allow it to close and let the kernel do
>>> writeback as it normally would.
>>>=20
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>=20
>> Thanks! Applied to topic-filecache-cleanups.
>>=20
>>=20
>>> ---
>>> fs/nfsd/filecache.c | 22 ----------------------
>>> 1 file changed, 22 deletions(-)
>>>=20
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index e6617431df7c..3b9a10378c83 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)
>>>         call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
>>> }
>>>=20
>>> -static bool
>>> -nfsd_file_check_writeback(struct nfsd_file *nf)
>>> -{
>>> -       struct file *file =3D nf->nf_file;
>>> -       struct address_space *mapping;
>>> -
>>> -       if (!file || !(file->f_mode & FMODE_WRITE))
>>> -               return false;
>>> -       mapping =3D file->f_mapping;
>>> -       return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
>>> -               mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
>>> -}
>>> -
>>> static bool nfsd_file_lru_add(struct nfsd_file *nf)
>>> {
>>>         set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
>>> @@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,
>>> struct list_lru_one *lru,
>>>         /* We should only be dealing with GC entries here */
>>>         WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));
>>>=20
>>> -       /*
>>> -        * Don't throw out files that are still undergoing I/O or
>>> -        * that have uncleared errors pending.
>>> -        */
>>> -       if (nfsd_file_check_writeback(nf)) {
>>> -               trace_nfsd_file_gc_writeback(nf);
>>> -               return LRU_SKIP;
>>> -       }
>>> -
>>>         /* If it was recently added to the list, skip it */
>>>         if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf-
>>>> nf_flags)) {
>>>                 trace_nfsd_file_gc_referenced(nf);
>>> --=20
>>> 2.39.1
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> Wait... There is a good reason for wanting to do this in the case of
> NFS re-exports, since close() is a very expensive operation if the file
> has dirty data.

Then perhaps skipping these files can be gated on an EXPORT_OP flag?


--
Chuck Lever