When I originally wrote these patches, I was under the mistaken impression that I didn't need to increment the stateid in the case of an existing stateid (because we weren't returning it). After some discussion upstream, it turns out that the server should ignore the WANT_OPEN_XOR_DELEGATION flag if there is an outstanding open stateid. Given that, there is no need to expand the scope of the st_mutex to cover acquiring the delegation. The server may end up bumping the seqid in a brand new open stateid that it ends up discarding, but that's not a problem. This also seems to lower the "App Overhead" on the fs_mark test that the kernel test robot reported. Reported-by: kernel test robot <oliver.sang@xxxxxxxxx> Closes: https://lore.kernel.org/oe-lkp/202409161645.d44bced5-oliver.sang@xxxxxxxxx Fixes: e816ca3f9ee0 ("nfsd: implement OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION") Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> --- We had a report of a performance regression (in the form of higer "App Overhead") in fs_mark. After some experimentation, I found that the cause seemed to be the change in how the mutex is handled in e816ca3f9ee0. This patch restores the App Overhead back to its previous levels (and may even improve it a bit -- go figure). Chuck, this should probably be squashed into e816ca3f9ee0. --- fs/nfsd/nfs4state.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 9c2b1d251ab31b4e504cf301d1deaa4945bd244f..73c4b983c048c101d16ec146b3f80922bcca3c69 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6120,7 +6120,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf struct nfs4_delegation *dp = NULL; __be32 status; bool new_stp = false; - bool deleg_only = false; /* * Lookup file; if found, lookup stateid and check open request, @@ -6175,6 +6174,9 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf open->op_odstate = NULL; } + nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid); + mutex_unlock(&stp->st_mutex); + if (nfsd4_has_session(&resp->cstate)) { if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) { open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE_EXT; @@ -6194,17 +6196,12 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf * returned. Only respect WANT_OPEN_XOR_DELEGATION when a new * open stateid would have to be created. */ - deleg_only = new_stp && open_xor_delegation(open); -nodeleg: - if (deleg_only) { + if (new_stp && open_xor_delegation(open)) { memcpy(&open->op_stateid, &zero_stateid, sizeof(open->op_stateid)); open->op_rflags |= OPEN4_RESULT_NO_OPEN_STATEID; release_open_stateid(stp); - } else { - nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid); } - mutex_unlock(&stp->st_mutex); - +nodeleg: status = nfs_ok; trace_nfsd_open(&stp->st_stid.sc_stateid); out: --- base-commit: 144cb1225cd863e1bd3ae3d577d86e1531afd932 change-id: 20241010-delstid-db2a12f242f3 Best regards, -- Jeff Layton <jlayton@xxxxxxxxxx>
