From 03b5546ae45a60ab41eb4f7159a45bfdbf959888 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 4 Jun 2010 13:33:08 +0930 Subject: [PATCH 1/1] libctdb: change callback for ctdb_readrecordlock. After discussion with Ronnie, we decided to revisit this interface. We use the name ctdb_readrecordlock_async, as it is *not* always a send, and we use a specific callback to avoid the "fake request" creation on the fast path. The request itself is never exposed: this means it can't be cancelled, but we can revisit that later if need be. This makes both use and implementation simpler. Signed-off-by: Rusty Russell --- include/ctdb.h | 28 ++++++++++---------- libctdb/ctdb.c | 69 ++++++++++++++++++-------------------------------- libctdb/tst.c | 16 ++++++------ 3 files changed, 47 insertions(+), 66 deletions(-) diff --git a/include/ctdb.h b/include/ctdb.h index 32a7a2c3..fa3f30dc 100644 --- a/include/ctdb.h +++ b/include/ctdb.h @@ -100,16 +100,16 @@ struct ctdb_lock; * When the lock is released, data is freed too, so make sure to copy the data * before that. * - * This returns true on success, and req will be non-NULL if a request was - * actually sent, otherwise callback will have already been called. + * This returns true on success: the callback may have already been called, + * or it might be awaiting a response from ctdbd. */ +typedef void (*ctdb_rrl_callback_t)(struct ctdb_db *ctdb_db, + struct ctdb_lock *lock, + TDB_DATA data, + void *private); bool -ctdb_readrecordlock_send(struct ctdb_db *ctdb_db, TDB_DATA key, - struct ctdb_request **req, - ctdb_callback_t callback, void *private_data); -struct ctdb_lock *ctdb_readrecordlock_recv(struct ctdb_db *ctdb_db, - struct ctdb_request *handle, - TDB_DATA *data); +ctdb_readrecordlock_async(struct ctdb_db *ctdb_db, TDB_DATA key, + ctdb_rrl_callback_t callback, void *private_data); /* Returns null on failure. */ struct ctdb_lock *ctdb_readrecordlock(struct ctdb_db *ctdb_db, TDB_DATA key, @@ -118,9 +118,7 @@ struct ctdb_lock *ctdb_readrecordlock(struct ctdb_db *ctdb_db, TDB_DATA key, /* * Function to write data to a record * This function may ONLY be called while holding a lock to the record - * created by ctdb_readrecordlock* - * Either from the callback provided to ctdb_readrecordlock_send() - * or after calling ctdb_readrecordlock_recv() but before calling + * created by ctdb_readrecordlock*, and before calling * ctdb_release_lock() to release the lock. */ int ctdb_writerecord(struct ctdb_lock *lock, TDB_DATA data); @@ -221,9 +219,11 @@ int ctdb_cancel(struct ctdb_request *); ctdb_attachdb_send((ctdb), (name), (persistent), (tdb_flags), \ ctdb_sendcb((cb), (cbdata)), (cbdata)) -#define ctdb_readrecordlock_send(ctdb_db, key, reqp, cb, cbdata) \ - ctdb_readrecordlock_send((ctdb_db), (key), (reqp), \ - ctdb_sendcb((cb), (cbdata)), (cbdata)) +#define ctdb_readrecordlock_async(_ctdb_db, key, cb, cbdata) \ + ctdb_readrecordlock_async((_ctdb_db), (key), \ + typesafe_cb_preargs(void, (cb), (cbdata), \ + struct ctdb_db *, struct ctdb_lock *, \ + TDB_DATA), (cbdata)) #define ctdb_set_message_handler_send(ctdb, srvid, handler, cb, cbdata) \ ctdb_set_message_handler_send((ctdb), (srvid), (handler), \ diff --git a/libctdb/ctdb.c b/libctdb/ctdb.c index 43c2829c..76e8d50a 100644 --- a/libctdb/ctdb.c +++ b/libctdb/ctdb.c @@ -33,7 +33,7 @@ /* Remove type-safety macros. */ #undef ctdb_attachdb_send -#undef ctdb_readrecordlock_send +#undef ctdb_readrecordlock_async /* FIXME: Could be in shared util code with rest of ctdb */ static void close_noerr(int fd) @@ -527,10 +527,9 @@ struct ctdb_lock { /* This will always be true by the time user sees this. */ bool held; struct ctdb_ltdb_header *hdr; - TDB_DATA data; /* For convenience, we stash original callback here. */ - ctdb_callback_t callback; + ctdb_rrl_callback_t callback; }; void ctdb_release_lock(struct ctdb_lock *lock) @@ -543,7 +542,7 @@ void ctdb_release_lock(struct ctdb_lock *lock) } /* We keep the lock if local node is the dmaster. */ -static bool try_readrecordlock(struct ctdb_lock *lock) +static bool try_readrecordlock(struct ctdb_lock *lock, TDB_DATA *data) { struct ctdb_ltdb_header *hdr; @@ -551,7 +550,7 @@ static bool try_readrecordlock(struct ctdb_lock *lock) return NULL; } - hdr = ctdb_local_fetch(lock->ctdb_db->tdb, lock->key, &lock->data); + hdr = ctdb_local_fetch(lock->ctdb_db->tdb, lock->key, data); if (hdr && hdr->dmaster == lock->ctdb_db->ctdb->pnn) { lock->held = true; lock->hdr = hdr; @@ -563,67 +562,52 @@ static bool try_readrecordlock(struct ctdb_lock *lock) return NULL; } -/* If they cancel *before* we hand them the lock from - * ctdb_readrecordlock_recv, we free it here. */ +/* If they shutdown before we hand them the lock, we free it here. */ static void destroy_lock(struct ctdb_request *req) { ctdb_release_lock(req->extra); } -struct ctdb_lock *ctdb_readrecordlock_recv(struct ctdb_db *ctdb_db, - struct ctdb_request *req, - TDB_DATA *data) -{ - struct ctdb_lock *lock = req->extra; - - if (!lock->held) { - /* Something went wrong. */ - return NULL; - } - - /* Now it's their responsibility to free! */ - req->extra_destructor = NULL; - *data = lock->data; - return lock; -} - static void readrecordlock_retry(struct ctdb_connection *ctdb, struct ctdb_request *req, void *private) { struct ctdb_lock *lock = req->extra; struct ctdb_reply_call *reply; + TDB_DATA data; /* OK, we've received reply to noop migration */ reply = unpack_reply_call(req, CTDB_NULL_FUNC); if (!reply || reply->status != 0) { - lock->callback(ctdb, req, private); + lock->callback(lock->ctdb_db, NULL, tdb_null, private); + ctdb_request_free(req); /* Also frees lock. */ return; } /* Can we get lock now? */ - if (try_readrecordlock(lock)) { - lock->callback(ctdb, req, private); + if (try_readrecordlock(lock, &data)) { + /* Now it's their responsibility to free lock & request! */ + req->extra_destructor = NULL; + lock->callback(lock->ctdb_db, lock, data, private); return; } /* Retransmit the same request again (we lost race). */ io_elem_reset(req->io); DLIST_ADD(ctdb->outq, req); - return; } bool -ctdb_readrecordlock_send(struct ctdb_db *ctdb_db, TDB_DATA key, - struct ctdb_request **reqp, - ctdb_callback_t callback, void *cbdata) +ctdb_readrecordlock_async(struct ctdb_db *ctdb_db, TDB_DATA key, + ctdb_rrl_callback_t callback, void *cbdata) { struct ctdb_request *req; struct ctdb_lock *lock; + TDB_DATA data; - /* Setup lock. */ + /* Setup lock */ lock = malloc(sizeof(*lock) + key.dsize); if (!lock) { - return NULL; + return false; } lock->key.dptr = (void *)(lock + 1); memcpy(lock->key.dptr, key.dptr, key.dsize); @@ -632,25 +616,23 @@ ctdb_readrecordlock_send(struct ctdb_db *ctdb_db, TDB_DATA key, lock->hdr = NULL; lock->held = false; - /* Get ready in case we need to send a migrate request. */ + /* Fast path. */ + if (try_readrecordlock(lock, &data)) { + callback(ctdb_db, lock, data, cbdata); + return true; + } + + /* Slow path: create request. */ req = new_ctdb_request(offsetof(struct ctdb_req_call, data) - + key.dsize, callback, cbdata); + + key.dsize, readrecordlock_retry, cbdata); if (!req) { ctdb_release_lock(lock); return NULL; } req->extra = lock; req->extra_destructor = destroy_lock; - - if (try_readrecordlock(lock)) { - *reqp = NULL; - callback(ctdb_db->ctdb, req, cbdata); - return true; - } - /* We store the original callback in the lock, and use our own. */ lock->callback = callback; - req->callback = readrecordlock_retry; io_elem_init_req_header(req->io, CTDB_REQ_CALL, CTDB_CURRENT_NODE, new_reqid(ctdb_db->ctdb)); @@ -663,7 +645,6 @@ ctdb_readrecordlock_send(struct ctdb_db *ctdb_db, TDB_DATA key, req->hdr.call->calldatalen = 0; memcpy(req->hdr.call->data, key.dptr, key.dsize); DLIST_ADD(ctdb_db->ctdb->outq, req); - *reqp = req; return true; } diff --git a/libctdb/tst.c b/libctdb/tst.c index 6fd376a7..090b26da 100644 --- a/libctdb/tst.c +++ b/libctdb/tst.c @@ -81,15 +81,15 @@ static void rm_cb(struct ctdb_connection *ctdb, * * Pure read, or pure write are just special cases of this cycle. */ -static void rrl_cb(struct ctdb_connection *ctdb, - struct ctdb_request *req, void *private) +static bool rrl_cb_called; + +static void rrl_cb(struct ctdb_db *ctdb_db, + struct ctdb_lock *lock, TDB_DATA outdata, void *private) { - struct ctdb_lock *lock; - TDB_DATA outdata; TDB_DATA data; char tmp[256]; - lock = ctdb_readrecordlock_recv(private, req, &outdata); + rrl_cb_called = true; if (!lock) { printf("rrl_cb returned error\n"); return; @@ -194,12 +194,12 @@ int main(int argc, char *argv[]) exit(10); } - if (!ctdb_readrecordlock_send(ctdb_db_context, key, &handle, - rrl_cb, ctdb_db_context)) { + if (!ctdb_readrecordlock_async(ctdb_db_context, key, + rrl_cb, ctdb_db_context)) { printf("Failed to send READRECORDLOCK\n"); exit(10); } - if (handle) { + if (!rrl_cb_called) { printf("READRECORDLOCK is async\n"); } for (;;) { -- 2.34.1