Re: [Qemu-devel] [PATCH 01/10] block/io: add bdrv_co_write_compressed

[Pavel Butsykin]

On 16.05.2016 19:52, Eric Blake wrote:
> On 05/14/2016 06:45 AM, Denis V. Lunev wrote:
> > From: Pavel Butsykin <address@hidden>
> >
> > This patch just adds the interface to the bdrv_co_write_compressed, which
> > is currently not used but will be useful for safe implementation of the
> > bdrv_co_write_compressed callback in format drivers.
> >
> > Signed-off-by: Pavel Butsykin <address@hidden>
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > CC: Jeff Cody <address@hidden>
> > CC: Markus Armbruster <address@hidden>
> > CC: Eric Blake <address@hidden>
> > CC: John Snow <address@hidden>
> > CC: Stefan Hajnoczi <address@hidden>
> > CC: Kevin Wolf <address@hidden>
> > ---
>
> > +++ b/block/io.c
> > @@ -1828,8 +1828,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
> >       return 0;
> >   }
> >
> > -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> > -                          const uint8_t *buf, int nb_sectors)
> > +int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
> > +                             int nb_sectors, QEMUIOVector *qiov)
>
> As long as we're adding a new public interface, I'd really like us to
> make it byte-based.  int64_t sector_num might be better represented as a
> byte offset, and int nb_sectors seems redundant with qiov->size.
>
> >   {
> >       BlockDriver *drv = bs->drv;
> >       int ret;
> > @@ -1837,7 +1837,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t 
> > sector_num,
> >       if (!drv) {
> >           return -ENOMEDIUM;
> >       }
> > -    if (!drv->bdrv_write_compressed) {
> > +    if (!drv->bdrv_co_write_compressed) {
> >           return -ENOTSUP;
> >       }
> >       ret = bdrv_check_request(bs, sector_num, nb_sectors);
> > @@ -1846,8 +1846,71 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t 
> > sector_num,
> >       }
> >
> >       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> > +    assert(qemu_in_coroutine());
> > +
> > +    return drv->bdrv_co_write_compressed(bs, sector_num, nb_sectors, qiov);
>
> Of course, if you make the public interface byte-based, then calling
> into the back end will have to scale back to sectors (after first
> asserting that we aren't violating the scaling); see how Kevin did it in
> commit 166fe9605.
>
> > +}
> > +
> > +typedef struct BdrvWriteCompressedCo {
> > +    BlockDriverState *bs;
> > +    int64_t sector_num;
>
> Again, I think a byte offset is smarter than a sector number.

Kevin used the byte offset for functions bdrv_driver_pread/_pwrite(It
looks like just an additional interface), which is not the same thing.
Here the bdrv_co/bdrv_write_compressed functions are analogues of the
bdrv_co/bdrv_write functions that still use sectors in the arguments.
So I'm not sure that the interface there needs to be some other.


Kevin, what do you think about this?