[RFC,v2] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache

  Fscache has an optimisation by which reads from the cache are skipped until
we know that (a) there's data there to be read and (b) that data isn't
entirely covered by pages resident in the netfs pagecache.  This is done
with two flags manipulated by fscache_note_page_release():

	if (...
	    test_bit(FSCACHE_COOKIE_HAVE_DATA, &cookie->flags) &&
	    test_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags))
		clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);

where the NO_DATA_TO_READ flag causes cachefiles_prepare_read() to indicate
that netfslib should download from the server or clear the page instead.

The fscache_note_page_release() function is intended to be called from
->releasepage() - but that only gets called if PG_private or PG_private_2
is set - and currently the former is at the discretion of the network
filesystem and the latter is only set whilst a page is being written to the
cache, so sometimes we miss clearing the optimisation.

Fix this by following Willy's suggestion[1] and adding an address_space
flag, AS_RELEASE_ALWAYS, that causes filemap_release_folio() to always call
->release_folio() if it's set, even if PG_private or PG_private_2 aren't
set.

Note that this would require folio_test_private() and page_has_private() to
become more complicated.  To avoid that, in the places[*] where these are
used to conditionalise calls to filemap_release_folio() and
try_to_release_page(), the tests are removed the those functions just
jumped to unconditionally and the test is performed there.

[*] There are some exceptions in vmscan.c where the check guards more than
just a call to the releaser.  I've added a function, folio_needs_release()
to wrap all the checks for that.

AS_RELEASE_ALWAYS should be set if a non-NULL cookie is obtained from
fscache and cleared in ->evict_inode() before truncate_inode_pages_final()
is called.

Additionally, the FSCACHE_COOKIE_NO_DATA_TO_READ flag needs to be cleared
and the optimisation cancelled if a cachefiles object already contains data
when we open it.

Reported-by: Rohith Surabattula <rohiths.msft@gmail.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Matthew Wilcox <willy@infradead.org>
cc: Steve French <sfrench@samba.org>
cc: Shyam Prasad N <nspmangalore@gmail.com>
cc: Rohith Surabattula <rohiths.msft@gmail.com>
cc: Dave Wysochanski <dwysocha@redhat.com>
cc: Dominique Martinet <asmadeus@codewreck.org>
cc: Ilya Dryomov <idryomov@gmail.com>
cc: linux-cachefs@redhat.com
cc: linux-cifs@vger.kernel.org
cc: linux-afs@lists.infradead.org
cc: v9fs-developer@lists.sourceforge.net
cc: ceph-devel@vger.kernel.org
cc: linux-nfs@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
Link: https://lore.kernel.org/r/Yk9V/03wgdYi65Lb@casper.infradead.org/ [1]
Link: https://lore.kernel.org/r/164928630577.457102.8519251179327601178.stgit@warthog.procyon.org.uk/ # v1
---

 fs/9p/cache.c           |    2 ++
 fs/9p/vfs_inode.c       |    1 +
 fs/afs/inode.c          |    1 +
 fs/afs/internal.h       |    2 ++
 fs/cachefiles/namei.c   |    2 ++
 fs/ceph/cache.c         |    2 ++
 fs/ceph/inode.c         |    1 +
 fs/cifs/cifsfs.c        |    1 +
 fs/cifs/fscache.c       |    2 ++
 fs/splice.c             |    3 +--
 include/linux/pagemap.h |   16 ++++++++++++++++
 mm/filemap.c            |    4 ++++
 mm/huge_memory.c        |    3 +--
 mm/khugepaged.c         |    3 +--
 mm/memory-failure.c     |    3 +--
 mm/migrate.c            |    3 +--
 mm/truncate.c           |    6 ++----
 mm/vmscan.c             |   15 +++++++++++----
 18 files changed, 52 insertions(+), 18 deletions(-)
  

