[PATCH RFC] NFSD: synchronously unhash a file on NFSv4 CLOSE

[PATCH RFC] NFSD: synchronously unhash a file on NFSv4 CLOSE

[Chuck Lever]

I recently observed a significant slowdown in a long-running
test on NFSv4.0 mounts.

An OPEN for write seems to block the NFS server from offering
delegations on that file for a few seconds. The problem is that
when that file is closed, the filecache retains an open-for-write
file descriptor until the laundrette runs. That keeps the inode's
i_writecount positive until the cached file is finally unhashed
and closed.

Force the NFSv4 CLOSE logic to call filp_close() to eliminate
the underlying cached open-for-write file as soon as the client
closes the file.

This minor change claws back about 80% of the performance
regression.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3ac40ba7efe1..0b3059b8b36c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -613,10 +613,14 @@ static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
 		if (atomic_read(&fp->fi_access[1 - oflag]) == 0)
 			swap(f2, fp->fi_fds[O_RDWR]);
 		spin_unlock(&fp->fi_lock);
-		if (f1)
+		if (f1) {
+			nfsd_file_close_inode_sync(locks_inode(f1->nf_file));
 			nfsd_file_put(f1);
-		if (f2)
+		}
+		if (f2) {
+			nfsd_file_close_inode_sync(locks_inode(f2->nf_file));
 			nfsd_file_put(f2);
+		}
 	}
 }
 

Re: [PATCH RFC] NFSD: synchronously unhash a file on NFSv4 CLOSE

[J. Bruce Fields]

On Thu, Sep 10, 2020 at 03:45:12PM -0400, Chuck Lever wrote:
> I recently observed a significant slowdown in a long-running
> test on NFSv4.0 mounts.
> 
> An OPEN for write seems to block the NFS server from offering
> delegations on that file for a few seconds. The problem is that
> when that file is closed, the filecache retains an open-for-write
> file descriptor until the laundrette runs. That keeps the inode's
> i_writecount positive until the cached file is finally unhashed
> and closed.
> 
> Force the NFSv4 CLOSE logic to call filp_close() to eliminate
> the underlying cached open-for-write file as soon as the client
> closes the file.
> 
> This minor change claws back about 80% of the performance
> regression.

That's really useful to know.  But mainly this makes me think that
nfsd4_check_conflicting_opens() is wrong.

I'm trying to determine whether a given file has a non-nfsd writer by
counting the number of write opens nfsd holds on a given file and
subtracting that from i_writecount.

But the way I'm counting write opens probably isn't correct.

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4state.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3ac40ba7efe1..0b3059b8b36c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -613,10 +613,14 @@ static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
>  		if (atomic_read(&fp->fi_access[1 - oflag]) == 0)
>  			swap(f2, fp->fi_fds[O_RDWR]);
>  		spin_unlock(&fp->fi_lock);
> -		if (f1)
> +		if (f1) {
> +			nfsd_file_close_inode_sync(locks_inode(f1->nf_file));
>  			nfsd_file_put(f1);
> -		if (f2)
> +		}
> +		if (f2) {
> +			nfsd_file_close_inode_sync(locks_inode(f2->nf_file));
>  			nfsd_file_put(f2);
> +		}
>  	}
>  }
>  
> 

Re: [PATCH RFC] NFSD: synchronously unhash a file on NFSv4 CLOSE

[Chuck Lever]

