On 05/20/2015 05:27 AM, Stefan Hajnoczi wrote: > On Tue, May 19, 2015 at 06:15:23PM -0400, John Snow wrote: >> On 05/18/2015 11:45 AM, Stefan Hajnoczi wrote: >>> On Mon, May 11, 2015 at 07:04:21PM -0400, John Snow wrote: >>>> If we want to get at the job after the life of the job, we'll >>>> need a refcount for this object. >>>> >>>> This may occur for example if we wish to inspect the actions >>>> taken by a particular job after a transactional group of >>>> jobs runs, and further actions are required. >>>> >>>> Signed-off-by: John Snow <js...@redhat.com> Reviewed-by: Max >>>> Reitz <mre...@redhat.com> --- blockjob.c | 18 >>>> ++++++++++++++++-- include/block/blockjob.h | 21 >>>> +++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 >>>> deletions(-) >>> >>> I think the only reason for this refcount is so that >>> backup_transaction_complete() can be called. It accesses >>> BackupBlockJob->sync_bitmap so the BackupBlockJob instance >>> needs to be alive. >>> >>> The bitmap refcount is incremented in blockdev.c, not >>> block/backup.c, so it is fine to drop >>> backup_transaction_complete() and decrement the bitmap refcount >>> in blockdev.c instead. >>> >>> If you do that then there is no need to add a refcount to block >>> job. This would simplify things. >>> >> >> So you are suggesting that I cache the bitmap reference (instead >> of the job reference) and then just increment/decrement it >> directly in .prepare, .abort and .cb as needed. >> >> You did find the disparity with the reference count for the >> bitmap, at least: that is kind of gross. I was coincidentally >> thinking of punting it back into a backup_transaction_start to >> keep more code /out/ of blockdev... >> >> I'll sit on this one for a few more minutes. I'll try to get rid >> of the job refcnt, but I also want to keep the transaction >> actions as tidy as I can. >> >> Maybe it's too much abstraction for a simple task, but I wanted >> to make sure I wasn't hacking in transaction callbacks in a >> manner where they'd only be useful to me, for only this one case. >> It's conceivable that if anyone else attempts to use this >> callback hijacking mechanism that they'll need to find a way to >> modify objects within the Job without pulling everything up to >> the transaction actions, too. > > Hmm...I think this could be achieved without refcounting in > transactions, actions, or block jobs. > > Create a separate MultiCompletion struct with a counter and list > of callbacks: > > typedef struct MultiCompletionCB { BlockCompletionFunc cb; void > *opaque; QSLIST_ENTRY(MultiCompletionCB) list; } > MultiCompletionCB; > > typedef struct { unsigned refcnt; /* callbacks invoked when this > reaches zero */ QSLIST_HEAD(, MultiCompletionCB) callbacks; int > ret; } MultiCompletion; > > void multicomp_add_cb(MultiCompletion *mc, BlockCompletionFunc cb, > void *opaque); > > /* Note the first argument is MultiCompletion* but this prototype * > allows it to be used as a blockjob cb. */ void > multicomp_complete(void *opaque, int ret) { MultiCompletion *mc = > opaque; > > if (ret < 0) { mc->ret = ret; } > > if (--mc->refcnt == 0) { MultiCompletionCB *cb_data; while > ((cb_data = QSLIST_FIRST(&mc->callbacks)) != NULL) { > cb_data->cb(cb_data->opaque, mc->ret); > > QSLIST_REMOVE_HEAD(&mc->callbacks, list); g_free(cb_data); } > > g_free(mc); } } > > qmp_transaction() creates a MultiCompletion and passes it to > action .prepare(). If an action wishes to join the > MultiCompletion, it calls multicomp_add_cb() after creating a block > job: > > static void backup_completion(void *opaque, int ret) { HBitmap > *bmap = opaque; if (ret < 0) { ...merge bitmap back in... } else { > ...drop frozen bitmap... } } > > ... backup_start(..., multicomp_completion, mc); > multicomp_add_cb(mc, backup_completion, bmap); > > The multicomp_complete() function gets called by a blockjob cb > function. > > This approach is more reusable since MultiCompletion is independent > of transactions or block jobs. It also doesn't hold on to > transactions, actions, or block jobs after they have served their > purpose. > > Is this along the lines you were thinking about? > > Stefan >
Yes, this is roughly what I was aiming for in the design of this series as it stands now, except I did essentially bake the MultiCompletion functionality into the BlkTransactionState structure instead of leaving it separate. I think it might be worth doing, though I could also just drop a lot of the refcounts now and rework who picks up the bitmap reference count and achieve something nearly similar. I'll play around with the patches I have now and see if I can't tidy it up to the point where I'm happy with the way it's managing references, and if not I might scrap it and try the even more abstracted version. I'll probably just choose whatever looks cleanest :) --js