Page MenuHomeFreeBSD
Differential D34334 Diff 103209 sys/fs/nfsclient/nfs_clstate.c

Changeset View

Standalone View

View Options

sys/fs/nfsclient/nfs_clstate.c

Show First 20 Lines • Show All 1,642 Lines • ▼ Show 20 Lines
} }
/* /*
* Free up an open owner structure. * Free up an open owner structure.
*/ */
static void static void
nfscl_freeopenowner(struct nfsclowner *owp, int local) nfscl_freeopenowner(struct nfsclowner *owp, int local)
{ {
int owned;
/*
* Make sure the NFSCLSTATE mutex is held, to avoid races with
* calls in nfscl_renewthread() that do not hold a reference
* count on the nfsclclient and just the mutex.
* The mutex will not be held for calls done with the exclusive
* nfsclclient lock held.
*/
owned = mtx_owned(NFSCLSTATEMUTEXPTR);
if (owned == 0)
NFSLOCKCLSTATE();
LIST_REMOVE(owp, nfsow_list); LIST_REMOVE(owp, nfsow_list);
if (owned == 0)
NFSUNLOCKCLSTATE();
free(owp, M_NFSCLOWNER); free(owp, M_NFSCLOWNER);
if (local) if (local)
nfsstatsv1.cllocalopenowners--; nfsstatsv1.cllocalopenowners--;
else else
nfsstatsv1.clopenowners--; nfsstatsv1.clopenowners--;
} }
/* /*
* Free up a byte range lock owner structure. * Free up a byte range lock owner structure.
*/ */
void void
nfscl_freelockowner(struct nfscllockowner *lp, int local) nfscl_freelockowner(struct nfscllockowner *lp, int local)
{ {
struct nfscllock *lop, *nlop; struct nfscllock *lop, *nlop;
int owned;
/*
* Make sure the NFSCLSTATE mutex is held, to avoid races with
* calls in nfscl_renewthread() that do not hold a reference
* count on the nfsclclient and just the mutex.
* The mutex will not be held for calls done with the exclusive
* nfsclclient lock held.
*/
owned = mtx_owned(NFSCLSTATEMUTEXPTR);
if (owned == 0)
NFSLOCKCLSTATE();
LIST_REMOVE(lp, nfsl_list); LIST_REMOVE(lp, nfsl_list);
if (owned == 0)
NFSUNLOCKCLSTATE();
LIST_FOREACH_SAFE(lop, &lp->nfsl_lock, nfslo_list, nlop) { LIST_FOREACH_SAFE(lop, &lp->nfsl_lock, nfslo_list, nlop) {
nfscl_freelock(lop, local); nfscl_freelock(lop, local);
} }
free(lp, M_NFSCLLOCKOWNER); free(lp, M_NFSCLLOCKOWNER);
if (local) if (local)
nfsstatsv1.cllocallockowners--; nfsstatsv1.cllocallockowners--;
else else
nfsstatsv1.cllockowners--; nfsstatsv1.cllockowners--;
▲ Show 20 Lines • Show All 173 Lines • ▼ Show 20 Lines
/* /*
* This function must be called after the process represented by "own" has * This function must be called after the process represented by "own" has
* exited. Must be called with CLSTATE lock held. * exited. Must be called with CLSTATE lock held.
*/ */
static void static void
nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own) nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own)
{ {
struct nfsclowner *owp, *nowp; struct nfsclowner *owp, *nowp;
struct nfscllockowner *lp, *nlp; struct nfscllockowner *lp;
struct nfscldeleg *dp; struct nfscldeleg *dp;
/* First, get rid of local locks on delegations. */ /* First, get rid of local locks on delegations. */
TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) { TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) {
LIST_FOREACH_SAFE(lp, &dp->nfsdl_lock, nfsl_list, nlp) { LIST_FOREACH(lp, &dp->nfsdl_lock, nfsl_list) {
if (!NFSBCMP(lp->nfsl_owner, own, NFSV4CL_LOCKNAMELEN)) { if (!NFSBCMP(lp->nfsl_owner, own, NFSV4CL_LOCKNAMELEN)) {
if ((lp->nfsl_rwlock.nfslock_lock & NFSV4LOCK_WANTED)) if ((lp->nfsl_rwlock.nfslock_lock & NFSV4LOCK_WANTED))
panic("nfscllckw"); panic("nfscllckw");
nfscl_freelockowner(lp, 1); nfscl_freelockowner(lp, 1);
break;
markjUnsubmitted
Not Done
Inline Actions

Why is a restart needed in this case?

markj: Why is a restart needed in this case?
rmacklemAuthorUnsubmitted
Done
Inline Actions

It is not. I just thought it would be weird to
return true when freeing open owners, but not
when freeing lock owners.

I can change the function to only return true
when open owners are free'd, if you think that
is appropriate? (And comment it as such.)

rmacklem: It is not. I just thought it would be weird to return true when freeing open owners, but not…
markjUnsubmitted
Not Done
Inline Actions

I think it's logically fine as-is, but I do wonder if the extra restarts might make nfscl_cleanupkext() a lot more expensive in the face of a large number of delegations. Consider that nfscl_cleanupkext() holds all of the PID hash locks. I don't have much intuition for whether this is likely to be a problem in practice, though.

Hmm, actually, isn't a restart indeed required for the second loop in nfscl_cleanupkext()? That is, nfscl_cleanup_common() can potentially free multiple lock owners for a given delegation.

markj: I think it's logically fine as-is, but I do wonder if the extra restarts might make…
rmacklemAuthorUnsubmitted
Done
Inline Actions

Yes, the second restart loop is for lock owners, so a return of "true" is
needed for the free lock owner case.

There is a certain amount of inefficiency caused by a boolean re

rmacklem: Yes, the second restart loop is for lock owners, so a return of "true" is needed for the free…
} }
} }
} }
owp = LIST_FIRST(&clp->nfsc_owner); owp = LIST_FIRST(&clp->nfsc_owner);
while (owp != NULL) { while (owp != NULL) {
nowp = LIST_NEXT(owp, nfsow_list); nowp = LIST_NEXT(owp, nfsow_list);
markjUnsubmitted
Not Done
Inline Actions

These three lines and the last line of the loop could just become a LIST_FOREACH(), I believe.

markj: These three lines and the last line of the loop could just become a LIST_FOREACH(), I believe.
rmacklemAuthorUnsubmitted
Done
Inline Actions

Yes, there are still places in the NFS code where FOREACH_SAFE is not used.
(This code is so old, some of it was written before FOREACH_SAFE
existed in all the BSDen I was maintaining a port to.)

I have replaced a bunch of them with FOREACH_SAFE and will
probably do this one with a FOREACH soon (as you noted, with
"break" added, it does not need to be FOREACH_SAFE), since I
noticed this while doing the patch.
However, changing it is just code cleanup, which I would rather
do separately and later. (No need to get this into 13.1.)

rmacklem: Yes, there are still places in the NFS code where FOREACH_SAFE is not used. (This code is so…
if (!NFSBCMP(owp->nfsow_owner, own, if (!NFSBCMP(owp->nfsow_owner, own,
NFSV4CL_LOCKNAMELEN)) { NFSV4CL_LOCKNAMELEN)) {
/* /*
* If there are children that haven't closed the * If there are children that haven't closed the
* file descriptors yet, the opens will still be * file descriptors yet, the opens will still be
* here. For that case, let the renew thread clear * here. For that case, let the renew thread clear
* out the OpenOwner later. * out the OpenOwner later.
*/ */
if (LIST_EMPTY(&owp->nfsow_open)) if (LIST_EMPTY(&owp->nfsow_open))
nfscl_freeopenowner(owp, 0); nfscl_freeopenowner(owp, 0);
else else
owp->nfsow_defunct = 1; owp->nfsow_defunct = 1;
break;
} }
owp = nowp; owp = nowp;
} }
} }
/* /*
* Find open/lock owners for processes that have exited. * Find open/lock owners for processes that have exited.
*/ */
static void static void
nfscl_cleanupkext(struct nfsclclient *clp, struct nfscllockownerfhhead *lhp) nfscl_cleanupkext(struct nfsclclient *clp, struct nfscllockownerfhhead *lhp)
{ {
struct nfsclowner *owp, *nowp; struct nfsclowner *owp, *nowp;
struct nfsclopen *op; struct nfsclopen *op;
struct nfscllockowner *lp, *nlp; struct nfscllockowner *lp, *nlp;
struct nfscldeleg *dp; struct nfscldeleg *dp;
uint8_t own[NFSV4CL_LOCKNAMELEN];
/* /*
* All the pidhash locks must be acquired, since they are sx locks * All the pidhash locks must be acquired, since they are sx locks
* and must be acquired before the mutexes. The pid(s) that will * and must be acquired before the mutexes. The pid(s) that will
* be used aren't known yet, so all the locks need to be acquired. * be used aren't known yet, so all the locks need to be acquired.
* Fortunately, this function is only performed once/sec. * Fortunately, this function is only performed once/sec.
*/ */
pidhash_slockall(); pidhash_slockall();
NFSLOCKCLSTATE(); NFSLOCKCLSTATE();
LIST_FOREACH_SAFE(owp, &clp->nfsc_owner, nfsow_list, nowp) { LIST_FOREACH_SAFE(owp, &clp->nfsc_owner, nfsow_list, nowp) {
LIST_FOREACH(op, &owp->nfsow_open, nfso_list) { LIST_FOREACH(op, &owp->nfsow_open, nfso_list) {
LIST_FOREACH_SAFE(lp, &op->nfso_lock, nfsl_list, nlp) { LIST_FOREACH_SAFE(lp, &op->nfso_lock, nfsl_list, nlp) {
if (LIST_EMPTY(&lp->nfsl_lock)) if (LIST_EMPTY(&lp->nfsl_lock))
nfscl_emptylockowner(lp, lhp); nfscl_emptylockowner(lp, lhp);
} }
} }
if (nfscl_procdoesntexist(owp->nfsow_owner)) if (nfscl_procdoesntexist(owp->nfsow_owner)) {
nfscl_cleanup_common(clp, owp->nfsow_owner); memcpy(own, owp->nfsow_owner, NFSV4CL_LOCKNAMELEN);
nfscl_cleanup_common(clp, own);
} }
}
/* /*
* For the single open_owner case, these lock owners need to be * For the single open_owner case, these lock owners need to be
* checked to see if they still exist separately. * checked to see if they still exist separately.
* This is because nfscl_procdoesntexist() never returns true for * This is because nfscl_procdoesntexist() never returns true for
* the single open_owner so that the above doesn't ever call * the single open_owner so that the above doesn't ever call
* nfscl_cleanup_common(). * nfscl_cleanup_common().
*/ */
TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) { TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) {
LIST_FOREACH_SAFE(lp, &dp->nfsdl_lock, nfsl_list, nlp) { LIST_FOREACH_SAFE(lp, &dp->nfsdl_lock, nfsl_list, nlp) {
if (nfscl_procdoesntexist(lp->nfsl_owner)) if (nfscl_procdoesntexist(lp->nfsl_owner)) {
nfscl_cleanup_common(clp, lp->nfsl_owner); memcpy(own, lp->nfsl_owner,
NFSV4CL_LOCKNAMELEN);
nfscl_cleanup_common(clp, own);
}
} }
} }
NFSUNLOCKCLSTATE(); NFSUNLOCKCLSTATE();
pidhash_sunlockall(); pidhash_sunlockall();
} }
/* /*
* Take the empty lock owner and move it to the local lhp list if the * Take the empty lock owner and move it to the local lhp list if the
▲ Show 20 Lines • Show All 4,122 Lines • Show Last 20 Lines