| Message ID | 1464686130-12265-2-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 31, 2016 at 12:15:20PM +0300, Denis V. Lunev wrote: > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > This is a preparatory patch, which continues the general trend of the transition > to the byte-based interfaces. > > 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> > --- > block/block-backend.c | 8 ++++---- > block/io.c | 9 +++++---- > include/block/block.h | 4 ++-- > include/sysemu/block-backend.h | 4 ++-- > qemu-img.c | 6 ++++-- > qemu-io-cmds.c | 2 +- > 6 files changed, 18 insertions(+), 15 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 05/31/2016 03:15 AM, Denis V. Lunev wrote: > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > This is a preparatory patch, which continues the general trend of the transition > to the byte-based interfaces. > > 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> > --- > block/block-backend.c | 8 ++++---- > block/io.c | 9 +++++---- > include/block/block.h | 4 ++-- > include/sysemu/block-backend.h | 4 ++-- > qemu-img.c | 6 ++++-- > qemu-io-cmds.c | 2 +- > 6 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 34500e6..3c1fc50 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, > flags | BDRV_REQ_ZERO_WRITE); > } > > -int blk_write_compressed(BlockBackend *blk, int64_t sector_num, > - const uint8_t *buf, int nb_sectors) > +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, > + int count) Why are you switching the type of buf? It's not necessarily wrong, but the commit message should call it out as intentional. > -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, > - const uint8_t *buf, int nb_sectors) > +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, > + const void *buf, int count) > { > BlockDriver *drv = bs->drv; > int ret; > @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, > if (!drv->bdrv_write_compressed) { > return -ENOTSUP; > } > - ret = bdrv_check_request(bs, sector_num, nb_sectors); > + ret = bdrv_check_byte_request(bs, offset, count); > 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, offset >> BDRV_SECTOR_BITS, buf, > + count >> BDRV_SECTOR_BITS); If you are going to shift right, you need to first assert that offset and count are aligned (and thus that our call to a sector interface isn't going to operate on the wrong data). See for example commit 166fe960.
On 13.06.2016 17:23, Eric Blake wrote: > On 05/31/2016 03:15 AM, Denis V. Lunev wrote: >> From: Pavel Butsykin <pbutsykin@virtuozzo.com> >> >> This is a preparatory patch, which continues the general trend of the transition >> to the byte-based interfaces. >> >> 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> >> --- >> block/block-backend.c | 8 ++++---- >> block/io.c | 9 +++++---- >> include/block/block.h | 4 ++-- >> include/sysemu/block-backend.h | 4 ++-- >> qemu-img.c | 6 ++++-- >> qemu-io-cmds.c | 2 +- >> 6 files changed, 18 insertions(+), 15 deletions(-) >> >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 34500e6..3c1fc50 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, >> flags | BDRV_REQ_ZERO_WRITE); >> } >> >> -int blk_write_compressed(BlockBackend *blk, int64_t sector_num, >> - const uint8_t *buf, int nb_sectors) >> +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, >> + int count) > > Why are you switching the type of buf? It's not necessarily wrong, but > the commit message should call it out as intentional. Here I just tried to make the interface like blk_pwrite, it has no other meaning more.. > >> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, >> - const uint8_t *buf, int nb_sectors) >> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, >> + const void *buf, int count) >> { >> BlockDriver *drv = bs->drv; >> int ret; >> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, >> if (!drv->bdrv_write_compressed) { >> return -ENOTSUP; >> } >> - ret = bdrv_check_request(bs, sector_num, nb_sectors); >> + ret = bdrv_check_byte_request(bs, offset, count); >> 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, offset >> BDRV_SECTOR_BITS, buf, >> + count >> BDRV_SECTOR_BITS); > > If you are going to shift right, you need to first assert that offset > and count are aligned (and thus that our call to a sector interface > isn't going to operate on the wrong data). See for example commit 166fe960. > ok, thanks
Am 13.06.2016 um 16:23 hat Eric Blake geschrieben: > On 05/31/2016 03:15 AM, Denis V. Lunev wrote: > > From: Pavel Butsykin <pbutsykin@virtuozzo.com> > > > > This is a preparatory patch, which continues the general trend of the transition > > to the byte-based interfaces. > > > > 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> > > -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, > > - const uint8_t *buf, int nb_sectors) > > +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, > > + const void *buf, int count) > > { > > BlockDriver *drv = bs->drv; > > int ret; > > @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, > > if (!drv->bdrv_write_compressed) { > > return -ENOTSUP; > > } > > - ret = bdrv_check_request(bs, sector_num, nb_sectors); > > + ret = bdrv_check_byte_request(bs, offset, count); > > 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, offset >> BDRV_SECTOR_BITS, buf, > > + count >> BDRV_SECTOR_BITS); > > If you are going to shift right, you need to first assert that offset > and count are aligned (and thus that our call to a sector interface > isn't going to operate on the wrong data). See for example commit 166fe960. Yes, I would like to have these assertions at least. But I'm wondering what the point of converting the interface is when we don't intend to actually support sub-sector requests and the sector alignment is still required at the end of the series. Kevin
On 28.06.2016 13:53, Kevin Wolf wrote: > Am 13.06.2016 um 16:23 hat Eric Blake geschrieben: >> On 05/31/2016 03:15 AM, Denis V. Lunev wrote: >>> From: Pavel Butsykin <pbutsykin@virtuozzo.com> >>> >>> This is a preparatory patch, which continues the general trend of the transition >>> to the byte-based interfaces. >>> >>> 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> > >>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, >>> - const uint8_t *buf, int nb_sectors) >>> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, >>> + const void *buf, int count) >>> { >>> BlockDriver *drv = bs->drv; >>> int ret; >>> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, >>> if (!drv->bdrv_write_compressed) { >>> return -ENOTSUP; >>> } >>> - ret = bdrv_check_request(bs, sector_num, nb_sectors); >>> + ret = bdrv_check_byte_request(bs, offset, count); >>> 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, offset >> BDRV_SECTOR_BITS, buf, >>> + count >> BDRV_SECTOR_BITS); >> >> If you are going to shift right, you need to first assert that offset >> and count are aligned (and thus that our call to a sector interface >> isn't going to operate on the wrong data). See for example commit 166fe960. > > Yes, I would like to have these assertions at least. > > But I'm wondering what the point of converting the interface is when we > don't intend to actually support sub-sector requests and the sector > alignment is still required at the end of the series. Because at the time of sending patches, the format drivers still had the sector-based interfaces. In the end, the assertions are not necessary, because the interface format driver now also byte-based. > Kevin >
diff --git a/block/block-backend.c b/block/block-backend.c index 34500e6..3c1fc50 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, flags | BDRV_REQ_ZERO_WRITE); } -int blk_write_compressed(BlockBackend *blk, int64_t sector_num, - const uint8_t *buf, int nb_sectors) +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, + int count) { - int ret = blk_check_request(blk, sector_num, nb_sectors); + int ret = blk_check_byte_request(blk, offset, count); if (ret < 0) { return ret; } - return bdrv_write_compressed(blk_bs(blk), sector_num, buf, nb_sectors); + return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count); } int blk_truncate(BlockBackend *blk, int64_t offset) diff --git a/block/io.c b/block/io.c index 2d832aa..c5bb6ae 100644 --- a/block/io.c +++ b/block/io.c @@ -1779,8 +1779,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_pwrite_compressed(BlockDriverState *bs, int64_t offset, + const void *buf, int count) { BlockDriver *drv = bs->drv; int ret; @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, if (!drv->bdrv_write_compressed) { return -ENOTSUP; } - ret = bdrv_check_request(bs, sector_num, nb_sectors); + ret = bdrv_check_byte_request(bs, offset, count); 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, offset >> BDRV_SECTOR_BITS, buf, + count >> BDRV_SECTOR_BITS); } int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, diff --git a/include/block/block.h b/include/block/block.h index 70ea299..b687c0d 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -421,8 +421,8 @@ 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_write_compressed(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors); +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 c04af8e..57df069 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -203,8 +203,8 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk, BlockCompletionFunc *cb, void *opaque); int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, int count, BdrvRequestFlags flags); -int blk_write_compressed(BlockBackend *blk, int64_t sector_num, - const uint8_t *buf, int nb_sectors); +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, + int count); 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, diff --git a/qemu-img.c b/qemu-img.c index 4b56ad3..eb744d4 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1580,7 +1580,9 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors, break; } - ret = blk_write_compressed(s->target, sector_num, buf, n); + ret = blk_pwrite_compressed(s->target, + sector_num << BDRV_SECTOR_BITS, + buf, n << BDRV_SECTOR_BITS); if (ret < 0) { return ret; } @@ -1717,7 +1719,7 @@ static int convert_do_copy(ImgConvertState *s) if (s->compressed) { /* signal EOF to align */ - ret = blk_write_compressed(s->target, 0, NULL, 0); + ret = blk_pwrite_compressed(s->target, 0, NULL, 0); if (ret < 0) { goto fail; } diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 09e879f..2ae3b14 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -504,7 +504,7 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset, return -ERANGE; } - ret = blk_write_compressed(blk, offset >> 9, (uint8_t *)buf, count >> 9); + ret = blk_pwrite_compressed(blk, offset, buf, count); if (ret < 0) { return ret; }