diff mbox series

[2/4] nfsd: NFSD_FILE_KEY_INODE only needs to find GC'ed entries

Message ID 20230105121512.21484-3-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series nfsd: filecache cleanups and optimizations | expand

Commit Message

Jeff Layton Jan. 5, 2023, 12:15 p.m. UTC 
Since v4 files are expected to be long-lived, there's little value in
closing them out of the cache when there is conflicting access.

Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC, and change the
comparator to also match the gc value in the key. Change both of the
current users of that key to set the gc value in the key to "true".

Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
!! from the condition later in the comparator.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Chuck Lever Jan. 5, 2023, 2:18 p.m. UTC | #1 
Hi Jeff-


> On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> Since v4 files are expected to be long-lived, there's little value in
> closing them out of the cache when there is conflicting access.

Seems like targeting long-lived nfsd_file items could actually
be a hazardous behavior. Are you sure it's safe to leave it in
stable kernels?


> Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC,

I'd prefer to keep the name as it is. It's for searching for
inodes, and adding the ".gc = true" to the search criteria is
enough to show what you are looking for.


> and change the
> comparator to also match the gc value in the key. Change both of the
> current users of that key to set the gc value in the key to "true".
> 
> Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
> !! from the condition later in the comparator.

And I'd prefer that you leave this clean up for another patch.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 9fff1fa09d08..a67b22579c6e 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -78,7 +78,7 @@ static struct rhashtable		nfsd_file_rhash_tbl
> 						____cacheline_aligned_in_smp;
> 
> enum nfsd_file_lookup_type {
> -	NFSD_FILE_KEY_INODE,
> +	NFSD_FILE_KEY_INODE_GC,
> 	NFSD_FILE_KEY_FULL,
> };
> 
> @@ -174,7 +174,9 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
> 	const struct nfsd_file *nf = ptr;
> 
> 	switch (key->type) {
> -	case NFSD_FILE_KEY_INODE:
> +	case NFSD_FILE_KEY_INODE_GC:
> +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> +			return 1;
> 		if (nf->nf_inode != key->inode)
> 			return 1;
> 		break;
> @@ -187,7 +189,7 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
> 			return 1;
> 		if (!nfsd_match_cred(nf->nf_cred, key->cred))
> 			return 1;
> -		if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> 			return 1;
> 		if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
> 			return 1;
> @@ -681,8 +683,9 @@ static void
> nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
> {
> 	struct nfsd_file_lookup_key key = {
> -		.type	= NFSD_FILE_KEY_INODE,
> +		.type	= NFSD_FILE_KEY_INODE_GC,
> 		.inode	= inode,
> +		.gc	= true,
> 	};
> 	struct nfsd_file *nf;
> 
> @@ -1057,8 +1060,9 @@ bool
> nfsd_file_is_cached(struct inode *inode)
> {
> 	struct nfsd_file_lookup_key key = {
> -		.type	= NFSD_FILE_KEY_INODE,
> +		.type	= NFSD_FILE_KEY_INODE_GC,
> 		.inode	= inode,
> +		.gc	= true,
> 	};
> 	bool ret = false;
> 
> -- 
> 2.39.0
> 

--
Chuck Lever
Jeff Layton Jan. 5, 2023, 2:29 p.m. UTC | #2 
On Thu, 2023-01-05 at 14:18 +0000, Chuck Lever III wrote:
> Hi Jeff-
> 
> 
> > On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > Since v4 files are expected to be long-lived, there's little value in
> > closing them out of the cache when there is conflicting access.
> 
> Seems like targeting long-lived nfsd_file items could actually
> be a hazardous behavior. Are you sure it's safe to leave it in
> stable kernels?
> 

Basically it just means we end up doing more opens than are needed in
this situation.

The notifiers just unnecessarily unhash a nfsd_file in this case, even
though it doesn't have any chance of freeing it until the client issues
a CLOSE, due to the persistent references held by the stateids.

So, this really is just an optimization and not a "real bug".

> 
> > Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC,
> 
> I'd prefer to keep the name as it is. It's for searching for
> inodes, and adding the ".gc = true" to the search criteria is
> enough to show what you are looking for.
> 

Ok.

> 
> > and change the
> > comparator to also match the gc value in the key. Change both of the
> > current users of that key to set the gc value in the key to "true".
> > 
> > Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
> > !! from the condition later in the comparator.
> 
> And I'd prefer that you leave this clean up for another patch.
> 

Ok, I'll drop that hunk.

> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/nfsd/filecache.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 9fff1fa09d08..a67b22579c6e 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -78,7 +78,7 @@ static struct rhashtable		nfsd_file_rhash_tbl
> > 						____cacheline_aligned_in_smp;
> > 
> > enum nfsd_file_lookup_type {
> > -	NFSD_FILE_KEY_INODE,
> > +	NFSD_FILE_KEY_INODE_GC,
> > 	NFSD_FILE_KEY_FULL,
> > };
> > 
> > @@ -174,7 +174,9 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
> > 	const struct nfsd_file *nf = ptr;
> > 
> > 	switch (key->type) {
> > -	case NFSD_FILE_KEY_INODE:
> > +	case NFSD_FILE_KEY_INODE_GC:
> > +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> > +			return 1;
> > 		if (nf->nf_inode != key->inode)
> > 			return 1;
> > 		break;
> > @@ -187,7 +189,7 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
> > 			return 1;
> > 		if (!nfsd_match_cred(nf->nf_cred, key->cred))
> > 			return 1;
> > -		if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> > +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
> > 			return 1;
> > 		if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
> > 			return 1;
> > @@ -681,8 +683,9 @@ static void
> > nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
> > {
> > 	struct nfsd_file_lookup_key key = {
> > -		.type	= NFSD_FILE_KEY_INODE,
> > +		.type	= NFSD_FILE_KEY_INODE_GC,
> > 		.inode	= inode,
> > +		.gc	= true,
> > 	};
> > 	struct nfsd_file *nf;
> > 
> > @@ -1057,8 +1060,9 @@ bool
> > nfsd_file_is_cached(struct inode *inode)
> > {
> > 	struct nfsd_file_lookup_key key = {
> > -		.type	= NFSD_FILE_KEY_INODE,
> > +		.type	= NFSD_FILE_KEY_INODE_GC,
> > 		.inode	= inode,
> > +		.gc	= true,
> > 	};
> > 	bool ret = false;
> > 
> > -- 
> > 2.39.0
> > 
> 
> --
> Chuck Lever
> 
> 
>
Chuck Lever Jan. 5, 2023, 2:32 p.m. UTC | #3 
> On Jan 5, 2023, at 9:29 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2023-01-05 at 14:18 +0000, Chuck Lever III wrote:
>> Hi Jeff-
>> 
>> 
>>> On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> Since v4 files are expected to be long-lived, there's little value in
>>> closing them out of the cache when there is conflicting access.
>> 
>> Seems like targeting long-lived nfsd_file items could actually
>> be a hazardous behavior. Are you sure it's safe to leave it in
>> stable kernels?
>> 
> 
> Basically it just means we end up doing more opens than are needed in
> this situation.
> 
> The notifiers just unnecessarily unhash a nfsd_file in this case, even
> though it doesn't have any chance of freeing it until the client issues
> a CLOSE, due to the persistent references held by the stateids.
> 
> So, this really is just an optimization and not a "real bug".

Good to know. Let's add this to the patch description when you
redrive...?


>>> Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC,
>> 
>> I'd prefer to keep the name as it is. It's for searching for
>> inodes, and adding the ".gc = true" to the search criteria is
>> enough to show what you are looking for.
>> 
> 
> Ok.
> 
>> 
>>> and change the
>>> comparator to also match the gc value in the key. Change both of the
>>> current users of that key to set the gc value in the key to "true".
>>> 
>>> Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
>>> !! from the condition later in the comparator.
>> 
>> And I'd prefer that you leave this clean up for another patch.
>> 
> 
> Ok, I'll drop that hunk.
> 
>> 
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>> fs/nfsd/filecache.c | 14 +++++++++-----
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index 9fff1fa09d08..a67b22579c6e 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -78,7 +78,7 @@ static struct rhashtable		nfsd_file_rhash_tbl
>>> 						____cacheline_aligned_in_smp;
>>> 
>>> enum nfsd_file_lookup_type {
>>> -	NFSD_FILE_KEY_INODE,
>>> +	NFSD_FILE_KEY_INODE_GC,
>>> 	NFSD_FILE_KEY_FULL,
>>> };
>>> 
>>> @@ -174,7 +174,9 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
>>> 	const struct nfsd_file *nf = ptr;
>>> 
>>> 	switch (key->type) {
>>> -	case NFSD_FILE_KEY_INODE:
>>> +	case NFSD_FILE_KEY_INODE_GC:
>>> +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
>>> +			return 1;
>>> 		if (nf->nf_inode != key->inode)
>>> 			return 1;
>>> 		break;
>>> @@ -187,7 +189,7 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
>>> 			return 1;
>>> 		if (!nfsd_match_cred(nf->nf_cred, key->cred))
>>> 			return 1;
>>> -		if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
>>> +		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
>>> 			return 1;
>>> 		if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
>>> 			return 1;
>>> @@ -681,8 +683,9 @@ static void
>>> nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
>>> {
>>> 	struct nfsd_file_lookup_key key = {
>>> -		.type	= NFSD_FILE_KEY_INODE,
>>> +		.type	= NFSD_FILE_KEY_INODE_GC,
>>> 		.inode	= inode,
>>> +		.gc	= true,
>>> 	};
>>> 	struct nfsd_file *nf;
>>> 
>>> @@ -1057,8 +1060,9 @@ bool
>>> nfsd_file_is_cached(struct inode *inode)
>>> {
>>> 	struct nfsd_file_lookup_key key = {
>>> -		.type	= NFSD_FILE_KEY_INODE,
>>> +		.type	= NFSD_FILE_KEY_INODE_GC,
>>> 		.inode	= inode,
>>> +		.gc	= true,
>>> 	};
>>> 	bool ret = false;
>>> 
>>> -- 
>>> 2.39.0
>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever
diff mbox series

Patch

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 9fff1fa09d08..a67b22579c6e 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -78,7 +78,7 @@  static struct rhashtable		nfsd_file_rhash_tbl
 						____cacheline_aligned_in_smp;
 
 enum nfsd_file_lookup_type {
-	NFSD_FILE_KEY_INODE,
+	NFSD_FILE_KEY_INODE_GC,
 	NFSD_FILE_KEY_FULL,
 };
 
@@ -174,7 +174,9 @@  static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
 	const struct nfsd_file *nf = ptr;
 
 	switch (key->type) {
-	case NFSD_FILE_KEY_INODE:
+	case NFSD_FILE_KEY_INODE_GC:
+		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
+			return 1;
 		if (nf->nf_inode != key->inode)
 			return 1;
 		break;
@@ -187,7 +189,7 @@  static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,
 			return 1;
 		if (!nfsd_match_cred(nf->nf_cred, key->cred))
 			return 1;
-		if (!!test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
+		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
 			return 1;
 		if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
 			return 1;
@@ -681,8 +683,9 @@  static void
 nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
 {
 	struct nfsd_file_lookup_key key = {
-		.type	= NFSD_FILE_KEY_INODE,
+		.type	= NFSD_FILE_KEY_INODE_GC,
 		.inode	= inode,
+		.gc	= true,
 	};
 	struct nfsd_file *nf;
 
@@ -1057,8 +1060,9 @@  bool
 nfsd_file_is_cached(struct inode *inode)
 {
 	struct nfsd_file_lookup_key key = {
-		.type	= NFSD_FILE_KEY_INODE,
+		.type	= NFSD_FILE_KEY_INODE_GC,
 		.inode	= inode,
+		.gc	= true,
 	};
 	bool ret = false;