block: change default of .has_zero_init to 0
PATCH 1/1 block: change default of .has_zero_init to 0
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after bdrv_create().
if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.
during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support host_devices as backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.
a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)
block.c +7 -1
@@ -2916,6 +2916,11 @@ void bdrv_flush_all(void) } } +int bdrv_has_zero_init_1 (BlockDriverState *bs) +{ +return 1 ; +} +int bdrv_has_zero_init (BlockDriverState *bs) {assert (bs->drv);
@@ -2924,7 +2929,8 @@ int bdrv_has_zero_init(BlockDriverState *bs)return bs->drv->bdrv_has_zero_init (bs); } -return 1 ; +/* save default */ +return 0 ; } typedef struct BdrvCoIsAllocatedData {
block/qcow.c +1 -0
@@ -892,6 +892,7 @@ static BlockDriver bdrv_qcow = { .bdrv_close = qcow_close, .bdrv_reopen_prepare = qcow_reopen_prepare, .bdrv_create = qcow_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_readv = qcow_co_readv, .bdrv_co_writev = qcow_co_writev,
block/qcow2.c +1 -0
@@ -1785,6 +1785,7 @@ static BlockDriver bdrv_qcow2 = { .bdrv_close = qcow2_close, .bdrv_reopen_prepare = qcow2_reopen_prepare, .bdrv_create = qcow2_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_is_allocated = qcow2_co_is_allocated, .bdrv_set_key = qcow2_set_key, .bdrv_make_empty = qcow2_make_empty,
block/qed.c +1 -0
@@ -1574,6 +1574,7 @@ static BlockDriver bdrv_qed = { .bdrv_close = bdrv_qed_close, .bdrv_reopen_prepare = bdrv_qed_reopen_prepare, .bdrv_create = bdrv_qed_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_is_allocated = bdrv_qed_co_is_allocated, .bdrv_make_empty = bdrv_qed_make_empty, .bdrv_aio_readv = bdrv_qed_aio_readv,
block/raw-posix.c +1 -9
@@ -1199,6 +1199,7 @@ static BlockDriver bdrv_file = { .bdrv_reopen_abort = raw_reopen_abort, .bdrv_close = raw_close, .bdrv_create = raw_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_is_allocated = raw_co_is_allocated, .bdrv_aio_readv = raw_aio_readv,
@@ -1526,11 +1527,6 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)return ret; } -static int hdev_has_zero_init (BlockDriverState *bs) -{ -return 0 ; -} -static BlockDriver bdrv_host_device = { .format_name ="host_device" , .protocol_name ="host_device" ,
@@ -1543,7 +1539,6 @@ static BlockDriver bdrv_host_device = { .bdrv_reopen_abort = raw_reopen_abort, .bdrv_create = hdev_create, .create_options = raw_create_options, - .bdrv_has_zero_init = hdev_has_zero_init, .bdrv_aio_readv = raw_aio_readv, .bdrv_aio_writev = raw_aio_writev,
@@ -1668,7 +1663,6 @@ static BlockDriver bdrv_host_floppy = { .bdrv_reopen_abort = raw_reopen_abort, .bdrv_create = hdev_create, .create_options = raw_create_options, - .bdrv_has_zero_init = hdev_has_zero_init, .bdrv_aio_readv = raw_aio_readv, .bdrv_aio_writev = raw_aio_writev,
@@ -1770,7 +1764,6 @@ static BlockDriver bdrv_host_cdrom = { .bdrv_reopen_abort = raw_reopen_abort, .bdrv_create = hdev_create, .create_options = raw_create_options, - .bdrv_has_zero_init = hdev_has_zero_init, .bdrv_aio_readv = raw_aio_readv, .bdrv_aio_writev = raw_aio_writev,
@@ -1892,7 +1885,6 @@ static BlockDriver bdrv_host_cdrom = { .bdrv_reopen_abort = raw_reopen_abort, .bdrv_create = hdev_create, .create_options = raw_create_options, - .bdrv_has_zero_init = hdev_has_zero_init, .bdrv_aio_readv = raw_aio_readv, .bdrv_aio_writev = raw_aio_writev,
block/raw-win32.c +1 -6
@@ -459,6 +459,7 @@ static BlockDriver bdrv_file = { .bdrv_file_open = raw_open, .bdrv_close = raw_close, .bdrv_create = raw_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_aio_readv = raw_aio_readv, .bdrv_aio_writev = raw_aio_writev,
@@ -570,11 +571,6 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags)return 0 ; } -static int hdev_has_zero_init (BlockDriverState *bs) -{ -return 0 ; -} -static BlockDriver bdrv_host_device = { .format_name ="host_device" , .protocol_name ="host_device" ,
@@ -582,7 +578,6 @@ static BlockDriver bdrv_host_device = { .bdrv_probe_device = hdev_probe_device, .bdrv_file_open = hdev_open, .bdrv_close = raw_close, - .bdrv_has_zero_init = hdev_has_zero_init, .bdrv_aio_readv = raw_aio_readv, .bdrv_aio_writev = raw_aio_writev,
block/rbd.c +1 -0
@@ -996,6 +996,7 @@ static BlockDriver bdrv_rbd = { .bdrv_file_open = qemu_rbd_open, .bdrv_close = qemu_rbd_close, .bdrv_create = qemu_rbd_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_get_info = qemu_rbd_getinfo, .create_options = qemu_rbd_create_options, .bdrv_getlength = qemu_rbd_getlength,
block/sheepdog.c +1 -0
@@ -2401,6 +2401,7 @@ static BlockDriver bdrv_sheepdog_unix = { .bdrv_file_open = sd_open, .bdrv_close = sd_close, .bdrv_create = sd_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, .bdrv_truncate = sd_truncate,
block/vdi.c +1 -0
@@ -779,6 +779,7 @@ static BlockDriver bdrv_vdi = { .bdrv_close = vdi_close, .bdrv_reopen_prepare = vdi_reopen_prepare, .bdrv_create = vdi_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_is_allocated = vdi_co_is_allocated, .bdrv_make_empty = vdi_make_empty,
block/vmdk.c +1 -0
@@ -1814,6 +1814,7 @@ static BlockDriver bdrv_vmdk = { .bdrv_co_write_zeroes = vmdk_co_write_zeroes, .bdrv_close = vmdk_close, .bdrv_create = vmdk_create, + .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_flush_to_disk = vmdk_co_flush, .bdrv_co_is_allocated = vmdk_co_is_allocated, .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
include/block/block.h +1 -0
@@ -272,6 +272,7 @@ void bdrv_drain_all(void);int bdrv_discard (BlockDriverState *bs,int64_t sector_num,int nb_sectors);int bdrv_co_discard (BlockDriverState *bs,int64_t sector_num,int nb_sectors); +int bdrv_has_zero_init_1 (BlockDriverState *bs);int bdrv_has_zero_init (BlockDriverState *bs);int bdrv_is_allocated (BlockDriverState *bs,int64_t sector_num,int nb_sectors,int *pnum);
Signature · 1 line
1.7.9.5
Quoted 1 line · Peter Lieven
+ /* save default */
A minor thing, but s/save/safe/ :-)
Rich.
Signature · 4 lines
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Quoted 17 lines · Peter Lieven
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.
Quoted 2 lines
a similar problem with the wrong default existed for iscsi mose likely because the driver developer did
s/mose/most/
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
Quoted 22 lines
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
Signed-off-by Peter Lieven <[email protected]>
Quoted 13 lines
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)
You forgot cow, which is also a simple case that can be handled in this patch.
vpc is still easy, but a bit more complicated than a constant return 1, because it depends on the subformat whether a new image will inherit the has_zero_init property from the underlying storage or whether it always produces zeros (for VHD_FIXED type images, it's basically raw + footer). I'll send a separate patch for this.
A similar situation exists for vmdk, I think, just that the difference can be per extent there. I guess we need to return 0 if one of the extents is flat and has an underlying storage returning 0. I'll leave this part to Fam. For now, please remove the bdrv_has_zero_init_1 for vmdk from this patch as it's unsafe.
Kevin
Quoted 36 lines · Kevin Wolf
Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
Quoted 22 lines
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
Signed-off-by Peter Lieven <[email protected]>Quoted 13 lines
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)
Quoted 2 lines
You forgot cow, which is also a simple case that can be handled in this patch.
ups.
Quoted 6 lines
vpc is still easy, but a bit more complicated than a constant return 1, because it depends on the subformat whether a new image will inherit the
has_zero_initproperty from the underlying storage or whether it always produces zeros (forVHD_FIXEDtype images, it's basically raw + footer). I'll send a separate patch for this.
shall I leave this to the new 0 default until your patch is ready?
Quoted 6 lines
A similar situation exists for vmdk, I think, just that the difference can be per extent there. I guess we need to return 0 if one of the extents is flat and has an underlying storage returning 0. I'll leave this part to Fam. For now, please remove the
bdrv_has_zero_init_1for vmdk from this patch as it's unsafe.
ok I will mention that the value is changed for vmdk (and vpc) in the commit message.
thanks
peter
Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
Quoted 39 lines · Kevin Wolf
Quoted 36 lines
Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
Quoted 22 lines
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
Signed-off-by Peter Lieven <[email protected]>Quoted 13 lines
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)Quoted 2 lines
You forgot cow, which is also a simple case that can be handled in this patch.
Quoted 8 lines
ups.
Quoted 6 lines
vpc is still easy, but a bit more complicated than a constant return 1, because it depends on the subformat whether a new image will inherit the
has_zero_initproperty from the underlying storage or whether it always produces zeros (forVHD_FIXEDtype images, it's basically raw + footer). I'll send a separate patch for this.shall I leave this to the new 0 default until your patch is ready?
Yes, please leave vpc alone. I guess my patch will be queued before yours anyway. ;-)
Quoted 7 lines
Quoted 5 lines
A similar situation exists for vmdk, I think, just that the difference can be per extent there. I guess we need to return 0 if one of the extents is flat and has an underlying storage returning 0. I'll leave this part to Fam. For now, please remove the
bdrv_has_zero_init_1for vmdk from this patch as it's unsafe.ok I will mention that the value is changed for vmdk (and vpc) in the commit message.
Good idea for vmdk. For vpc, don't mention it, because see above.
Kevin
Quoted 41 lines · Kevin Wolf
Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
Quoted 40 lines · Kevin Wolf
Quoted 36 lines
Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
Quoted 22 lines
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
Signed-off-by Peter Lieven <[email protected]>Quoted 13 lines
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)Quoted 2 lines
You forgot cow, which is also a simple case that can be handled in this patch.
ups.
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
vpc is still easy, but a bit more complicated than a constant return 1, because it depends on the subformat whether a new image will inherit the
has_zero_initproperty from the underlying storage or whether it always produces zeros (forVHD_FIXEDtype images, it's basically raw + footer). I'll send a separate patch for this.shall I leave this to the new 0 default until your patch is ready?
Yes, please leave vpc alone. I guess my patch will be queued before yours anyway. ;-)
once v2 is ready and noone has objections maybe you can put it in your queue? ;-)
Peter
Am 28.06.2013 um 10:25 hat Peter Lieven geschrieben:
Quoted 47 lines · Kevin Wolf
Quoted 46 lines
Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
Quoted 45 lines · Kevin Wolf
Quoted 36 lines
Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
Quoted 22 lines
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
Signed-off-by Peter Lieven <[email protected]>Quoted 13 lines
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)Quoted 2 lines
You forgot cow, which is also a simple case that can be handled in this patch.
ups.
Quoted 5 lines
vpc is still easy, but a bit more complicated than a constant return 1, because it depends on the subformat whether a new image will inherit the
has_zero_initproperty from the underlying storage or whether it always produces zeros (forVHD_FIXEDtype images, it's basically raw + footer). I'll send a separate patch for this.
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
shall I leave this to the new 0 default until your patch is ready?
Yes, please leave vpc alone. I guess my patch will be queued before yours anyway. ;-)
once v2 is ready and noone has objections maybe you can put it in your queue? ;-)
Sure. I'm going to send a pull request today, so it would be good to have v2 of this patch soon. (Otherwise it would have to wait for Stefan's pull request probably next Friday)
Kevin
Quoted 48 lines · Kevin Wolf
Am 28.06.2013 um 10:25 hat Peter Lieven geschrieben:
Quoted 47 lines · Kevin Wolf
Quoted 46 lines
Am 28.06.2013 um 10:16 hat Peter Lieven geschrieben:
Quoted 45 lines · Kevin Wolf
Quoted 36 lines
Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
Quoted 22 lines
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
Signed-off-by Peter Lieven <[email protected]>Quoted 13 lines
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)Quoted 2 lines
You forgot cow, which is also a simple case that can be handled in this patch.
ups.
Quoted 5 lines
vpc is still easy, but a bit more complicated than a constant return 1, because it depends on the subformat whether a new image will inherit the
has_zero_initproperty from the underlying storage or whether it always produces zeros (forVHD_FIXEDtype images, it's basically raw + footer). I'll send a separate patch for this.
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
shall I leave this to the new 0 default until your patch is ready?
Yes, please leave vpc alone. I guess my patch will be queued before yours anyway. ;-)
once v2 is ready and noone has objections maybe you can put it in your queue? ;-)
Sure. I'm going to send a pull request today, so it would be good to have v2 of this patch soon. (Otherwise it would have to wait for Stefan's pull request probably next Friday)
expect it in the next 60 minutes.
Peter
Quoted 36 lines · Kevin Wolf
Am 27.06.2013 um 15:52 hat Peter Lieven geschrieben:
Quoted 22 lines
.has_zero_init defaults to 1 for all formats and protocols.
this is a dangerous default since this means that all new added drivers need to manually overwrite it to 0 if they do not ensure that a device is zero initialized after
bdrv_create().if a driver needs to explicitly set this value to
1 its easier to verify the correctness in the review process.during review of the existing drivers it turned out that ssh and gluster had a wrong default of 1. both protocols support
host_devicesas backend which are not by default zero initialized. this wrong assumption will lead to possible corruption if qemu-img convert is used to write to such a backend.a similar problem with the wrong default existed for iscsi mose likely because the driver developer did oversee the default value of 1.
Signed-off-by Peter Lieven <[email protected]>Quoted 13 lines
--- block.c | 8 +++++++- block/qcow.c | 1 + block/qcow2.c | 1 + block/qed.c | 1 + block/raw-posix.c | 10 +--------- block/raw-win32.c | 7 +------ block/rbd.c | 1 + block/sheepdog.c | 1 + block/vdi.c | 1 + block/vmdk.c | 1 + include/block/block.h | 1 + 11 files changed, 17 insertions(+), 16 deletions(-)
Quoted 2 lines
You forgot cow, which is also a simple case that can be handled in this patch.
Do we need to check the return value of bdrv_has_zero_init(bs->backing_hd) in case bs-backing_hd != NULL?