NFS4_CLOSED_DELEG_STID and NFS4_REVOKED_DELEG_STID are similar in
purpose.
REVOKED is used for NFSv4.1 states which have been revoked because the
lease has expired. CLOSED is used in other cases.
The difference has two practical effects.
1/ REVOKED states are on the ->cl_revoked list
2/ REVOKED states result in nfserr_deleg_revoked from
nfsd4_verify_open_stid() asnd nfsd4_validate_stateid while
CLOSED states result in nfserr_bad_stid.
Currently a state that is being revoked is first set to "CLOSED" in
unhash_delegation_locked(), then possibly to "REVOKED" in
revoke_delegation(), at which point it is added to the cl_revoked list.
It is possible that a stateid test could see the CLOSED state
which really should be REVOKED, and so return the wrong error code. So
it is safest to remove this window of inconsistency.
With this patch, unhash_delegation_locked() always set the state
correctly, and revoke_delegation() no longer changes the state.
Also remove a redundant test on minorversion when
NFS4_REVOKED_DELEG_STID is seen - it can only be seen when minorversion
is non-zero.
Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/nfs4state.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6368788a7d4e..7469583382fb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1334,7 +1334,7 @@ static bool delegation_hashed(struct nfs4_delegation *dp)
}
static bool
-unhash_delegation_locked(struct nfs4_delegation *dp)
+unhash_delegation_locked(struct nfs4_delegation *dp, unsigned char type)
{
struct nfs4_file *fp = dp->dl_stid.sc_file;
@@ -1343,7 +1343,9 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
if (!delegation_hashed(dp))
return false;
- dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
+ if (dp->dl_stid.sc_client->cl_minorversion == 0)
+ type = NFS4_CLOSED_DELEG_STID;
+ dp->dl_stid.sc_type = type;
/* Ensure that deleg break won't try to requeue it */
++dp->dl_time;
spin_lock(&fp->fi_lock);
@@ -1359,7 +1361,7 @@ static void destroy_delegation(struct nfs4_delegation *dp)
bool unhashed;
spin_lock(&state_lock);
- unhashed = unhash_delegation_locked(dp);
+ unhashed = unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID);
spin_unlock(&state_lock);
if (unhashed)
destroy_unhashed_deleg(dp);
@@ -1373,9 +1375,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
trace_nfsd_stid_revoke(&dp->dl_stid);
- if (clp->cl_minorversion) {
+ if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
spin_lock(&clp->cl_lock);
- dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
refcount_inc(&dp->dl_stid.sc_count);
list_add(&dp->dl_recall_lru, &clp->cl_revoked);
spin_unlock(&clp->cl_lock);
@@ -2234,7 +2235,7 @@ __destroy_client(struct nfs4_client *clp)
spin_lock(&state_lock);
while (!list_empty(&clp->cl_delegations)) {
dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
- WARN_ON(!unhash_delegation_locked(dp));
+ WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
list_add(&dp->dl_recall_lru, &reaplist);
}
spin_unlock(&state_lock);
@@ -5197,8 +5198,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
goto out;
if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
nfs4_put_stid(&deleg->dl_stid);
- if (cl->cl_minorversion)
- status = nfserr_deleg_revoked;
+ status = nfserr_deleg_revoked;
goto out;
}
flags = share_access_to_flags(open->op_share_access);
@@ -6235,7 +6235,7 @@ nfs4_laundromat(struct nfsd_net *nn)
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
if (!state_expired(<, dp->dl_time))
break;
- WARN_ON(!unhash_delegation_locked(dp));
+ WARN_ON(!unhash_delegation_locked(dp, NFS4_REVOKED_DELEG_STID));
list_add(&dp->dl_recall_lru, &reaplist);
}
spin_unlock(&state_lock);
@@ -8350,7 +8350,7 @@ nfs4_state_shutdown_net(struct net *net)
spin_lock(&state_lock);
list_for_each_safe(pos, next, &nn->del_recall_lru) {
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
- WARN_ON(!unhash_delegation_locked(dp));
+ WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
list_add(&dp->dl_recall_lru, &reaplist);
}
spin_unlock(&state_lock);
--
2.42.0
On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> NFS4_CLOSED_DELEG_STID and NFS4_REVOKED_DELEG_STID are similar in
> purpose.
> REVOKED is used for NFSv4.1 states which have been revoked because the
> lease has expired. CLOSED is used in other cases.
> The difference has two practical effects.
> 1/ REVOKED states are on the ->cl_revoked list
> 2/ REVOKED states result in nfserr_deleg_revoked from
> nfsd4_verify_open_stid() asnd nfsd4_validate_stateid while
> CLOSED states result in nfserr_bad_stid.
>
> Currently a state that is being revoked is first set to "CLOSED" in
> unhash_delegation_locked(), then possibly to "REVOKED" in
> revoke_delegation(), at which point it is added to the cl_revoked list.
>
> It is possible that a stateid test could see the CLOSED state
> which really should be REVOKED, and so return the wrong error code. So
> it is safest to remove this window of inconsistency.
>
> With this patch, unhash_delegation_locked() always set the state
> correctly, and revoke_delegation() no longer changes the state.
>
> Also remove a redundant test on minorversion when
> NFS4_REVOKED_DELEG_STID is seen - it can only be seen when minorversion
> is non-zero.
>
> Signed-off-by: NeilBrown <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6368788a7d4e..7469583382fb 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1334,7 +1334,7 @@ static bool delegation_hashed(struct nfs4_delegation *dp)
> }
>
> static bool
> -unhash_delegation_locked(struct nfs4_delegation *dp)
> +unhash_delegation_locked(struct nfs4_delegation *dp, unsigned char type)
> {
> struct nfs4_file *fp = dp->dl_stid.sc_file;
>
> @@ -1343,7 +1343,9 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
> if (!delegation_hashed(dp))
> return false;
>
> - dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> + if (dp->dl_stid.sc_client->cl_minorversion == 0)
> + type = NFS4_CLOSED_DELEG_STID;
> + dp->dl_stid.sc_type = type;
> /* Ensure that deleg break won't try to requeue it */
> ++dp->dl_time;
> spin_lock(&fp->fi_lock);
> @@ -1359,7 +1361,7 @@ static void destroy_delegation(struct nfs4_delegation *dp)
> bool unhashed;
>
> spin_lock(&state_lock);
> - unhashed = unhash_delegation_locked(dp);
> + unhashed = unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID);
> spin_unlock(&state_lock);
> if (unhashed)
> destroy_unhashed_deleg(dp);
> @@ -1373,9 +1375,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
>
> trace_nfsd_stid_revoke(&dp->dl_stid);
>
> - if (clp->cl_minorversion) {
> + if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> spin_lock(&clp->cl_lock);
> - dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
> refcount_inc(&dp->dl_stid.sc_count);
> list_add(&dp->dl_recall_lru, &clp->cl_revoked);
> spin_unlock(&clp->cl_lock);
> @@ -2234,7 +2235,7 @@ __destroy_client(struct nfs4_client *clp)
> spin_lock(&state_lock);
> while (!list_empty(&clp->cl_delegations)) {
> dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
> - WARN_ON(!unhash_delegation_locked(dp));
> + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
> list_add(&dp->dl_recall_lru, &reaplist);
> }
> spin_unlock(&state_lock);
> @@ -5197,8 +5198,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
> goto out;
> if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> nfs4_put_stid(&deleg->dl_stid);
> - if (cl->cl_minorversion)
> - status = nfserr_deleg_revoked;
> + status = nfserr_deleg_revoked;
> goto out;
> }
> flags = share_access_to_flags(open->op_share_access);
> @@ -6235,7 +6235,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> if (!state_expired(<, dp->dl_time))
> break;
> - WARN_ON(!unhash_delegation_locked(dp));
> + WARN_ON(!unhash_delegation_locked(dp, NFS4_REVOKED_DELEG_STID));
> list_add(&dp->dl_recall_lru, &reaplist);
> }
> spin_unlock(&state_lock);
> @@ -8350,7 +8350,7 @@ nfs4_state_shutdown_net(struct net *net)
> spin_lock(&state_lock);
> list_for_each_safe(pos, next, &nn->del_recall_lru) {
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> - WARN_ON(!unhash_delegation_locked(dp));
> + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
> list_add(&dp->dl_recall_lru, &reaplist);
> }
> spin_unlock(&state_lock);
Same question here. Should this go to stable? I guess the race is not
generally fatal...
Reviewed-by: Jeff Layton <[email protected]>
On Fri, Nov 17, 2023 at 06:41:37AM -0500, Jeff Layton wrote:
> On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > NFS4_CLOSED_DELEG_STID and NFS4_REVOKED_DELEG_STID are similar in
> > purpose.
> > REVOKED is used for NFSv4.1 states which have been revoked because the
> > lease has expired. CLOSED is used in other cases.
> > The difference has two practical effects.
> > 1/ REVOKED states are on the ->cl_revoked list
> > 2/ REVOKED states result in nfserr_deleg_revoked from
> > nfsd4_verify_open_stid() asnd nfsd4_validate_stateid while
> > CLOSED states result in nfserr_bad_stid.
> >
> > Currently a state that is being revoked is first set to "CLOSED" in
> > unhash_delegation_locked(), then possibly to "REVOKED" in
> > revoke_delegation(), at which point it is added to the cl_revoked list.
> >
> > It is possible that a stateid test could see the CLOSED state
> > which really should be REVOKED, and so return the wrong error code. So
> > it is safest to remove this window of inconsistency.
> >
> > With this patch, unhash_delegation_locked() always set the state
> > correctly, and revoke_delegation() no longer changes the state.
> >
> > Also remove a redundant test on minorversion when
> > NFS4_REVOKED_DELEG_STID is seen - it can only be seen when minorversion
> > is non-zero.
> >
> > Signed-off-by: NeilBrown <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 6368788a7d4e..7469583382fb 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1334,7 +1334,7 @@ static bool delegation_hashed(struct nfs4_delegation *dp)
> > }
> >
> > static bool
> > -unhash_delegation_locked(struct nfs4_delegation *dp)
> > +unhash_delegation_locked(struct nfs4_delegation *dp, unsigned char type)
unsigned short type ?
> > {
> > struct nfs4_file *fp = dp->dl_stid.sc_file;
> >
> > @@ -1343,7 +1343,9 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
> > if (!delegation_hashed(dp))
> > return false;
> >
> > - dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> > + if (dp->dl_stid.sc_client->cl_minorversion == 0)
> > + type = NFS4_CLOSED_DELEG_STID;
> > + dp->dl_stid.sc_type = type;
> > /* Ensure that deleg break won't try to requeue it */
> > ++dp->dl_time;
> > spin_lock(&fp->fi_lock);
> > @@ -1359,7 +1361,7 @@ static void destroy_delegation(struct nfs4_delegation *dp)
> > bool unhashed;
> >
> > spin_lock(&state_lock);
> > - unhashed = unhash_delegation_locked(dp);
> > + unhashed = unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID);
> > spin_unlock(&state_lock);
> > if (unhashed)
> > destroy_unhashed_deleg(dp);
> > @@ -1373,9 +1375,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
> >
> > trace_nfsd_stid_revoke(&dp->dl_stid);
> >
> > - if (clp->cl_minorversion) {
> > + if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> > spin_lock(&clp->cl_lock);
> > - dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
> > refcount_inc(&dp->dl_stid.sc_count);
> > list_add(&dp->dl_recall_lru, &clp->cl_revoked);
> > spin_unlock(&clp->cl_lock);
> > @@ -2234,7 +2235,7 @@ __destroy_client(struct nfs4_client *clp)
> > spin_lock(&state_lock);
> > while (!list_empty(&clp->cl_delegations)) {
> > dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
> > - WARN_ON(!unhash_delegation_locked(dp));
> > + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
> > list_add(&dp->dl_recall_lru, &reaplist);
> > }
> > spin_unlock(&state_lock);
> > @@ -5197,8 +5198,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
> > goto out;
> > if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> > nfs4_put_stid(&deleg->dl_stid);
> > - if (cl->cl_minorversion)
> > - status = nfserr_deleg_revoked;
> > + status = nfserr_deleg_revoked;
> > goto out;
> > }
> > flags = share_access_to_flags(open->op_share_access);
> > @@ -6235,7 +6235,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > if (!state_expired(<, dp->dl_time))
> > break;
> > - WARN_ON(!unhash_delegation_locked(dp));
> > + WARN_ON(!unhash_delegation_locked(dp, NFS4_REVOKED_DELEG_STID));
> > list_add(&dp->dl_recall_lru, &reaplist);
> > }
> > spin_unlock(&state_lock);
> > @@ -8350,7 +8350,7 @@ nfs4_state_shutdown_net(struct net *net)
> > spin_lock(&state_lock);
> > list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > - WARN_ON(!unhash_delegation_locked(dp));
> > + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
Neil, what say we get rid of these WARN_ONs?
> > list_add(&dp->dl_recall_lru, &reaplist);
> > }
> > spin_unlock(&state_lock);
>
> Same question here. Should this go to stable? I guess the race is not
> generally fatal...
Again, there's no existing bug report, so no urgency to get this
backported. I would see these changes soak in upstream rather than
pull them into stable quickly only to discover another bug has been
introduced.
We don't have a failing test or a data corruption risk, as far as
I can tell.
> Reviewed-by: Jeff Layton <[email protected]>
--
Chuck Lever
On Sat, 18 Nov 2023, Chuck Lever wrote:
> On Fri, Nov 17, 2023 at 06:41:37AM -0500, Jeff Layton wrote:
> > On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > > NFS4_CLOSED_DELEG_STID and NFS4_REVOKED_DELEG_STID are similar in
> > > purpose.
> > > REVOKED is used for NFSv4.1 states which have been revoked because the
> > > lease has expired. CLOSED is used in other cases.
> > > The difference has two practical effects.
> > > 1/ REVOKED states are on the ->cl_revoked list
> > > 2/ REVOKED states result in nfserr_deleg_revoked from
> > > nfsd4_verify_open_stid() asnd nfsd4_validate_stateid while
> > > CLOSED states result in nfserr_bad_stid.
> > >
> > > Currently a state that is being revoked is first set to "CLOSED" in
> > > unhash_delegation_locked(), then possibly to "REVOKED" in
> > > revoke_delegation(), at which point it is added to the cl_revoked list.
> > >
> > > It is possible that a stateid test could see the CLOSED state
> > > which really should be REVOKED, and so return the wrong error code. So
> > > it is safest to remove this window of inconsistency.
> > >
> > > With this patch, unhash_delegation_locked() always set the state
> > > correctly, and revoke_delegation() no longer changes the state.
> > >
> > > Also remove a redundant test on minorversion when
> > > NFS4_REVOKED_DELEG_STID is seen - it can only be seen when minorversion
> > > is non-zero.
> > >
> > > Signed-off-by: NeilBrown <[email protected]>
> > > ---
> > > fs/nfsd/nfs4state.c | 20 ++++++++++----------
> > > 1 file changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 6368788a7d4e..7469583382fb 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -1334,7 +1334,7 @@ static bool delegation_hashed(struct nfs4_delegation *dp)
> > > }
> > >
> > > static bool
> > > -unhash_delegation_locked(struct nfs4_delegation *dp)
> > > +unhash_delegation_locked(struct nfs4_delegation *dp, unsigned char type)
>
> unsigned short type ?
At this stage in the series 'type' is still an unsigned char. I don't
want to get ahead of myself.
>
>
> > > {
> > > struct nfs4_file *fp = dp->dl_stid.sc_file;
> > >
> > > @@ -1343,7 +1343,9 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
> > > if (!delegation_hashed(dp))
> > > return false;
> > >
> > > - dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> > > + if (dp->dl_stid.sc_client->cl_minorversion == 0)
> > > + type = NFS4_CLOSED_DELEG_STID;
> > > + dp->dl_stid.sc_type = type;
> > > /* Ensure that deleg break won't try to requeue it */
> > > ++dp->dl_time;
> > > spin_lock(&fp->fi_lock);
> > > @@ -1359,7 +1361,7 @@ static void destroy_delegation(struct nfs4_delegation *dp)
> > > bool unhashed;
> > >
> > > spin_lock(&state_lock);
> > > - unhashed = unhash_delegation_locked(dp);
> > > + unhashed = unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID);
> > > spin_unlock(&state_lock);
> > > if (unhashed)
> > > destroy_unhashed_deleg(dp);
> > > @@ -1373,9 +1375,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
> > >
> > > trace_nfsd_stid_revoke(&dp->dl_stid);
> > >
> > > - if (clp->cl_minorversion) {
> > > + if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> > > spin_lock(&clp->cl_lock);
> > > - dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
> > > refcount_inc(&dp->dl_stid.sc_count);
> > > list_add(&dp->dl_recall_lru, &clp->cl_revoked);
> > > spin_unlock(&clp->cl_lock);
> > > @@ -2234,7 +2235,7 @@ __destroy_client(struct nfs4_client *clp)
> > > spin_lock(&state_lock);
> > > while (!list_empty(&clp->cl_delegations)) {
> > > dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
> > > - WARN_ON(!unhash_delegation_locked(dp));
> > > + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
> > > list_add(&dp->dl_recall_lru, &reaplist);
> > > }
> > > spin_unlock(&state_lock);
> > > @@ -5197,8 +5198,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
> > > goto out;
> > > if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> > > nfs4_put_stid(&deleg->dl_stid);
> > > - if (cl->cl_minorversion)
> > > - status = nfserr_deleg_revoked;
> > > + status = nfserr_deleg_revoked;
> > > goto out;
> > > }
> > > flags = share_access_to_flags(open->op_share_access);
> > > @@ -6235,7 +6235,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > > dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > > if (!state_expired(<, dp->dl_time))
> > > break;
> > > - WARN_ON(!unhash_delegation_locked(dp));
> > > + WARN_ON(!unhash_delegation_locked(dp, NFS4_REVOKED_DELEG_STID));
> > > list_add(&dp->dl_recall_lru, &reaplist);
> > > }
> > > spin_unlock(&state_lock);
> > > @@ -8350,7 +8350,7 @@ nfs4_state_shutdown_net(struct net *net)
> > > spin_lock(&state_lock);
> > > list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > > dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > > - WARN_ON(!unhash_delegation_locked(dp));
> > > + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
>
> Neil, what say we get rid of these WARN_ONs?
>
I've added a patch with this intro:
Author: NeilBrown <[email protected]>
Date: Mon Nov 20 09:15:46 2023 +1100
nfsd: don't call functions with side-effecting inside WARN_ON()
Code like:
WARN_ON(foo())
looks like an assertion and might not be expected to have any side
effects.
When testing if a function with side-effects fails a construct like
if (foo())
WARN_ON(1);
makes the intent more obvious.
nfsd has several WARN_ON calls where the test has side effects, so it
would be good to change them. These cases don't really need the
WARN_ON. They have never failed in 8 years of usage so let's just
remove the WARN_ON wrapper.
Suggested-by: Chuck Lever <[email protected]>
Signed-off-by: NeilBrown <[email protected]>
it removes 5 WARN_ONs from unhash_delegation_locked() calls.
They were added by
Commit 3fcbbd244ed1 ("nfsd: ensure that delegation stateid hash references are only put once")
in 4.3
Thanks,
NeilBrown
>
> > > list_add(&dp->dl_recall_lru, &reaplist);
> > > }
> > > spin_unlock(&state_lock);
> >
> > Same question here. Should this go to stable? I guess the race is not
> > generally fatal...
>
> Again, there's no existing bug report, so no urgency to get this
> backported. I would see these changes soak in upstream rather than
> pull them into stable quickly only to discover another bug has been
> introduced.
>
> We don't have a failing test or a data corruption risk, as far as
> I can tell.
>
>
> > Reviewed-by: Jeff Layton <[email protected]>
>
> --
> Chuck Lever
>
On Mon, Nov 20, 2023 at 09:23:43AM +1100, NeilBrown wrote:
> On Sat, 18 Nov 2023, Chuck Lever wrote:
> > On Fri, Nov 17, 2023 at 06:41:37AM -0500, Jeff Layton wrote:
> > > On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > > > NFS4_CLOSED_DELEG_STID and NFS4_REVOKED_DELEG_STID are similar in
> > > > purpose.
> > > > REVOKED is used for NFSv4.1 states which have been revoked because the
> > > > lease has expired. CLOSED is used in other cases.
> > > > The difference has two practical effects.
> > > > 1/ REVOKED states are on the ->cl_revoked list
> > > > 2/ REVOKED states result in nfserr_deleg_revoked from
> > > > nfsd4_verify_open_stid() asnd nfsd4_validate_stateid while
> > > > CLOSED states result in nfserr_bad_stid.
> > > >
> > > > Currently a state that is being revoked is first set to "CLOSED" in
> > > > unhash_delegation_locked(), then possibly to "REVOKED" in
> > > > revoke_delegation(), at which point it is added to the cl_revoked list.
> > > >
> > > > It is possible that a stateid test could see the CLOSED state
> > > > which really should be REVOKED, and so return the wrong error code. So
> > > > it is safest to remove this window of inconsistency.
> > > >
> > > > With this patch, unhash_delegation_locked() always set the state
> > > > correctly, and revoke_delegation() no longer changes the state.
> > > >
> > > > Also remove a redundant test on minorversion when
> > > > NFS4_REVOKED_DELEG_STID is seen - it can only be seen when minorversion
> > > > is non-zero.
> > > >
> > > > Signed-off-by: NeilBrown <[email protected]>
> > > > ---
> > > > fs/nfsd/nfs4state.c | 20 ++++++++++----------
> > > > 1 file changed, 10 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index 6368788a7d4e..7469583382fb 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > > @@ -1334,7 +1334,7 @@ static bool delegation_hashed(struct nfs4_delegation *dp)
> > > > }
> > > >
> > > > static bool
> > > > -unhash_delegation_locked(struct nfs4_delegation *dp)
> > > > +unhash_delegation_locked(struct nfs4_delegation *dp, unsigned char type)
> >
> > unsigned short type ?
>
> At this stage in the series 'type' is still an unsigned char. I don't
> want to get ahead of myself.
>
> >
> >
> > > > {
> > > > struct nfs4_file *fp = dp->dl_stid.sc_file;
> > > >
> > > > @@ -1343,7 +1343,9 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
> > > > if (!delegation_hashed(dp))
> > > > return false;
> > > >
> > > > - dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> > > > + if (dp->dl_stid.sc_client->cl_minorversion == 0)
> > > > + type = NFS4_CLOSED_DELEG_STID;
> > > > + dp->dl_stid.sc_type = type;
> > > > /* Ensure that deleg break won't try to requeue it */
> > > > ++dp->dl_time;
> > > > spin_lock(&fp->fi_lock);
> > > > @@ -1359,7 +1361,7 @@ static void destroy_delegation(struct nfs4_delegation *dp)
> > > > bool unhashed;
> > > >
> > > > spin_lock(&state_lock);
> > > > - unhashed = unhash_delegation_locked(dp);
> > > > + unhashed = unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID);
> > > > spin_unlock(&state_lock);
> > > > if (unhashed)
> > > > destroy_unhashed_deleg(dp);
> > > > @@ -1373,9 +1375,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
> > > >
> > > > trace_nfsd_stid_revoke(&dp->dl_stid);
> > > >
> > > > - if (clp->cl_minorversion) {
> > > > + if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> > > > spin_lock(&clp->cl_lock);
> > > > - dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
> > > > refcount_inc(&dp->dl_stid.sc_count);
> > > > list_add(&dp->dl_recall_lru, &clp->cl_revoked);
> > > > spin_unlock(&clp->cl_lock);
> > > > @@ -2234,7 +2235,7 @@ __destroy_client(struct nfs4_client *clp)
> > > > spin_lock(&state_lock);
> > > > while (!list_empty(&clp->cl_delegations)) {
> > > > dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
> > > > - WARN_ON(!unhash_delegation_locked(dp));
> > > > + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
> > > > list_add(&dp->dl_recall_lru, &reaplist);
> > > > }
> > > > spin_unlock(&state_lock);
> > > > @@ -5197,8 +5198,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
> > > > goto out;
> > > > if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> > > > nfs4_put_stid(&deleg->dl_stid);
> > > > - if (cl->cl_minorversion)
> > > > - status = nfserr_deleg_revoked;
> > > > + status = nfserr_deleg_revoked;
> > > > goto out;
> > > > }
> > > > flags = share_access_to_flags(open->op_share_access);
> > > > @@ -6235,7 +6235,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> > > > dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > > > if (!state_expired(<, dp->dl_time))
> > > > break;
> > > > - WARN_ON(!unhash_delegation_locked(dp));
> > > > + WARN_ON(!unhash_delegation_locked(dp, NFS4_REVOKED_DELEG_STID));
> > > > list_add(&dp->dl_recall_lru, &reaplist);
> > > > }
> > > > spin_unlock(&state_lock);
> > > > @@ -8350,7 +8350,7 @@ nfs4_state_shutdown_net(struct net *net)
> > > > spin_lock(&state_lock);
> > > > list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > > > dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > > > - WARN_ON(!unhash_delegation_locked(dp));
> > > > + WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
> >
> > Neil, what say we get rid of these WARN_ONs?
> >
>
> I've added a patch with this intro:
> Author: NeilBrown <[email protected]>
> Date: Mon Nov 20 09:15:46 2023 +1100
>
> nfsd: don't call functions with side-effecting inside WARN_ON()
>
> Code like:
>
> WARN_ON(foo())
>
> looks like an assertion and might not be expected to have any side
> effects.
> When testing if a function with side-effects fails a construct like
>
> if (foo())
> WARN_ON(1);
>
> makes the intent more obvious.
>
> nfsd has several WARN_ON calls where the test has side effects, so it
> would be good to change them. These cases don't really need the
> WARN_ON. They have never failed in 8 years of usage so let's just
> remove the WARN_ON wrapper.
>
> Suggested-by: Chuck Lever <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
>
> it removes 5 WARN_ONs from unhash_delegation_locked() calls.
> They were added by
> Commit 3fcbbd244ed1 ("nfsd: ensure that delegation stateid hash references are only put once")
> in 4.3
Very sensible, thank you!
> Thanks,
> NeilBrown
>
> >
> > > > list_add(&dp->dl_recall_lru, &reaplist);
> > > > }
> > > > spin_unlock(&state_lock);
> > >
> > > Same question here. Should this go to stable? I guess the race is not
> > > generally fatal...
> >
> > Again, there's no existing bug report, so no urgency to get this
> > backported. I would see these changes soak in upstream rather than
> > pull them into stable quickly only to discover another bug has been
> > introduced.
> >
> > We don't have a failing test or a data corruption risk, as far as
> > I can tell.
> >
> >
> > > Reviewed-by: Jeff Layton <[email protected]>
> >
> > --
> > Chuck Lever
> >
>
--
Chuck Lever