The current implementation of the readdir page cache requires that all
pages contain entries ordered such that the cookie references lead to the
first entry as represented by cookie 0. The invalidation of the cache
truncates either the entire cache or every page beyond a known good page.
A process that wants to emit directory entries near the end of a directory
must first fill in any entries missing in the cache near the beginning of
the directory in order that the entries decoded from READDIR XDR are
appropriately page-aligned for any readers thay may come later (and for
some error handling).
However, if we're careful to check the alignment of directory entries on
each page when the page is read, then it should be permissable to allow
"disconnected" filling of the pagecache. Rather than requiring pagecache
data to always be positionally aligned, we can instead validate that each
page is properly aligned to the reading process' directory context. If it
doesn't match our alignment, we'll refresh the entries in the page so that
it does.
This patch implements a check for validity for each page as it is obtained
from the pagecache. A page is valid if it was filled within the client's
current version of the directory and if the entries are aligned with the
current reader's directory context.
Invalid pages are re-filled by READDIR operations before being used to emit
entries for the current reader.
Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfs/dir.c | 71 +++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 57 insertions(+), 14 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 7f6c84c8a412..5fc22d97054a 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -188,13 +188,16 @@ static void nfs_readdir_page_init_array(struct page *page, u64 last_cookie)
array->last_cookie = last_cookie;
array->cookies_are_ordered = 1;
kunmap_atomic(array);
- set_page_private(page, 0);
+ if (page->mapping)
+ set_page_private(page, nfs_save_change_attribute(page->mapping->host));
+ SetPageUptodate(page);
}
static int
nfs_readdir_clear_page(struct page *page, gfp_t gfp_mask)
{
- detach_page_private(page);
+ unsigned long change_attr;
+ change_attr = (unsigned long)detach_page_private(page);
return 1;
}
@@ -225,6 +228,15 @@ void nfs_readdir_clear_array(struct page *page)
kfree(array->array[i].name);
nfs_readdir_array_init(array);
kunmap_atomic(array);
+ ClearPageUptodate(page);
+}
+
+static void
+nfs_readdir_recycle_page(struct page *page, u64 last_cookie)
+{
+ nfs_readdir_clear_array(page);
+ nfs_readdir_invalidatepage(page, 0, 0);
+ nfs_readdir_page_init_array(page, last_cookie);
}
static struct page *
@@ -341,18 +353,47 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
return ret;
}
+static bool
+nfs_readdir_page_valid(struct page *page, unsigned int entry_index, u64 index_cookie)
+{
+ bool ret = false;
+ struct nfs_cache_array *array;
+
+ if (page_private(page) != nfs_save_change_attribute(page->mapping->host))
+ goto out;
+
+ ret = true;
+ array = kmap_atomic(page);
+
+ if (array->size == 0 && array->last_cookie == index_cookie)
+ goto out_unmap;
+
+ if (array->size > entry_index &&
+ array->array[entry_index].cookie == index_cookie)
+ goto out_unmap;
+
+ ret = false;
+out_unmap:
+ kunmap_atomic(array);
+out:
+ return ret;
+}
+
static struct page *nfs_readdir_page_get_locked(struct address_space *mapping,
- pgoff_t index, u64 last_cookie)
+ struct nfs_dir_page_cursor *pgc)
{
struct page *page;
- page = grab_cache_page(mapping, index);
- if (page && !PageUptodate(page)) {
- nfs_readdir_page_init_array(page, last_cookie);
- if (invalidate_inode_pages2_range(mapping, index + 1, -1) < 0)
- nfs_zap_mapping(mapping->host, mapping);
- SetPageUptodate(page);
- }
+ page = grab_cache_page(mapping, pgc->page_index);
+
+ if (!page)
+ return page;
+
+ if (!PageUptodate(page))
+ nfs_readdir_page_init_array(page, pgc->index_cookie);
+
+ if (!nfs_readdir_page_valid(page, pgc->entry_index, pgc->index_cookie))
+ nfs_readdir_recycle_page(page, pgc->index_cookie);
return page;
}
@@ -398,8 +439,12 @@ static struct page *nfs_readdir_page_get_next(struct address_space *mapping,
pgoff_t index, u64 cookie)
{
struct page *page;
+ struct nfs_dir_page_cursor pgc = {
+ .page_index = index,
+ .index_cookie = cookie,
+ };
- page = nfs_readdir_page_get_locked(mapping, index, cookie);
+ page = nfs_readdir_page_get_locked(mapping, &pgc);
if (page) {
if (nfs_readdir_page_last_cookie(page) == cookie)
return page;
@@ -943,9 +988,7 @@ nfs_readdir_page_unlock_and_put_cached(struct nfs_readdir_descriptor *desc)
static struct page *
nfs_readdir_page_get_cached(struct nfs_readdir_descriptor *desc)
{
- return nfs_readdir_page_get_locked(desc->file->f_mapping,
- desc->pgc.page_index,
- desc->pgc.index_cookie);
+ return nfs_readdir_page_get_locked(desc->file->f_mapping, &desc->pgc);
}
/*
--
2.25.4
Hi Benjamin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on nfs/linux-next]
[also build test WARNING on v5.11-rc4 next-20210120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Benjamin-Coddington/NFS-client-readdir-per-page-validation/20210121-011011
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: nds32-defconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/27dada15cc13542a100bb36e20b49c6315c6170c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Benjamin-Coddington/NFS-client-readdir-per-page-validation/20210121-011011
git checkout 27dada15cc13542a100bb36e20b49c6315c6170c
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
fs/nfs/dir.c: In function 'nfs_readdir_clear_page':
>> fs/nfs/dir.c:199:16: warning: variable 'change_attr' set but not used [-Wunused-but-set-variable]
199 | unsigned long change_attr;
| ^~~~~~~~~~~
vim +/change_attr +199 fs/nfs/dir.c
195
196 static int
197 nfs_readdir_clear_page(struct page *page, gfp_t gfp_mask)
198 {
> 199 unsigned long change_attr;
200 change_attr = (unsigned long)detach_page_private(page);
201 return 1;
202 }
203
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]