[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: John Snow
Subject: Re: [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object
Date: Fri, 22 May 2015 18:38:08 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

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 :)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]