> On Sep 14, 2020, at 4:51 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Thu, Sep 10, 2020 at 03:45:12PM -0400, Chuck Lever wrote:
>> I recently observed a significant slowdown in a long-running
>> test on NFSv4.0 mounts.
>> 
>> An OPEN for write seems to block the NFS server from offering
>> delegations on that file for a few seconds. The problem is that
>> when that file is closed, the filecache retains an open-for-write
>> file descriptor until the laundrette runs. That keeps the inode's
>> i_writecount positive until the cached file is finally unhashed
>> and closed.
>> 
>> Force the NFSv4 CLOSE logic to call filp_close() to eliminate
>> the underlying cached open-for-write file as soon as the client
>> closes the file.
>> 
>> This minor change claws back about 80% of the performance
>> regression.
> 
> That's really useful to know.  But mainly this makes me think that
> nfsd4_check_conflicting_opens() is wrong.
> 
> I'm trying to determine whether a given file has a non-nfsd writer by
> counting the number of write opens nfsd holds on a given file and
> subtracting that from i_writecount.
> 
> But the way I'm counting write opens probably isn't correct.

I entertained that possibility, but I couldn't figure out
a better way to count opens-for-write.


> --b.
> 
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfsd/nfs4state.c |    8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 3ac40ba7efe1..0b3059b8b36c 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -613,10 +613,14 @@ static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
>> 		if (atomic_read(&fp->fi_access[1 - oflag]) == 0)
>> 			swap(f2, fp->fi_fds[O_RDWR]);
>> 		spin_unlock(&fp->fi_lock);
>> -		if (f1)
>> +		if (f1) {
>> +			nfsd_file_close_inode_sync(locks_inode(f1->nf_file));
>> 			nfsd_file_put(f1);
>> -		if (f2)
>> +		}
>> +		if (f2) {
>> +			nfsd_file_close_inode_sync(locks_inode(f2->nf_file));
>> 			nfsd_file_put(f2);
>> +		}
>> 	}
>> }

--
Chuck Lever

Re: [PATCH RFC] NFSD: synchronously unhash a file on NFSv4 CLOSE

[Chuck Lever]

> On Sep 14, 2020, at 4:53 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> 
> 
>> On Sep 14, 2020, at 4:51 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> 
>> On Thu, Sep 10, 2020 at 03:45:12PM -0400, Chuck Lever wrote:
>>> I recently observed a significant slowdown in a long-running
>>> test on NFSv4.0 mounts.
>>> 
>>> An OPEN for write seems to block the NFS server from offering
>>> delegations on that file for a few seconds. The problem is that
>>> when that file is closed, the filecache retains an open-for-write
>>> file descriptor until the laundrette runs. That keeps the inode's
>>> i_writecount positive until the cached file is finally unhashed
>>> and closed.
>>> 
>>> Force the NFSv4 CLOSE logic to call filp_close() to eliminate
>>> the underlying cached open-for-write file as soon as the client
>>> closes the file.
>>> 
>>> This minor change claws back about 80% of the performance
>>> regression.
>> 
>> That's really useful to know.  But mainly this makes me think that
>> nfsd4_check_conflicting_opens() is wrong.
>> 
>> I'm trying to determine whether a given file has a non-nfsd writer by
>> counting the number of write opens nfsd holds on a given file and
>> subtracting that from i_writecount.
>> 
>> But the way I'm counting write opens probably isn't correct.
> 
> I entertained that possibility, but I couldn't figure out
> a better way to count opens-for-write.

Also, the regression started long before nfsd4_check_conflicting_opens
was added.


>> --b.
>> 
>>> 
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> fs/nfsd/nfs4state.c |    8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 3ac40ba7efe1..0b3059b8b36c 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -613,10 +613,14 @@ static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
>>> 		if (atomic_read(&fp->fi_access[1 - oflag]) == 0)
>>> 			swap(f2, fp->fi_fds[O_RDWR]);
>>> 		spin_unlock(&fp->fi_lock);
>>> -		if (f1)
>>> +		if (f1) {
>>> +			nfsd_file_close_inode_sync(locks_inode(f1->nf_file));
>>> 			nfsd_file_put(f1);
>>> -		if (f2)
>>> +		}
>>> +		if (f2) {
>>> +			nfsd_file_close_inode_sync(locks_inode(f2->nf_file));
>>> 			nfsd_file_put(f2);
>>> +		}
>>> 	}
>>> }
> 
> --
> Chuck Lever

--
Chuck Lever