David Howells wrote on Mon, Nov 14, 2022 at 04:02:20PM +0000:
> Fscache has an optimisation by which reads from the cache are skipped until
> we know that (a) there's data there to be read and (b) that data isn't
> entirely covered by pages resident in the netfs pagecache.  This is done
> with two flags manipulated by fscache_note_page_release():
> 
> 	if (...
> 	    test_bit(FSCACHE_COOKIE_HAVE_DATA, &cookie->flags) &&
> 	    test_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags))
> 		clear_bit(FSCACHE_COOKIE_NO_DATA_TO_READ, &cookie->flags);
> 
> where the NO_DATA_TO_READ flag causes cachefiles_prepare_read() to indicate
> that netfslib should download from the server or clear the page instead.
> 
> The fscache_note_page_release() function is intended to be called from
> ->releasepage() - but that only gets called if PG_private or PG_private_2
> is set - and currently the former is at the discretion of the network
> filesystem and the latter is only set whilst a page is being written to the
> cache, so sometimes we miss clearing the optimisation.
> 
> Fix this by following Willy's suggestion[1] and adding an address_space
> flag, AS_RELEASE_ALWAYS, that causes filemap_release_folio() to always call
> ->release_folio() if it's set, even if PG_private or PG_private_2 aren't
> set.

Not familiar with the common code so just glanced at it and asked stupid
questions.

> diff --git a/fs/9p/cache.c b/fs/9p/cache.c
> index cebba4eaa0b5..12c0ae29f185 100644
> --- a/fs/9p/cache.c
> +++ b/fs/9p/cache.c
> @@ -68,6 +68,8 @@ void v9fs_cache_inode_get_cookie(struct inode *inode)
>  				       &path, sizeof(path),
>  				       &version, sizeof(version),
>  				       i_size_read(&v9inode->netfs.inode));
> +	if (v9inode->netfs.cache)
> +		mapping_set_release_always(inode->i_mapping);
>  
>  	p9_debug(P9_DEBUG_FSC, "inode %p get cookie %p\n",
>  		 inode, v9fs_inode_cookie(v9inode));
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 4d1a4a8d9277..b553fe3484c1 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -394,6 +394,7 @@ void v9fs_evict_inode(struct inode *inode)
>  	version = cpu_to_le32(v9inode->qid.version);
>  	fscache_clear_inode_writeback(v9fs_inode_cookie(v9inode), inode,
>  				      &version);
> +	mapping_clear_release_always(inode->i_mapping);

any harm in setting this if netfs isn't enabled?
(just asking because you checked in fs/9p/cache.c above)

