| Message ID | 20220223211305.296816-5-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Readdir improvements | expand |
On 23 Feb 2022, at 16:12, trondmy@kernel.org wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > Instead of relying on counting the page offsets as we walk through the > page cache, switch to calculating them algorithmically. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfs/dir.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index 8f17aaebcd77..f2258e926df2 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -248,17 +248,20 @@ static const char *nfs_readdir_copy_name(const > char *name, unsigned int len) > return ret; > } > > +static size_t nfs_readdir_array_maxentries(void) > +{ > + return (PAGE_SIZE - sizeof(struct nfs_cache_array)) / > + sizeof(struct nfs_cache_array_entry); > +} > + Why the choice to use a runtime function call rather than the compiler's calculation? I suspect that the end result is the same, as the compiler will optimize it away, but I'm curious if there's a good reason for this. Ben
On Thu, 2022-02-24 at 09:15 -0500, Benjamin Coddington wrote: > On 23 Feb 2022, at 16:12, trondmy@kernel.org wrote: > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > Instead of relying on counting the page offsets as we walk through > > the > > page cache, switch to calculating them algorithmically. > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfs/dir.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > > index 8f17aaebcd77..f2258e926df2 100644 > > --- a/fs/nfs/dir.c > > +++ b/fs/nfs/dir.c > > @@ -248,17 +248,20 @@ static const char > > *nfs_readdir_copy_name(const > > char *name, unsigned int len) > > return ret; > > } > > > > +static size_t nfs_readdir_array_maxentries(void) > > +{ > > + return (PAGE_SIZE - sizeof(struct nfs_cache_array)) / > > + sizeof(struct nfs_cache_array_entry); > > +} > > + > > Why the choice to use a runtime function call rather than the > compiler's > calculation? I suspect that the end result is the same, as the > compiler > will optimize it away, but I'm curious if there's a good reason for > this. > The comparison is more efficient because no pointer arithmetic is needed. As you said, the above function always evaluates to a constant, and the array->size has been pre-calculated.
On 24 Feb 2022, at 21:11, Trond Myklebust wrote: > On Thu, 2022-02-24 at 09:15 -0500, Benjamin Coddington wrote: >> On 23 Feb 2022, at 16:12, trondmy@kernel.org wrote: >> >>> From: Trond Myklebust <trond.myklebust@hammerspace.com> >>> >>> Instead of relying on counting the page offsets as we walk through >>> the >>> page cache, switch to calculating them algorithmically. >>> >>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> >>> --- >>> fs/nfs/dir.c | 18 +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c >>> index 8f17aaebcd77..f2258e926df2 100644 >>> --- a/fs/nfs/dir.c >>> +++ b/fs/nfs/dir.c >>> @@ -248,17 +248,20 @@ static const char >>> *nfs_readdir_copy_name(const >>> char *name, unsigned int len) >>> return ret; >>> } >>> >>> +static size_t nfs_readdir_array_maxentries(void) >>> +{ >>> + return (PAGE_SIZE - sizeof(struct nfs_cache_array)) / >>> + sizeof(struct nfs_cache_array_entry); >>> +} >>> + >> >> Why the choice to use a runtime function call rather than the >> compiler's >> calculation? I suspect that the end result is the same, as the >> compiler >> will optimize it away, but I'm curious if there's a good reason for >> this. >> > > The comparison is more efficient because no pointer arithmetic is > needed. As you said, the above function always evaluates to a constant, > and the array->size has been pre-calculated. Comparisons are more efficient than using something like this?: static const int nfs_readdir_array_maxentries = (PAGE_SIZE - sizeof(struct nfs_cache_array)) / sizeof(struct nfs_cache_array_entry); I don't understand why, I must admit. I'm not saying it should be changed, I'm just trying to figure out the reason for the function declaration when the value is a constant, and I thought there was a hole in my head. Ben
On Fri, 2022-02-25 at 06:28 -0500, Benjamin Coddington wrote: > On 24 Feb 2022, at 21:11, Trond Myklebust wrote: > > > On Thu, 2022-02-24 at 09:15 -0500, Benjamin Coddington wrote: > > > On 23 Feb 2022, at 16:12, trondmy@kernel.org wrote: > > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > > > Instead of relying on counting the page offsets as we walk > > > > through > > > > the > > > > page cache, switch to calculating them algorithmically. > > > > > > > > Signed-off-by: Trond Myklebust > > > > <trond.myklebust@hammerspace.com> > > > > --- > > > > fs/nfs/dir.c | 18 +++++++++++++----- > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > > > > index 8f17aaebcd77..f2258e926df2 100644 > > > > --- a/fs/nfs/dir.c > > > > +++ b/fs/nfs/dir.c > > > > @@ -248,17 +248,20 @@ static const char > > > > *nfs_readdir_copy_name(const > > > > char *name, unsigned int len) > > > > return ret; > > > > } > > > > > > > > +static size_t nfs_readdir_array_maxentries(void) > > > > +{ > > > > + return (PAGE_SIZE - sizeof(struct nfs_cache_array)) / > > > > + sizeof(struct nfs_cache_array_entry); > > > > +} > > > > + > > > > > > Why the choice to use a runtime function call rather than the > > > compiler's > > > calculation? I suspect that the end result is the same, as the > > > compiler > > > will optimize it away, but I'm curious if there's a good reason > > > for > > > this. > > > > > > > The comparison is more efficient because no pointer arithmetic is > > needed. As you said, the above function always evaluates to a > > constant, > > and the array->size has been pre-calculated. > > Comparisons are more efficient than using something like this?: > > static const int nfs_readdir_array_maxentries = > (PAGE_SIZE - sizeof(struct nfs_cache_array)) / > sizeof(struct nfs_cache_array_entry); > > I don't understand why, I must admit. I'm not saying it should be > changed, > I'm just trying to figure out the reason for the function declaration > when > the value is a constant, and I thought there was a hole in my head. > Unless we're talking about a compiler from the 1960s, there is little difference between the two proposals. Any modern C compiler worth its salt will know to inline the numeric value into the comparison (and will optimise away the storage of your variable).
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 8f17aaebcd77..f2258e926df2 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -248,17 +248,20 @@ static const char *nfs_readdir_copy_name(const char *name, unsigned int len) return ret; } +static size_t nfs_readdir_array_maxentries(void) +{ + return (PAGE_SIZE - sizeof(struct nfs_cache_array)) / + sizeof(struct nfs_cache_array_entry); +} + /* * Check that the next array entry lies entirely within the page bounds */ static int nfs_readdir_array_can_expand(struct nfs_cache_array *array) { - struct nfs_cache_array_entry *cache_entry; - if (array->page_full) return -ENOSPC; - cache_entry = &array->array[array->size + 1]; - if ((char *)cache_entry - (char *)array > PAGE_SIZE) { + if (array->size == nfs_readdir_array_maxentries()) { array->page_full = 1; return -ENOSPC; } @@ -317,6 +320,11 @@ static struct page *nfs_readdir_page_get_locked(struct address_space *mapping, return page; } +static loff_t nfs_readdir_page_offset(struct page *page) +{ + return (loff_t)page->index * (loff_t)nfs_readdir_array_maxentries(); +} + static u64 nfs_readdir_page_last_cookie(struct page *page) { struct nfs_cache_array *array; @@ -447,7 +455,7 @@ static int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, if (array->array[i].cookie == desc->dir_cookie) { struct nfs_inode *nfsi = NFS_I(file_inode(desc->file)); - new_pos = desc->current_index + i; + new_pos = nfs_readdir_page_offset(desc->page) + i; if (desc->attr_gencount != nfsi->attr_gencount || !nfs_readdir_inode_mapping_valid(nfsi)) { desc->duped = 0;