[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y3L6t0U89o27gJru@codewreck.org>
Date: Tue, 15 Nov 2022 11:34:31 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: David Howells <dhowells@...hat.com>
Cc: willy@...radead.org, dwysocha@...hat.com,
Rohith Surabattula <rohiths.msft@...il.com>,
Steve French <sfrench@...ba.org>,
Shyam Prasad N <nspmangalore@...il.com>,
Ilya Dryomov <idryomov@...il.com>, linux-cachefs@...hat.com,
linux-cifs@...r.kernel.org, linux-afs@...ts.infradead.org,
v9fs-developer@...ts.sourceforge.net, ceph-devel@...r.kernel.org,
linux-nfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2] mm, netfs, fscache: Stop read optimisation when
folio removed from pagecache
David Howells wrote on Tue, Nov 15, 2022 at 12:41:02AM +0000:
> Dominique Martinet <asmadeus@...ewreck.org> 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@casper.infradead.org/
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
Powered by blists - more mailing lists