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

[Stefan Hajnoczi]

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

pgp5N4_WzQEUj.pgp
Description: PGP signature