| Message ID | 1465574722-27656-6-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/10/2016 10:05 AM, Kevin Wolf wrote: > We don't really want to go through the block layer in order to read from > or write to the vmstate in a qcow2 image. Doing so required a few ugly > hacks like saving and restoring the old image size (because writing to > vmstate offsets would increase the image size) or disabling the "reads > after EOF = zeroes" logic. When calling the right functions directly, > these hacks aren't necessary any more. > > Note that .bdrv_vmstate_load/save() return 0 instead of the number of > bytes in case of success now. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qcow2.c | 24 ++++-------------------- > 1 file changed, 4 insertions(+), 20 deletions(-) > > +++ b/block/qcow2.c > @@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, > int64_t pos) > { > BDRVQcow2State *s = bs->opaque; > - int64_t total_sectors = bs->total_sectors; > - bool zero_beyond_eof = bs->zero_beyond_eof; > - int ret; > > BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE); > - bs->zero_beyond_eof = false; > - ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov); > - bs->zero_beyond_eof = zero_beyond_eof; > - > - /* bdrv_co_do_writev will have increased the total_sectors value to include > - * the VM state - the VM state is however not an actual part of the block > - * device, therefore, we need to restore the old value. */ > - bs->total_sectors = total_sectors; > - > - return ret; > + return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos, > + qiov->size, qiov, 0); > } bs->drv->bdrv_co_pwritev() is an optional interface; not all the drivers have it yet. Should you be asserting that it exists, and/or returning an error if it does not? > > static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, > int64_t pos) > { > BDRVQcow2State *s = bs->opaque; > - bool zero_beyond_eof = bs->zero_beyond_eof; > - int ret; > > BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD); > - bs->zero_beyond_eof = false; > - ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov); > - bs->zero_beyond_eof = zero_beyond_eof; > - > - return ret; > + return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos, > + qiov->size, qiov, 0); Ditto.
On Fri, 06/10 16:39, Eric Blake wrote: > > +++ b/block/qcow2.c > > @@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, > > int64_t pos) > > { > > BDRVQcow2State *s = bs->opaque; > > - int64_t total_sectors = bs->total_sectors; > > - bool zero_beyond_eof = bs->zero_beyond_eof; > > - int ret; > > > > BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE); > > - bs->zero_beyond_eof = false; > > - ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov); > > - bs->zero_beyond_eof = zero_beyond_eof; > > - > > - /* bdrv_co_do_writev will have increased the total_sectors value to include > > - * the VM state - the VM state is however not an actual part of the block > > - * device, therefore, we need to restore the old value. */ > > - bs->total_sectors = total_sectors; > > - > > - return ret; > > + return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos, > > + qiov->size, qiov, 0); > > } > > bs->drv->bdrv_co_pwritev() is an optional interface; not all the drivers > have it yet. Should you be asserting that it exists, and/or returning > an error if it does not? bs->drv is always qcow2 driver here so this is correct. > > > > > static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, > > int64_t pos) > > { > > BDRVQcow2State *s = bs->opaque; > > - bool zero_beyond_eof = bs->zero_beyond_eof; > > - int ret; > > > > BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD); > > - bs->zero_beyond_eof = false; > > - ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov); > > - bs->zero_beyond_eof = zero_beyond_eof; > > - > > - return ret; > > + return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos, > > + qiov->size, qiov, 0); > > Ditto. > Fam
On 06/11/2016 08:58 PM, Fam Zheng wrote: >>> - return ret; >>> + return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos, >>> + qiov->size, qiov, 0); >>> } >> >> bs->drv->bdrv_co_pwritev() is an optional interface; not all the drivers >> have it yet. Should you be asserting that it exists, and/or returning >> an error if it does not? > > bs->drv is always qcow2 driver here so this is correct. In that case, Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/block/qcow2.c b/block/qcow2.c index 72ae2bf..c40baca 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { BDRVQcow2State *s = bs->opaque; - int64_t total_sectors = bs->total_sectors; - bool zero_beyond_eof = bs->zero_beyond_eof; - int ret; BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE); - bs->zero_beyond_eof = false; - ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov); - bs->zero_beyond_eof = zero_beyond_eof; - - /* bdrv_co_do_writev will have increased the total_sectors value to include - * the VM state - the VM state is however not an actual part of the block - * device, therefore, we need to restore the old value. */ - bs->total_sectors = total_sectors; - - return ret; + return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos, + qiov->size, qiov, 0); } static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { BDRVQcow2State *s = bs->opaque; - bool zero_beyond_eof = bs->zero_beyond_eof; - int ret; BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD); - bs->zero_beyond_eof = false; - ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov); - bs->zero_beyond_eof = zero_beyond_eof; - - return ret; + return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos, + qiov->size, qiov, 0); } /*
We don't really want to go through the block layer in order to read from or write to the vmstate in a qcow2 image. Doing so required a few ugly hacks like saving and restoring the old image size (because writing to vmstate offsets would increase the image size) or disabling the "reads after EOF = zeroes" logic. When calling the right functions directly, these hacks aren't necessary any more. Note that .bdrv_vmstate_load/save() return 0 instead of the number of bytes in case of success now. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-)