Date: Wed, 28 Jan 2009 19:45:00 +0100 From: Attilio Rao <attilio@freebsd.org> To: FreeBSD Current <freebsd-current@freebsd.org>, freebsd-fs@freebsd.org Cc: bp@freebsd.org, Robert Watson <rwatson@freebsd.org> Subject: [PATCH] improving netncp locking Message-ID: <3bbf2fe10901281045s7e3280dep5a26df6541595cfe@mail.gmail.com>
index | next in thread | raw e-mail
[-- Attachment #1 --]
Attached there is a patch that fixes netncp locking.
Actually, netncp tries to drain connections without any protection for
the nc_id field (which is supposed to discriminate between a valid
connection and one under draining).
Also, the drain still uses loose ending LK_DRAIN.
This patch adds correct locking for draining path adding an interlock,
control flags, a refcount and using it accordingly.
Ultimately 2 locks are switched to be sx as they don't rely on any
particular lockmgr feature.
The patch compiles and boots ok, but if someone could test and review
it I would appreciate a lot.
Thanks,
Attilio
--
Peace can only be achieved by understanding - A. Einstein
[-- Attachment #2 --]
--- /usr/src/sys/netncp/ncp_conn.c 2009-01-10 13:39:48.000000000 +0100
+++ sys/netncp/ncp_conn.c 2009-01-28 19:20:02.000000000 +0100
@@ -38,11 +38,13 @@
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/kernel.h>
+#include <sys/lock.h>
#include <sys/malloc.h>
+#include <sys/mutex.h>
#include <sys/priv.h>
#include <sys/proc.h>
-#include <sys/lock.h>
#include <sys/sysctl.h>
+#include <sys/sx.h>
#include <netncp/ncp.h>
#include <netncp/nwerror.h>
@@ -51,6 +53,17 @@
#include <netncp/ncp_sock.h>
#include <netncp/ncp_ncp.h>
+#define NCP_REFLCNT(ncp) ((ncp)->nc_lockcnt++)
+#define NCP_RELLCNT(ncp) do { \
+ (ncp)->nc_lockcnt--; \
+ if ((ncp)->nc_lockcnt == 0 && \
+ ((ncp)->flags & NCPFL_DRAINING) != 0) { \
+ MPASS((ncp)->nc_id == 0); \
+ (ncp)->flags &= ~NCPFL_DRAINING; \
+ wakeup(&(ncp)->nc_lockcnt); \
+ } \
+} while(0)
+
SLIST_HEAD(ncp_handle_head,ncp_handle);
int ncp_burst_enabled = 1;
@@ -58,15 +71,16 @@
struct ncp_conn_head conn_list={NULL};
static int ncp_conn_cnt = 0;
static int ncp_next_ref = 1;
-static struct lock listlock;
+static struct sx listlock;
struct ncp_handle_head lhlist={NULL};
static int ncp_next_handle = 1;
-static struct lock lhlock;
+static struct sx lhlock;
static int ncp_sysctl_connstat(SYSCTL_HANDLER_ARGS);
static int ncp_conn_lock_any(struct ncp_conn *conn, struct thread *td,
struct ucred *cred);
+static void ncp_conn_lock_assert(struct ncp_conn *conn, int what);
SYSCTL_DECL(_net_ncp);
SYSCTL_INT (_net_ncp, OID_AUTO, burst_enabled, CTLFLAG_RD, &ncp_burst_enabled, 0, "");
@@ -79,8 +93,8 @@
int
ncp_conn_init(void)
{
- lockinit(&listlock, PSOCK, "ncpll", 0, 0);
- lockinit(&lhlock, PSOCK, "ncplh", 0, 0);
+ sx_init_flags(&listlock, "ncpll", SX_RECURSE);
+ sx_init(&lhlock, "ncplh");
return 0;
}
@@ -91,21 +105,51 @@
NCPERROR("There are %d connections active\n", ncp_conn_cnt);
return EBUSY;
}
- lockdestroy(&listlock);
- lockdestroy(&lhlock);
+ sx_destroy(&listlock);
+ sx_destroy(&lhlock);
return 0;
}
+void
+ncp_conn_slocklist()
+{
+
+ sx_slock(&listlock);
+}
+
+void
+ncp_conn_xlocklist()
+{
+
+ sx_xlock(&listlock);
+}
+
int
-ncp_conn_locklist(int flags, struct thread *td)
+ncp_conn_try_slocklist()
{
- return lockmgr(&listlock, flags | LK_CANRECURSE, 0);
+
+ return (sx_try_slock(&listlock));
+}
+
+int
+ncp_conn_try_xlocklist()
+{
+
+ return (sx_try_xlock(&listlock));
}
void
-ncp_conn_unlocklist(struct thread *td)
+ncp_conn_sunlocklist()
{
- lockmgr(&listlock, LK_RELEASE, 0);
+
+ sx_sunlock(&listlock);
+}
+
+void
+ncp_conn_xunlocklist()
+{
+
+ sx_xunlock(&listlock);
}
int
@@ -128,17 +172,31 @@
{
int error;
- if (conn->nc_id == 0) return EACCES;
- error = lockmgr(&conn->nc_lock, LK_EXCLUSIVE | LK_CANRECURSE, 0);
- if (error == ERESTART)
+ NCP_ILOCK(conn);
+ if (conn->nc_id == 0)
+ return EACCES;
+ NCP_REFLCNT(conn);
+ error = lockmgr(&conn->nc_lock, LK_EXCLUSIVE | LK_CANRECURSE |
+ LK_INTERLOCK, NCP_MTX(conn));
+ if (error == ERESTART) {
+ NCP_ILOCK(conn);
+ NCP_RELLCNT(conn);
+ NCP_IUNLOCK(conn);
return EINTR;
+ }
error = ncp_chkintr(conn, td);
if (error) {
+ NCP_ILOCK(conn);
+ NCP_RELLCNT(conn);
+ NCP_IUNLOCK(conn);
lockmgr(&conn->nc_lock, LK_RELEASE, 0);
return error;
}
if (conn->nc_id == 0) {
+ NCP_ILOCK(conn);
+ NCP_RELLCNT(conn);
+ NCP_IUNLOCK(conn);
lockmgr(&conn->nc_lock, LK_RELEASE, 0);
return EACCES;
}
@@ -147,6 +205,13 @@
return 0;
}
+static void
+ncp_conn_lock_assert(struct ncp_conn *conn, int what)
+{
+
+ lockmgr_assert(&conn->nc_lock, what);
+}
+
int
ncp_conn_lock(struct ncp_conn *conn, struct thread *td, struct ucred *cred, int mode)
{
@@ -167,11 +232,11 @@
error = ncp_conn_access(conn, cred, mode);
if (error) {
- ncp_conn_unlocklist(td);
+ ncp_conn_sunlocklist();
return error;
}
conn->nc_lwant++;
- ncp_conn_unlocklist(td);
+ ncp_conn_sunlocklist();
error = ncp_conn_lock_any(conn, td, cred);
conn->nc_lwant--;
if (conn->nc_lwant == 0) {
@@ -183,19 +248,11 @@
void
ncp_conn_unlock(struct ncp_conn *conn, struct thread *td)
{
- /*
- * note, that LK_RELASE will do wakeup() instead of wakeup_one().
- * this will do a little overhead
- */
- lockmgr(&conn->nc_lock, LK_RELEASE, 0);
-}
-int
-ncp_conn_assert_locked(struct ncp_conn *conn, const char *checker, struct thread *td)
-{
- if (lockstatus(&conn->nc_lock) == LK_EXCLUSIVE) return 0;
- printf("%s: connection isn't locked!\n", checker);
- return EIO;
+ NCP_ILOCK(conn);
+ NCP_RELLCNT(conn);
+ NCP_IUNLOCK(conn);
+ lockmgr(&conn->nc_lock, LK_RELEASE, 0);
}
void
@@ -242,7 +299,9 @@
M_NCPDATA, M_WAITOK | M_ZERO);
error = 0;
lockinit(&ncp->nc_lock, PZERO, "ncplck", 0, 0);
+ mtx_init(&ncp->nc_ilock, "ncpilck", NULL, 0);
ncp_conn_cnt++;
+ ncp->nc_lockcnt = 0;
ncp->nc_id = ncp_next_ref++;
ncp->nc_owner = owner;
ncp->seq = 0;
@@ -257,9 +316,9 @@
ncp->li.timeout = NCP_RETRY_TIMEOUT;
ncp_conn_lock_any(ncp, td, ncp->nc_owner);
*conn = ncp;
- ncp_conn_locklist(LK_EXCLUSIVE, td);
+ ncp_conn_xlocklist();
SLIST_INSERT_HEAD(&conn_list,ncp,nc_next);
- ncp_conn_unlocklist(td);
+ ncp_conn_xunlocklist();
return (error);
}
@@ -270,20 +329,12 @@
ncp_conn_free(struct ncp_conn *ncp)
{
struct thread *td;
- int error;
- if (ncp == NULL) {
- NCPFATAL("ncp == NULL\n");
- return 0;
- }
- if (ncp->nc_id == 0) {
- NCPERROR("nc_id == 0\n");
- return EACCES;
- }
+ MPASS(ncp != NULL);
+ MPASS(ncp->nc_id != 0);
+ ncp_conn_lock_assert(ncp, KA_XLOCKED);
+
td = ncp->td;
- error = ncp_conn_assert_locked(ncp, __func__, td);
- if (error)
- return error;
if (ncp->ref_cnt != 0 || (ncp->flags & NCPFL_PERMANENT))
return EBUSY;
if (ncp_conn_access(ncp, ncp->ucred, NCPM_WRITE))
@@ -292,26 +343,28 @@
if (ncp->flags & NCPFL_ATTACHED)
ncp_ncp_disconnect(ncp);
ncp_sock_disconnect(ncp);
+ ncp_conn_unlock(ncp, td);
/*
* Mark conn as dead and wait for other process
*/
+ NCP_ILOCK(ncp);
ncp->nc_id = 0;
- ncp_conn_unlock(ncp, td);
- /*
- * if signal is raised - how I do react ?
- */
- lockmgr(&ncp->nc_lock, LK_DRAIN, 0);
- lockmgr(&ncp->nc_lock, LK_RELEASE, 0);
+ if (ncp->nc_lockcnt != 0) {
+ ncp->flags |= NCPFL_DRAINING;
+ msleep(&ncp->nc_lockcnt, NCP_MTX(ncp), PSOCK,
+ "ncp connection drain", 0);
+ }
+ NCP_IUNLOCK(ncp);
lockdestroy(&ncp->nc_lock);
while (ncp->nc_lwant) {
printf("lwant = %d\n", ncp->nc_lwant);
tsleep(&ncp->nc_lwant, PZERO,"ncpdr",2*hz);
}
- ncp_conn_locklist(LK_EXCLUSIVE, td);
+ ncp_conn_xlocklist();
SLIST_REMOVE(&conn_list, ncp, ncp_conn, nc_next);
ncp_conn_cnt--;
- ncp_conn_unlocklist(td);
+ ncp_conn_xunlocklist();
if (ncp->li.user)
free(ncp->li.user, M_NCPDATA);
if (ncp->li.password)
@@ -390,11 +443,11 @@
struct ncp_conn *ncp;
int error = 0;
- ncp_conn_locklist(LK_SHARED, td);
+ ncp_conn_slocklist();
SLIST_FOREACH(ncp, &conn_list, nc_next)
if (ncp->nc_id == ref) break;
if (ncp == NULL) {
- ncp_conn_unlocklist(td);
+ ncp_conn_sunlocklist();
return(EBADF);
}
error = ncp_conn_lock2(ncp, td, cred, mode);
@@ -412,7 +465,7 @@
struct ncp_conn *ncp, *ncp2 = NULL;
int error = 0;
- ncp_conn_locklist(LK_SHARED, td);
+ ncp_conn_slocklist();
SLIST_FOREACH(ncp, &conn_list, nc_next) {
if ((ncp->flags & NCPFL_LOGGED) != 0 ||
strcmp(ncp->li.server,li->server) != 0 ||
@@ -428,7 +481,7 @@
}
if (ncp == NULL) ncp = ncp2;
if (ncp == NULL) {
- ncp_conn_unlocklist(td);
+ ncp_conn_sunlocklist();
return(EBADF);
}
error = ncp_conn_lock2(ncp, td, cred, mode);
@@ -456,7 +509,7 @@
partial = (li == NULL || li->server[0] == 0 || li->user == NULL);
haveserv = (li && li->server[0]);
- ncp_conn_locklist(LK_SHARED, td);
+ ncp_conn_slocklist();
SLIST_FOREACH(ncp, &conn_list, nc_next) {
if (partial) {
if (cred->cr_uid == ncp->nc_owner->cr_uid) {
@@ -486,7 +539,7 @@
}
if (ncp == NULL) ncp = ncp2;
if (ncp == NULL) {
- ncp_conn_unlocklist(td);
+ ncp_conn_sunlocklist();
return(EBADF);
}
error = ncp_conn_lock2(ncp, td, cred,mode);
@@ -507,12 +560,12 @@
if (conn->ucred->cr_uid != conn->nc_owner->cr_uid)
return EACCES;
- ncp_conn_locklist(LK_SHARED, conn->td);
+ ncp_conn_slocklist();
SLIST_FOREACH(ncp, &conn_list, nc_next) {
if (conn->ucred->cr_uid == ncp->nc_owner->cr_uid)
ncp->flags &= ~NCPFL_PRIMARY;
}
- ncp_conn_unlocklist(conn->td);
+ ncp_conn_sunlocklist();
if (on)
conn->flags |= NCPFL_PRIMARY;
return 0;
@@ -526,14 +579,14 @@
{
struct ncp_handle *refp;
- lockmgr(&lhlock, LK_EXCLUSIVE, 0);
+ sx_xlock(&lhlock);
SLIST_FOREACH(refp, &lhlist, nh_next)
if (refp->nh_conn == conn && td == refp->nh_td) break;
if (refp) {
conn->ref_cnt++;
refp->nh_ref++;
*handle = refp;
- lockmgr(&lhlock, LK_RELEASE, 0);
+ sx_xunlock(&lhlock);
return 0;
}
refp = malloc(sizeof(struct ncp_handle),M_NCPDATA,
@@ -545,7 +598,7 @@
refp->nh_id = ncp_next_handle++;
*handle = refp;
conn->ref_cnt++;
- lockmgr(&lhlock, LK_RELEASE, 0);
+ sx_xunlock(&lhlock);
return 0;
}
/*
@@ -556,7 +609,7 @@
{
struct ncp_handle *refp = handle;
- lockmgr(&lhlock, LK_EXCLUSIVE, 0);
+ sx_xlock(&lhlock);
refp->nh_ref--;
refp->nh_conn->ref_cnt--;
if (force) {
@@ -567,7 +620,7 @@
SLIST_REMOVE(&lhlist, refp, ncp_handle, nh_next);
free(refp, M_NCPDATA);
}
- lockmgr(&lhlock, LK_RELEASE, 0);
+ sx_xunlock(&lhlock);
return 0;
}
/*
@@ -577,10 +630,10 @@
ncp_conn_findhandle(int connHandle, struct thread *td, struct ncp_handle **handle) {
struct ncp_handle *refp;
- lockmgr(&lhlock, LK_SHARED, 0);
+ sx_slock(&lhlock);
SLIST_FOREACH(refp, &lhlist, nh_next)
if (refp->nh_td == td && refp->nh_id == connHandle) break;
- lockmgr(&lhlock, LK_RELEASE, 0);
+ sx_sunlock(&lhlock);
if (refp == NULL) {
return EBADF;
}
@@ -596,7 +649,7 @@
struct ncp_handle *hp, *nhp;
int haveone = 0;
- lockmgr(&lhlock, LK_EXCLUSIVE, 0);
+ sx_xlock(&lhlock);
for (hp = SLIST_FIRST(&lhlist); hp; hp = nhp) {
nhp = SLIST_NEXT(hp, nh_next);
if (hp->nh_td != td) continue;
@@ -605,7 +658,7 @@
SLIST_REMOVE(&lhlist, hp, ncp_handle, nh_next);
free(hp, M_NCPDATA);
}
- lockmgr(&lhlock, LK_RELEASE, 0);
+ sx_xunlock(&lhlock);
return haveone;
}
/*
@@ -616,11 +669,11 @@
ncp_conn_list_rm_ref(pid_t pid) {
struct ncp_conn *ncp;
- ncp_conn_locklist(LK_SHARED, NULL);
+ ncp_conn_slocklist();
SLIST_FOREACH(ncp, &conn_list, nc_next) {
ncp_conn_rm_ref(ncp,pid,1);
}
- ncp_conn_unlocklist(NULL);
+ ncp_conn_sunlocklist();
return;
}
*/
@@ -653,7 +706,7 @@
error = sysctl_wire_old_buffer(req, 0);
if (error != 0)
return (error);
- ncp_conn_locklist(LK_SHARED, req->td);
+ ncp_conn_slocklist();
error = SYSCTL_OUT(req, &ncp_conn_cnt, sizeof(ncp_conn_cnt));
SLIST_FOREACH(ncp, &conn_list, nc_next) {
if (error) break;
@@ -663,6 +716,6 @@
ncp->nc_lwant--;
error = SYSCTL_OUT(req, &ncs, sizeof(ncs));
}
- ncp_conn_unlocklist(req->td);
+ ncp_conn_sunlocklist();
return(error);
}
--- /usr/src/sys/netncp/ncp_conn.h 2008-03-12 14:55:40.000000000 +0100
+++ sys/netncp/ncp_conn.h 2009-01-28 19:20:02.000000000 +0100
@@ -63,6 +63,7 @@
#define NCPFL_WASLOGGED 0x0200 /* there was at least one successfull login */
#define NCPFL_SIGNACTIVE 0x1000 /* packet signing active */
#define NCPFL_SIGNWANTED 0x2000 /* signing should start */
+#define NCPFL_DRAINING 0x4000 /* structure under draining */
/* access mode for connection */
#define NCPM_READ 0400 /* able to fetch conn params */
@@ -72,6 +73,9 @@
#define NCP_DEFAULT_OWNER ((uid_t)-1)
#define NCP_DEFAULT_GROUP ((uid_t)-1)
+#define NCP_ILOCK(ncp) mtx_lock(&(ncp)->nc_ilock)
+#define NCP_IUNLOCK(ncp) mtx_unlock(&(ncp)->nc_ilock)
+#define NCP_MTX(ncp) (&(ncp)->nc_ilock)
/* args used to create connection */
#define ncp_conn_loginfo ncp_conn_args
@@ -173,6 +177,8 @@
int ref_cnt; /* how many handles leased */
SLIST_HEAD(ncp_ref_hd,ncp_ref) ref_list;/* list of handles */
struct lock nc_lock; /* excl locks */
+ struct mtx nc_ilock; /* connection interlock */
+ int nc_lockcnt; /* lock refcount */
int nc_lwant; /* number of wanted locks */
struct thread *td; /* pid currently operates */
struct ucred *ucred; /* usr currently operates */
@@ -201,7 +207,6 @@
int ncp_conn_access(struct ncp_conn *conn,struct ucred *cred,mode_t mode);
int ncp_conn_lock(struct ncp_conn *conn,struct thread *td, struct ucred *cred,int mode);
void ncp_conn_unlock(struct ncp_conn *conn,struct thread *td);
-int ncp_conn_assert_locked(struct ncp_conn *conn,const char *checker,struct thread *td);
void ncp_conn_invalidate(struct ncp_conn *ncp);
int ncp_conn_invalid(struct ncp_conn *ncp);
/*int ncp_conn_ref(struct ncp_conn *conn, pid_t pid);
@@ -212,8 +217,12 @@
int ncp_conn_getbyli(struct ncp_conn_loginfo *li,struct thread *td, struct ucred *cred,
int mode, struct ncp_conn **connpp);
int ncp_conn_setprimary(struct ncp_conn *conn, int on);
-int ncp_conn_locklist(int flags, struct thread *td);
-void ncp_conn_unlocklist(struct thread *td);
+void ncp_conn_slocklist(void);
+void ncp_conn_xlocklist(void);
+int ncp_conn_try_slocklist(void);
+int ncp_conn_try_xlocklist(void);
+void ncp_conn_sunlocklist(void);
+void ncp_conn_xunlocklist(void);
int ncp_conn_gethandle(struct ncp_conn *conn, struct thread *td, struct ncp_handle **handle);
int ncp_conn_puthandle(struct ncp_handle *handle, struct thread *td, int force);
int ncp_conn_findhandle(int connHandle, struct thread *td, struct ncp_handle **handle);
--- /usr/src/sys/netncp/ncp_subr.c 2009-01-10 13:39:48.000000000 +0100
+++ sys/netncp/ncp_subr.c 2009-01-28 19:24:08.000000000 +0100
@@ -90,7 +90,7 @@
if (ncp_conn_putprochandles(td) == 0)
continue;
- ncp_conn_locklist(LK_EXCLUSIVE, td);
+ ncp_conn_xlocklist();
for (ncp = SLIST_FIRST(&conn_list); ncp; ncp = nncp) {
nncp = SLIST_NEXT(ncp, nc_next);
if (ncp_conn_lock(ncp, td, td->td_ucred,
@@ -99,7 +99,7 @@
if (ncp_conn_free(ncp) != 0)
ncp_conn_unlock(ncp, td);
}
- ncp_conn_unlocklist(td);
+ ncp_conn_xunlocklist();
}
mtx_unlock(&Giant);
}
@@ -134,10 +134,10 @@
{
struct ncp_conn *conn;
- if(ncp_conn_locklist(LK_SHARED | LK_NOWAIT, NULL) == 0) {
+ if (ncp_conn_try_slocklist()) {
SLIST_FOREACH(conn, &conn_list, nc_next)
ncp_check_conn(conn);
- ncp_conn_unlocklist(NULL);
+ ncp_conn_sunlocklist();
}
ncp_timer_handle = timeout(ncp_timer, NULL, NCP_TIMER_TICK);
}
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3bbf2fe10901281045s7e3280dep5a26df6541595cfe>