On 03/17/2015 01:47 PM, Max Reitz wrote:
On 2015-03-04 at 23:15, John Snow wrote:
The goal here is to add a new method to transactions that allows
developers to specify a callback that will get invoked only once
all jobs spawned by a transaction are completed, allowing developers
the chance to perform actions conditionally pending complete success
or complete failure.
In order to register the new callback to be invoked, a user must
request
a callback pointer and closure by calling new_transaction_wrapper,
which
creates a wrapper around a closure and callback that would originally
have
been passed to e.g. backup_start().
The function will return a function pointer and a new closure to be
passed
instead. The transaction system will effectively intercept these
callbacks
and execute the desired actions upon reception of all intercepted
callbacks.
This means that the registered callbacks will be called after all other
transaction actions that requested a callback have completed. The
feature
has no knowledge of jobs spawned without informing the
BlkTransactionList.
For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an
example
for how to hook in drive_backup (or any block job launched by
transactions).
Note 1: Defining a callback method alone is not sufficient to have the
new
method invoked. You must call new_transaction_wrapper AND
ensure the
callback it returns to you is used as the callback for the
job.
Note 2: You can use this feature for any system that registers
completions of
an action via a callback of the form (void *opaque, int ret),
not just
block job callbacks.
Note 3: new_blk_transaction has no users in this patch, but will in
the next patch where it will become static and local to
blockdev.c.
Signed-off-by: John Snow <address@hidden>
---
blockdev.c | 225
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 223 insertions(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 5120af1..3153ee7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1207,6 +1207,8 @@ static BdrvDirtyBitmap
*block_dirty_bitmap_lookup(const char *node,
/* New and old BlockDriverState structs for atomic group
operations */
typedef struct BlkTransactionState BlkTransactionState;
+typedef struct BlkTransactionList BlkTransactionList;
+typedef struct BlkTransactionData BlkTransactionData;
/* Only prepare() may fail. In a single transaction, only one of
commit() or
abort() will be called, clean() will always be called if it
present. */
@@ -1221,6 +1223,8 @@ typedef struct BdrvActionOps {
void (*abort)(BlkTransactionState *common);
/* Clean up resource in the end, can be NULL. */
void (*clean)(BlkTransactionState *common);
+ /* Execute this after +all+ jobs in the transaction finish */
+ void (*cb)(BlkTransactionState *common);
} BdrvActionOps;
/*
@@ -1231,9 +1235,220 @@ typedef struct BdrvActionOps {
struct BlkTransactionState {
TransactionAction *action;
const BdrvActionOps *ops;
+ BlkTransactionList *list;
+ void *opaque;
+ /* Allow external users (callbacks) to reference this obj past
.clean() */
+ int refcount;
+ /* All transactions in the current group */
QSIMPLEQ_ENTRY(BlkTransactionState) entry;
+ /* Transactions in the current group with callbacks */
+ QSIMPLEQ_ENTRY(BlkTransactionState) list_entry;
"list_entry" seems very generic and it's hard for me to see a
fundamental difference to just "entry". How about "cb_entry", or
"cb_list_entry", or "cb_group_entry" or something like that?
};
+struct BlkTransactionList {
+ int jobs; /* Effectively: A refcount */
Good to know, but I'd like the reason why it's called like this to be
here anyway ("Number of jobs remaining" or I don't know).
Yes, it's basically a refcount where the references are held by jobs.
+ int status; /* Cumulative retcode */
The only places I currently find "retcode" in are linux-user/signal.c
and tests/image-fuzzer/runner.py. How about the more common "return
value" (or simply "cumulative status")?
("retcode" reads a bit like "retcon" to me, that's why I had to think
about it for a second)
Sure.
+ QSIMPLEQ_HEAD(actions, BlkTransactionState) actions;
+};
+
+typedef struct BlkTransactionData {
+ void *opaque; /* Data given to encapsulated callback */
+ int ret; /* Return code given to encapsulated callback */
+} BlkTransactionData;
+
+typedef struct BlkTransactionWrapper {
+ void *opaque; /* Data to be given to encapsulated callback */
+ void (*callback)(void *opaque, int ret); /* Encapsulated
callback */
+} BlkTransactionWrapper;
I find it pretty difficult to figure out what these objects are each
for. Care to add comments for that, too?
Will do.
+
+static BlkTransactionList *new_blk_transaction_list(void)
+{
+ BlkTransactionList *btl = g_malloc0(sizeof(*btl));
Maybe use g_new0(BlkTransactionList, 1); (It's typesafe! And who doesn't
love typesafety?).
(Optional, though, the foo = malloc(sizeof(*foo)) pattern is pretty
wide-spread, and as far as I remember, Markus purposely did not replace
it by foo = g_new(Foo, 1))
+
+ /* Implicit 'job' for qmp_transaction itself */
+ btl->jobs = 1;
Well, if it is a refcount, just call it that way...
(Update: Okay, I see why you didn't call it that way. Maybe the comment
needs improvement, like "The transaction itself is a job, too, that
needs to be completed before the callbacks are called")
That's exactly the trick in play, here.
+ QSIMPLEQ_INIT(&btl->actions);
+ return btl;
+}
+
+static BlkTransactionState
*blk_put_transaction_state(BlkTransactionState *bts)
+{
+ bts->refcount--;
+ if (bts->refcount == 0) {
+ g_free(bts);
How about removing it from the lists it's in? Doesn't appear to be
necessary, but I'd find it cleaner.
+ return NULL;
+ }
+ return bts;
+}
+
+static void del_blk_transaction_list(BlkTransactionList *btl)
+{
+ BlkTransactionState *bts, *bts_next;
+
+ /* The list should in normal cases be empty,
+ * but in case someone really just wants to kibosh the whole
deal: */
Thank you for teaching me a new word.
+ QSIMPLEQ_FOREACH_SAFE(bts, &btl->actions, list_entry, bts_next) {
+ g_free(bts->opaque);
Urp... Are you sure? :-/
I'd rather have some callback for destroying the object... Apparently
this will (for now) be always useless overhead, because that callback is
only calling g_free(), but it's an opaque pointer, so it's not really up
to you to do anything with it other than passing it around.
+ blk_put_transaction_state(bts);
+ }
+
+ g_free(btl);
+}
+
+static void blk_run_transaction_callbacks(BlkTransactionList *btl)
+{
+ BlkTransactionState *bts, *bts_next;
+
+ QSIMPLEQ_FOREACH_SAFE(bts, &btl->actions, list_entry, bts_next) {
+ if (bts->ops->cb) {
+ bts->ops->cb(bts);
+ }
+
+ /* Free the BlkTransactionData */
+ g_free(bts->opaque);
Again... Urp. If you know it's a BlkTransactionData object, please make
it a BlkTransactionData pointer and don't call it "opaque" if it's not
so opaque in the end.
A good point. I use this "opaque" pointer to actually hold two very
specific and knowable things; it's just that those things change
during the lifetime of the object.
For whatever reason, I decided it was nicer to not have two pointers
that are never used simultaneously.
I can either make two dedicated fields, or just introduce it as an
union. The state of the object otherwise dictates which union field to
access.
Because I am a complicated individual, I am thinking fondly of the
union right now.
And then, sure, "blk_transaction_list_job_completed" would be a fine
alternative to "put."
+ blk_run_transaction_callbacks(btl);
+ del_blk_transaction_list(btl);
+ return NULL;
+ }
+ return btl;
+}
+
+static void blk_transaction_complete(BlkTransactionState *common)
+{
+ BlkTransactionList *btl = common->list;
+
+ /* Add this action into the pile to be completed */
+ QSIMPLEQ_INSERT_TAIL(&btl->actions, common, list_entry);
+
+ /* Inform the list that we have a completion;
+ * possibly run all the pending actions. */
+ put_blk_transaction_list(btl);
+}
+
+/**
+ * Intercept a callback that was issued due to a transactional action.
+ */
+static void transaction_callback(void *opaque, int ret)
+{
+ BlkTransactionState *common = opaque;
+ BlkTransactionWrapper *btw = common->opaque;
+
+ /* Prepare data for ops->cb() */
+ BlkTransactionData *btd = g_malloc0(sizeof(*btd));
g_new0(BlkTransactionData, 1);
+ btd->opaque = btw->opaque;
+ btd->ret = ret;
+
+ /* Transaction state now tracks OUR data */
+ common->opaque = btd;
Sorry, but I really have a hard time following this opaqueness... Again,
if you can, please clarify what the objects are for, and it would be
very nice to separate truly opaque objects from these internally used
objects (which are managed by you and thus are to be managed by you
(g_free()), because reusing void * pointers for different kinds of
objects like this makes my brain go strawberry.
I'll shoot for Raspberry in v2.
},
[TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
.instance_size = sizeof(BlockdevBackupState),
@@ -1815,10 +2030,12 @@ void qmp_transaction(TransactionActionList
*dev_list, Error **errp)
{
TransactionActionList *dev_entry = dev_list;
BlkTransactionState *state, *next;
+ BlkTransactionList *btl;
Error *local_err = NULL;
QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState)
snap_bdrv_states;
QSIMPLEQ_INIT(&snap_bdrv_states);
+ btl = new_blk_transaction_list();
/* drain all i/o before any operations */
bdrv_drain_all();
@@ -1837,8 +2054,10 @@ void qmp_transaction(TransactionActionList
*dev_list, Error **errp)
assert(ops->instance_size > 0);
state = g_malloc0(ops->instance_size);
+ state->refcount = 1;
state->ops = ops;
state->action = dev_info;
+ state->list = btl;
QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
state->ops->prepare(state, &local_err);
@@ -1869,8 +2088,10 @@ exit:
if (state->ops->clean) {
state->ops->clean(state);
}
- g_free(state);
+ blk_put_transaction_state(state);
}
+
+ put_blk_transaction_list(btl);
}
Sorry, as I said, I need more context on the objects and very much would
like a separation between truly opaque pointers and not-so-opaque
pointers, before I can really understand what's going on (or I put more
effort into it, but since it's always more probable to belong to the
majority, I guess I won't be the only one getting confused).
Maybe the next patches will clear it up, I don't know.
Max
I'll put some more elbow grease into explaining the flow.