| Message ID | 20231124002504.19515-3-neilb@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Fri, 2023-11-24 at 11:23 +1100, NeilBrown wrote: > 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 <chuck.lever@oracle.com> > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 042c7a50f425..dd01d0b9e21e 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1605,7 +1605,7 @@ static void release_open_stateid_locks(struct nfs4_ol_stateid *open_stp, > while (!list_empty(&open_stp->st_locks)) { > stp = list_entry(open_stp->st_locks.next, > struct nfs4_ol_stateid, st_locks); > - WARN_ON(!unhash_lock_stateid(stp)); > + unhash_lock_stateid(stp); > put_ol_stateid_locked(stp, reaplist); > } > } > @@ -2234,7 +2234,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)); > + unhash_delegation_locked(dp); > list_add(&dp->dl_recall_lru, &reaplist); > } > spin_unlock(&state_lock); > @@ -6234,7 +6234,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)); > + unhash_delegation_locked(dp); > list_add(&dp->dl_recall_lru, &reaplist); > } > spin_unlock(&state_lock); > @@ -8060,7 +8060,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, > stp = list_first_entry(&lo->lo_owner.so_stateids, > struct nfs4_ol_stateid, > st_perstateowner); > - WARN_ON(!unhash_lock_stateid(stp)); > + unhash_lock_stateid(stp); > put_ol_stateid_locked(stp, &reaplist); > } > spin_unlock(&clp->cl_lock); > @@ -8353,7 +8353,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)); > + unhash_delegation_locked(dp); > list_add(&dp->dl_recall_lru, &reaplist); > } > spin_unlock(&state_lock); Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 042c7a50f425..dd01d0b9e21e 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1605,7 +1605,7 @@ static void release_open_stateid_locks(struct nfs4_ol_stateid *open_stp, while (!list_empty(&open_stp->st_locks)) { stp = list_entry(open_stp->st_locks.next, struct nfs4_ol_stateid, st_locks); - WARN_ON(!unhash_lock_stateid(stp)); + unhash_lock_stateid(stp); put_ol_stateid_locked(stp, reaplist); } } @@ -2234,7 +2234,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)); + unhash_delegation_locked(dp); list_add(&dp->dl_recall_lru, &reaplist); } spin_unlock(&state_lock); @@ -6234,7 +6234,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)); + unhash_delegation_locked(dp); list_add(&dp->dl_recall_lru, &reaplist); } spin_unlock(&state_lock); @@ -8060,7 +8060,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, stp = list_first_entry(&lo->lo_owner.so_stateids, struct nfs4_ol_stateid, st_perstateowner); - WARN_ON(!unhash_lock_stateid(stp)); + unhash_lock_stateid(stp); put_ol_stateid_locked(stp, &reaplist); } spin_unlock(&clp->cl_lock); @@ -8353,7 +8353,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)); + unhash_delegation_locked(dp); list_add(&dp->dl_recall_lru, &reaplist); } spin_unlock(&state_lock);
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 <chuck.lever@oracle.com> Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs4state.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)