From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yvva2-0004AZ-VT for qemu-devel@nongnu.org; Fri, 22 May 2015 18:38:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YvvZz-0004jw-Ng for qemu-devel@nongnu.org; Fri, 22 May 2015 18:38:14 -0400 Message-ID: <555FAFD0.5090408@redhat.com> Date: Fri, 22 May 2015 18:38:08 -0400 From: John Snow MIME-Version: 1.0 References: <1431385466-4868-1-git-send-email-jsnow@redhat.com> <1431385466-4868-7-git-send-email-jsnow@redhat.com> <20150518154559.GB27654@stefanha-thinkpad.redhat.com> <555BB5FB.5020804@redhat.com> <20150520092703.GA6945@stefanha-thinkpad.redhat.com> In-Reply-To: <20150520092703.GA6945@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com 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 Reviewed-by: Max >>>> Reitz --- 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