It is now possible that a reader will resume a directory listing after an
invalidation and fill the rest of the pages with the offset left over from
the last partially-filled page. These pages will then be recycled and
refilled by the next reader since their alignment is incorrect.
Add an index to the nfs_cache_array that will indicate where the next entry
should be filled. This allows partially-filled pages to have the best
alignment possible. They are more likely to be useful to readers that
follow.
This optimization targets the case when there are multiple processes
listing the directory simultaneously. Often the processes will collect and
block on the same page waiting for a READDIR call to fill the pagecache.
If the pagecache is invalidated, a partially-filled page will usually
result. This partially-filled page can immediately be used by all
processes to emit entries rather than having to discard and refill it for
every process.
The addition of another integer to struct nfs_cache_array increases its
size to 24 bytes. We do not lose the original capacity of 127 entries per
page.
Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfs/dir.c | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 4e21b21c75d0..d6101e45fd66 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -143,6 +143,7 @@ struct nfs_cache_array_entry {
struct nfs_cache_array {
u64 last_cookie;
+ unsigned int index;
unsigned int size;
unsigned char page_full : 1,
page_is_eof : 1,
@@ -179,13 +180,15 @@ static void nfs_readdir_array_init(struct nfs_cache_array *array)
memset(array, 0, sizeof(struct nfs_cache_array));
}
-static void nfs_readdir_page_init_array(struct page *page, u64 last_cookie)
+static void
+nfs_readdir_page_init_array(struct page *page, struct nfs_dir_page_cursor *pgc)
{
struct nfs_cache_array *array;
array = kmap_atomic(page);
nfs_readdir_array_init(array);
- array->last_cookie = last_cookie;
+ array->last_cookie = pgc->index_cookie;
+ array->index = pgc->entry_index;
array->cookies_are_ordered = 1;
kunmap_atomic(array);
if (page->mapping)
@@ -224,7 +227,7 @@ void nfs_readdir_clear_array(struct page *page)
int i;
array = kmap_atomic(page);
- for (i = 0; i < array->size; i++)
+ for (i = array->index - array->size; i < array->size; i++)
kfree(array->array[i].name);
nfs_readdir_array_init(array);
kunmap_atomic(array);
@@ -232,19 +235,20 @@ void nfs_readdir_clear_array(struct page *page)
}
static void
-nfs_readdir_recycle_page(struct page *page, u64 last_cookie)
+nfs_readdir_recycle_page(struct page *page, struct nfs_dir_page_cursor *pgc)
{
nfs_readdir_clear_array(page);
nfs_readdir_invalidatepage(page, 0, 0);
- nfs_readdir_page_init_array(page, last_cookie);
+ nfs_readdir_page_init_array(page, pgc);
}
static struct page *
nfs_readdir_page_array_alloc(u64 last_cookie, gfp_t gfp_flags)
{
struct page *page = alloc_page(gfp_flags);
+ struct nfs_dir_page_cursor pgc = { .index_cookie = last_cookie };
if (page)
- nfs_readdir_page_init_array(page, last_cookie);
+ nfs_readdir_page_init_array(page, &pgc);
return page;
}
@@ -309,7 +313,7 @@ static int nfs_readdir_array_can_expand(struct nfs_cache_array *array)
if (array->page_full)
return -ENOSPC;
- cache_entry = &array->array[array->size + 1];
+ cache_entry = &array->array[array->index + 1];
if ((char *)cache_entry - (char *)array > PAGE_SIZE) {
array->page_full = 1;
return -ENOSPC;
@@ -336,7 +340,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
goto out;
}
- cache_entry = &array->array[array->size];
+ cache_entry = &array->array[array->index];
cache_entry->cookie = entry->prev_cookie;
cache_entry->ino = entry->ino;
cache_entry->d_type = entry->d_type;
@@ -345,6 +349,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
array->last_cookie = entry->cookie;
if (array->last_cookie <= cache_entry->cookie)
array->cookies_are_ordered = 0;
+ array->index++;
array->size++;
if (entry->eof != 0)
nfs_readdir_array_set_eof(array);
@@ -365,13 +370,15 @@ nfs_readdir_page_valid(struct page *page, unsigned int entry_index, u64 index_co
ret = true;
array = kmap_atomic(page);
- if ((array->size == 0 || array->size == entry_index)
- && array->last_cookie == index_cookie)
- goto out_unmap;
+ if (entry_index >= array->index - array->size) {
+ if ((array->size == 0 || array->size == entry_index)
+ && array->last_cookie == index_cookie)
+ goto out_unmap;
- if (array->size > entry_index &&
- array->array[entry_index].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:
@@ -391,10 +398,10 @@ static struct page *nfs_readdir_page_get_locked(struct address_space *mapping,
return page;
if (!PageUptodate(page))
- nfs_readdir_page_init_array(page, pgc->index_cookie);
+ nfs_readdir_page_init_array(page, pgc);
if (!nfs_readdir_page_valid(page, pgc->entry_index, pgc->index_cookie))
- nfs_readdir_recycle_page(page, pgc->index_cookie);
+ nfs_readdir_recycle_page(page, pgc);
return page;
}
@@ -513,7 +520,7 @@ static bool nfs_readdir_array_cookie_in_range(struct nfs_cache_array *array,
/* Optimisation for monotonically increasing cookies */
if (cookie >= array->last_cookie)
return false;
- if (array->size && cookie < array->array[0].cookie)
+ if (array->size && cookie < array->array[array->index - array->size].cookie)
return false;
return true;
}
@@ -528,7 +535,7 @@ static int nfs_readdir_search_for_cookie(struct nfs_cache_array *array,
if (!nfs_readdir_array_cookie_in_range(array, desc->dir_cookie))
goto check_eof;
- for (i = 0; i < array->size; i++) {
+ for (i = array->index - array->size; i < array->index; i++) {
if (array->array[i].cookie == desc->dir_cookie) {
struct nfs_inode *nfsi = NFS_I(file_inode(desc->file));
@@ -1072,7 +1079,7 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
unsigned int i = 0;
array = kmap(desc->page);
- for (i = desc->pgc.entry_index; i < array->size; i++) {
+ for (i = desc->pgc.entry_index; i < array->index; i++) {
struct nfs_cache_array_entry *ent;
ent = &array->array[i];
--
2.25.4
Hi Ben,
On Wed, Jan 20, 2021 at 12:04 PM Benjamin Coddington
<[email protected]> wrote:
>
> It is now possible that a reader will resume a directory listing after an
> invalidation and fill the rest of the pages with the offset left over from
> the last partially-filled page. These pages will then be recycled and
> refilled by the next reader since their alignment is incorrect.
>
> Add an index to the nfs_cache_array that will indicate where the next entry
> should be filled. This allows partially-filled pages to have the best
> alignment possible. They are more likely to be useful to readers that
> follow.
>
> This optimization targets the case when there are multiple processes
> listing the directory simultaneously. Often the processes will collect and
> block on the same page waiting for a READDIR call to fill the pagecache.
> If the pagecache is invalidated, a partially-filled page will usually
> result. This partially-filled page can immediately be used by all
> processes to emit entries rather than having to discard and refill it for
> every process.
>
> The addition of another integer to struct nfs_cache_array increases its
> size to 24 bytes. We do not lose the original capacity of 127 entries per
> page.
This patch causes cthon basic test #6 to start failing with unexpected
dir entries across all NFS versions:
./test6: readdir
basic tests failed
./test6: (/mnt/test/anna/Connectathon/cthon04) unexpected dir entry 'h'
Luckily, the next patch seems to resolve this issue. Could they maybe
be squashed together?
Thanks,
Anna
Anna
>
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> fs/nfs/dir.c | 45 ++++++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 4e21b21c75d0..d6101e45fd66 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -143,6 +143,7 @@ struct nfs_cache_array_entry {
>
> struct nfs_cache_array {
> u64 last_cookie;
> + unsigned int index;
> unsigned int size;
> unsigned char page_full : 1,
> page_is_eof : 1,
> @@ -179,13 +180,15 @@ static void nfs_readdir_array_init(struct nfs_cache_array *array)
> memset(array, 0, sizeof(struct nfs_cache_array));
> }
>
> -static void nfs_readdir_page_init_array(struct page *page, u64 last_cookie)
> +static void
> +nfs_readdir_page_init_array(struct page *page, struct nfs_dir_page_cursor *pgc)
> {
> struct nfs_cache_array *array;
>
> array = kmap_atomic(page);
> nfs_readdir_array_init(array);
> - array->last_cookie = last_cookie;
> + array->last_cookie = pgc->index_cookie;
> + array->index = pgc->entry_index;
> array->cookies_are_ordered = 1;
> kunmap_atomic(array);
> if (page->mapping)
> @@ -224,7 +227,7 @@ void nfs_readdir_clear_array(struct page *page)
> int i;
>
> array = kmap_atomic(page);
> - for (i = 0; i < array->size; i++)
> + for (i = array->index - array->size; i < array->size; i++)
> kfree(array->array[i].name);
> nfs_readdir_array_init(array);
> kunmap_atomic(array);
> @@ -232,19 +235,20 @@ void nfs_readdir_clear_array(struct page *page)
> }
>
> static void
> -nfs_readdir_recycle_page(struct page *page, u64 last_cookie)
> +nfs_readdir_recycle_page(struct page *page, struct nfs_dir_page_cursor *pgc)
> {
> nfs_readdir_clear_array(page);
> nfs_readdir_invalidatepage(page, 0, 0);
> - nfs_readdir_page_init_array(page, last_cookie);
> + nfs_readdir_page_init_array(page, pgc);
> }
>
> static struct page *
> nfs_readdir_page_array_alloc(u64 last_cookie, gfp_t gfp_flags)
> {
> struct page *page = alloc_page(gfp_flags);
> + struct nfs_dir_page_cursor pgc = { .index_cookie = last_cookie };
> if (page)
> - nfs_readdir_page_init_array(page, last_cookie);
> + nfs_readdir_page_init_array(page, &pgc);
> return page;
> }
>
> @@ -309,7 +313,7 @@ static int nfs_readdir_array_can_expand(struct nfs_cache_array *array)
>
> if (array->page_full)
> return -ENOSPC;
> - cache_entry = &array->array[array->size + 1];
> + cache_entry = &array->array[array->index + 1];
> if ((char *)cache_entry - (char *)array > PAGE_SIZE) {
> array->page_full = 1;
> return -ENOSPC;
> @@ -336,7 +340,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
> goto out;
> }
>
> - cache_entry = &array->array[array->size];
> + cache_entry = &array->array[array->index];
> cache_entry->cookie = entry->prev_cookie;
> cache_entry->ino = entry->ino;
> cache_entry->d_type = entry->d_type;
> @@ -345,6 +349,7 @@ int nfs_readdir_add_to_array(struct nfs_entry *entry, struct page *page)
> array->last_cookie = entry->cookie;
> if (array->last_cookie <= cache_entry->cookie)
> array->cookies_are_ordered = 0;
> + array->index++;
> array->size++;
> if (entry->eof != 0)
> nfs_readdir_array_set_eof(array);
> @@ -365,13 +370,15 @@ nfs_readdir_page_valid(struct page *page, unsigned int entry_index, u64 index_co
> ret = true;
> array = kmap_atomic(page);
>
> - if ((array->size == 0 || array->size == entry_index)
> - && array->last_cookie == index_cookie)
> - goto out_unmap;
> + if (entry_index >= array->index - array->size) {
> + if ((array->size == 0 || array->size == entry_index)
> + && array->last_cookie == index_cookie)
> + goto out_unmap;
>
> - if (array->size > entry_index &&
> - array->array[entry_index].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:
> @@ -391,10 +398,10 @@ static struct page *nfs_readdir_page_get_locked(struct address_space *mapping,
> return page;
>
> if (!PageUptodate(page))
> - nfs_readdir_page_init_array(page, pgc->index_cookie);
> + nfs_readdir_page_init_array(page, pgc);
>
> if (!nfs_readdir_page_valid(page, pgc->entry_index, pgc->index_cookie))
> - nfs_readdir_recycle_page(page, pgc->index_cookie);
> + nfs_readdir_recycle_page(page, pgc);
>
> return page;
> }
> @@ -513,7 +520,7 @@ static bool nfs_readdir_array_cookie_in_range(struct nfs_cache_array *array,
> /* Optimisation for monotonically increasing cookies */
> if (cookie >= array->last_cookie)
> return false;
> - if (array->size && cookie < array->array[0].cookie)
> + if (array->size && cookie < array->array[array->index - array->size].cookie)
> return false;
> return true;
> }
> @@ -528,7 +535,7 @@ static int nfs_readdir_search_for_cookie(struct nfs_cache_array *array,
> if (!nfs_readdir_array_cookie_in_range(array, desc->dir_cookie))
> goto check_eof;
>
> - for (i = 0; i < array->size; i++) {
> + for (i = array->index - array->size; i < array->index; i++) {
> if (array->array[i].cookie == desc->dir_cookie) {
> struct nfs_inode *nfsi = NFS_I(file_inode(desc->file));
>
> @@ -1072,7 +1079,7 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc)
> unsigned int i = 0;
>
> array = kmap(desc->page);
> - for (i = desc->pgc.entry_index; i < array->size; i++) {
> + for (i = desc->pgc.entry_index; i < array->index; i++) {
> struct nfs_cache_array_entry *ent;
>
> ent = &array->array[i];
> --
> 2.25.4
>
On 21 Jan 2021, at 12:24, Anna Schumaker wrote:
> Hi Ben,
>
> On Wed, Jan 20, 2021 at 12:04 PM Benjamin Coddington
> <[email protected]> wrote:
>>
>> It is now possible that a reader will resume a directory listing
>> after an
>> invalidation and fill the rest of the pages with the offset left over
>> from
>> the last partially-filled page. These pages will then be recycled
>> and
>> refilled by the next reader since their alignment is incorrect.
>>
>> Add an index to the nfs_cache_array that will indicate where the next
>> entry
>> should be filled. This allows partially-filled pages to have the
>> best
>> alignment possible. They are more likely to be useful to readers
>> that
>> follow.
>>
>> This optimization targets the case when there are multiple processes
>> listing the directory simultaneously. Often the processes will
>> collect and
>> block on the same page waiting for a READDIR call to fill the
>> pagecache.
>> If the pagecache is invalidated, a partially-filled page will usually
>> result. This partially-filled page can immediately be used by all
>> processes to emit entries rather than having to discard and refill it
>> for
>> every process.
>>
>> The addition of another integer to struct nfs_cache_array increases
>> its
>> size to 24 bytes. We do not lose the original capacity of 127 entries
>> per
>> page.
>
> This patch causes cthon basic test #6 to start failing with unexpected
> dir entries across all NFS versions:
>
> ./test6: readdir
> basic tests failed
> ./test6: (/mnt/test/anna/Connectathon/cthon04) unexpected dir
> entry 'h'
Ah, yes -- because we /must/ revalidate before using any cached pages.
I
think this test unlinks files and that would be a breakage.
> Luckily, the next patch seems to resolve this issue. Could they maybe
> be squashed together?
Yes, they should be squashed - I'll do that in another pass. I'd like
to
get rid of uncached_readdir() altogether and then send another version.
Thanks for the test!
Ben