Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Nov 2022 11:34:31 +0900
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     David Howells <dhowells@redhat.com>
Cc:     willy@infradead.org, dwysocha@redhat.com,
        Rohith Surabattula <rohiths.msft@gmail.com>,
        Steve French <sfrench@samba.org>,
        Shyam Prasad N <nspmangalore@gmail.com>,
        Ilya Dryomov <idryomov@gmail.com>, linux-cachefs@redhat.com,
        linux-cifs@vger.kernel.org, linux-afs@lists.infradead.org,
        v9fs-developer@lists.sourceforge.net, ceph-devel@vger.kernel.org,
        linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2] mm, netfs, fscache: Stop read optimisation when
 folio removed from pagecache
Message-ID: <Y3L6t0U89o27gJru@codewreck.org>
References: <Y3Lbul7FZncNVwVZ@codewreck.org>
 <166844174069.1124521.10890506360974169994.stgit@warthog.procyon.org.uk>
 <1457985.1668472862@warthog.procyon.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1457985.1668472862@warthog.procyon.org.uk>
Precedence: bulk

David Howells wrote on Tue, Nov 15, 2022 at 12:41:02AM +0000:
> Dominique Martinet <asmadeus@codewreck.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