summaryrefslogtreecommitdiff
path: root/source3
diff options
context:
space:
mode:
authorMichael Adam <obnox@samba.org>2010-01-23 01:17:06 +0100
committerMichael Adam <obnox@samba.org>2010-02-12 23:12:12 +0100
commit83fffbeb44441a87569e543054af21d975eb20ae (patch)
tree6aae54a15631d447db506578e71b14907cb3b7c5 /source3
parente4af0bc5af2c3ee025ca7fac251c3672ba2c8dd5 (diff)
downloadsamba-83fffbeb44441a87569e543054af21d975eb20ae.tar.gz
samba-83fffbeb44441a87569e543054af21d975eb20ae.tar.bz2
samba-83fffbeb44441a87569e543054af21d975eb20ae.zip
s3:g_lock: remove a nested event loop, replacing the inner loop by select
This made smbd crash in g_lock_lock() when trying to start a transaction on a db with an already started transaction, e.g. in a tcon_and_X where the share_info.tdb was not yet initialized but share_info.tdb was already locked by another process or writing acces to the winreg rpc pipe where the registry tdb was already locked by another process. What we really _want_ to do here by design is to react to MSG_DBWRAP_G_LOCK_RETRY messages that are either sent by a client doing g_lock_unlock or by ourselves when we receive a CTDB_SRVID_SAMBA_NOTIFY or CTDB_SRVID_RECONFIGURE message from ctdbd, i.e. when either a client holding a lock or a complete node has died. Doing this properly involves calling tevent_loop_once(), but doing this here with the main ctdbd messaging context creates a nested event loop when g_lock_lock() is called from the main event loop. So as a quick fix, we act a little corasely here: we do a select on the ctdb connection fd and when it is readable or we get EINTR, then we retry without actually parsing any ctdb packages or dispatching messages. This means that we retry more often than necessary and intended by design, but this does not harm and it is unobtrusive. When we have finished, the main loop will pick up all the messages and ctdb packets. The only extra twist is that we cannot use timed events here but have to handcode a timeout for select. Michael
Diffstat (limited to 'source3')
-rw-r--r--source3/lib/g_lock.c139
1 files changed, 101 insertions, 38 deletions
diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
index 0eae3f2131..8a44b22e11 100644
--- a/source3/lib/g_lock.c
+++ b/source3/lib/g_lock.c
@@ -266,7 +266,9 @@ NTSTATUS g_lock_lock(struct g_lock_ctx *ctx, const char *name,
struct tevent_timer *te = NULL;
NTSTATUS status;
bool retry = false;
- bool timedout = false;
+ struct timeval timeout_end;
+ struct timeval timeout_remaining;
+ struct timeval time_now;
DEBUG(10, ("Trying to acquire lock %d for %s\n", (int)lock_type,
name));
@@ -295,52 +297,113 @@ NTSTATUS g_lock_lock(struct g_lock_ctx *ctx, const char *name,
nt_errstr(status)));
return status;
}
-again:
- retry = false;
- status = g_lock_trylock(ctx, name, lock_type);
- if (NT_STATUS_IS_OK(status)) {
- DEBUG(10, ("Got lock %s\n", name));
- goto done;
- }
- if (!NT_STATUS_EQUAL(status, STATUS_PENDING)) {
- DEBUG(10, ("g_lock_trylock failed: %s\n",
- nt_errstr(status)));
- goto done;
- }
-
- DEBUG(10, ("g_lock_trylock: Did not get lock, waiting...\n"));
-
- if (te == NULL) {
- te = tevent_add_timer(
- ctx->msg->event_ctx, talloc_tos(),
- timeval_current_ofs(timeout.tv_sec, timeout.tv_usec),
- g_lock_timedout, &timedout);
- if (te == NULL) {
- DEBUG(10, ("tevent_add_timer failed\n"));
- status = NT_STATUS_NO_MEMORY;
- goto done;
- }
- }
+ time_now = timeval_current();
+ timeout_end = timeval_sum(&time_now, &timeout);
while (true) {
- if (tevent_loop_once(ctx->msg->event_ctx) == -1) {
- DEBUG(1, ("tevent_loop_once failed\n"));
- status = NT_STATUS_INTERNAL_ERROR;
- goto done;
+ fd_set _r_fds;
+ fd_set *r_fds = NULL;
+ int max_fd = 0;
+ int ret;
+
+ status = g_lock_trylock(ctx, name, lock_type);
+ if (NT_STATUS_IS_OK(status)) {
+ DEBUG(10, ("Got lock %s\n", name));
+ break;
+ }
+ if (!NT_STATUS_EQUAL(status, STATUS_PENDING)) {
+ DEBUG(10, ("g_lock_trylock failed: %s\n",
+ nt_errstr(status)));
+ break;
}
- if (retry) {
- goto again;
+
+ DEBUG(10, ("g_lock_trylock: Did not get lock, waiting...\n"));
+
+ /* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * !!! HACK ALERT --- FIX ME !!!
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * What we really want to do here is to react to
+ * MSG_DBWRAP_G_LOCK_RETRY messages that are either sent
+ * by a client doing g_lock_unlock or by ourselves when
+ * we receive a CTDB_SRVID_SAMBA_NOTIFY or
+ * CTDB_SRVID_RECONFIGURE message from ctdbd, i.e. when
+ * either a client holding a lock or a complete node
+ * has died.
+ *
+ * Doing this properly involves calling tevent_loop_once(),
+ * but doing this here with the main ctdbd messaging context
+ * creates a nested event loop when g_lock_lock() is called
+ * from the main event loop, e.g. in a tcon_and_X where the
+ * share_info.tdb needs to be initialized and is locked by
+ * another process, or when the remore registry is accessed
+ * for writing and some other process already holds a lock
+ * on the registry.tdb.
+ *
+ * So as a quick fix, we act a little corasely here: we do
+ * a select on the ctdb connection fd and when it is readable
+ * or we get EINTR, then we retry without actually parsing
+ * any ctdb packages or dispatching messages. This means that
+ * we retry more often than intended by design, but this does
+ * not harm and it is unobtrusive. When we have finished,
+ * the main loop will pick up all the messages and ctdb
+ * packets. The only extra twist is that we cannot use timed
+ * events here but have to handcode a timeout.
+ */
+
+#ifdef CLUSTER_SUPPORT
+ if (lp_clustering()) {
+ struct ctdbd_connection *conn = messaging_ctdbd_connection();
+
+ r_fds = &_r_fds;
+ FD_ZERO(r_fds);
+ max_fd = ctdbd_conn_get_fd(conn);
+ FD_SET(max_fd, r_fds);
}
- if (timedout) {
- DEBUG(10, ("g_lock_lock timed out\n"));
+#endif
- te = NULL;
+ time_now = timeval_current();
+ timeout_remaining = timeval_until(&time_now, &timeout_end);
- status = NT_STATUS_LOCK_NOT_GRANTED;
- goto done;
+ ret = sys_select(max_fd + 1, r_fds, NULL, NULL,
+ &timeout_remaining);
+
+ if (ret == -1) {
+ if (errno != EINTR) {
+ DEBUG(1, ("error calling select: %s\n",
+ strerror(errno)));
+ status = NT_STATUS_INTERNAL_ERROR;
+ break;
+ }
+ /*
+ * errno == EINTR:
+ * This means a signal was received.
+ * It might have been a MSG_DBWRAP_G_LOCK_RETRY message.
+ * ==> retry
+ */
+ } else if (ret == 0) {
+ if (timeval_expired(&timeout_end)) {
+ DEBUG(10, ("g_lock_lock timed out\n"));
+ status = NT_STATUS_LOCK_NOT_GRANTED;
+ break;
+ } else {
+ DEBUG(10, ("select returned 0 but timeout not "
+ "not expired: strange - retrying\n"));
+ }
+ } else if (ret != 1) {
+ DEBUG(1, ("invalid return code of select: %d\n", ret));
+ status = NT_STATUS_INTERNAL_ERROR;
+ break;
}
+ /*
+ * ret == 1:
+ * This means ctdbd has sent us some data.
+ * Might be a CTDB_SRVID_RECONFIGURE or a
+ * CTDB_SRVID_SAMBA_NOTIFY message.
+ * ==> retry
+ */
}
+
done:
if (!NT_STATUS_IS_OK(status)) {