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

[Kevin Wolf]

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?

Kevin