linux-kernel - Re: [RFC PATCH v2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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