How to handle revocation of locking state

How to handle revocation of locking state

[Olga Kornievskaia]

Hi folks,

Looking for recommendation on what the client is suppose to be doing
in the following situation. Client opens a file and has a byte-range
lock which returned a locking state. Client is acquiring another byte
range lock. It uses the returned locking stated for the 2nd lock.
Server returns ADMIN_REVOKED.

Currently the client goes into an infinite loop of just resending the
same LOCK operation with
the same locking stateid.

Is this a recoverable situation? The fact that the lock state was
revoked, should it be an automatic EIO since previous lock is lost (so
why bother going forward)? Or should the client retry the lock but
send it with the open stateid?

Thank you.

Re: How to handle revocation of locking state

[Trond Myklebust]

Hi Olga,

On Thu, 2020-05-28 at 16:42 -0400, Olga Kornievskaia wrote:
> Hi folks,
> 
> Looking for recommendation on what the client is suppose to be doing
> in the following situation. Client opens a file and has a byte-range
> lock which returned a locking state. Client is acquiring another byte
> range lock. It uses the returned locking stated for the 2nd lock.
> Server returns ADMIN_REVOKED.
> 
> Currently the client goes into an infinite loop of just resending the
> same LOCK operation with
> the same locking stateid.
> 
> Is this a recoverable situation? The fact that the lock state was
> revoked, should it be an automatic EIO since previous lock is lost
> (so
> why bother going forward)? Or should the client retry the lock but
> send it with the open stateid?
> 
> Thank you.

I think the right behaviour should be to just call
nfs_inode_find_state_and_recover(). In principle that will end up
either recovering the lock (if the user set the nfs.recover_lost_locks
kernel parameter to 'true') or marking it as a lost lock, using
NFS_LOCK_LOST.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: How to handle revocation of locking state

[Olga Kornievskaia]

On Thu, May 28, 2020 at 5:10 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> Hi Olga,
>
> On Thu, 2020-05-28 at 16:42 -0400, Olga Kornievskaia wrote:
> > Hi folks,
> >
> > Looking for recommendation on what the client is suppose to be doing
> > in the following situation. Client opens a file and has a byte-range
> > lock which returned a locking state. Client is acquiring another byte
> > range lock. It uses the returned locking stated for the 2nd lock.
> > Server returns ADMIN_REVOKED.
> >
> > Currently the client goes into an infinite loop of just resending the
> > same LOCK operation with
> > the same locking stateid.
> >
> > Is this a recoverable situation? The fact that the lock state was
> > revoked, should it be an automatic EIO since previous lock is lost
> > (so
> > why bother going forward)? Or should the client retry the lock but
> > send it with the open stateid?
> >
> > Thank you.
>
> I think the right behaviour should be to just call
> nfs_inode_find_state_and_recover(). In principle that will end up
> either recovering the lock (if the user set the nfs.recover_lost_locks
> kernel parameter to 'true') or marking it as a lost lock, using
> NFS_LOCK_LOST.

Why should acquiring of the 2nd lock depend on recovering the lock
who's stateid it was trying to use? I think the 1st stateid is lost
unrecoverable?

