For NFSv4.1 and later the client easily discovers if there is any
admin-revoked state and will then find and explicitly free it.
For NFSv4.0 there is no such mechanism. The client can only find that
state is admin-revoked if it tries to use that state, and there is no
way for it to explicitly free the state. So the server must hold on to
the stateid (at least) for an indefinite amount of time. A
RELEASE_LOCKOWNER request might justify forgetting some of these
stateids, as would the whole clients lease lapsing, but these are not
reliable.
This patch takes two approaches.
Whenever a client uses an revoked stateid, that stateid is then
discarded and will not be recognised again. This might confuse a client
which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
all, but should mostly work. Hopefully one error will lead to other
resources being closed (e.g. process exits), which will result in more
stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.
Also, any admin-revoked stateids that have been that way for more than
one lease time are periodically revoke.
No actual freeing of state happens in this patch. That will come in
future patches which handle the different sorts of revoked state.
Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/netns.h | 4 ++
fs/nfsd/nfs4state.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 100 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index ec49b200b797..02f8fa095b0f 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -197,6 +197,10 @@ struct nfsd_net {
atomic_t nfsd_courtesy_clients;
struct shrinker nfsd_client_shrinker;
struct work_struct nfsd_shrinker_work;
+
+ /* last time an admin-revoke happened for NFSv4.0 */
+ time64_t nfs40_last_revoke;
+
};
/* Simple check to find out if a given net was properly initialized */
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8debd148840f..8a1b8376ff08 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1724,6 +1724,14 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
}
nfs4_put_stid(stid);
spin_lock(&nn->client_lock);
+ if (clp->cl_minorversion == 0)
+ /* Allow cleanup after a lease period.
+ * store_release ensures cleanup will
+ * see any newly revoked states if it
+ * sees the time updated.
+ */
+ nn->nfs40_last_revoke =
+ ktime_get_boottime_seconds();
goto retry;
}
}
@@ -4650,6 +4658,39 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
return ret;
}
+static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
+{
+ struct nfs4_client *cl = s->sc_client;
+
+ switch (s->sc_type) {
+ default:
+ spin_unlock(&cl->cl_lock);
+ }
+}
+
+static void nfs40_drop_revoked_stid(struct nfs4_client *cl,
+ stateid_t *stid)
+{
+ /* NFSv4.0 has no way for the client to tell the server
+ * that it can forget an admin-revoked stateid.
+ * So we keep it around until the first time that the
+ * client uses it, and drop it the first time
+ * nfserr_admin_revoked is returned.
+ * For v4.1 and later we wait until explicitly told
+ * to free the stateid.
+ */
+ if (cl->cl_minorversion == 0) {
+ struct nfs4_stid *st;
+
+ spin_lock(&cl->cl_lock);
+ st = find_stateid_locked(cl, stid);
+ if (st)
+ nfsd_drop_revoked_stid(st);
+ else
+ spin_unlock(&cl->cl_lock);
+ }
+}
+
static __be32
nfsd4_verify_open_stid(struct nfs4_stid *s)
{
@@ -4672,6 +4713,10 @@ nfsd4_lock_ol_stateid(struct nfs4_ol_stateid *stp)
mutex_lock_nested(&stp->st_mutex, LOCK_STATEID_MUTEX);
ret = nfsd4_verify_open_stid(&stp->st_stid);
+ if (ret == nfserr_admin_revoked)
+ nfs40_drop_revoked_stid(stp->st_stid.sc_client,
+ &stp->st_stid.sc_stateid);
+
if (ret != nfs_ok)
mutex_unlock(&stp->st_mutex);
return ret;
@@ -5255,6 +5300,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
}
if (deleg->dl_stid.sc_status & NFS4_STID_REVOKED) {
nfs4_put_stid(&deleg->dl_stid);
+ nfs40_drop_revoked_stid(cl, &open->op_delegate_stateid);
status = nfserr_deleg_revoked;
goto out;
}
@@ -6253,6 +6299,43 @@ nfs4_process_client_reaplist(struct list_head *reaplist)
}
}
+static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
+ struct laundry_time *lt)
+{
+ struct nfs4_client *clp;
+
+ spin_lock(&nn->client_lock);
+ if (nn->nfs40_last_revoke == 0 ||
+ nn->nfs40_last_revoke > lt->cutoff) {
+ spin_unlock(&nn->client_lock);
+ return;
+ }
+ nn->nfs40_last_revoke = 0;
+
+retry:
+ list_for_each_entry(clp, &nn->client_lru, cl_lru) {
+ unsigned long id, tmp;
+ struct nfs4_stid *stid;
+
+ if (atomic_read(&clp->cl_admin_revoked) == 0)
+ continue;
+
+ spin_lock(&clp->cl_lock);
+ idr_for_each_entry_ul(&clp->cl_stateids, stid, tmp, id)
+ if (stid->sc_status & NFS4_STID_ADMIN_REVOKED) {
+ refcount_inc(&stid->sc_count);
+ spin_unlock(&nn->client_lock);
+ /* this function drops ->cl_lock */
+ nfsd_drop_revoked_stid(stid);
+ nfs4_put_stid(stid);
+ spin_lock(&nn->client_lock);
+ goto retry;
+ }
+ spin_unlock(&clp->cl_lock);
+ }
+ spin_unlock(&nn->client_lock);
+}
+
static time64_t
nfs4_laundromat(struct nfsd_net *nn)
{
@@ -6286,6 +6369,8 @@ nfs4_laundromat(struct nfsd_net *nn)
nfs4_get_client_reaplist(nn, &reaplist, <);
nfs4_process_client_reaplist(&reaplist);
+ nfs40_clean_admin_revoked(nn, <);
+
spin_lock(&state_lock);
list_for_each_safe(pos, next, &nn->del_recall_lru) {
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
@@ -6504,6 +6589,9 @@ static __be32 nfsd4_stid_check_stateid_generation(stateid_t *in, struct nfs4_sti
if (ret == nfs_ok)
ret = check_stateid_generation(in, &s->sc_stateid, has_session);
spin_unlock(&s->sc_lock);
+ if (ret == nfserr_admin_revoked)
+ nfs40_drop_revoked_stid(s->sc_client,
+ &s->sc_stateid);
return ret;
}
@@ -6548,6 +6636,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
}
out_unlock:
spin_unlock(&cl->cl_lock);
+ if (status == nfserr_admin_revoked)
+ nfs40_drop_revoked_stid(cl, stateid);
return status;
}
@@ -6594,6 +6684,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
return nfserr_deleg_revoked;
}
if (stid->sc_type & NFS4_STID_ADMIN_REVOKED) {
+ nfs40_drop_revoked_stid(cstate->clp, stateid);
nfs4_put_stid(stid);
return nfserr_admin_revoked;
}
@@ -6886,6 +6977,11 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
s = find_stateid_locked(cl, stateid);
if (!s || s->sc_status & NFS4_STID_CLOSED)
goto out_unlock;
+ if (s->sc_status & NFS4_STID_ADMIN_REVOKED) {
+ nfsd_drop_revoked_stid(s);
+ ret = nfs_ok;
+ goto out;
+ }
spin_lock(&s->sc_lock);
switch (s->sc_type) {
case NFS4_DELEG_STID:
@@ -6912,7 +7008,6 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
spin_unlock(&cl->cl_lock);
ret = nfsd4_free_lock_stateid(stateid, s);
goto out;
- /* Default falls through and returns nfserr_bad_stateid */
}
spin_unlock(&s->sc_lock);
out_unlock:
--
2.42.0
On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> For NFSv4.1 and later the client easily discovers if there is any
> admin-revoked state and will then find and explicitly free it.
>
> For NFSv4.0 there is no such mechanism. The client can only find that
> state is admin-revoked if it tries to use that state, and there is no
> way for it to explicitly free the state. So the server must hold on to
> the stateid (at least) for an indefinite amount of time. A
> RELEASE_LOCKOWNER request might justify forgetting some of these
> stateids, as would the whole clients lease lapsing, but these are not
> reliable.
>
> This patch takes two approaches.
>
> Whenever a client uses an revoked stateid, that stateid is then
> discarded and will not be recognised again. This might confuse a client
> which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
> all, but should mostly work. Hopefully one error will lead to other
> resources being closed (e.g. process exits), which will result in more
> stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.
>
> Also, any admin-revoked stateids that have been that way for more than
> one lease time are periodically revoke.
>
Why a single lease period?
Is that a long enough time for v4.0? Given that the protocol has no
mechanism to detect revoked state, I have to wonder if a single lease
period is enough time for the client to detect the problem?
> No actual freeing of state happens in this patch. That will come in
> future patches which handle the different sorts of revoked state.
>
> Signed-off-by: NeilBrown <[email protected]>
> ---
> fs/nfsd/netns.h | 4 ++
> fs/nfsd/nfs4state.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index ec49b200b797..02f8fa095b0f 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -197,6 +197,10 @@ struct nfsd_net {
> atomic_t nfsd_courtesy_clients;
> struct shrinker nfsd_client_shrinker;
> struct work_struct nfsd_shrinker_work;
> +
> + /* last time an admin-revoke happened for NFSv4.0 */
> + time64_t nfs40_last_revoke;
> +
> };
>
> /* Simple check to find out if a given net was properly initialized */
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8debd148840f..8a1b8376ff08 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1724,6 +1724,14 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
> }
> nfs4_put_stid(stid);
> spin_lock(&nn->client_lock);
> + if (clp->cl_minorversion == 0)
> + /* Allow cleanup after a lease period.
> + * store_release ensures cleanup will
> + * see any newly revoked states if it
> + * sees the time updated.
> + */
> + nn->nfs40_last_revoke =
> + ktime_get_boottime_seconds();
> goto retry;
> }
> }
> @@ -4650,6 +4658,39 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
> return ret;
> }
>
> +static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
> +{
> + struct nfs4_client *cl = s->sc_client;
> +
> + switch (s->sc_type) {
> + default:
> + spin_unlock(&cl->cl_lock);
> + }
> +}
> +
> +static void nfs40_drop_revoked_stid(struct nfs4_client *cl,
> + stateid_t *stid)
> +{
> + /* NFSv4.0 has no way for the client to tell the server
> + * that it can forget an admin-revoked stateid.
> + * So we keep it around until the first time that the
> + * client uses it, and drop it the first time
> + * nfserr_admin_revoked is returned.
> + * For v4.1 and later we wait until explicitly told
> + * to free the stateid.
> + */
> + if (cl->cl_minorversion == 0) {
> + struct nfs4_stid *st;
> +
> + spin_lock(&cl->cl_lock);
> + st = find_stateid_locked(cl, stid);
> + if (st)
> + nfsd_drop_revoked_stid(st);
> + else
> + spin_unlock(&cl->cl_lock);
> + }
> +}
> +
> static __be32
> nfsd4_verify_open_stid(struct nfs4_stid *s)
> {
> @@ -4672,6 +4713,10 @@ nfsd4_lock_ol_stateid(struct nfs4_ol_stateid *stp)
>
> mutex_lock_nested(&stp->st_mutex, LOCK_STATEID_MUTEX);
> ret = nfsd4_verify_open_stid(&stp->st_stid);
> + if (ret == nfserr_admin_revoked)
> + nfs40_drop_revoked_stid(stp->st_stid.sc_client,
> + &stp->st_stid.sc_stateid);
> +
> if (ret != nfs_ok)
> mutex_unlock(&stp->st_mutex);
> return ret;
> @@ -5255,6 +5300,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
> }
> if (deleg->dl_stid.sc_status & NFS4_STID_REVOKED) {
> nfs4_put_stid(&deleg->dl_stid);
> + nfs40_drop_revoked_stid(cl, &open->op_delegate_stateid);
> status = nfserr_deleg_revoked;
> goto out;
> }
> @@ -6253,6 +6299,43 @@ nfs4_process_client_reaplist(struct list_head *reaplist)
> }
> }
>
> +static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
> + struct laundry_time *lt)
> +{
> + struct nfs4_client *clp;
> +
> + spin_lock(&nn->client_lock);
> + if (nn->nfs40_last_revoke == 0 ||
> + nn->nfs40_last_revoke > lt->cutoff) {
> + spin_unlock(&nn->client_lock);
> + return;
> + }
> + nn->nfs40_last_revoke = 0;
> +
> +retry:
> + list_for_each_entry(clp, &nn->client_lru, cl_lru) {
> + unsigned long id, tmp;
> + struct nfs4_stid *stid;
> +
> + if (atomic_read(&clp->cl_admin_revoked) == 0)
> + continue;
> +
> + spin_lock(&clp->cl_lock);
> + idr_for_each_entry_ul(&clp->cl_stateids, stid, tmp, id)
> + if (stid->sc_status & NFS4_STID_ADMIN_REVOKED) {
> + refcount_inc(&stid->sc_count);
> + spin_unlock(&nn->client_lock);
> + /* this function drops ->cl_lock */
> + nfsd_drop_revoked_stid(stid);
> + nfs4_put_stid(stid);
> + spin_lock(&nn->client_lock);
> + goto retry;
> + }
> + spin_unlock(&clp->cl_lock);
> + }
> + spin_unlock(&nn->client_lock);
> +}
> +
> static time64_t
> nfs4_laundromat(struct nfsd_net *nn)
> {
> @@ -6286,6 +6369,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> nfs4_get_client_reaplist(nn, &reaplist, <);
> nfs4_process_client_reaplist(&reaplist);
>
> + nfs40_clean_admin_revoked(nn, <);
> +
> spin_lock(&state_lock);
> list_for_each_safe(pos, next, &nn->del_recall_lru) {
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> @@ -6504,6 +6589,9 @@ static __be32 nfsd4_stid_check_stateid_generation(stateid_t *in, struct nfs4_sti
> if (ret == nfs_ok)
> ret = check_stateid_generation(in, &s->sc_stateid, has_session);
> spin_unlock(&s->sc_lock);
> + if (ret == nfserr_admin_revoked)
> + nfs40_drop_revoked_stid(s->sc_client,
> + &s->sc_stateid);
> return ret;
> }
>
> @@ -6548,6 +6636,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> }
> out_unlock:
> spin_unlock(&cl->cl_lock);
> + if (status == nfserr_admin_revoked)
> + nfs40_drop_revoked_stid(cl, stateid);
> return status;
> }
>
> @@ -6594,6 +6684,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> return nfserr_deleg_revoked;
> }
> if (stid->sc_type & NFS4_STID_ADMIN_REVOKED) {
> + nfs40_drop_revoked_stid(cstate->clp, stateid);
> nfs4_put_stid(stid);
> return nfserr_admin_revoked;
> }
> @@ -6886,6 +6977,11 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> s = find_stateid_locked(cl, stateid);
> if (!s || s->sc_status & NFS4_STID_CLOSED)
> goto out_unlock;
> + if (s->sc_status & NFS4_STID_ADMIN_REVOKED) {
> + nfsd_drop_revoked_stid(s);
> + ret = nfs_ok;
> + goto out;
> + }
> spin_lock(&s->sc_lock);
> switch (s->sc_type) {
> case NFS4_DELEG_STID:
> @@ -6912,7 +7008,6 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> spin_unlock(&cl->cl_lock);
> ret = nfsd4_free_lock_stateid(stateid, s);
> goto out;
> - /* Default falls through and returns nfserr_bad_stateid */
> }
> spin_unlock(&s->sc_lock);
> out_unlock:
--
Jeff Layton <[email protected]>
On Fri, Nov 17, 2023 at 01:18:52PM +1100, NeilBrown wrote:
> For NFSv4.1 and later the client easily discovers if there is any
> admin-revoked state and will then find and explicitly free it.
>
> For NFSv4.0 there is no such mechanism. The client can only find that
> state is admin-revoked if it tries to use that state, and there is no
> way for it to explicitly free the state. So the server must hold on to
> the stateid (at least) for an indefinite amount of time. A
> RELEASE_LOCKOWNER request might justify forgetting some of these
> stateids, as would the whole clients lease lapsing, but these are not
> reliable.
>
> This patch takes two approaches.
>
> Whenever a client uses an revoked stateid, that stateid is then
> discarded and will not be recognised again. This might confuse a client
> which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
> all, but should mostly work. Hopefully one error will lead to other
> resources being closed (e.g. process exits), which will result in more
> stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.
>
> Also, any admin-revoked stateids that have been that way for more than
> one lease time are periodically revoke.
>
> No actual freeing of state happens in this patch. That will come in
> future patches which handle the different sorts of revoked state.
>
> Signed-off-by: NeilBrown <[email protected]>
> ---
> fs/nfsd/netns.h | 4 ++
> fs/nfsd/nfs4state.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index ec49b200b797..02f8fa095b0f 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -197,6 +197,10 @@ struct nfsd_net {
> atomic_t nfsd_courtesy_clients;
> struct shrinker nfsd_client_shrinker;
> struct work_struct nfsd_shrinker_work;
> +
> + /* last time an admin-revoke happened for NFSv4.0 */
> + time64_t nfs40_last_revoke;
> +
> };
This hunk doesn't apply to nfsd-next due to v6.7-rc changes to NFSD
to implement a dynamic shrinker. So I stopped my review here for
now.
> /* Simple check to find out if a given net was properly initialized */
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8debd148840f..8a1b8376ff08 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1724,6 +1724,14 @@ void nfsd4_revoke_states(struct net *net, struct super_block *sb)
> }
> nfs4_put_stid(stid);
> spin_lock(&nn->client_lock);
> + if (clp->cl_minorversion == 0)
> + /* Allow cleanup after a lease period.
> + * store_release ensures cleanup will
> + * see any newly revoked states if it
> + * sees the time updated.
> + */
> + nn->nfs40_last_revoke =
> + ktime_get_boottime_seconds();
> goto retry;
> }
> }
> @@ -4650,6 +4658,39 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
> return ret;
> }
>
> +static void nfsd_drop_revoked_stid(struct nfs4_stid *s)
> +{
> + struct nfs4_client *cl = s->sc_client;
> +
> + switch (s->sc_type) {
> + default:
> + spin_unlock(&cl->cl_lock);
> + }
> +}
> +
> +static void nfs40_drop_revoked_stid(struct nfs4_client *cl,
> + stateid_t *stid)
> +{
> + /* NFSv4.0 has no way for the client to tell the server
> + * that it can forget an admin-revoked stateid.
> + * So we keep it around until the first time that the
> + * client uses it, and drop it the first time
> + * nfserr_admin_revoked is returned.
> + * For v4.1 and later we wait until explicitly told
> + * to free the stateid.
> + */
> + if (cl->cl_minorversion == 0) {
> + struct nfs4_stid *st;
> +
> + spin_lock(&cl->cl_lock);
> + st = find_stateid_locked(cl, stid);
> + if (st)
> + nfsd_drop_revoked_stid(st);
> + else
> + spin_unlock(&cl->cl_lock);
> + }
> +}
> +
> static __be32
> nfsd4_verify_open_stid(struct nfs4_stid *s)
> {
> @@ -4672,6 +4713,10 @@ nfsd4_lock_ol_stateid(struct nfs4_ol_stateid *stp)
>
> mutex_lock_nested(&stp->st_mutex, LOCK_STATEID_MUTEX);
> ret = nfsd4_verify_open_stid(&stp->st_stid);
> + if (ret == nfserr_admin_revoked)
> + nfs40_drop_revoked_stid(stp->st_stid.sc_client,
> + &stp->st_stid.sc_stateid);
> +
> if (ret != nfs_ok)
> mutex_unlock(&stp->st_mutex);
> return ret;
> @@ -5255,6 +5300,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
> }
> if (deleg->dl_stid.sc_status & NFS4_STID_REVOKED) {
> nfs4_put_stid(&deleg->dl_stid);
> + nfs40_drop_revoked_stid(cl, &open->op_delegate_stateid);
> status = nfserr_deleg_revoked;
> goto out;
> }
> @@ -6253,6 +6299,43 @@ nfs4_process_client_reaplist(struct list_head *reaplist)
> }
> }
>
> +static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
> + struct laundry_time *lt)
> +{
> + struct nfs4_client *clp;
> +
> + spin_lock(&nn->client_lock);
> + if (nn->nfs40_last_revoke == 0 ||
> + nn->nfs40_last_revoke > lt->cutoff) {
> + spin_unlock(&nn->client_lock);
> + return;
> + }
> + nn->nfs40_last_revoke = 0;
> +
> +retry:
> + list_for_each_entry(clp, &nn->client_lru, cl_lru) {
> + unsigned long id, tmp;
> + struct nfs4_stid *stid;
> +
> + if (atomic_read(&clp->cl_admin_revoked) == 0)
> + continue;
> +
> + spin_lock(&clp->cl_lock);
> + idr_for_each_entry_ul(&clp->cl_stateids, stid, tmp, id)
> + if (stid->sc_status & NFS4_STID_ADMIN_REVOKED) {
> + refcount_inc(&stid->sc_count);
> + spin_unlock(&nn->client_lock);
> + /* this function drops ->cl_lock */
> + nfsd_drop_revoked_stid(stid);
> + nfs4_put_stid(stid);
> + spin_lock(&nn->client_lock);
> + goto retry;
> + }
> + spin_unlock(&clp->cl_lock);
> + }
> + spin_unlock(&nn->client_lock);
> +}
> +
> static time64_t
> nfs4_laundromat(struct nfsd_net *nn)
> {
> @@ -6286,6 +6369,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> nfs4_get_client_reaplist(nn, &reaplist, <);
> nfs4_process_client_reaplist(&reaplist);
>
> + nfs40_clean_admin_revoked(nn, <);
> +
> spin_lock(&state_lock);
> list_for_each_safe(pos, next, &nn->del_recall_lru) {
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> @@ -6504,6 +6589,9 @@ static __be32 nfsd4_stid_check_stateid_generation(stateid_t *in, struct nfs4_sti
> if (ret == nfs_ok)
> ret = check_stateid_generation(in, &s->sc_stateid, has_session);
> spin_unlock(&s->sc_lock);
> + if (ret == nfserr_admin_revoked)
> + nfs40_drop_revoked_stid(s->sc_client,
> + &s->sc_stateid);
> return ret;
> }
>
> @@ -6548,6 +6636,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> }
> out_unlock:
> spin_unlock(&cl->cl_lock);
> + if (status == nfserr_admin_revoked)
> + nfs40_drop_revoked_stid(cl, stateid);
> return status;
> }
>
> @@ -6594,6 +6684,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> return nfserr_deleg_revoked;
> }
> if (stid->sc_type & NFS4_STID_ADMIN_REVOKED) {
> + nfs40_drop_revoked_stid(cstate->clp, stateid);
> nfs4_put_stid(stid);
> return nfserr_admin_revoked;
> }
> @@ -6886,6 +6977,11 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> s = find_stateid_locked(cl, stateid);
> if (!s || s->sc_status & NFS4_STID_CLOSED)
> goto out_unlock;
> + if (s->sc_status & NFS4_STID_ADMIN_REVOKED) {
> + nfsd_drop_revoked_stid(s);
> + ret = nfs_ok;
> + goto out;
> + }
> spin_lock(&s->sc_lock);
> switch (s->sc_type) {
> case NFS4_DELEG_STID:
> @@ -6912,7 +7008,6 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> spin_unlock(&cl->cl_lock);
> ret = nfsd4_free_lock_stateid(stateid, s);
> goto out;
> - /* Default falls through and returns nfserr_bad_stateid */
> }
> spin_unlock(&s->sc_lock);
> out_unlock:
> --
> 2.42.0
>
--
Chuck Lever
On Fri, 17 Nov 2023, Jeff Layton wrote:
> On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > For NFSv4.1 and later the client easily discovers if there is any
> > admin-revoked state and will then find and explicitly free it.
> >
> > For NFSv4.0 there is no such mechanism. The client can only find that
> > state is admin-revoked if it tries to use that state, and there is no
> > way for it to explicitly free the state. So the server must hold on to
> > the stateid (at least) for an indefinite amount of time. A
> > RELEASE_LOCKOWNER request might justify forgetting some of these
> > stateids, as would the whole clients lease lapsing, but these are not
> > reliable.
> >
> > This patch takes two approaches.
> >
> > Whenever a client uses an revoked stateid, that stateid is then
> > discarded and will not be recognised again. This might confuse a client
> > which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
> > all, but should mostly work. Hopefully one error will lead to other
> > resources being closed (e.g. process exits), which will result in more
> > stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.
> >
> > Also, any admin-revoked stateids that have been that way for more than
> > one lease time are periodically revoke.
> >
>
> Why a single lease period?
Because it was easy to code.
>
> Is that a long enough time for v4.0? Given that the protocol has no
> mechanism to detect revoked state, I have to wonder if a single lease
> period is enough time for the client to detect the problem?
There is no amount of time that is long enough.
In many cases the client will notice quickly. In some cases it might
not notice for days or weeks.
Given that there is no perfect solution, and given that v4.0 is really
just a prototype that probably shouldn't be used any more, I don't think
there is any benefit is trying any harder.
Thanks,
NeilBrown
On Sat, 18 Nov 2023, Chuck Lever wrote:
> On Fri, Nov 17, 2023 at 01:18:52PM +1100, NeilBrown wrote:
> > For NFSv4.1 and later the client easily discovers if there is any
> > admin-revoked state and will then find and explicitly free it.
> >
> > For NFSv4.0 there is no such mechanism. The client can only find that
> > state is admin-revoked if it tries to use that state, and there is no
> > way for it to explicitly free the state. So the server must hold on to
> > the stateid (at least) for an indefinite amount of time. A
> > RELEASE_LOCKOWNER request might justify forgetting some of these
> > stateids, as would the whole clients lease lapsing, but these are not
> > reliable.
> >
> > This patch takes two approaches.
> >
> > Whenever a client uses an revoked stateid, that stateid is then
> > discarded and will not be recognised again. This might confuse a client
> > which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
> > all, but should mostly work. Hopefully one error will lead to other
> > resources being closed (e.g. process exits), which will result in more
> > stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.
> >
> > Also, any admin-revoked stateids that have been that way for more than
> > one lease time are periodically revoke.
> >
> > No actual freeing of state happens in this patch. That will come in
> > future patches which handle the different sorts of revoked state.
> >
> > Signed-off-by: NeilBrown <[email protected]>
> > ---
> > fs/nfsd/netns.h | 4 ++
> > fs/nfsd/nfs4state.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 100 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > index ec49b200b797..02f8fa095b0f 100644
> > --- a/fs/nfsd/netns.h
> > +++ b/fs/nfsd/netns.h
> > @@ -197,6 +197,10 @@ struct nfsd_net {
> > atomic_t nfsd_courtesy_clients;
> > struct shrinker nfsd_client_shrinker;
> > struct work_struct nfsd_shrinker_work;
> > +
> > + /* last time an admin-revoke happened for NFSv4.0 */
> > + time64_t nfs40_last_revoke;
> > +
> > };
>
> This hunk doesn't apply to nfsd-next due to v6.7-rc changes to NFSD
> to implement a dynamic shrinker. So I stopped my review here for
> now.
I didn't a rebase onto nfsd-next and there were no conflicts!
I guess the change from
struct work_struct nfsd_shrinker_work;
to
struct work_struct *nfsd_shrinker_work;
was technically a conflict but I'm surprised your tool complained..
NeilBrown
On Mon, Nov 20, 2023 at 09:47:03AM +1100, NeilBrown wrote:
> On Sat, 18 Nov 2023, Chuck Lever wrote:
> > On Fri, Nov 17, 2023 at 01:18:52PM +1100, NeilBrown wrote:
> > > For NFSv4.1 and later the client easily discovers if there is any
> > > admin-revoked state and will then find and explicitly free it.
> > >
> > > For NFSv4.0 there is no such mechanism. The client can only find that
> > > state is admin-revoked if it tries to use that state, and there is no
> > > way for it to explicitly free the state. So the server must hold on to
> > > the stateid (at least) for an indefinite amount of time. A
> > > RELEASE_LOCKOWNER request might justify forgetting some of these
> > > stateids, as would the whole clients lease lapsing, but these are not
> > > reliable.
> > >
> > > This patch takes two approaches.
> > >
> > > Whenever a client uses an revoked stateid, that stateid is then
> > > discarded and will not be recognised again. This might confuse a client
> > > which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
> > > all, but should mostly work. Hopefully one error will lead to other
> > > resources being closed (e.g. process exits), which will result in more
> > > stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.
> > >
> > > Also, any admin-revoked stateids that have been that way for more than
> > > one lease time are periodically revoke.
> > >
> > > No actual freeing of state happens in this patch. That will come in
> > > future patches which handle the different sorts of revoked state.
> > >
> > > Signed-off-by: NeilBrown <[email protected]>
> > > ---
> > > fs/nfsd/netns.h | 4 ++
> > > fs/nfsd/nfs4state.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
> > > 2 files changed, 100 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > > index ec49b200b797..02f8fa095b0f 100644
> > > --- a/fs/nfsd/netns.h
> > > +++ b/fs/nfsd/netns.h
> > > @@ -197,6 +197,10 @@ struct nfsd_net {
> > > atomic_t nfsd_courtesy_clients;
> > > struct shrinker nfsd_client_shrinker;
> > > struct work_struct nfsd_shrinker_work;
> > > +
> > > + /* last time an admin-revoke happened for NFSv4.0 */
> > > + time64_t nfs40_last_revoke;
> > > +
> > > };
> >
> > This hunk doesn't apply to nfsd-next due to v6.7-rc changes to NFSD
> > to implement a dynamic shrinker. So I stopped my review here for
> > now.
>
> I didn't a rebase onto nfsd-next and there were no conflicts!
>
> I guess the change from
> struct work_struct nfsd_shrinker_work;
> to
> struct work_struct *nfsd_shrinker_work;
>
> was technically a conflict but I'm surprised your tool complained..
That was the change I noticed when it failed to apply. "stg import"
is pretty picky, unfortunately...
--
Chuck Lever