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

[John Snow <jsnow@redhat.com>]

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 <jsnow@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@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.

Ho hum.

--js