Right now what happens is code initiates recovery. open is sent. But
the retry of the 2nd lock has the INITIALIZED_LOCK set and so it takes
the bad lock stateid (how about instead letting it use the recovered
open stateid?). How about instead do the follow.

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 2b7f6dc..7723bd7 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -147,6 +147,7 @@ struct nfs4_lock_state {
        struct nfs4_state *     ls_state;       /* Pointer to open state */
 #define NFS_LOCK_INITIALIZED 0
 #define NFS_LOCK_LOST        1
+#define NFS_LOCK_REVOKED     2
        unsigned long           ls_flags;
        struct nfs_seqid_counter        ls_seqid;
        nfs4_stateid            ls_stateid;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9056f3d..6769370 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6792,7 +6792,8 @@ static void nfs4_lock_prepare(struct rpc_task *task, void
        if (nfs_wait_on_sequence(data->arg.lock_seqid, task) != 0)
                goto out_wait;
        /* Do we need to do an open_to_lock_owner? */
-       if (!test_bit(NFS_LOCK_INITIALIZED, &data->lsp->ls_flags)) {
+       if (!test_bit(NFS_LOCK_INITIALIZED, &data->lsp->ls_flags) ||
+               test_and_clear_bit(NFS_LOCK_REVOKED, &data->lsp->ls_flags)) {
                if (nfs_wait_on_sequence(data->arg.open_seqid, task) != 0) {
                        goto out_release_lock_seqid;
                }

Alternatively I have also thought about clearing the
NFS_LOCK_INITIALIZED in the nfs4_handle_setlk_error(). It leads to the
1st lock being marked lost (and having a message in the log) and then
2nd lock can proceed.

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9056f3d..2c78464 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6908,8 +6908,10 @@ static void nfs4_handle_setlk_error(struct
nfs_server *server, struct nfs4_lock_
        case -NFS4ERR_BAD_STATEID:
                lsp->ls_seqid.flags &= ~NFS_SEQID_CONFIRMED;
                if (new_lock_owner != 0 ||
-                  test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags) != 0)
+                  test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags) != 0) {
                        nfs4_schedule_stateid_recovery(server, lsp->ls_state);
+                       clear_bif(NFS_LOCK_INITIALIZED, &lsp->ls_flags);
+               }
                break;
        case -NFS4ERR_STALE_STATEID:
                lsp->ls_seqid.flags &= ~NFS_SEQID_CONFIRMED;

>
> Cheers
>   Trond
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>

Re: How to handle revocation of locking state

[Trond Myklebust]

On Thu, 2020-05-28 at 17:43 -0400, Olga Kornievskaia wrote:
> On Thu, May 28, 2020 at 5:10 PM Trond Myklebust <
> trondmy@hammerspace.com> wrote:
> > Hi Olga,
> > 
> > On Thu, 2020-05-28 at 16:42 -0400, Olga Kornievskaia wrote:
> > > Hi folks,
> > > 
> > > Looking for recommendation on what the client is suppose to be
> > > doing
> > > in the following situation. Client opens a file and has a byte-
> > > range
> > > lock which returned a locking state. Client is acquiring another
> > > byte
> > > range lock. It uses the returned locking stated for the 2nd lock.
> > > Server returns ADMIN_REVOKED.
> > > 
> > > Currently the client goes into an infinite loop of just resending
> > > the
> > > same LOCK operation with
> > > the same locking stateid.
> > > 
> > > Is this a recoverable situation? The fact that the lock state was
> > > revoked, should it be an automatic EIO since previous lock is
> > > lost
> > > (so
> > > why bother going forward)? Or should the client retry the lock
> > > but
> > > send it with the open stateid?
> > > 
> > > Thank you.
> > 
> > I think the right behaviour should be to just call
> > nfs_inode_find_state_and_recover(). In principle that will end up
> > either recovering the lock (if the user set the
> > nfs.recover_lost_locks
> > kernel parameter to 'true') or marking it as a lost lock, using
> > NFS_LOCK_LOST.
> 
> Why should acquiring of the 2nd lock depend on recovering the lock
> who's stateid it was trying to use? I think the 1st stateid is lost
> unrecoverable?

Agreed. However that means the application needs to know that it may
have corrupt data on its hands. We do know that this is the same
application that took the first lock, because any close of the file
(including due to application crashes) would result in the locks being
returned.

Some *NIX implementations have a special SIGLOST signal that their NFS
clients can use to let the application know its state was lost. Linux
unfortunately does not have such a signal, so we have to rely on error
codes.

> Right now what happens is code initiates recovery. open is sent. But
> the retry of the 2nd lock has the INITIALIZED_LOCK set and so it
> takes
> the bad lock stateid (how about instead letting it use the recovered
> open stateid?). How about instead do the follow.

NFSv4.1 requires us to call FREE_STATEID on any stateid that is
revoked, in order to let the server know when we've discovered that the
lock was lost. So we also have to go through the recovery machinery to
ensure that happens before we can deal with taking the second lock.

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: How to handle revocation of locking state

[Olga Kornievskaia]

On Thu, May 28, 2020 at 6:24 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Thu, 2020-05-28 at 17:43 -0400, Olga Kornievskaia wrote:
> > On Thu, May 28, 2020 at 5:10 PM Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> > > Hi Olga,
> > >
> > > On Thu, 2020-05-28 at 16:42 -0400, Olga Kornievskaia wrote:
> > > > Hi folks,
> > > >
> > > > Looking for recommendation on what the client is suppose to be
> > > > doing
> > > > in the following situation. Client opens a file and has a byte-
> > > > range
> > > > lock which returned a locking state. Client is acquiring another
> > > > byte
> > > > range lock. It uses the returned locking stated for the 2nd lock.
> > > > Server returns ADMIN_REVOKED.
> > > >
> > > > Currently the client goes into an infinite loop of just resending
> > > > the
> > > > same LOCK operation with
> > > > the same locking stateid.
> > > >
> > > > Is this a recoverable situation? The fact that the lock state was
> > > > revoked, should it be an automatic EIO since previous lock is
> > > > lost
> > > > (so
> > > > why bother going forward)? Or should the client retry the lock
> > > > but
> > > > send it with the open stateid?
> > > >
> > > > Thank you.
> > >
> > > I think the right behaviour should be to just call
> > > nfs_inode_find_state_and_recover(). In principle that will end up
> > > either recovering the lock (if the user set the
> > > nfs.recover_lost_locks
> > > kernel parameter to 'true') or marking it as a lost lock, using
> > > NFS_LOCK_LOST.
> >
> > Why should acquiring of the 2nd lock depend on recovering the lock
> > who's stateid it was trying to use? I think the 1st stateid is lost
> > unrecoverable?
>
> Agreed. However that means the application needs to know that it may
> have corrupt data on its hands. We do know that this is the same
> application that took the first lock, because any close of the file
> (including due to application crashes) would result in the locks being
> returned.
>
> Some *NIX implementations have a special SIGLOST signal that their NFS
> clients can use to let the application know its state was lost. Linux
> unfortunately does not have such a signal, so we have to rely on error
> codes.
>
> > Right now what happens is code initiates recovery. open is sent. But
> > the retry of the 2nd lock has the INITIALIZED_LOCK set and so it
> > takes
> > the bad lock stateid (how about instead letting it use the recovered
> > open stateid?). How about instead do the follow.
>
> NFSv4.1 requires us to call FREE_STATEID on any stateid that is
> revoked, in order to let the server know when we've discovered that the
> lock was lost. So we also have to go through the recovery machinery to
> ensure that happens before we can deal with taking the second lock.

Please bear with me I'm still loss:
1. If you say "application needs to know", the only outcome of this I
see is failing with EIO the 2nd lock. Which was my initial suggestion
saying if this is at all recoverable or should this be a failure
(instead of the infinite loop).
2. In nfs4_handle_setlk_error() we already call
nfs4_schedule_stateid_recovery(), I interpret this as "recovery was
initiated". I'm not sure what you envision the recovery steps are
suppose to be (or are missing. I guess the only one I see is lack of
free_stateid of the 1st lock). If you're saying the recovery includes
recovery of the 1st lock, then that's step#1 (but we don't send
free_stateid() for say a delegation stateid if it was revoked either
(or at least I don't think we do, I can test that)).  But after all
the recovery is done, the 2nd lock request needs to be re-tried and
what I see unless we change something about NFS_LOCK_INITALIZED
setting, it will once again pick a bad locking stateid.

>
> Cheers
>   Trond
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>

Re: How to handle revocation of locking state

[Trond Myklebust]

On Thu, 2020-05-28 at 18:50 -0400, Olga Kornievskaia wrote:
> On Thu, May 28, 2020 at 6:24 PM Trond Myklebust <
> trondmy@hammerspace.com> wrote:
> > On Thu, 2020-05-28 at 17:43 -0400, Olga Kornievskaia wrote:
> > > On Thu, May 28, 2020 at 5:10 PM Trond Myklebust <
> > > trondmy@hammerspace.com> wrote:
> > > > Hi Olga,
> > > > 
> > > > On Thu, 2020-05-28 at 16:42 -0400, Olga Kornievskaia wrote:
> > > > > Hi folks,
> > > > > 
> > > > > Looking for recommendation on what the client is suppose to
> > > > > be
> > > > > doing
> > > > > in the following situation. Client opens a file and has a
> > > > > byte-
> > > > > range
> > > > > lock which returned a locking state. Client is acquiring
> > > > > another
> > > > > byte
> > > > > range lock. It uses the returned locking stated for the 2nd
> > > > > lock.
> > > > > Server returns ADMIN_REVOKED.
> > > > > 
> > > > > Currently the client goes into an infinite loop of just
> > > > > resending
> > > > > the
> > > > > same LOCK operation with
> > > > > the same locking stateid.
> > > > > 
> > > > > Is this a recoverable situation? The fact that the lock state
> > > > > was
> > > > > revoked, should it be an automatic EIO since previous lock is
> > > > > lost
> > > > > (so
> > > > > why bother going forward)? Or should the client retry the
> > > > > lock
> > > > > but
> > > > > send it with the open stateid?
> > > > > 
> > > > > Thank you.
> > > > 
> > > > I think the right behaviour should be to just call
> > > > nfs_inode_find_state_and_recover(). In principle that will end
> > > > up
> > > > either recovering the lock (if the user set the
> > > > nfs.recover_lost_locks
> > > > kernel parameter to 'true') or marking it as a lost lock, using
> > > > NFS_LOCK_LOST.
> > > 
> > > Why should acquiring of the 2nd lock depend on recovering the
> > > lock
> > > who's stateid it was trying to use? I think the 1st stateid is
> > > lost
> > > unrecoverable?
> > 
> > Agreed. However that means the application needs to know that it
> > may
> > have corrupt data on its hands. We do know that this is the same
> > application that took the first lock, because any close of the file
> > (including due to application crashes) would result in the locks
> > being
> > returned.
> > 
> > Some *NIX implementations have a special SIGLOST signal that their
> > NFS
> > clients can use to let the application know its state was lost.
> > Linux
> > unfortunately does not have such a signal, so we have to rely on
> > error
> > codes.
> > 
> > > Right now what happens is code initiates recovery. open is sent.
> > > But
> > > the retry of the 2nd lock has the INITIALIZED_LOCK set and so it
> > > takes
> > > the bad lock stateid (how about instead letting it use the
> > > recovered
> > > open stateid?). How about instead do the follow.
> > 
> > NFSv4.1 requires us to call FREE_STATEID on any stateid that is
> > revoked, in order to let the server know when we've discovered that
> > the
> > lock was lost. So we also have to go through the recovery machinery
> > to
> > ensure that happens before we can deal with taking the second lock.
> 
> Please bear with me I'm still loss:
> 1. If you say "application needs to know", the only outcome of this I
> see is failing with EIO the 2nd lock. Which was my initial suggestion
> saying if this is at all recoverable or should this be a failure
> (instead of the infinite loop).

It should be a failure unless the nfs.recover_lost_locks kernel
parameter is set, in which case it should silently recover the lost
lock. The default behaviour should therefore be failure.

> 2. In nfs4_handle_setlk_error() we already call
> nfs4_schedule_stateid_recovery(), I interpret this as "recovery was
> initiated". I'm not sure what you envision the recovery steps are
> suppose to be (or are missing. I guess the only one I see is lack of
> free_stateid of the 1st lock). If you're saying the recovery includes
> recovery of the 1st lock, then that's step#1 (but we don't send
> free_stateid() for say a delegation stateid if it was revoked either
> (or at least I don't think we do, I can test that)).  But after all
> the recovery is done, the 2nd lock request needs to be re-tried and
> what I see unless we change something about NFS_LOCK_INITALIZED
> setting, it will once again pick a bad locking stateid.
> 

Recovery of the lost lock only happens in the non-default case
described previously. However whether or not we recover the lock, we
need to call FREE_STATEID on the stateid that is returning
NFS4ERR_ADMIN_REVOKED (or NFS4ERR_DELEG_REVOKED if the stateid is a
delegation).
After the call to FREE_STATEID, the server will start returning
NFS4ERR_BAD_STATEID if we ever present the revoked lock stateid again.


So yes, once we're done sending FREE_STATEID, we can clear
NFS_LOCK_INITIALIZED and start new locking requests from scratch.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: How to handle revocation of locking state

[Olga Kornievskaia]

On Thu, May 28, 2020 at 7:54 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Thu, 2020-05-28 at 18:50 -0400, Olga Kornievskaia wrote:
> > On Thu, May 28, 2020 at 6:24 PM Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> > > On Thu, 2020-05-28 at 17:43 -0400, Olga Kornievskaia wrote:
> > > > On Thu, May 28, 2020 at 5:10 PM Trond Myklebust <
> > > > trondmy@hammerspace.com> wrote:
> > > > > Hi Olga,
> > > > >
> > > > > On Thu, 2020-05-28 at 16:42 -0400, Olga Kornievskaia wrote:
> > > > > > Hi folks,
> > > > > >
> > > > > > Looking for recommendation on what the client is suppose to
> > > > > > be
> > > > > > doing
> > > > > > in the following situation. Client opens a file and has a
> > > > > > byte-
> > > > > > range
> > > > > > lock which returned a locking state. Client is acquiring
> > > > > > another
> > > > > > byte
> > > > > > range lock. It uses the returned locking stated for the 2nd
> > > > > > lock.
> > > > > > Server returns ADMIN_REVOKED.
> > > > > >
> > > > > > Currently the client goes into an infinite loop of just
> > > > > > resending
> > > > > > the
> > > > > > same LOCK operation with
> > > > > > the same locking stateid.
> > > > > >
> > > > > > Is this a recoverable situation? The fact that the lock state
> > > > > > was
> > > > > > revoked, should it be an automatic EIO since previous lock is
> > > > > > lost
> > > > > > (so
> > > > > > why bother going forward)? Or should the client retry the
> > > > > > lock
> > > > > > but
> > > > > > send it with the open stateid?
> > > > > >
> > > > > > Thank you.
> > > > >
> > > > > I think the right behaviour should be to just call
> > > > > nfs_inode_find_state_and_recover(). In principle that will end
> > > > > up
> > > > > either recovering the lock (if the user set the
> > > > > nfs.recover_lost_locks
> > > > > kernel parameter to 'true') or marking it as a lost lock, using
> > > > > NFS_LOCK_LOST.
> > > >
> > > > Why should acquiring of the 2nd lock depend on recovering the
> > > > lock
> > > > who's stateid it was trying to use? I think the 1st stateid is
> > > > lost
> > > > unrecoverable?
> > >
> > > Agreed. However that means the application needs to know that it
> > > may
> > > have corrupt data on its hands. We do know that this is the same
> > > application that took the first lock, because any close of the file
> > > (including due to application crashes) would result in the locks
> > > being
> > > returned.
> > >
> > > Some *NIX implementations have a special SIGLOST signal that their
> > > NFS
> > > clients can use to let the application know its state was lost.
> > > Linux
> > > unfortunately does not have such a signal, so we have to rely on
> > > error
> > > codes.
> > >
> > > > Right now what happens is code initiates recovery. open is sent.
> > > > But
> > > > the retry of the 2nd lock has the INITIALIZED_LOCK set and so it
> > > > takes
> > > > the bad lock stateid (how about instead letting it use the
> > > > recovered
> > > > open stateid?). How about instead do the follow.
> > >
> > > NFSv4.1 requires us to call FREE_STATEID on any stateid that is
> > > revoked, in order to let the server know when we've discovered that
> > > the
> > > lock was lost. So we also have to go through the recovery machinery
> > > to
> > > ensure that happens before we can deal with taking the second lock.
> >
> > Please bear with me I'm still loss:
> > 1. If you say "application needs to know", the only outcome of this I
> > see is failing with EIO the 2nd lock. Which was my initial suggestion
> > saying if this is at all recoverable or should this be a failure
> > (instead of the infinite loop).
>
> It should be a failure unless the nfs.recover_lost_locks kernel
> parameter is set, in which case it should silently recover the lost
> lock. The default behaviour should therefore be failure.

Ok got it. Let me figure out how to make it fail.

> > 2. In nfs4_handle_setlk_error() we already call
> > nfs4_schedule_stateid_recovery(), I interpret this as "recovery was
> > initiated". I'm not sure what you envision the recovery steps are
> > suppose to be (or are missing. I guess the only one I see is lack of
> > free_stateid of the 1st lock). If you're saying the recovery includes
> > recovery of the 1st lock, then that's step#1 (but we don't send
> > free_stateid() for say a delegation stateid if it was revoked either
> > (or at least I don't think we do, I can test that)).  But after all
> > the recovery is done, the 2nd lock request needs to be re-tried and
> > what I see unless we change something about NFS_LOCK_INITALIZED
> > setting, it will once again pick a bad locking stateid.
> >
>
> Recovery of the lost lock only happens in the non-default case
> described previously. However whether or not we recover the lock, we
> need to call FREE_STATEID on the stateid that is returning
> NFS4ERR_ADMIN_REVOKED (or NFS4ERR_DELEG_REVOKED if the stateid is a
> delegation).
> After the call to FREE_STATEID, the server will start returning
> NFS4ERR_BAD_STATEID if we ever present the revoked lock stateid again.
>
>
> So yes, once we're done sending FREE_STATEID, we can clear
> NFS_LOCK_INITIALIZED and start new locking requests from scratch.

But the latter would happen only if the previous lost lock would be
recovered. And if it's recovered then we don't need to clear anything
and let the 2nd lock again use the locking state as before?

>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>