Message ID | 1463229957-14253-2-git-send-email-den@openvz.org |
---|---|
State | New |
Headers | show |
On 05/14/2016 06:45 AM, Denis V. Lunev wrote: > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > 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 <pbutsykin@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Jeff Cody <jcody@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Eric Blake <eblake@redhat.com> > CC: John Snow <jsnow@redhat.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > --- > +++ 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.
On 16.05.2016 19:52, Eric Blake wrote: > On 05/14/2016 06:45 AM, Denis V. Lunev wrote: >> From: Pavel Butsykin <pbutsykin@virtuozzo.com> >> >> 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 <pbutsykin@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Jeff Cody <jcody@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> CC: John Snow <jsnow@redhat.com> >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> CC: Kevin Wolf <kwolf@redhat.com> >> --- > >> +++ 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?
On Sat, May 14, 2016 at 03:45:49PM +0300, Denis V. Lunev wrote: > diff --git a/block/io.c b/block/io.c > index cd6d71a..88af10c 100644 > --- a/block/io.c > +++ 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) Please use the coroutine_fn attribute to declare that this function must be called in coroutine context. > diff --git a/include/block/block_int.h b/include/block/block_int.h > index a029c20..3c93ddb 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -208,6 +208,9 @@ struct BlockDriver { > 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); Please add the coroutine_fn attribute just like .bdrv_co_readv() and friends.
On 05/20/2016 12:25 AM, Stefan Hajnoczi wrote: > On Sat, May 14, 2016 at 03:45:49PM +0300, Denis V. Lunev wrote: >> diff --git a/block/io.c b/block/io.c >> index cd6d71a..88af10c 100644 >> --- a/block/io.c >> +++ 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) > Please use the coroutine_fn attribute to declare that this function must > be called in coroutine context. > >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index a029c20..3c93ddb 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -208,6 +208,9 @@ struct BlockDriver { >> 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); > Please add the coroutine_fn attribute just like .bdrv_co_readv() and > friends. yep. This seems right thing to do.
diff --git a/block/io.c b/block/io.c index cd6d71a..88af10c 100644 --- a/block/io.c +++ 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) { 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); +} + +typedef struct BdrvWriteCompressedCo { + BlockDriverState *bs; + int64_t sector_num; + const uint8_t *buf; + int nb_sectors; + int ret; +} BdrvWriteCompressedCo; + +static void bdrv_write_compressed_co_entry(void *opaque) +{ + BdrvWriteCompressedCo *co = opaque; + QEMUIOVector qiov; + struct iovec iov = { + .iov_base = (uint8_t *)co->buf, + .iov_len = co->nb_sectors << BDRV_SECTOR_BITS, + }; + qemu_iovec_init_external(&qiov, &iov, 1); + + co->ret = bdrv_co_write_compressed(co->bs, co->sector_num, + co->nb_sectors, &qiov); +} + +int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, + const uint8_t *buf, int nb_sectors) +{ + BlockDriver *drv = bs->drv; + BdrvWriteCompressedCo data = { + .bs = bs, + .sector_num = sector_num, + .buf = buf, + .nb_sectors = nb_sectors, + .ret = -EINPROGRESS, + }; + + if (!drv) { + return -ENOMEDIUM; + } + + if (drv->bdrv_write_compressed) { + int ret = bdrv_check_request(bs, sector_num, nb_sectors); + if (ret < 0) { + return ret; + } + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); + return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors); + } - return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors); + if (qemu_in_coroutine()) { + /* Fast-path if already in coroutine context */ + bdrv_write_compressed_co_entry(&data); + } else { + AioContext *aio_context = bdrv_get_aio_context(bs); + + Coroutine *co = qemu_coroutine_create(bdrv_write_compressed_co_entry); + qemu_coroutine_enter(co, &data); + while (data.ret == -EINPROGRESS) { + aio_poll(aio_context, true); + } + } + return data.ret; } int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, diff --git a/include/block/block.h b/include/block/block.h index b210832..ae67fd8 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -423,6 +423,8 @@ const char *bdrv_get_device_or_node_name(const BlockDriverState *bs); int bdrv_get_flags(BlockDriverState *bs); 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); int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs); void bdrv_round_to_clusters(BlockDriverState *bs, diff --git a/include/block/block_int.h b/include/block/block_int.h index a029c20..3c93ddb 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -208,6 +208,9 @@ struct BlockDriver { 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); + int (*bdrv_snapshot_create)(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int (*bdrv_snapshot_goto)(BlockDriverState *bs, diff --git a/qemu-img.c b/qemu-img.c index 4792366..0d38eac 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2022,7 +2022,7 @@ static int img_convert(int argc, char **argv) const char *preallocation = qemu_opt_get(opts, BLOCK_OPT_PREALLOC); - if (!drv->bdrv_write_compressed) { + if (!drv->bdrv_write_compressed && !drv->bdrv_co_write_compressed) { error_report("Compression not supported for this file format"); ret = -1; goto out;