Re: [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object

[John Snow]

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 <address@hidden> Reviewed-by: Max
>>>> Reitz <address@hidden> --- 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