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

[Dominique Martinet <asmadeus@codewreck.org>]

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