Date: Sat, 22 Jun 2024 22:57:48 GMT From: Rick Macklem <rmacklem@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: dfaeeacc2cc2 - main - nfsd: Allow a mutex lock for clientID handling Message-ID: <202406222257.45MMvmdl039945@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by rmacklem: URL: https://cgit.FreeBSD.org/src/commit/?id=dfaeeacc2cc29d0497ecd4cd5b7fd0d5ab61fcd5 commit dfaeeacc2cc29d0497ecd4cd5b7fd0d5ab61fcd5 Author: Rick Macklem <rmacklem@FreeBSD.org> AuthorDate: 2024-06-22 22:56:40 +0000 Commit: Rick Macklem <rmacklem@FreeBSD.org> CommitDate: 2024-06-22 22:56:40 +0000 nfsd: Allow a mutex lock for clientID handling On Feb. 28, a problem was reported on freebsd-stable@ where a nfsd thread processing an ExchangeID operation was blocked for a long time by another nfsd thread performing a copy_file_range. This occurred because the copy_file_range was taking a long time, but also because handling a clientID requires that all other nfsd threads be blocked via an exclusive lock, as required by ExchangeID. This patch allows clientID handling to be done with only a mutex held (instead of an exclusive lock that blocks all other nfsd threads) when vfs.nfsd.enable_locallocks is 0. For the case of vfs.nfsd.enable_locallocks set to 1, the exclusive lock that blocks all nfsd threads is still required. This patch does make changing the value of vfs.nfsd.enable_locallocks somewhat racy. A future commit will ensure any change is done when all nfsd threads are blocked to avoid this racyness. MFC after: 1 month --- sys/fs/nfsserver/nfs_nfsdstate.c | 245 ++++++++++++++++++++++++++------------- 1 file changed, 162 insertions(+), 83 deletions(-) diff --git a/sys/fs/nfsserver/nfs_nfsdstate.c b/sys/fs/nfsserver/nfs_nfsdstate.c index 7a28e51e21fc..6b40e0f64141 100644 --- a/sys/fs/nfsserver/nfs_nfsdstate.c +++ b/sys/fs/nfsserver/nfs_nfsdstate.c @@ -245,6 +245,45 @@ static void nfsrv_issuedelegation(struct vnode *vp, struct nfsclient *clp, u_quad_t filerev, uint64_t rdonly, struct nfsstate **new_delegp, struct nfsstate *new_stp, struct nfslockfile *lfp, uint32_t *rflagsp, nfsv4stateid_t *delegstateidp); +static void nfsrv_clientlock(bool mlocked); +static void nfsrv_clientunlock(bool mlocked); + +/* + * Lock the client structure, either with the mutex or the exclusive nfsd lock. + */ +static void +nfsrv_clientlock(bool mlocked) +{ + int igotlock; + + if (mlocked) { + NFSLOCKSTATE(); + } else { + NFSLOCKV4ROOTMUTEX(); + nfsv4_relref(&nfsv4rootfs_lock); + do { + igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL, + NFSV4ROOTLOCKMUTEXPTR, NULL); + } while (!igotlock); + NFSUNLOCKV4ROOTMUTEX(); + } +} + +/* + * Unlock the client structure. + */ +static void +nfsrv_clientunlock(bool mlocked) +{ + + if (mlocked) { + NFSUNLOCKSTATE(); + } else { + NFSLOCKV4ROOTMUTEX(); + nfsv4_unlock(&nfsv4rootfs_lock, 1); + NFSUNLOCKV4ROOTMUTEX(); + } +} /* * Scan the client list for a match and either return the current one, @@ -266,7 +305,10 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, struct sockaddr_in6 *sin6, *rin6; #endif struct nfsdsession *sep, *nsep; - int zapit = 0, gotit, hasstate = 0, igotlock; + SVCXPRT *old_xprt; + struct nfssessionhead old_sess; + int zapit = 0, gotit, hasstate = 0; + bool mlocked; static u_int64_t confirm_index = 0; /* @@ -294,14 +336,11 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, */ new_clp->lc_program = 0; + mlocked = true; + if (nfsrv_dolocallocks != 0) + mlocked = false; /* Lock out other nfsd threads */ - NFSLOCKV4ROOTMUTEX(); - nfsv4_relref(&nfsv4rootfs_lock); - do { - igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL, - NFSV4ROOTLOCKMUTEXPTR, NULL); - } while (!igotlock); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientlock(mlocked); /* * Search for a match in the client list. @@ -318,6 +357,7 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, if (gotit == 0) i++; } + old_xprt = NULL; if (!gotit || (clp->lc_flags & (LCL_NEEDSCONFIRM | LCL_ADMINREVOKED))) { if ((nd->nd_flag & ND_NFSV41) != 0 && confirmp->lval[1] != 0) { @@ -325,9 +365,7 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, * For NFSv4.1, if confirmp->lval[1] is non-zero, the * client is trying to update a confirmed clientid. */ - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); confirmp->lval[1] = 0; error = NFSERR_NOENT; goto out; @@ -337,7 +375,10 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, */ if (i != nfsrv_clienthashsize) { LIST_REMOVE(clp, lc_hash); - nfsrv_cleanclient(clp, p, false, NULL); + if (mlocked) + nfsrv_cleanclient(clp, p, true, &old_xprt); + else + nfsrv_cleanclient(clp, p, false, NULL); nfsrv_freedeleglist(&clp->lc_deleg); nfsrv_freedeleglist(&clp->lc_olddeleg); zapit = 1; @@ -372,11 +413,12 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, NFSD_VNET(nfsstatsv1_p)->srvclients++; nfsrv_openpluslock++; nfsrv_clients++; - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); - if (zapit) + nfsrv_clientunlock(mlocked); + if (zapit != 0) { + if (old_xprt != NULL) + SVC_RELEASE(old_xprt); nfsrv_zapclient(clp, p); + } *new_clpp = NULL; goto out; } @@ -390,7 +432,10 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, */ if (clp->lc_expiry < NFSD_MONOSEC && (!LIST_EMPTY(&clp->lc_open) || !LIST_EMPTY(&clp->lc_deleg))) { - nfsrv_cleanclient(clp, p, false, NULL); + if (mlocked) + nfsrv_cleanclient(clp, p, true, &old_xprt); + else + nfsrv_cleanclient(clp, p, false, NULL); nfsrv_freedeleglist(&clp->lc_deleg); } @@ -435,9 +480,9 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, break; #endif } - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); + if (old_xprt != NULL) + SVC_RELEASE(old_xprt); error = NFSERR_CLIDINUSE; goto out; } @@ -452,13 +497,7 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, */ LIST_REMOVE(clp, lc_hash); - /* Get rid of all sessions on this clientid. */ - LIST_FOREACH_SAFE(sep, &clp->lc_session, sess_list, nsep) { - ret = nfsrv_freesession(NULL, sep, NULL, false, NULL); - if (ret != 0) - printf("nfsrv_setclient: verifier changed free" - " session failed=%d\n", ret); - } + LIST_NEWHEAD(&old_sess, &clp->lc_session, sess_list); new_clp->lc_flags |= LCL_NEEDSCONFIRM; if ((nd->nd_flag & ND_NFSV41) != 0) { @@ -502,21 +541,31 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, NFSD_VNET(nfsstatsv1_p)->srvclients++; nfsrv_openpluslock++; nfsrv_clients++; - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + if (!mlocked) { + nfsrv_clientunlock(mlocked); + NFSLOCKSTATE(); + } /* * Must wait until any outstanding callback on the old clp * completes. */ - NFSLOCKSTATE(); while (clp->lc_cbref) { clp->lc_flags |= LCL_WAKEUPWANTED; (void)mtx_sleep(clp, NFSSTATEMUTEXPTR, PZERO - 1, "nfsd clp", 10 * hz); } NFSUNLOCKSTATE(); + if (old_xprt != NULL) + SVC_RELEASE(old_xprt); + /* Get rid of all sessions on this clientid. */ + LIST_FOREACH_SAFE(sep, &old_sess, sess_list, nsep) { + ret = nfsrv_freesession(NULL, sep, NULL, false, NULL); + if (ret != 0) + printf("nfsrv_setclient: verifier changed free" + " session failed=%d\n", ret); + } + nfsrv_zapclient(clp, p); *new_clpp = NULL; goto out; @@ -568,24 +617,31 @@ nfsrv_setclient(struct nfsrv_descript *nd, struct nfsclient **new_clpp, nfsrv_openpluslock++; nfsrv_clients++; } - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + if (!mlocked) + nfsrv_clientunlock(mlocked); if ((nd->nd_flag & ND_NFSV41) == 0) { /* * Must wait until any outstanding callback on the old clp * completes. */ - NFSLOCKSTATE(); + if (!mlocked) + NFSLOCKSTATE(); while (clp->lc_cbref) { clp->lc_flags |= LCL_WAKEUPWANTED; (void)mtx_sleep(clp, NFSSTATEMUTEXPTR, PZERO - 1, "nfsdclp", 10 * hz); } NFSUNLOCKSTATE(); + if (old_xprt != NULL) + SVC_RELEASE(old_xprt); nfsrv_zapclient(clp, p); *new_clpp = NULL; + } else { + if (mlocked) + NFSUNLOCKSTATE(); + if (old_xprt != NULL) + SVC_RELEASE(old_xprt); } out: @@ -605,11 +661,13 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, struct nfsstate *stp; int i; struct nfsclienthashhead *hp; - int error = 0, igotlock, doneok; + int error = 0, doneok, igotlock; struct nfssessionhash *shp; struct nfsdsession *sep; uint64_t sessid[2]; - bool sess_replay; + CLIENT *client; + SVCXPRT *old_xprt; + bool mlocked, sess_replay; static uint64_t next_sess = 0; if (clpp) @@ -626,13 +684,27 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, * already held. Otherwise, we need to get either that or, * for the case of Confirm, lock out the nfsd threads. */ + client = NULL; + old_xprt = NULL; + mlocked = true; + if (nfsrv_dolocallocks != 0) + mlocked = false; if (opflags & CLOPS_CONFIRM) { - NFSLOCKV4ROOTMUTEX(); - nfsv4_relref(&nfsv4rootfs_lock); - do { - igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL, - NFSV4ROOTLOCKMUTEXPTR, NULL); - } while (!igotlock); + if (nsep != NULL && + (nsep->sess_crflags & NFSV4CRSESS_CONNBACKCHAN) != 0) + client = (struct __rpc_client *) + clnt_bck_create(nd->nd_xprt->xp_socket, + cbprogram, NFSV4_CBVERS); + if (mlocked) { + nfsrv_clientlock(mlocked); + } else { + NFSLOCKV4ROOTMUTEX(); + nfsv4_relref(&nfsv4rootfs_lock); + do { + igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, + NULL, NFSV4ROOTLOCKMUTEXPTR, NULL); + } while (!igotlock); + } /* * Create a new sessionid here, since we need to do it where * there is a mutex held to serialize update of next_sess. @@ -641,7 +713,8 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, sessid[0] = ++next_sess; sessid[1] = clientid.qval; } - NFSUNLOCKV4ROOTMUTEX(); + if (!mlocked) + NFSUNLOCKV4ROOTMUTEX(); } else if (opflags != CLOPS_RENEW) { NFSLOCKSTATE(); } @@ -678,9 +751,9 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, } if (error) { if (opflags & CLOPS_CONFIRM) { - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); + if (client != NULL) + CLNT_RELEASE(client); } else if (opflags != CLOPS_RENEW) { NFSUNLOCKSTATE(); } @@ -725,7 +798,10 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, * for an Open with CLAIM_DELEGATE_PREV unless in * grace, but get rid of the rest of the state. */ - nfsrv_cleanclient(clp, p, false, NULL); + if (mlocked) + nfsrv_cleanclient(clp, p, true, &old_xprt); + else + nfsrv_cleanclient(clp, p, false, NULL); nfsrv_freedeleglist(&clp->lc_olddeleg); if (nfsrv_checkgrace(nd, clp, 0)) { /* In grace, so just delete delegations */ @@ -749,10 +825,10 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, /* Hold a reference on the xprt for a backchannel. */ if ((nsep->sess_crflags & NFSV4CRSESS_CONNBACKCHAN) != 0 && !sess_replay) { - if (clp->lc_req.nr_client == NULL) - clp->lc_req.nr_client = (struct __rpc_client *) - clnt_bck_create(nd->nd_xprt->xp_socket, - cbprogram, NFSV4_CBVERS); + if (clp->lc_req.nr_client == NULL) { + clp->lc_req.nr_client = client; + client = NULL; + } if (clp->lc_req.nr_client != NULL) { SVC_ACQUIRE(nd->nd_xprt); CLNT_ACQUIRE(clp->lc_req.nr_client); @@ -769,13 +845,15 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, NFSX_V4SESSIONID); if (!sess_replay) { shp = NFSSESSIONHASH(nsep->sess_sessionid); - NFSLOCKSTATE(); + if (!mlocked) + NFSLOCKSTATE(); NFSLOCKSESSION(shp); LIST_INSERT_HEAD(&shp->list, nsep, sess_hash); LIST_INSERT_HEAD(&clp->lc_session, nsep, sess_list); nsep->sess_clp = clp; NFSUNLOCKSESSION(shp); - NFSUNLOCKSTATE(); + if (!mlocked) + NFSUNLOCKSTATE(); } } } @@ -809,9 +887,11 @@ nfsrv_getclient(nfsquad_t clientid, int opflags, struct nfsclient **clpp, clp->lc_expiry = nfsrv_leaseexpiry(); } if (opflags & CLOPS_CONFIRM) { - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); + if (client != NULL) + CLNT_RELEASE(client); + if (old_xprt != NULL) + SVC_RELEASE(old_xprt); } else if (opflags != CLOPS_RENEW) { NFSUNLOCKSTATE(); } @@ -831,21 +911,20 @@ nfsrv_destroyclient(struct nfsrv_descript *nd, nfsquad_t clientid, NFSPROC_T *p) { struct nfsclient *clp; struct nfsclienthashhead *hp; - int error = 0, i, igotlock; + SVCXPRT *old_xprt; + int error = 0, i; + bool mlocked; if (NFSD_VNET(nfsrvboottime) != clientid.lval[0]) { error = NFSERR_STALECLIENTID; goto out; } + mlocked = true; + if (nfsrv_dolocallocks != 0) + mlocked = false; /* Lock out other nfsd threads */ - NFSLOCKV4ROOTMUTEX(); - nfsv4_relref(&nfsv4rootfs_lock); - do { - igotlock = nfsv4_lock(&nfsv4rootfs_lock, 1, NULL, - NFSV4ROOTLOCKMUTEXPTR, NULL); - } while (igotlock == 0); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientlock(mlocked); hp = NFSCLIENTHASH(clientid); LIST_FOREACH(clp, hp, lc_hash) { @@ -853,9 +932,7 @@ nfsrv_destroyclient(struct nfsrv_descript *nd, nfsquad_t clientid, NFSPROC_T *p) break; } if (clp == NULL) { - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); /* Just return ok, since it is gone. */ goto out; } @@ -863,9 +940,7 @@ nfsrv_destroyclient(struct nfsrv_descript *nd, nfsquad_t clientid, NFSPROC_T *p) /* Check for the SP4_MACH_CRED case. */ error = nfsrv_checkmachcred(NFSV4OP_DESTROYCLIENTID, nd, clp); if (error != 0) { - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); goto out; } @@ -878,28 +953,28 @@ nfsrv_destroyclient(struct nfsrv_descript *nd, nfsquad_t clientid, NFSPROC_T *p) /* Scan for state on the clientid. */ for (i = 0; i < nfsrv_statehashsize; i++) if (!LIST_EMPTY(&clp->lc_stateid[i])) { - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); error = NFSERR_CLIENTIDBUSY; goto out; } if (!LIST_EMPTY(&clp->lc_session) || !LIST_EMPTY(&clp->lc_deleg)) { - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); error = NFSERR_CLIENTIDBUSY; goto out; } /* Destroy the clientid and return ok. */ - nfsrv_cleanclient(clp, p, false, NULL); + old_xprt = NULL; + if (mlocked) + nfsrv_cleanclient(clp, p, true, &old_xprt); + else + nfsrv_cleanclient(clp, p, false, NULL); nfsrv_freedeleglist(&clp->lc_deleg); nfsrv_freedeleglist(&clp->lc_olddeleg); LIST_REMOVE(clp, lc_hash); - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); + nfsrv_clientunlock(mlocked); + if (old_xprt != NULL) + SVC_RELEASE(old_xprt); nfsrv_zapclient(clp, p); out: NFSEXITCODE2(error, nd); @@ -1388,8 +1463,12 @@ nfsrv_cleanclient(struct nfsclient *clp, NFSPROC_T *p, bool locked, struct nfsstate *stp, *nstp; struct nfsdsession *sep, *nsep; - LIST_FOREACH_SAFE(stp, &clp->lc_open, ls_list, nstp) - nfsrv_freeopenowner(stp, 1, p); + LIST_FOREACH_SAFE(stp, &clp->lc_open, ls_list, nstp) { + if (locked) + nfsrv_freeopenowner(stp, 0, p); + else + nfsrv_freeopenowner(stp, 1, p); + } if ((clp->lc_flags & LCL_ADMINREVOKED) == 0) LIST_FOREACH_SAFE(sep, &clp->lc_session, sess_list, nsep) (void)nfsrv_freesession(NULL, sep, NULL, locked,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202406222257.45MMvmdl039945>