| Message ID | 1464686130-12265-8-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote: > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index 57df069..3d7b446 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, > int count, BdrvRequestFlags flags); > int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, > int count); > +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset, > + unsigned int bytes, > + QEMUIOVector *qiov); Perhaps blk_co_pwritev_compressed() isn't necessary at all since blk_co_pwritev() already exists and has the flags argument: int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags);
On 06/13/2016 07:11 AM, Stefan Hajnoczi wrote: > On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote: >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h >> index 57df069..3d7b446 100644 >> --- a/include/sysemu/block-backend.h >> +++ b/include/sysemu/block-backend.h >> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, >> int count, BdrvRequestFlags flags); >> int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, >> int count); >> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset, >> + unsigned int bytes, >> + QEMUIOVector *qiov); > > Perhaps blk_co_pwritev_compressed() isn't necessary at all since > blk_co_pwritev() already exists and has the flags argument: > > int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, > unsigned int bytes, QEMUIOVector *qiov, > BdrvRequestFlags flags); Are you arguing that we should have a new BDRV_REQ_COMPRESSED flag that can be set in .supported_write_flags for drivers that know how to do a compressed write?
On Mon, Jun 13, 2016 at 02:16:08PM -0600, Eric Blake wrote: > On 06/13/2016 07:11 AM, Stefan Hajnoczi wrote: > > On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote: > >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > >> index 57df069..3d7b446 100644 > >> --- a/include/sysemu/block-backend.h > >> +++ b/include/sysemu/block-backend.h > >> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, > >> int count, BdrvRequestFlags flags); > >> int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, > >> int count); > >> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset, > >> + unsigned int bytes, > >> + QEMUIOVector *qiov); > > > > Perhaps blk_co_pwritev_compressed() isn't necessary at all since > > blk_co_pwritev() already exists and has the flags argument: > > > > int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, > > unsigned int bytes, QEMUIOVector *qiov, > > BdrvRequestFlags flags); > > Are you arguing that we should have a new BDRV_REQ_COMPRESSED flag that > can be set in .supported_write_flags for drivers that know how to do a > compressed write? Never mind, it's too much noise and out of scope for this series. I'm fine with blk_co_pwritev_compressed(). Stefan
On 05/31/2016 03:15 AM, Denis V. Lunev wrote: > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > For bdrv_pwrite_compressed() it looks like most of the code creating coroutine > is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED) > and use the blk_prw() as a generic one. > > 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/include/block/block.h > @@ -65,6 +65,7 @@ typedef enum { > BDRV_REQ_MAY_UNMAP = 0x4, > BDRV_REQ_NO_SERIALISING = 0x8, > BDRV_REQ_FUA = 0x10, > + BDRV_REQ_WRITE_COMPRESSED = 0x20, > } BdrvRequestFlags; > Needs to be rebased on top of commit fa166538, and adjust BDRV_REQ_MASK at that time.
Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben: > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > For bdrv_pwrite_compressed() it looks like most of the code creating coroutine > is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED) > and use the blk_prw() as a generic one. > > 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> Oh, so you already do use a flag. Nice. :-) > diff --git a/block/block-backend.c b/block/block-backend.c > index 3c1fc50..9e1c793 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, > flags |= BDRV_REQ_FUA; > } > > - return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); > + if (flags & BDRV_REQ_WRITE_COMPRESSED) { > + return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov); > + } else { > + return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); > + } > } If you move the processing of the flag inside bdrv_co_pwritev(), where I think it belongs anyway, you could use the flag from the start (by going through bdrv_prwv_co()) instead of temporarily introducing your own coroutine wrapper. I think that would make the initial conversion patches quite a bit simpler. Kevin
On 28.06.2016 14:47, Kevin Wolf wrote: > Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben: >> From: Pavel Butsykin <pbutsykin@virtuozzo.com> >> >> For bdrv_pwrite_compressed() it looks like most of the code creating coroutine >> is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED) >> and use the blk_prw() as a generic one. >> >> 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> > > Oh, so you already do use a flag. Nice. :-) > >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 3c1fc50..9e1c793 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, >> flags |= BDRV_REQ_FUA; >> } >> >> - return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); >> + if (flags & BDRV_REQ_WRITE_COMPRESSED) { >> + return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov); >> + } else { >> + return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); >> + } >> } > > If you move the processing of the flag inside bdrv_co_pwritev(), where I > think it belongs anyway, you could use the flag from the start (by going > through bdrv_prwv_co()) instead of temporarily introducing your own > coroutine wrapper. I think that would make the initial conversion > patches quite a bit simpler. You propose to add bdrv_driver_compressed and call it from bdrv_aligned_pwritev ? > Kevin >
Am 28.06.2016 um 14:32 hat Pavel Butsykin geschrieben: > On 28.06.2016 14:47, Kevin Wolf wrote: > >Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben: > >>From: Pavel Butsykin <pbutsykin@virtuozzo.com> > >> > >>For bdrv_pwrite_compressed() it looks like most of the code creating coroutine > >>is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED) > >>and use the blk_prw() as a generic one. > >> > >>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> > > > >Oh, so you already do use a flag. Nice. :-) > > > >>diff --git a/block/block-backend.c b/block/block-backend.c > >>index 3c1fc50..9e1c793 100644 > >>--- a/block/block-backend.c > >>+++ b/block/block-backend.c > >>@@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, > >> flags |= BDRV_REQ_FUA; > >> } > >> > >>- return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); > >>+ if (flags & BDRV_REQ_WRITE_COMPRESSED) { > >>+ return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov); > >>+ } else { > >>+ return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); > >>+ } > >> } > > > >If you move the processing of the flag inside bdrv_co_pwritev(), where I > >think it belongs anyway, you could use the flag from the start (by going > >through bdrv_prwv_co()) instead of temporarily introducing your own > >coroutine wrapper. I think that would make the initial conversion > >patches quite a bit simpler. > > You propose to add bdrv_driver_compressed and call it from > bdrv_aligned_pwritev ? I'm not sure if we can easily move it that much down the caller chain. If we can, great. But here I was just thinking of making the switch at the start of bdrv_co_pwritev() so that you can reuse the existing wrappers like bdrv_prwv_co(). The long-term advantage of putting it into bdrv_aligned_pwritev() could be that we would ge the common alignment handling. But I think qcow2 still errors out if you rewrite an already allocated cluster with qcow2_pwritev_compressed(), so currently doing sub-cluster (or even sub-sector) writes doesn't make much sense anyway. So I doubt it's worth the hassle now. Kevin
diff --git a/block/block-backend.c b/block/block-backend.c index 3c1fc50..9e1c793 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset, flags |= BDRV_REQ_FUA; } - return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); + if (flags & BDRV_REQ_WRITE_COMPRESSED) { + return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov); + } else { + return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags); + } } typedef struct BlkRwCo { @@ -1480,12 +1484,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, int count) { - int ret = blk_check_byte_request(blk, offset, count); - if (ret < 0) { - return ret; - } + return blk_prw(blk, offset, (void *) buf, count, blk_write_entry, + BDRV_REQ_WRITE_COMPRESSED); +} - return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count); +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset, + unsigned int bytes, + QEMUIOVector *qiov) +{ + return blk_co_pwritev(blk, offset, bytes, qiov, BDRV_REQ_WRITE_COMPRESSED); } int blk_truncate(BlockBackend *blk, int64_t offset) diff --git a/block/io.c b/block/io.c index 4b06de8..29f5962 100644 --- a/block/io.c +++ b/block/io.c @@ -1805,54 +1805,6 @@ int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs, bytes >> BDRV_SECTOR_BITS, qiov); } -typedef struct BdrvWriteCompressedCo { - BlockDriverState *bs; - int64_t offset; - QEMUIOVector *qiov; - int ret; -} BdrvWriteCompressedCo; - -static void bdrv_write_compressed_co_entry(void *opaque) -{ - BdrvWriteCompressedCo *co = opaque; - - co->ret = bdrv_co_pwritev_compressed(co->bs, co->offset, co->qiov->size, - co->qiov); -} - -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, - const void *buf, int count) -{ - BdrvWriteCompressedCo data; - QEMUIOVector qiov; - struct iovec iov = { - .iov_base = (void *)buf, - .iov_len = count, - }; - qemu_iovec_init_external(&qiov, &iov, 1); - - data = (BdrvWriteCompressedCo) { - .bs = bs, - .offset = offset, - .qiov = &qiov, - .ret = -EINPROGRESS, - }; - - 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, int64_t pos, int size) { diff --git a/include/block/block.h b/include/block/block.h index b687c0d..56fcab6 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -65,6 +65,7 @@ typedef enum { BDRV_REQ_MAY_UNMAP = 0x4, BDRV_REQ_NO_SERIALISING = 0x8, BDRV_REQ_FUA = 0x10, + BDRV_REQ_WRITE_COMPRESSED = 0x20, } BdrvRequestFlags; typedef struct BlockSizes { @@ -421,8 +422,6 @@ const char *bdrv_get_node_name(const BlockDriverState *bs); const char *bdrv_get_device_name(const BlockDriverState *bs); const char *bdrv_get_device_or_node_name(const BlockDriverState *bs); int bdrv_get_flags(BlockDriverState *bs); -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, - const void *buf, int count); 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/sysemu/block-backend.h b/include/sysemu/block-backend.h index 57df069..3d7b446 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, int count, BdrvRequestFlags flags); int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, int count); +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset, + unsigned int bytes, + QEMUIOVector *qiov); int blk_truncate(BlockBackend *blk, int64_t offset); int blk_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors); int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,