* [PATCH v2 0/1] Allow FREE_STATEID to free delegations @ 2025-05-01 12:29 Benjamin Coddington 2025-05-01 12:29 ` [PATCH v2 1/1] NFSv4: Allow FREE_STATEID to clean up delegations Benjamin Coddington 0 siblings, 1 reply; 3+ messages in thread From: Benjamin Coddington @ 2025-05-01 12:29 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker, Jeff Layton; +Cc: linux-nfs A problem observed for some clients is that the list of nfs_server->delegations can grow unweildy, leading to the clients spinning in tight loops walking across delegations that have been marked revoked. These two patches attempt to solve that problem by using the result of FREE_STATEID to clean up the list of delegations which keeps that list pruned to an operable size. Changes on v2: - dropped the first patch which was unnecessary - add the FREED_STATEID case to nfs41_test_and_free_expired() Benjamin Coddington (1): NFSv4: Allow FREE_STATEID to clean up delegations fs/nfs/delegation.c | 25 ++++++++++++++++++------- fs/nfs/nfs4_fs.h | 3 +-- fs/nfs/nfs4proc.c | 12 ++++++------ include/linux/nfs4.h | 1 + 4 files changed, 26 insertions(+), 15 deletions(-) -- 2.47.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v2 1/1] NFSv4: Allow FREE_STATEID to clean up delegations 2025-05-01 12:29 [PATCH v2 0/1] Allow FREE_STATEID to free delegations Benjamin Coddington @ 2025-05-01 12:29 ` Benjamin Coddington 2025-05-01 15:03 ` Jeff Layton 0 siblings, 1 reply; 3+ messages in thread From: Benjamin Coddington @ 2025-05-01 12:29 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker, Jeff Layton; +Cc: linux-nfs The NFS client's list of delegations can grow quite large (well beyond the delegation watermark) if the server is revoking or there are repeated events that expire state. Once this happens, the revoked delegations can cause a performance problem for subsequent walks of the servers->delegations list when the client tries to test and free state. If we can determine that the FREE_STATEID operation has completed without error, we can prune the delegation from the list. Since the NFS client combines TEST_STATEID with FREE_STATEID in its minor version operations, there isn't an easy way to communicate success of FREE_STATEID. Rather than re-arrange quite a number of calling paths to break out the separate procedures, let's signal the success of FREE_STATEID by setting the stateid's type. Set NFS4_FREED_STATEID_TYPE for stateids that have been successfully discarded from the server, and use that type to signal that the delegation can be cleaned up. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/delegation.c | 25 ++++++++++++++++++------- fs/nfs/nfs4_fs.h | 3 +-- fs/nfs/nfs4proc.c | 12 ++++++------ include/linux/nfs4.h | 1 + 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c index 4db912f56230..b746793cf730 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -1006,13 +1006,6 @@ static void nfs_revoke_delegation(struct inode *inode, nfs_inode_find_state_and_recover(inode, stateid); } -void nfs_remove_bad_delegation(struct inode *inode, - const nfs4_stateid *stateid) -{ - nfs_revoke_delegation(inode, stateid); -} -EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation); - void nfs_delegation_mark_returned(struct inode *inode, const nfs4_stateid *stateid) { @@ -1054,6 +1047,24 @@ void nfs_delegation_mark_returned(struct inode *inode, nfs_inode_find_state_and_recover(inode, stateid); } +/** + * nfs_remove_bad_delegation - handle delegations that are unusable + * @inode: inode to process + * @stateid: the delegation's stateid + * + * If the server ACK-ed our FREE_STATEID then clean + * up the delegation, else mark and keep the revoked state. + */ +void nfs_remove_bad_delegation(struct inode *inode, + const nfs4_stateid *stateid) +{ + if (stateid && stateid->type == NFS4_FREED_STATEID_TYPE) + nfs_delegation_mark_returned(inode, stateid); + else + nfs_revoke_delegation(inode, stateid); +} +EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation); + /** * nfs_expire_unused_delegation_types * @clp: client to process diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index 7d383d29a995..d3ca91f60fc1 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -67,8 +67,7 @@ struct nfs4_minor_version_ops { void (*free_lock_state)(struct nfs_server *, struct nfs4_lock_state *); int (*test_and_free_expired)(struct nfs_server *, - const nfs4_stateid *, - const struct cred *); + nfs4_stateid *, const struct cred *); struct nfs_seqid * (*alloc_seqid)(struct nfs_seqid_counter *, gfp_t); void (*session_trunk)(struct rpc_clnt *clnt, diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 6e95db6c17e9..c969e6b0dd84 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -105,7 +105,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, bool is_privileged); static int nfs41_test_stateid(struct nfs_server *, const nfs4_stateid *, const struct cred *); -static int nfs41_free_stateid(struct nfs_server *, const nfs4_stateid *, +static int nfs41_free_stateid(struct nfs_server *, nfs4_stateid *, const struct cred *, bool); #endif @@ -2886,16 +2886,14 @@ static int nfs40_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *st } static int nfs40_test_and_free_expired_stateid(struct nfs_server *server, - const nfs4_stateid *stateid, - const struct cred *cred) + nfs4_stateid *stateid, const struct cred *cred) { return -NFS4ERR_BAD_STATEID; } #if defined(CONFIG_NFS_V4_1) static int nfs41_test_and_free_expired_stateid(struct nfs_server *server, - const nfs4_stateid *stateid, - const struct cred *cred) + nfs4_stateid *stateid, const struct cred *cred) { int status; @@ -2904,6 +2902,7 @@ static int nfs41_test_and_free_expired_stateid(struct nfs_server *server, break; case NFS4_INVALID_STATEID_TYPE: case NFS4_SPECIAL_STATEID_TYPE: + case NFS4_FREED_STATEID_TYPE: return -NFS4ERR_BAD_STATEID; case NFS4_REVOKED_STATEID_TYPE: goto out_free; @@ -10570,7 +10569,7 @@ static const struct rpc_call_ops nfs41_free_stateid_ops = { * Note: this function is always asynchronous. */ static int nfs41_free_stateid(struct nfs_server *server, - const nfs4_stateid *stateid, + nfs4_stateid *stateid, const struct cred *cred, bool privileged) { @@ -10610,6 +10609,7 @@ static int nfs41_free_stateid(struct nfs_server *server, if (IS_ERR(task)) return PTR_ERR(task); rpc_put_task(task); + stateid->type = NFS4_FREED_STATEID_TYPE; return 0; } diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h index 9ac83ca88326..8ec5766cb22f 100644 --- a/include/linux/nfs4.h +++ b/include/linux/nfs4.h @@ -72,6 +72,7 @@ struct nfs4_stateid_struct { NFS4_LAYOUT_STATEID_TYPE, NFS4_PNFS_DS_STATEID_TYPE, NFS4_REVOKED_STATEID_TYPE, + NFS4_FREED_STATEID_TYPE, } type; }; -- 2.47.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] NFSv4: Allow FREE_STATEID to clean up delegations 2025-05-01 12:29 ` [PATCH v2 1/1] NFSv4: Allow FREE_STATEID to clean up delegations Benjamin Coddington @ 2025-05-01 15:03 ` Jeff Layton 0 siblings, 0 replies; 3+ messages in thread From: Jeff Layton @ 2025-05-01 15:03 UTC (permalink / raw) To: Benjamin Coddington, Trond Myklebust, Anna Schumaker; +Cc: linux-nfs On Thu, 2025-05-01 at 08:29 -0400, Benjamin Coddington wrote: > The NFS client's list of delegations can grow quite large (well beyond the > delegation watermark) if the server is revoking or there are repeated > events that expire state. Once this happens, the revoked delegations can > cause a performance problem for subsequent walks of the > servers->delegations list when the client tries to test and free state. > > If we can determine that the FREE_STATEID operation has completed without > error, we can prune the delegation from the list. > > Since the NFS client combines TEST_STATEID with FREE_STATEID in its minor > version operations, there isn't an easy way to communicate success of > FREE_STATEID. Rather than re-arrange quite a number of calling paths to > break out the separate procedures, let's signal the success of FREE_STATEID > by setting the stateid's type. > > Set NFS4_FREED_STATEID_TYPE for stateids that have been successfully > discarded from the server, and use that type to signal that the delegation > can be cleaned up. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/delegation.c | 25 ++++++++++++++++++------- > fs/nfs/nfs4_fs.h | 3 +-- > fs/nfs/nfs4proc.c | 12 ++++++------ > include/linux/nfs4.h | 1 + > 4 files changed, 26 insertions(+), 15 deletions(-) > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c > index 4db912f56230..b746793cf730 100644 > --- a/fs/nfs/delegation.c > +++ b/fs/nfs/delegation.c > @@ -1006,13 +1006,6 @@ static void nfs_revoke_delegation(struct inode *inode, > nfs_inode_find_state_and_recover(inode, stateid); > } > > -void nfs_remove_bad_delegation(struct inode *inode, > - const nfs4_stateid *stateid) > -{ > - nfs_revoke_delegation(inode, stateid); > -} > -EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation); > - > void nfs_delegation_mark_returned(struct inode *inode, > const nfs4_stateid *stateid) > { > @@ -1054,6 +1047,24 @@ void nfs_delegation_mark_returned(struct inode *inode, > nfs_inode_find_state_and_recover(inode, stateid); > } > > +/** > + * nfs_remove_bad_delegation - handle delegations that are unusable > + * @inode: inode to process > + * @stateid: the delegation's stateid > + * > + * If the server ACK-ed our FREE_STATEID then clean > + * up the delegation, else mark and keep the revoked state. > + */ > +void nfs_remove_bad_delegation(struct inode *inode, > + const nfs4_stateid *stateid) > +{ > + if (stateid && stateid->type == NFS4_FREED_STATEID_TYPE) > + nfs_delegation_mark_returned(inode, stateid); > + else > + nfs_revoke_delegation(inode, stateid); > +} > +EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation); > + > /** > * nfs_expire_unused_delegation_types > * @clp: client to process > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h > index 7d383d29a995..d3ca91f60fc1 100644 > --- a/fs/nfs/nfs4_fs.h > +++ b/fs/nfs/nfs4_fs.h > @@ -67,8 +67,7 @@ struct nfs4_minor_version_ops { > void (*free_lock_state)(struct nfs_server *, > struct nfs4_lock_state *); > int (*test_and_free_expired)(struct nfs_server *, > - const nfs4_stateid *, > - const struct cred *); > + nfs4_stateid *, const struct cred *); > struct nfs_seqid * > (*alloc_seqid)(struct nfs_seqid_counter *, gfp_t); > void (*session_trunk)(struct rpc_clnt *clnt, > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 6e95db6c17e9..c969e6b0dd84 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -105,7 +105,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, > bool is_privileged); > static int nfs41_test_stateid(struct nfs_server *, const nfs4_stateid *, > const struct cred *); > -static int nfs41_free_stateid(struct nfs_server *, const nfs4_stateid *, > +static int nfs41_free_stateid(struct nfs_server *, nfs4_stateid *, > const struct cred *, bool); > #endif > > @@ -2886,16 +2886,14 @@ static int nfs40_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *st > } > > static int nfs40_test_and_free_expired_stateid(struct nfs_server *server, > - const nfs4_stateid *stateid, > - const struct cred *cred) > + nfs4_stateid *stateid, const struct cred *cred) > { > return -NFS4ERR_BAD_STATEID; > } > > #if defined(CONFIG_NFS_V4_1) > static int nfs41_test_and_free_expired_stateid(struct nfs_server *server, > - const nfs4_stateid *stateid, > - const struct cred *cred) > + nfs4_stateid *stateid, const struct cred *cred) > { > int status; > > @@ -2904,6 +2902,7 @@ static int nfs41_test_and_free_expired_stateid(struct nfs_server *server, > break; > case NFS4_INVALID_STATEID_TYPE: > case NFS4_SPECIAL_STATEID_TYPE: > + case NFS4_FREED_STATEID_TYPE: > return -NFS4ERR_BAD_STATEID; > case NFS4_REVOKED_STATEID_TYPE: > goto out_free; > @@ -10570,7 +10569,7 @@ static const struct rpc_call_ops nfs41_free_stateid_ops = { > * Note: this function is always asynchronous. > */ > static int nfs41_free_stateid(struct nfs_server *server, > - const nfs4_stateid *stateid, > + nfs4_stateid *stateid, > const struct cred *cred, > bool privileged) > { > @@ -10610,6 +10609,7 @@ static int nfs41_free_stateid(struct nfs_server *server, > if (IS_ERR(task)) > return PTR_ERR(task); > rpc_put_task(task); > + stateid->type = NFS4_FREED_STATEID_TYPE; > return 0; > } > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > index 9ac83ca88326..8ec5766cb22f 100644 > --- a/include/linux/nfs4.h > +++ b/include/linux/nfs4.h > @@ -72,6 +72,7 @@ struct nfs4_stateid_struct { > NFS4_LAYOUT_STATEID_TYPE, > NFS4_PNFS_DS_STATEID_TYPE, > NFS4_REVOKED_STATEID_TYPE, > + NFS4_FREED_STATEID_TYPE, > } type; > }; > Seems like a good idea to me! Reviewed-by: Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-01 15:03 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-01 12:29 [PATCH v2 0/1] Allow FREE_STATEID to free delegations Benjamin Coddington 2025-05-01 12:29 ` [PATCH v2 1/1] NFSv4: Allow FREE_STATEID to clean up delegations Benjamin Coddington 2025-05-01 15:03 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox