| Message ID | 1467202266-15088-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 29.06.2016 14:11, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > > This patch contains non-trivial fixes, so I think it's worth sending out a v2 > for it even though I already applied the series. I added a coroutine entry > wrapper qcow(2)_write that can be used from .bdrv_write_compressed. These > wrappers will soon disappear again when .bdrv_write_compressed is changed into > .bdrv_co_pwritev_compressed (Pavel Butsykin's backup compression series). > > block/io.c | 5 +++-- > block/qcow.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- > block/qcow2-cluster.c | 2 +- > block/qcow2-refcount.c | 2 +- > block/qcow2.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > block/vdi.c | 4 ++-- > block/vvfat.c | 5 ++--- > include/block/block.h | 2 +- > 8 files changed, 100 insertions(+), 12 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> [...] > diff --git a/block/qcow2.c b/block/qcow2.c > index 0178931..cd9c27b 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) > return 0; > } > > +typedef struct Qcow2WriteCo { > + BlockDriverState *bs; > + int64_t sector_num; > + const uint8_t *buf; > + int nb_sectors; > + int ret; > +} Qcow2WriteCo; > + > +static void qcow2_write_co_entry(void *opaque) > +{ > + Qcow2WriteCo *co = opaque; > + QEMUIOVector qiov; > + uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE; > + uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE; It doesn't make much sense to make this a uint64_t, and I'm afraid Coverity will complain about it... It's not wrong, though, but an int would have been more "honest". Max > + > + struct iovec iov = (struct iovec) { > + .iov_base = (uint8_t*) co->buf, > + .iov_len = bytes, > + }; > + qemu_iovec_init_external(&qiov, &iov, 1); > + > + co->ret = qcow2_co_pwritev(co->bs, offset, bytes, &qiov, 0); > +}
Am 29.06.2016 um 17:22 hat Max Reitz geschrieben: > On 29.06.2016 14:11, Kevin Wolf wrote: > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > > > This patch contains non-trivial fixes, so I think it's worth sending out a v2 > > for it even though I already applied the series. I added a coroutine entry > > wrapper qcow(2)_write that can be used from .bdrv_write_compressed. These > > wrappers will soon disappear again when .bdrv_write_compressed is changed into > > .bdrv_co_pwritev_compressed (Pavel Butsykin's backup compression series). > > > > block/io.c | 5 +++-- > > block/qcow.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- > > block/qcow2-cluster.c | 2 +- > > block/qcow2-refcount.c | 2 +- > > block/qcow2.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > > block/vdi.c | 4 ++-- > > block/vvfat.c | 5 ++--- > > include/block/block.h | 2 +- > > 8 files changed, 100 insertions(+), 12 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > [...] > > > diff --git a/block/qcow2.c b/block/qcow2.c > > index 0178931..cd9c27b 100644 > > --- a/block/qcow2.c > > +++ b/block/qcow2.c > > @@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) > > return 0; > > } > > > > +typedef struct Qcow2WriteCo { > > + BlockDriverState *bs; > > + int64_t sector_num; > > + const uint8_t *buf; > > + int nb_sectors; > > + int ret; > > +} Qcow2WriteCo; > > + > > +static void qcow2_write_co_entry(void *opaque) > > +{ > > + Qcow2WriteCo *co = opaque; > > + QEMUIOVector qiov; > > + uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE; > > + uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE; > > It doesn't make much sense to make this a uint64_t, and I'm afraid > Coverity will complain about it... It's not wrong, though, but an int > would have been more "honest". Hm, just copied from vmdk... Anyway, you right that we don't really need uint64_t here because of BDRV_REQUEST_MAX_SECTORS, but BDRV_SECTOR_SIZE is unsigned long long, so at least this is a proper 64 bit calculation and Coverity should stay silent. Kevin
On 29.06.2016 17:33, Kevin Wolf wrote: > Am 29.06.2016 um 17:22 hat Max Reitz geschrieben: >> On 29.06.2016 14:11, Kevin Wolf wrote: >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> >>> This patch contains non-trivial fixes, so I think it's worth sending out a v2 >>> for it even though I already applied the series. I added a coroutine entry >>> wrapper qcow(2)_write that can be used from .bdrv_write_compressed. These >>> wrappers will soon disappear again when .bdrv_write_compressed is changed into >>> .bdrv_co_pwritev_compressed (Pavel Butsykin's backup compression series). >>> >>> block/io.c | 5 +++-- >>> block/qcow.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- >>> block/qcow2-cluster.c | 2 +- >>> block/qcow2-refcount.c | 2 +- >>> block/qcow2.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- >>> block/vdi.c | 4 ++-- >>> block/vvfat.c | 5 ++--- >>> include/block/block.h | 2 +- >>> 8 files changed, 100 insertions(+), 12 deletions(-) >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> [...] >> >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 0178931..cd9c27b 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) >>> return 0; >>> } >>> >>> +typedef struct Qcow2WriteCo { >>> + BlockDriverState *bs; >>> + int64_t sector_num; >>> + const uint8_t *buf; >>> + int nb_sectors; >>> + int ret; >>> +} Qcow2WriteCo; >>> + >>> +static void qcow2_write_co_entry(void *opaque) >>> +{ >>> + Qcow2WriteCo *co = opaque; >>> + QEMUIOVector qiov; >>> + uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE; >>> + uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE; >> >> It doesn't make much sense to make this a uint64_t, and I'm afraid >> Coverity will complain about it... It's not wrong, though, but an int >> would have been more "honest". > > Hm, just copied from vmdk... Anyway, you right that we don't really need > uint64_t here because of BDRV_REQUEST_MAX_SECTORS, but BDRV_SECTOR_SIZE > is unsigned long long, so at least this is a proper 64 bit calculation > and Coverity should stay silent. Oh, right, I missed that. That is actually very clever, I should remember that. Max
diff --git a/block/io.c b/block/io.c index 6dfc0eb..2e04a80 100644 --- a/block/io.c +++ b/block/io.c @@ -642,10 +642,11 @@ int bdrv_read(BdrvChild *child, int64_t sector_num, -EINVAL Invalid sector number or nb_sectors -EACCES Trying to write a read-only device */ -int bdrv_write(BlockDriverState *bs, int64_t sector_num, +int bdrv_write(BdrvChild *child, int64_t sector_num, const uint8_t *buf, int nb_sectors) { - return bdrv_rw_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true, 0); + return bdrv_rw_co(child->bs, sector_num, (uint8_t *)buf, nb_sectors, + true, 0); } int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset, diff --git a/block/qcow.c b/block/qcow.c index 0db43f8..674595e 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -913,6 +913,49 @@ static int qcow_make_empty(BlockDriverState *bs) return 0; } +typedef struct QcowWriteCo { + BlockDriverState *bs; + int64_t sector_num; + const uint8_t *buf; + int nb_sectors; + int ret; +} QcowWriteCo; + +static void qcow_write_co_entry(void *opaque) +{ + QcowWriteCo *co = opaque; + QEMUIOVector qiov; + + struct iovec iov = (struct iovec) { + .iov_base = (uint8_t*) co->buf, + .iov_len = co->nb_sectors * BDRV_SECTOR_SIZE, + }; + qemu_iovec_init_external(&qiov, &iov, 1); + + co->ret = qcow_co_writev(co->bs, co->sector_num, co->nb_sectors, &qiov); +} + +/* Wrapper for non-coroutine contexts */ +static int qcow_write(BlockDriverState *bs, int64_t sector_num, + const uint8_t *buf, int nb_sectors) +{ + Coroutine *co; + AioContext *aio_context = bdrv_get_aio_context(bs); + QcowWriteCo data = { + .bs = bs, + .sector_num = sector_num, + .buf = buf, + .nb_sectors = nb_sectors, + .ret = -EINPROGRESS, + }; + co = qemu_coroutine_create(qcow_write_co_entry); + qemu_coroutine_enter(co, &data); + while (data.ret == -EINPROGRESS) { + aio_poll(aio_context, true); + } + return data.ret; +} + /* XXX: put compressed sectors first, then all the cluster aligned tables to avoid losing bytes in alignment */ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num, @@ -969,7 +1012,7 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num, if (ret != Z_STREAM_END || out_len >= s->cluster_size) { /* could not compress: write normal cluster */ - ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors); + ret = qcow_write(bs, sector_num, buf, s->cluster_sectors); if (ret < 0) { goto fail; } diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index c1e9eee..a2490d7 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1784,7 +1784,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, goto fail; } - ret = bdrv_write(bs->file->bs, l2_offset / BDRV_SECTOR_SIZE, + ret = bdrv_write(bs->file, l2_offset / BDRV_SECTOR_SIZE, (void *)l2_table, s->cluster_sectors); if (ret < 0) { goto fail; diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 3bef410..12e7e6b 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -2098,7 +2098,7 @@ write_refblocks: on_disk_refblock = (void *)((char *) *refcount_table + refblock_index * s->cluster_size); - ret = bdrv_write(bs->file->bs, refblock_offset / BDRV_SECTOR_SIZE, + ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE, on_disk_refblock, s->cluster_sectors); if (ret < 0) { fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret)); diff --git a/block/qcow2.c b/block/qcow2.c index 0178931..cd9c27b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2533,6 +2533,51 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) return 0; } +typedef struct Qcow2WriteCo { + BlockDriverState *bs; + int64_t sector_num; + const uint8_t *buf; + int nb_sectors; + int ret; +} Qcow2WriteCo; + +static void qcow2_write_co_entry(void *opaque) +{ + Qcow2WriteCo *co = opaque; + QEMUIOVector qiov; + uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE; + uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE; + + struct iovec iov = (struct iovec) { + .iov_base = (uint8_t*) co->buf, + .iov_len = bytes, + }; + qemu_iovec_init_external(&qiov, &iov, 1); + + co->ret = qcow2_co_pwritev(co->bs, offset, bytes, &qiov, 0); +} + +/* Wrapper for non-coroutine contexts */ +static int qcow2_write(BlockDriverState *bs, int64_t sector_num, + const uint8_t *buf, int nb_sectors) +{ + Coroutine *co; + AioContext *aio_context = bdrv_get_aio_context(bs); + Qcow2WriteCo data = { + .bs = bs, + .sector_num = sector_num, + .buf = buf, + .nb_sectors = nb_sectors, + .ret = -EINPROGRESS, + }; + co = qemu_coroutine_create(qcow2_write_co_entry); + qemu_coroutine_enter(co, &data); + while (data.ret == -EINPROGRESS) { + aio_poll(aio_context, true); + } + return data.ret; +} + /* XXX: put compressed sectors first, then all the cluster aligned tables to avoid losing bytes in alignment */ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num, @@ -2596,7 +2641,7 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num, if (ret != Z_STREAM_END || out_len >= s->cluster_size) { /* could not compress: write normal cluster */ - ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors); + ret = qcow2_write(bs, sector_num, buf, s->cluster_sectors); if (ret < 0) { goto fail; } diff --git a/block/vdi.c b/block/vdi.c index 46a3436..b2871ca 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -719,7 +719,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, assert(VDI_IS_ALLOCATED(bmap_first)); *header = s->header; vdi_header_to_le(header); - ret = bdrv_write(bs->file->bs, 0, block, 1); + ret = bdrv_write(bs->file, 0, block, 1); g_free(block); block = NULL; @@ -737,7 +737,7 @@ vdi_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE; logout("will write %u block map sectors starting from entry %u\n", n_sectors, bmap_first); - ret = bdrv_write(bs->file->bs, offset, base, n_sectors); + ret = bdrv_write(bs->file, offset, base, n_sectors); } return ret; diff --git a/block/vvfat.c b/block/vvfat.c index 5f980bb..c3f24c6 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1834,8 +1834,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s, if (res) { return -1; } - res = bdrv_write(s->qcow->bs, offset, - s->cluster_buffer, 1); + res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1); if (res) { return -2; } @@ -2889,7 +2888,7 @@ DLOG(checkpoint()); * Use qcow backend. Commit later. */ DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors)); - ret = bdrv_write(s->qcow->bs, sector_num, buf, nb_sectors); + ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors); if (ret < 0) { fprintf(stderr, "Error writing to qcow backend\n"); return ret; diff --git a/include/block/block.h b/include/block/block.h index b6744ab..ea17936 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -228,7 +228,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state); void bdrv_reopen_abort(BDRVReopenState *reopen_state); int bdrv_read(BdrvChild *child, int64_t sector_num, uint8_t *buf, int nb_sectors); -int bdrv_write(BlockDriverState *bs, int64_t sector_num, +int bdrv_write(BdrvChild *child, int64_t sector_num, const uint8_t *buf, int nb_sectors); int bdrv_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int count, BdrvRequestFlags flags);