David Howells wrote on Tue, Nov 15, 2022 at 12:41:02AM +0000: > Dominique Martinet <asmadeus@xxxxxxxxxxxxx> wrote: > > any harm in setting this if netfs isn't enabled? > > (just asking because you checked in fs/9p/cache.c above) > > Well, it forces a call to ->release_folio() every time a folio is released, if > set, rather than just if PG_private/PG_private_2 is set. Yes, that's what I gathered from your explanation, but I don't understand what release_folio() actually implies in practice which is why I asked -- it looked a bit odd that you're checking for v9inode->netfs.cache in one case and not in the other; especially as all inodes should go through both v9fs_cache_inode_get_cookie() (when created) and v9fs_evict_inode() so I was a bit curious. In the 9p-without-cache case, we're normally not going through page cache at all, so I guess there won't be any mapping and this will be free anyway... > > > - if (folio_has_private(folio) && !filemap_release_folio(folio, 0)) > > > + if (!filemap_release_folio(folio, 0)) > > > > should this (and all others) check for folio_needs_release instead of has_private? > > filemap_release_folio doesn't check as far as I can see, but perhaps > > it's already fast and noop for another reason I didn't see. > > Willy suggested merging the checks from folio_has_private() into > filemap_release_folio(): > > https://lore.kernel.org/r/Yk9V/03wgdYi65Lb@xxxxxxxxxxxxxxxxxxxx/ Ah, I didn't understand the suggestion in your patch was a separate patch and didn't follow the link. It doesn't look like a patch per se, perhaps sending both together would make sense -- but on top of this change these should indeed be fine, thanks. -- Dominique
