This set adds a prequel series to my previous posting which addresses
some locking problems around ->sc_type and splits that field into
to separate fields: sc_type and sc_status.
The recovation code is modified to accomodate these changed.
Thanks
NeilBrown
[PATCH 1/9] nfsd: hold ->cl_lock for hash_delegation_locked()
[PATCH 2/9] nfsd: avoid race after unhash_delegation_locked()
[PATCH 3/9] nfsd: split sc_status out of sc_type
[PATCH 4/9] nfsd: prepare for supporting admin-revocation of state
[PATCH 5/9] nfsd: allow admin-revoked state to appear in
[PATCH 6/9] nfsd: allow admin-revoked NFSv4.0 state to be freed.
[PATCH 7/9] nfsd: allow lock state ids to be revoked and then freed
[PATCH 8/9] nfsd: allow open state ids to be revoked and then freed
[PATCH 9/9] nfsd: allow delegation state ids to be revoked and then
Revoking state through 'unlock_filesystem' now revokes any delegation
states found. When the stateids are then freed by the client, the
revoked stateids will be cleaned up correctly.
As there is already support for revoking delegations, we build on that
for admin-revoking.
Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfs4state.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index dd454f50383e..feceaf4bf638 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1340,9 +1340,12 @@ unhash_delegation_locked(struct nfs4_delegation *dp, unsigned int statusmask)
if (!delegation_hashed(dp))
return false;
- if (dp->dl_stid.sc_client->cl_minorversion == 0)
+ if (statusmask == NFS4_STID_REVOKED &&
+ dp->dl_stid.sc_client->cl_minorversion == 0)
statusmask = NFS4_STID_CLOSED;
dp->dl_stid.sc_status |= statusmask;
+ if (statusmask & NFS4_STID_ADMIN_REVOKED)
+ atomic_inc(&dp->dl_stid.sc_client->cl_admin_revoked);
/* Ensure that deleg break won't try to requeue it */
++dp->dl_time;
@@ -1373,7 +1376,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
trace_nfsd_stid_revoke(&dp->dl_stid);
- if (dp->dl_stid.sc_status & NFS4_STID_REVOKED) {
+ if (dp->dl_stid.sc_status &
+ (NFS4_STID_REVOKED | NFS4_STID_ADMIN_REVOKED)) {
spin_lock(&clp->cl_lock);
refcount_inc(&dp->dl_stid.sc_count);
list_add(&dp->dl_recall_lru, &clp->cl_revoked);
@@ -1708,7 +1712,7 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
unsigned int idhashval;
unsigned int sc_types;
- sc_types = NFS4_OPEN_STID | NFS4_LOCK_STID;
+ sc_types = NFS4_OPEN_STID | NFS4_LOCK_STID | NFS4_DELEG_STID;
spin_lock(&nn->client_lock);
for (idhashval = 0; idhashval < CLIENT_HASH_MASK; idhashval++) {
@@ -1720,6 +1724,7 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
sc_types);
if (stid) {
struct nfs4_ol_stateid *stp;
+ struct nfs4_delegation *dp;
spin_unlock(&nn->client_lock);
switch (stid->sc_type) {
@@ -1765,6 +1770,18 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
spin_unlock(&clp->cl_lock);
mutex_unlock(&stp->st_mutex);
break;
+ case NFS4_DELEG_STID:
+ dp = delegstateid(stid);
+ spin_lock(&state_lock);
+ if (!unhash_delegation_locked(
+ dp, NFS4_STID_ADMIN_REVOKED))
+ dp = NULL;
+ spin_unlock(&state_lock);
+ if (dp) {
+ list_del_init(&dp->dl_recall_lru);
+ revoke_delegation(dp);
+ }
+ break;
}
nfs4_put_stid(stid);
spin_lock(&nn->client_lock);
@@ -4707,6 +4724,7 @@ static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
struct nfs4_client *cl = s->sc_client;
LIST_HEAD(reaplist);
struct nfs4_ol_stateid *stp;
+ struct nfs4_delegation *dp;
bool unhashed;
switch (s->sc_type) {
@@ -4724,6 +4742,12 @@ static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
if (unhashed)
nfs4_put_stid(s);
break;
+ case NFS4_DELEG_STID:
+ dp = delegstateid(s);
+ list_del_init(&dp->dl_recall_lru);
+ spin_unlock(&cl->cl_lock);
+ nfs4_put_stid(s);
+ break;
default:
spin_unlock(&cl->cl_lock);
}
--
2.42.0
On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> This set adds a prequel series to my previous posting which addresses
> some locking problems around ->sc_type and splits that field into
> to separate fields: sc_type and sc_status.
> The recovation code is modified to accomodate these changed.
>
> Thanks
> NeilBrown
>
> [PATCH 1/9] nfsd: hold ->cl_lock for hash_delegation_locked()
> [PATCH 2/9] nfsd: avoid race after unhash_delegation_locked()
> [PATCH 3/9] nfsd: split sc_status out of sc_type
> [PATCH 4/9] nfsd: prepare for supporting admin-revocation of state
> [PATCH 5/9] nfsd: allow admin-revoked state to appear in
> [PATCH 6/9] nfsd: allow admin-revoked NFSv4.0 state to be freed.
> [PATCH 7/9] nfsd: allow lock state ids to be revoked and then freed
> [PATCH 8/9] nfsd: allow open state ids to be revoked and then freed
> [PATCH 9/9] nfsd: allow delegation state ids to be revoked and then
>
Nice set. I like this overall. One (other) question: do we need to add
handling for revoking layout stateids as well?
I'll plan to pull this into my local branch for some testing over the
weekend.
--
Jeff Layton <[email protected]>
On Fri, 17 Nov 2023, Jeff Layton wrote:
> On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > This set adds a prequel series to my previous posting which addresses
> > some locking problems around ->sc_type and splits that field into
> > to separate fields: sc_type and sc_status.
> > The recovation code is modified to accomodate these changed.
> >
> > Thanks
> > NeilBrown
> >
> > [PATCH 1/9] nfsd: hold ->cl_lock for hash_delegation_locked()
> > [PATCH 2/9] nfsd: avoid race after unhash_delegation_locked()
> > [PATCH 3/9] nfsd: split sc_status out of sc_type
> > [PATCH 4/9] nfsd: prepare for supporting admin-revocation of state
> > [PATCH 5/9] nfsd: allow admin-revoked state to appear in
> > [PATCH 6/9] nfsd: allow admin-revoked NFSv4.0 state to be freed.
> > [PATCH 7/9] nfsd: allow lock state ids to be revoked and then freed
> > [PATCH 8/9] nfsd: allow open state ids to be revoked and then freed
> > [PATCH 9/9] nfsd: allow delegation state ids to be revoked and then
> >
>
> Nice set. I like this overall. One (other) question: do we need to add
> handling for revoking layout stateids as well?
I guess so... I don't give much thought to layout stateids.
They are used in LAYOUTGET LAYOUTCOMMIT LAYOUTRETURN.
(it's seems odd that they aren't used in READ/WRITE....)
I guess we need to drop the ref on ->ls_file and maybe unlock the vfs
lease....
I wonder if I should just use ->sc_free for that.
Maybe ->sc_free could take a 'revoke_only' arg which causes it to free
resources but not free the stateid itself.
Maybe that would make the code cleaner.
NeilBrown
>
> I'll plan to pull this into my local branch for some testing over the
> weekend.
> --
> Jeff Layton <[email protected]>
>