>  	clear_inode(inode);
>  	filemap_fdatawrite(&inode->i_data);
>  
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index bbccb4044222..3db9a6225bc0 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -199,6 +199,7 @@ enum mapping_flags {
>  	/* writeback related tags are not used */
>  	AS_NO_WRITEBACK_TAGS = 5,
>  	AS_LARGE_FOLIO_SUPPORT = 6,
> +	AS_RELEASE_ALWAYS,	/* Call ->release_folio(), even if no private data */
>  };
>  
>  /**
> @@ -269,6 +270,21 @@ static inline int mapping_use_writeback_tags(struct address_space *mapping)
>  	return !test_bit(AS_NO_WRITEBACK_TAGS, &mapping->flags);
>  }
>  
> +static inline bool mapping_release_always(const struct address_space *mapping)
> +{
> +	return test_bit(AS_RELEASE_ALWAYS, &mapping->flags);
> +}
> +
> +static inline void mapping_set_release_always(struct address_space *mapping)
> +{
> +	set_bit(AS_RELEASE_ALWAYS, &mapping->flags);
> +}
> +
> +static inline void mapping_clear_release_always(struct address_space *mapping)
> +{
> +	set_bit(AS_RELEASE_ALWAYS, &mapping->flags);

clear_bit certainly?

> +}
> +
>  static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
>  {
>  	return mapping->gfp_mask;
> diff --git a/mm/truncate.c b/mm/truncate.c
> index c0be77e5c008..0d4dd233f518 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -19,7 +19,6 @@
>  #include <linux/highmem.h>
>  #include <linux/pagevec.h>
>  #include <linux/task_io_accounting_ops.h>
> -#include <linux/buffer_head.h>	/* grr. try_to_release_page */
>  #include <linux/shmem_fs.h>
>  #include <linux/rmap.h>
>  #include "internal.h"
> @@ -276,7 +275,7 @@ static long mapping_evict_folio(struct address_space *mapping,
>  	if (folio_ref_count(folio) >
>  			folio_nr_pages(folio) + folio_has_private(folio) + 1)
>  		return 0;
> -	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.

>  		return 0;
>  
>  	return remove_mapping(mapping, folio);

--
Dominique
  

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.

> > +static inline void mapping_clear_release_always(struct address_space *mapping)
> > +{
> > +	set_bit(AS_RELEASE_ALWAYS, &mapping->flags);
> 
> clear_bit certainly?

Bah.  Yes.

> > -	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/

David
  

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
  

On Mon, Nov 14, 2022 at 04:02:20PM +0000, David Howells wrote:
> +++ b/mm/filemap.c
> @@ -3941,6 +3941,10 @@ bool filemap_release_folio(struct folio *folio, gfp_t gfp)
>  	struct address_space * const mapping = folio->mapping;
>  
>  	BUG_ON(!folio_test_locked(folio));
> +	if ((!mapping || !mapping_release_always(mapping))
> +	    && !folio_test_private(folio) &&
> +	    !folio_test_private_2(folio))
> +		return true;

Why do you need to test 'mapping' here?  Also this is the most
inconsistent style ...

	if ((!mapping || !mapping_release_always(mapping)) &&
	    !folio_test_private(folio) && !folio_test_private_2(folio))

works fine, but if you insist on splitting over three lines, then:

	if ((!mapping || !mapping_release_always(mapping)) &&
	    !folio_test_private(folio) && 
	    !folio_test_private_2(folio))

> @@ -276,7 +275,7 @@ static long mapping_evict_folio(struct address_space *mapping,
>  	if (folio_ref_count(folio) >
>  			folio_nr_pages(folio) + folio_has_private(folio) + 1)

I think this line is incorrect, right?  You don't increment the folio
refcount just because the folio has private2 set, do you?

>  		return 0;
> -	if (folio_has_private(folio) && !filemap_release_folio(folio, 0))
> +	if (!filemap_release_folio(folio, 0))
>  		return 0;
>  
>  	return remove_mapping(mapping, folio);

Can we get rid of folio_has_private() / page_has_private() now?
  

Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Nov 14, 2022 at 04:02:20PM +0000, David Howells wrote:
> > +++ b/mm/filemap.c
> > @@ -3941,6 +3941,10 @@ bool filemap_release_folio(struct folio *folio, gfp_t gfp)
> >  	struct address_space * const mapping = folio->mapping;
> >  
> >  	BUG_ON(!folio_test_locked(folio));
> > +	if ((!mapping || !mapping_release_always(mapping))
> > +	    && !folio_test_private(folio) &&
> > +	    !folio_test_private_2(folio))
> > +		return true;
> 
> Why do you need to test 'mapping' here?

Why does the function do:

	if (mapping && mapping->a_ops->release_folio)

later then?  There are callers of the function, such as shrink_folio_list(),
that seem to think that folio->mapping might be NULL.

> Also this is the most inconsistent style ...

Yeah, I accidentally pushed the '&&' onto the next line.

> > @@ -276,7 +275,7 @@ static long mapping_evict_folio(struct address_space *mapping,
> >  	if (folio_ref_count(folio) >
> >  			folio_nr_pages(folio) + folio_has_private(folio) + 1)
> 
> I think this line is incorrect, right?  You don't increment the folio
> refcount just because the folio has private2 set, do you?

Errr, yes:

	static inline void folio_start_fscache(struct folio *folio)
	{
		VM_BUG_ON_FOLIO(folio_test_private_2(folio), folio);
		folio_get(folio);
		folio_set_private_2(folio);
	}

Someone insisted - might even have been you;-)

I'm working on getting rid of the use of PG_private_2 from the network
filesystems, but it's still in progress.  Kind of blocked on the iov_iter
stuff.

> >  		return 0;
> > -	if (folio_has_private(folio) && !filemap_release_folio(folio, 0))
> > +	if (!filemap_release_folio(folio, 0))
> >  		return 0;
> >  
> >  	return remove_mapping(mapping, folio);
> 
> Can we get rid of folio_has_private()

That would be nice, but there are still places that check it, and until we get
rid of the use of PG_private_2, we can't reduce it to just a check on
PG_private.  Truncate, for example, checks it to see if it should can
->invalidate_folio().

It's only used in mm/, so it could be moved into mm/internal.h.

> / page_has_private() now?

That's used in some a number of places outside of mm/.  The arch/s390/ usage
is just to calculate the expected refcount.  I wonder if calculation of the
expected refcount could be potted into a function as it's performed in a
number of places - though the expectation isn't always the same.

Ext3 and fuse both use it - but those probably need to check PG_private_2 and
could use a "folio_test_private()" function when fully foliated.

David