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
