* [PATCH 0/2] NFS: Delay Retry Proposed Changes
@ 2020-11-02 16:24 Wenle Chen
2020-11-02 16:24 ` [PATCH 1/2] NFS: Reduce redundant comparison Wenle Chen
2020-11-02 16:24 ` [PATCH 2/2] NFS: Limit the number of retries Wenle Chen
0 siblings, 2 replies; 7+ messages in thread
From: Wenle Chen @ 2020-11-02 16:24 UTC (permalink / raw)
To: trond.myklebust, anna.schumaker
Cc: linux-nfs, linux-kernel, chenwenle, solachenclever, nixiaoming,
solachenclever
Little modify for nfs4_lock_delegation_recall delay retry
Wenle Chen (2):
NFS: Reduce redundant comparison
NFS: Limit the number of retries
fs/nfs/nfs4proc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--
2.29.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] NFS: Reduce redundant comparison
2020-11-02 16:24 [PATCH 0/2] NFS: Delay Retry Proposed Changes Wenle Chen
@ 2020-11-02 16:24 ` Wenle Chen
2020-11-02 16:24 ` [PATCH 2/2] NFS: Limit the number of retries Wenle Chen
1 sibling, 0 replies; 7+ messages in thread
From: Wenle Chen @ 2020-11-02 16:24 UTC (permalink / raw)
To: trond.myklebust, anna.schumaker
Cc: linux-nfs, linux-kernel, chenwenle, solachenclever, nixiaoming,
solachenclever
The err wouldn't be change after sleep. There is no need
to compare err and -NFS4ERR_DELAY
Signed-off-by: Wenle Chen <chenwenle@huawei.com>
---
fs/nfs/nfs4proc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e0ca9b2b210..f6b5dc792b33 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7399,7 +7399,7 @@ int nfs4_lock_delegation_recall(struct file_lock *fl, struct nfs4_state *state,
if (err != -NFS4ERR_DELAY)
break;
ssleep(1);
- } while (err == -NFS4ERR_DELAY);
+ } while (1);
return nfs4_handle_delegation_recall_error(server, state, stateid, fl, err);
}
--
2.29.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] NFS: Limit the number of retries
2020-11-02 16:24 [PATCH 0/2] NFS: Delay Retry Proposed Changes Wenle Chen
2020-11-02 16:24 ` [PATCH 1/2] NFS: Reduce redundant comparison Wenle Chen
@ 2020-11-02 16:24 ` Wenle Chen
2020-11-02 17:45 ` Trond Myklebust
1 sibling, 1 reply; 7+ messages in thread
From: Wenle Chen @ 2020-11-02 16:24 UTC (permalink / raw)
To: trond.myklebust, anna.schumaker
Cc: linux-nfs, linux-kernel, chenwenle, solachenclever, nixiaoming,
solachenclever
We can't wait forever, even if the state
is always delayed.
Signed-off-by: Wenle Chen <chenwenle@huawei.com>
---
fs/nfs/nfs4proc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f6b5dc792b33..bb2316bf13f6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7390,15 +7390,17 @@ int nfs4_lock_delegation_recall(struct file_lock *fl, struct nfs4_state *state,
{
struct nfs_server *server = NFS_SERVER(state->inode);
int err;
+ int retry = 3;
err = nfs4_set_lock_state(state, fl);
if (err != 0)
return err;
do {
err = _nfs4_do_setlk(state, F_SETLK, fl, NFS_LOCK_NEW);
- if (err != -NFS4ERR_DELAY)
+ if (err != -NFS4ERR_DELAY || retry == 0)
break;
ssleep(1);
+ --retry;
} while (1);
return nfs4_handle_delegation_recall_error(server, state, stateid, fl, err);
}
--
2.29.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] NFS: Limit the number of retries
2020-11-02 16:24 ` [PATCH 2/2] NFS: Limit the number of retries Wenle Chen
@ 2020-11-02 17:45 ` Trond Myklebust
2020-11-04 11:33 ` Wenle Chen
0 siblings, 1 reply; 7+ messages in thread
From: Trond Myklebust @ 2020-11-02 17:45 UTC (permalink / raw)
To: anna.schumaker, solomonchenclever
Cc: chenwenle, linux-nfs, solachenclever, linux-kernel, nixiaoming,
solachenclever
On Tue, 2020-11-03 at 00:24 +0800, Wenle Chen wrote:
> We can't wait forever, even if the state
> is always delayed.
>
> Signed-off-by: Wenle Chen <chenwenle@huawei.com>
> ---
> fs/nfs/nfs4proc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index f6b5dc792b33..bb2316bf13f6 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7390,15 +7390,17 @@ int nfs4_lock_delegation_recall(struct
> file_lock *fl, struct nfs4_state *state,
> {
> struct nfs_server *server = NFS_SERVER(state->inode);
> int err;
> + int retry = 3;
>
> err = nfs4_set_lock_state(state, fl);
> if (err != 0)
> return err;
> do {
> err = _nfs4_do_setlk(state, F_SETLK, fl,
> NFS_LOCK_NEW);
> - if (err != -NFS4ERR_DELAY)
> + if (err != -NFS4ERR_DELAY || retry == 0)
> break;
> ssleep(1);
> + --retry;
> } while (1);
> return nfs4_handle_delegation_recall_error(server, state,
> stateid, fl, err);
> }
This patch will just cause the locks to be silently lost, no?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] NFS: Limit the number of retries
2020-11-02 17:45 ` Trond Myklebust
@ 2020-11-04 11:33 ` Wenle Chen
2020-11-04 13:22 ` Olga Kornievskaia
0 siblings, 1 reply; 7+ messages in thread
From: Wenle Chen @ 2020-11-04 11:33 UTC (permalink / raw)
To: Trond Myklebust, anna.schumaker
Cc: chenwenle, linux-nfs, linux-kernel, nixiaoming
Trond Myklebust 於 2020/11/3 上午1:45 寫道:
> On Tue, 2020-11-03 at 00:24 +0800, Wenle Chen wrote:
>> We can't wait forever, even if the state
>> is always delayed.
>>
>> Signed-off-by: Wenle Chen <chenwenle@huawei.com>
>> ---
>> fs/nfs/nfs4proc.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index f6b5dc792b33..bb2316bf13f6 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -7390,15 +7390,17 @@ int nfs4_lock_delegation_recall(struct
>> file_lock *fl, struct nfs4_state *state,
>> {
>> struct nfs_server *server = NFS_SERVER(state->inode);
>> int err;
>> + int retry = 3;
>>
>> err = nfs4_set_lock_state(state, fl);
>> if (err != 0)
>> return err;
>> do {
>> err = _nfs4_do_setlk(state, F_SETLK, fl,
>> NFS_LOCK_NEW);
>> - if (err != -NFS4ERR_DELAY)
>> + if (err != -NFS4ERR_DELAY || retry == 0)
>> break;
>> ssleep(1);
>> + --retry;
>> } while (1);
>> return nfs4_handle_delegation_recall_error(server, state,
>> stateid, fl, err);
>> }
>
> This patch will just cause the locks to be silently lost, no?
>
This loop was introduced in commit 3d7a9520f0c3e to simplify the delay
retry loop. Before this, the function nfs4_lock_delegation_recall would
return a -EAGAIN to do a whole retry loop.
When we retried three times and waited three seconds, it was still in
delay. I think we can get a whole loop and check the other points if it
was changed or not. It is just a proposal.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] NFS: Limit the number of retries
2020-11-04 11:33 ` Wenle Chen
@ 2020-11-04 13:22 ` Olga Kornievskaia
2020-11-04 15:51 ` Wenle Chen
0 siblings, 1 reply; 7+ messages in thread
From: Olga Kornievskaia @ 2020-11-04 13:22 UTC (permalink / raw)
To: Wenle Chen
Cc: Trond Myklebust, anna.schumaker, chenwenle, linux-nfs,
linux-kernel, nixiaoming
On Wed, Nov 4, 2020 at 6:36 AM Wenle Chen <solomonchenclever@gmail.com> wrote:
>
>
>
> Trond Myklebust 於 2020/11/3 上午1:45 寫道:
> > On Tue, 2020-11-03 at 00:24 +0800, Wenle Chen wrote:
> >> We can't wait forever, even if the state
> >> is always delayed.
> >>
> >> Signed-off-by: Wenle Chen <chenwenle@huawei.com>
> >> ---
> >> fs/nfs/nfs4proc.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index f6b5dc792b33..bb2316bf13f6 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> >> @@ -7390,15 +7390,17 @@ int nfs4_lock_delegation_recall(struct
> >> file_lock *fl, struct nfs4_state *state,
> >> {
> >> struct nfs_server *server = NFS_SERVER(state->inode);
> >> int err;
> >> + int retry = 3;
> >>
> >> err = nfs4_set_lock_state(state, fl);
> >> if (err != 0)
> >> return err;
> >> do {
> >> err = _nfs4_do_setlk(state, F_SETLK, fl,
> >> NFS_LOCK_NEW);
> >> - if (err != -NFS4ERR_DELAY)
> >> + if (err != -NFS4ERR_DELAY || retry == 0)
> >> break;
> >> ssleep(1);
> >> + --retry;
> >> } while (1);
> >> return nfs4_handle_delegation_recall_error(server, state,
> >> stateid, fl, err);
> >> }
> >
> > This patch will just cause the locks to be silently lost, no?
> >
> This loop was introduced in commit 3d7a9520f0c3e to simplify the delay
> retry loop. Before this, the function nfs4_lock_delegation_recall would
> return a -EAGAIN to do a whole retry loop.
This commit was not simplifying retry but actually handling the error.
Without it the error isn't handled and client falsely thinks it holds
the lock. Limiting the number of retries as Trond points out would
lead to the same problem which in the end is data corruption.
Alternative would be to fail the application. However ERR_DELAY is a
transient error and the server would, when ready, return something
else. If server is broken and continues to do so then the server needs
to be fix (client isn't coded to the broken server). I don't see a
good argument for limiting the number of re-tries.
> When we retried three times and waited three seconds, it was still in
> delay. I think we can get a whole loop and check the other points if it
> was changed or not. It is just a proposal.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] NFS: Limit the number of retries
2020-11-04 13:22 ` Olga Kornievskaia
@ 2020-11-04 15:51 ` Wenle Chen
0 siblings, 0 replies; 7+ messages in thread
From: Wenle Chen @ 2020-11-04 15:51 UTC (permalink / raw)
To: Olga Kornievskaia
Cc: Trond Myklebust, anna.schumaker, chenwenle, linux-nfs,
linux-kernel, nixiaoming
Olga Kornievskaia 於 2020/11/4 下午9:22 寫道:
> On Wed, Nov 4, 2020 at 6:36 AM Wenle Chen <solomonchenclever@gmail.com> wrote:
>>
>>
>>
>> Trond Myklebust 於 2020/11/3 上午1:45 寫道:
>>> On Tue, 2020-11-03 at 00:24 +0800, Wenle Chen wrote:
>>>> We can't wait forever, even if the state
>>>> is always delayed.
>>>>
>>>> Signed-off-by: Wenle Chen <chenwenle@huawei.com>
>>>> ---
>>>> fs/nfs/nfs4proc.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index f6b5dc792b33..bb2316bf13f6 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -7390,15 +7390,17 @@ int nfs4_lock_delegation_recall(struct
>>>> file_lock *fl, struct nfs4_state *state,
>>>> {
>>>> struct nfs_server *server = NFS_SERVER(state->inode);
>>>> int err;
>>>> + int retry = 3;
>>>>
>>>> err = nfs4_set_lock_state(state, fl);
>>>> if (err != 0)
>>>> return err;
>>>> do {
>>>> err = _nfs4_do_setlk(state, F_SETLK, fl,
>>>> NFS_LOCK_NEW);
>>>> - if (err != -NFS4ERR_DELAY)
>>>> + if (err != -NFS4ERR_DELAY || retry == 0)
>>>> break;
>>>> ssleep(1);
>>>> + --retry;
>>>> } while (1);
>>>> return nfs4_handle_delegation_recall_error(server, state,
>>>> stateid, fl, err);
>>>> }
>>>
>>> This patch will just cause the locks to be silently lost, no?
>>>
>> This loop was introduced in commit 3d7a9520f0c3e to simplify the delay
>> retry loop. Before this, the function nfs4_lock_delegation_recall would
>> return a -EAGAIN to do a whole retry loop.
>
> This commit was not simplifying retry but actually handling the error.
> Without it the error isn't handled and client falsely thinks it holds
> the lock. Limiting the number of retries as Trond points out would
> lead to the same problem which in the end is data corruption.
> Alternative would be to fail the application. However ERR_DELAY is a
> transient error and the server would, when ready, return something
> else. If server is broken and continues to do so then the server needs
> to be fix (client isn't coded to the broken server). I don't see a
> good argument for limiting the number of re-tries.
>
>> When we retried three times and waited three seconds, it was still in
>> delay. I think we can get a whole loop and check the other points if it
>> was changed or not. It is just a proposal.
In the function nfs_end_delegation_return, it would get the return
err=-EAGAIN and check the client is active and get a retry. I has so
thought. Maybe I think wrong. I will understand more carefully. Thinks.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-04 15:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 16:24 [PATCH 0/2] NFS: Delay Retry Proposed Changes Wenle Chen
2020-11-02 16:24 ` [PATCH 1/2] NFS: Reduce redundant comparison Wenle Chen
2020-11-02 16:24 ` [PATCH 2/2] NFS: Limit the number of retries Wenle Chen
2020-11-02 17:45 ` Trond Myklebust
2020-11-04 11:33 ` Wenle Chen
2020-11-04 13:22 ` Olga Kornievskaia
2020-11-04 15:51 ` Wenle Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).