Re: [Qemu-devel] [PATCH v2 1/3] block: add basic backup support to block driver

[Stefan Hajnoczi]

On Tue, May 14, 2013 at 3:43 PM, Kevin Wolf <address@hidden> wrote:
> Am 14.05.2013 um 15:24 hat Stefan Hajnoczi geschrieben:
>> On Wed, May 08, 2013 at 02:39:25PM +0200, Kevin Wolf wrote:
>> > Am 29.04.2013 um 09:42 hat Stefan Hajnoczi geschrieben:
>> > > diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>> > > index c290d07..6f42495 100644
>> > > --- a/include/block/blockjob.h
>> > > +++ b/include/block/blockjob.h
>> > > @@ -50,6 +50,13 @@ typedef struct BlockJobType {
>> > >       * manually.
>> > >       */
>> > >      void (*complete)(BlockJob *job, Error **errp);
>> > > +
>> > > +    /** tracked requests */
>> > > +    int coroutine_fn (*before_read)(BlockDriverState *bs, int64_t 
>> > > sector_num,
>> > > +                                    int nb_sectors, QEMUIOVector *qiov);
>> > > +    int coroutine_fn (*before_write)(BlockDriverState *bs, int64_t 
>> > > sector_num,
>> > > +                                     int nb_sectors, QEMUIOVector 
>> > > *qiov);
>> > > +
>> > >  } BlockJobType;
>> >
>> > This is actually a sign that a block job isn't the right tool. Jobs are
>> > something that runs in the background and doesn't have callbacks. You
>> > really want to have a filter here (that happens to be coupled to a job).
>> > Need the BlockBackend split before we can do this right.
>> >
>> > The second thing that this conflicts with is generalising block jobs to
>> > generic background jobs.
>> >
>> > Each hack like this that we accumulate makes it harder to get the real
>> > thing eventually.
>>
>> In v3 I added a slightly cleaner interface:
>>
>> bdrv_set_before_write_cb(bs, backup_before_write);
>>
>> This way the "before write" callback is not tied to block jobs and
>> could be reused in the future.
>>
>> The alternative is to create a BlockDriver that mostly delegates plus
>> uses bdrv_swap().  The boilerplate involved in that is ugly though, so
>> I'm using bdrv_set_before_write_cb() instead.
>
> The clean implementation of filters is putting a BlockDriver on top, it
> gives you flexibility to intercept anything and you can stack multiple
> of them instead of having just one callback per BDS.
>
> But I'm not sure if simply bdrv_swap is good enough. You would definitely
> have to disable snapshots while the filter is active and there may be
> more cases. This is the part that I think getting right is a bit more
> complex and might need the BlockBackend split.
>
> So you would vote to introduce bdrv_set_before_write_cb() now and later
> change how everything works?

Yes, I'd like to use bdrv_set_before_write_cb().  That's mainly
because I'm working on dataplane for QEMU 1.6 and I know that Ian and
Fam want to build on drive-backup, so the sooner it gets upstream, the
better for them.

I know these are non-technical reasons.  bdrv_set_before_write_cb() is
simple enough to convert though.

Stefan