* [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file @ 2013-08-05 8:11 Asias He 2013-08-05 12:41 ` Kevin Wolf 2013-08-16 8:41 ` [Qemu-devel] [PATCH] " Stefan Hajnoczi 0 siblings, 2 replies; 11+ messages in thread From: Asias He @ 2013-08-05 8:11 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, MORITA Kazutaka, Stefan Hajnoczi From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> While Asias is debugging an issue creating qcow2 images on top of non-file protocols. It boils down to this example using NBD: $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' Notice the open -g option to set bs->growable. This means you can read/write beyond end of file. Reading beyond end of file is supposed to produce zeroes. We rely on this behavior in qcow2_create2() during qcow2 image creation. We create a new file and then write the qcow2 header structure using bdrv_pwrite(). Since QCowHeader is not a multiple of sector size, block.c first uses bdrv_read() on the empty file to fetch the first sector (should be all zeroes). Here is the output from the qemu-io NBD example above: $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ ... We are not zeroing the buffer! As a result qcow2 image creation on top of protocols is not guaranteed to work even when file creation is supported by the protocol. Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> Signed-off-by: Asias He <asias@redhat.com> --- block.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 01b66d8..deaf0a0 100644 --- a/block.c +++ b/block.c @@ -2544,7 +2544,35 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs, } } - ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); + if (!bs->drv->protocol_name) { + ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); + } else { + /* NBD doesn't support reading beyond end of file. */ + int64_t len, total_sectors, max_nb_sectors; + + len = bdrv_getlength(bs); + if (len < 0) { + ret = len; + goto out; + } + + total_sectors = len >> BDRV_SECTOR_BITS; + max_nb_sectors = MAX(0, total_sectors - sector_num); + if (max_nb_sectors > 0) { + ret = drv->bdrv_co_readv(bs, sector_num, + MIN(nb_sectors, max_nb_sectors), qiov); + } else { + ret = 0; + } + + /* Reading beyond end of file is supposed to produce zeroes */ + if (ret == 0 && total_sectors < sector_num + nb_sectors) { + size_t offset = MAX(0, total_sectors - sector_num); + size_t bytes = (sector_num + nb_sectors - offset) * + BDRV_SECTOR_SIZE; + qemu_iovec_memset(qiov, offset * BDRV_SECTOR_SIZE, 0, bytes); + } + } out: tracked_request_end(&req); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file 2013-08-05 8:11 [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file Asias He @ 2013-08-05 12:41 ` Kevin Wolf 2013-08-06 1:53 ` [Qemu-devel] [PATCH v2] " Asias He 2013-08-16 8:41 ` [Qemu-devel] [PATCH] " Stefan Hajnoczi 1 sibling, 1 reply; 11+ messages in thread From: Kevin Wolf @ 2013-08-05 12:41 UTC (permalink / raw) To: Asias He; +Cc: MORITA Kazutaka, qemu-devel, Stefan Hajnoczi Am 05.08.2013 um 10:11 hat Asias He geschrieben: > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > While Asias is debugging an issue creating qcow2 images on top of > non-file protocols. It boils down to this example using NBD: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > Notice the open -g option to set bs->growable. This means you can > read/write beyond end of file. Reading beyond end of file is supposed > to produce zeroes. > > We rely on this behavior in qcow2_create2() during qcow2 image > creation. We create a new file and then write the qcow2 header > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > sector size, block.c first uses bdrv_read() on the empty file to fetch > the first sector (should be all zeroes). > > Here is the output from the qemu-io NBD example above: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > ... > > We are not zeroing the buffer! As a result qcow2 image creation on top > of protocols is not guaranteed to work even when file creation is > supported by the protocol. > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > Signed-off-by: Asias He <asias@redhat.com> > --- > block.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 01b66d8..deaf0a0 100644 > --- a/block.c > +++ b/block.c > @@ -2544,7 +2544,35 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs, > } > } > > - ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > + if (!bs->drv->protocol_name) { I think !bs->growable is the right check. Checking for the protocol name is always a hack and most times wrong. > + ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > + } else { > + /* NBD doesn't support reading beyond end of file. */ This is not only for NBD, make it a neutral comment like: /* Read zeros after EOF of growable BDSes */ > + int64_t len, total_sectors, max_nb_sectors; > + > + len = bdrv_getlength(bs); > + if (len < 0) { > + ret = len; > + goto out; > + } > + > + total_sectors = len >> BDRV_SECTOR_BITS; > + max_nb_sectors = MAX(0, total_sectors - sector_num); > + if (max_nb_sectors > 0) { > + ret = drv->bdrv_co_readv(bs, sector_num, > + MIN(nb_sectors, max_nb_sectors), qiov); > + } else { > + ret = 0; > + } > + > + /* Reading beyond end of file is supposed to produce zeroes */ > + if (ret == 0 && total_sectors < sector_num + nb_sectors) { > + size_t offset = MAX(0, total_sectors - sector_num); > + size_t bytes = (sector_num + nb_sectors - offset) * > + BDRV_SECTOR_SIZE; uint64_t for both offset and bytes, size_t can be 32 bits. > + qemu_iovec_memset(qiov, offset * BDRV_SECTOR_SIZE, 0, bytes); > + } > + } > > out: > tracked_request_end(&req); Kevin ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2] block: Produce zeros when protocols reading beyond end of file 2013-08-05 12:41 ` Kevin Wolf @ 2013-08-06 1:53 ` Asias He 2013-08-06 2:02 ` Fam Zheng ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Asias He @ 2013-08-06 1:53 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Asias He, MORITA Kazutaka, Stefan Hajnoczi From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> While Asias is debugging an issue creating qcow2 images on top of non-file protocols. It boils down to this example using NBD: $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' Notice the open -g option to set bs->growable. This means you can read/write beyond end of file. Reading beyond end of file is supposed to produce zeroes. We rely on this behavior in qcow2_create2() during qcow2 image creation. We create a new file and then write the qcow2 header structure using bdrv_pwrite(). Since QCowHeader is not a multiple of sector size, block.c first uses bdrv_read() on the empty file to fetch the first sector (should be all zeroes). Here is the output from the qemu-io NBD example above: $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ ... We are not zeroing the buffer! As a result qcow2 image creation on top of protocols is not guaranteed to work even when file creation is supported by the protocol. Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> Signed-off-by: Asias He <asias@redhat.com> --- block.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 01b66d8..f3cd9fb 100644 --- a/block.c +++ b/block.c @@ -2544,7 +2544,35 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs, } } - ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); + if (!bs->growable) { + ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); + } else { + /* Read zeros after EOF of growable BDSes */ + int64_t len, total_sectors, max_nb_sectors; + + len = bdrv_getlength(bs); + if (len < 0) { + ret = len; + goto out; + } + + total_sectors = len >> BDRV_SECTOR_BITS; + max_nb_sectors = MAX(0, total_sectors - sector_num); + if (max_nb_sectors > 0) { + ret = drv->bdrv_co_readv(bs, sector_num, + MIN(nb_sectors, max_nb_sectors), qiov); + } else { + ret = 0; + } + + /* Reading beyond end of file is supposed to produce zeroes */ + if (ret == 0 && total_sectors < sector_num + nb_sectors) { + uint64_t offset = MAX(0, total_sectors - sector_num); + uint64_t bytes = (sector_num + nb_sectors - offset) * + BDRV_SECTOR_SIZE; + qemu_iovec_memset(qiov, offset * BDRV_SECTOR_SIZE, 0, bytes); + } + } out: tracked_request_end(&req); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: Produce zeros when protocols reading beyond end of file 2013-08-06 1:53 ` [Qemu-devel] [PATCH v2] " Asias He @ 2013-08-06 2:02 ` Fam Zheng 2013-08-06 2:38 ` Asias He 2013-08-13 13:50 ` Stefan Hajnoczi 2013-08-22 12:13 ` Stefan Hajnoczi 2 siblings, 1 reply; 11+ messages in thread From: Fam Zheng @ 2013-08-06 2:02 UTC (permalink / raw) To: Asias He; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, MORITA Kazutaka On Tue, 08/06 09:53, Asias He wrote: > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > While Asias is debugging an issue creating qcow2 images on top of > non-file protocols. It boils down to this example using NBD: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > Notice the open -g option to set bs->growable. This means you can > read/write beyond end of file. Reading beyond end of file is supposed > to produce zeroes. > > We rely on this behavior in qcow2_create2() during qcow2 image > creation. We create a new file and then write the qcow2 header > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > sector size, block.c first uses bdrv_read() on the empty file to fetch > the first sector (should be all zeroes). > > Here is the output from the qemu-io NBD example above: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > ... > > We are not zeroing the buffer! As a result qcow2 image creation on top > of protocols is not guaranteed to work even when file creation is > supported by the protocol. It seems to me that "read beyond EOF" is more protocol defined than to be forced in block layer. Is this behavior common to all protocols, is it reasonable if some protocol wants other values than zero? Thanks. -- Fam ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: Produce zeros when protocols reading beyond end of file 2013-08-06 2:02 ` Fam Zheng @ 2013-08-06 2:38 ` Asias He 2013-08-07 8:04 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Asias He @ 2013-08-06 2:38 UTC (permalink / raw) To: Fam Zheng; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, MORITA Kazutaka On Tue, Aug 06, 2013 at 10:02:22AM +0800, Fam Zheng wrote: > On Tue, 08/06 09:53, Asias He wrote: > > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > > > While Asias is debugging an issue creating qcow2 images on top of > > non-file protocols. It boils down to this example using NBD: > > > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > > > Notice the open -g option to set bs->growable. This means you can > > read/write beyond end of file. Reading beyond end of file is supposed > > to produce zeroes. > > > > We rely on this behavior in qcow2_create2() during qcow2 image > > creation. We create a new file and then write the qcow2 header > > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > > sector size, block.c first uses bdrv_read() on the empty file to fetch > > the first sector (should be all zeroes). > > > > Here is the output from the qemu-io NBD example above: > > > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > ... > > > > We are not zeroing the buffer! As a result qcow2 image creation on top > > of protocols is not guaranteed to work even when file creation is > > supported by the protocol. > > It seems to me that "read beyond EOF" is more protocol defined than to > be forced in block layer. Is this behavior common to all protocols, is > it reasonable if some protocol wants other values than zero? The reason to do this in block layer is that we do not want to duplicate the memset in all protocols. Do we actually have protocols that really want values other than zero? > Thanks. > > -- > Fam -- Asias ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: Produce zeros when protocols reading beyond end of file 2013-08-06 2:38 ` Asias He @ 2013-08-07 8:04 ` Stefan Hajnoczi 0 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2013-08-07 8:04 UTC (permalink / raw) To: Asias He; +Cc: Kevin Wolf, Fam Zheng, qemu-devel, MORITA Kazutaka On Tue, Aug 06, 2013 at 10:38:32AM +0800, Asias He wrote: > On Tue, Aug 06, 2013 at 10:02:22AM +0800, Fam Zheng wrote: > > On Tue, 08/06 09:53, Asias He wrote: > > > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > > > > > While Asias is debugging an issue creating qcow2 images on top of > > > non-file protocols. It boils down to this example using NBD: > > > > > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > > > > > Notice the open -g option to set bs->growable. This means you can > > > read/write beyond end of file. Reading beyond end of file is supposed > > > to produce zeroes. > > > > > > We rely on this behavior in qcow2_create2() during qcow2 image > > > creation. We create a new file and then write the qcow2 header > > > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > > > sector size, block.c first uses bdrv_read() on the empty file to fetch > > > the first sector (should be all zeroes). > > > > > > Here is the output from the qemu-io NBD example above: > > > > > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > > ... > > > > > > We are not zeroing the buffer! As a result qcow2 image creation on top > > > of protocols is not guaranteed to work even when file creation is > > > supported by the protocol. > > > > It seems to me that "read beyond EOF" is more protocol defined than to > > be forced in block layer. Is this behavior common to all protocols, is > > it reasonable if some protocol wants other values than zero? > > The reason to do this in block layer is that we do not want to duplicate > the memset in all protocols. > > Do we actually have protocols that really want values other than zero? I think we rely on zeroes when bs->growable is true, because bdrv_pwrite() handles sub-sector I/O using read-modify-write. So it reads beyond EOF first (expects to get zeroes), then copies the sub-sector data from the caller, then writes it back. If we don't zero beyond-EOF data then we would write unexpected values to the image. Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: Produce zeros when protocols reading beyond end of file 2013-08-06 1:53 ` [Qemu-devel] [PATCH v2] " Asias He 2013-08-06 2:02 ` Fam Zheng @ 2013-08-13 13:50 ` Stefan Hajnoczi 2013-08-22 12:13 ` Stefan Hajnoczi 2 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2013-08-13 13:50 UTC (permalink / raw) To: Asias He; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, MORITA Kazutaka On Tue, Aug 06, 2013 at 09:53:40AM +0800, Asias He wrote: > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > While Asias is debugging an issue creating qcow2 images on top of > non-file protocols. It boils down to this example using NBD: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > Notice the open -g option to set bs->growable. This means you can > read/write beyond end of file. Reading beyond end of file is supposed > to produce zeroes. > > We rely on this behavior in qcow2_create2() during qcow2 image > creation. We create a new file and then write the qcow2 header > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > sector size, block.c first uses bdrv_read() on the empty file to fetch > the first sector (should be all zeroes). > > Here is the output from the qemu-io NBD example above: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > ... > > We are not zeroing the buffer! As a result qcow2 image creation on top > of protocols is not guaranteed to work even when file creation is > supported by the protocol. > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > Signed-off-by: Asias He <asias@redhat.com> > --- > block.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) Thanks, applied to my block-next tree: https://github.com/stefanha/qemu/commits/block-next Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block: Produce zeros when protocols reading beyond end of file 2013-08-06 1:53 ` [Qemu-devel] [PATCH v2] " Asias He 2013-08-06 2:02 ` Fam Zheng 2013-08-13 13:50 ` Stefan Hajnoczi @ 2013-08-22 12:13 ` Stefan Hajnoczi 2 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2013-08-22 12:13 UTC (permalink / raw) To: Asias He; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, MORITA Kazutaka On Tue, Aug 06, 2013 at 09:53:40AM +0800, Asias He wrote: > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > While Asias is debugging an issue creating qcow2 images on top of > non-file protocols. It boils down to this example using NBD: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > Notice the open -g option to set bs->growable. This means you can > read/write beyond end of file. Reading beyond end of file is supposed > to produce zeroes. > > We rely on this behavior in qcow2_create2() during qcow2 image > creation. We create a new file and then write the qcow2 header > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > sector size, block.c first uses bdrv_read() on the empty file to fetch > the first sector (should be all zeroes). > > Here is the output from the qemu-io NBD example above: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > ... > > We are not zeroing the buffer! As a result qcow2 image creation on top > of protocols is not guaranteed to work even when file creation is > supported by the protocol. > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > Signed-off-by: Asias He <asias@redhat.com> > --- > block.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) Applied again on top of Asias' fix so qcow2 vmstate doesn't break. Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file 2013-08-05 8:11 [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file Asias He 2013-08-05 12:41 ` Kevin Wolf @ 2013-08-16 8:41 ` Stefan Hajnoczi 2013-08-19 6:36 ` Asias He 1 sibling, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2013-08-16 8:41 UTC (permalink / raw) To: Asias He; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, MORITA Kazutaka On Mon, Aug 5, 2013 at 10:11 AM, Asias He <asias@redhat.com> wrote: > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > While Asias is debugging an issue creating qcow2 images on top of > non-file protocols. It boils down to this example using NBD: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > Notice the open -g option to set bs->growable. This means you can > read/write beyond end of file. Reading beyond end of file is supposed > to produce zeroes. > > We rely on this behavior in qcow2_create2() during qcow2 image > creation. We create a new file and then write the qcow2 header > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > sector size, block.c first uses bdrv_read() on the empty file to fetch > the first sector (should be all zeroes). > > Here is the output from the qemu-io NBD example above: > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > ... > > We are not zeroing the buffer! As a result qcow2 image creation on top > of protocols is not guaranteed to work even when file creation is > supported by the protocol. > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > Signed-off-by: Asias He <asias@redhat.com> > --- > block.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 01b66d8..deaf0a0 100644 > --- a/block.c > +++ b/block.c > @@ -2544,7 +2544,35 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs, > } > } > > - ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > + if (!bs->drv->protocol_name) { > + ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > + } else { > + /* NBD doesn't support reading beyond end of file. */ > + int64_t len, total_sectors, max_nb_sectors; > + > + len = bdrv_getlength(bs); > + if (len < 0) { > + ret = len; > + goto out; > + } > + > + total_sectors = len >> BDRV_SECTOR_BITS; > + max_nb_sectors = MAX(0, total_sectors - sector_num); > + if (max_nb_sectors > 0) { > + ret = drv->bdrv_co_readv(bs, sector_num, > + MIN(nb_sectors, max_nb_sectors), qiov); > + } else { > + ret = 0; > + } > + > + /* Reading beyond end of file is supposed to produce zeroes */ > + if (ret == 0 && total_sectors < sector_num + nb_sectors) { > + size_t offset = MAX(0, total_sectors - sector_num); > + size_t bytes = (sector_num + nb_sectors - offset) * > + BDRV_SECTOR_SIZE; > + qemu_iovec_memset(qiov, offset * BDRV_SECTOR_SIZE, 0, bytes); > + } > + } This patch breaks qemu-iotests ./check -qcow2 022. This happens because qcow2 temporarily sets ->growable = 1 for vmstate accesses (which are stored beyond the end of regular image data). static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf, int64_t pos, int size) { BDRVQcowState *s = bs->opaque; int growable = bs->growable; int ret; BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD); bs->growable = 1; ret = bdrv_pread(bs, qcow2_vm_state_offset(s) + pos, buf, size); bs->growable = growable; return ret; } Please *always* run qemu-iotests before submitting block patches: http://qemu-project.org/Documentation/QemuIoTests A simple but ugly way to fix this is for block.c to also have a ->zero_beyond_eof flag which enables the behavior you are adding. qcow2_load_vmstate() would disable ->zero_beyond_eof temporarily in addition to enabling ->growable. It's not easy to call the internal qcow2_co_readv() from qcow2_load_vmstate() because the vmstate functions are byte granularity (like bdrv_pread()) while .bdrv_co_readv() is sector-granularity. Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file 2013-08-16 8:41 ` [Qemu-devel] [PATCH] " Stefan Hajnoczi @ 2013-08-19 6:36 ` Asias He 2013-08-19 12:09 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Asias He @ 2013-08-19 6:36 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, MORITA Kazutaka On Fri, Aug 16, 2013 at 10:41:36AM +0200, Stefan Hajnoczi wrote: > On Mon, Aug 5, 2013 at 10:11 AM, Asias He <asias@redhat.com> wrote: > > From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > > > While Asias is debugging an issue creating qcow2 images on top of > > non-file protocols. It boils down to this example using NBD: > > > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > > > Notice the open -g option to set bs->growable. This means you can > > read/write beyond end of file. Reading beyond end of file is supposed > > to produce zeroes. > > > > We rely on this behavior in qcow2_create2() during qcow2 image > > creation. We create a new file and then write the qcow2 header > > structure using bdrv_pwrite(). Since QCowHeader is not a multiple of > > sector size, block.c first uses bdrv_read() on the empty file to fetch > > the first sector (should be all zeroes). > > > > Here is the output from the qemu-io NBD example above: > > > > $ qemu-io -c 'open -g nbd+unix:///?socket=/tmp/nbd.sock' -c 'read -v 0 512' > > 00000000: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > 00000010: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > 00000020: ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ................ > > ... > > > > We are not zeroing the buffer! As a result qcow2 image creation on top > > of protocols is not guaranteed to work even when file creation is > > supported by the protocol. > > > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > Signed-off-by: Asias He <asias@redhat.com> > > --- > > block.c | 30 +++++++++++++++++++++++++++++- > > 1 file changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/block.c b/block.c > > index 01b66d8..deaf0a0 100644 > > --- a/block.c > > +++ b/block.c > > @@ -2544,7 +2544,35 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs, > > } > > } > > > > - ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > > + if (!bs->drv->protocol_name) { > > + ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > > + } else { > > + /* NBD doesn't support reading beyond end of file. */ > > + int64_t len, total_sectors, max_nb_sectors; > > + > > + len = bdrv_getlength(bs); > > + if (len < 0) { > > + ret = len; > > + goto out; > > + } > > + > > + total_sectors = len >> BDRV_SECTOR_BITS; > > + max_nb_sectors = MAX(0, total_sectors - sector_num); > > + if (max_nb_sectors > 0) { > > + ret = drv->bdrv_co_readv(bs, sector_num, > > + MIN(nb_sectors, max_nb_sectors), qiov); > > + } else { > > + ret = 0; > > + } > > + > > + /* Reading beyond end of file is supposed to produce zeroes */ > > + if (ret == 0 && total_sectors < sector_num + nb_sectors) { > > + size_t offset = MAX(0, total_sectors - sector_num); > > + size_t bytes = (sector_num + nb_sectors - offset) * > > + BDRV_SECTOR_SIZE; > > + qemu_iovec_memset(qiov, offset * BDRV_SECTOR_SIZE, 0, bytes); > > + } > > + } > > This patch breaks qemu-iotests ./check -qcow2 022. This happens > because qcow2 temporarily sets ->growable = 1 for vmstate accesses > (which are stored beyond the end of regular image data). I am a bit confused. This is from the other mail: """ > > I think it would break qcow2_load_vmstate(), which is basically a > > bdrv_pread() after the end of the image. > > I see, then only protocols have to zeroing the buffer? In case of > protocols, I think bdrv_getlength() returns the underlying file > length, so qcow2_load_vmstate() would be a bdrv_pread() within the > result of bdrv_getlength(). Limiting it to protocols solves the problem, I think. """ And in v1 of this patch, Kevin wanted bs->growable check instad of the protocol_name one. """ > - ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); > + if (!bs->drv->protocol_name) { I think !bs->growable is the right check. Checking for the protocol name is always a hack and most times wrong. """ Switching back to the protocol_name check, ./check -qcow2 022 test passes. > static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf, > int64_t pos, int size) > { > BDRVQcowState *s = bs->opaque; > int growable = bs->growable; > int ret; > > BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD); > bs->growable = 1; > ret = bdrv_pread(bs, qcow2_vm_state_offset(s) + pos, buf, size); > bs->growable = growable; > > return ret; > } > > Please *always* run qemu-iotests before submitting block patches: > http://qemu-project.org/Documentation/QemuIoTests OK. > A simple but ugly way to fix this is for block.c to also have a > ->zero_beyond_eof flag which enables the behavior you are adding. > qcow2_load_vmstate() would disable ->zero_beyond_eof temporarily in > addition to enabling ->growable. I am wondering why the ->growable logic is introduced in the first place. Adding yet another this kind of flag looks realy ugly ;( > It's not easy to call the internal qcow2_co_readv() from > qcow2_load_vmstate() because the vmstate functions are byte > granularity (like bdrv_pread()) while .bdrv_co_readv() is > sector-granularity. > > Stefan -- Asias ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file 2013-08-19 6:36 ` Asias He @ 2013-08-19 12:09 ` Stefan Hajnoczi 0 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2013-08-19 12:09 UTC (permalink / raw) To: Asias He; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, MORITA Kazutaka On Mon, Aug 19, 2013 at 02:36:14PM +0800, Asias He wrote: > On Fri, Aug 16, 2013 at 10:41:36AM +0200, Stefan Hajnoczi wrote: > > On Mon, Aug 5, 2013 at 10:11 AM, Asias He <asias@redhat.com> wrote: > > A simple but ugly way to fix this is for block.c to also have a > > ->zero_beyond_eof flag which enables the behavior you are adding. > > qcow2_load_vmstate() would disable ->zero_beyond_eof temporarily in > > addition to enabling ->growable. > > I am wondering why the ->growable logic is introduced in the first > place. Adding yet another this kind of flag looks realy ugly ;( bs->growable serves two functions: 1. It means you can read/write beyond the end of the file, for example, when creating a new image file. Normally BlockDriverState rejects requests beyond the EOF. 2. qcow2 uses it as a hack to implement the vmstate area after the end of regular guest data. This is the ugly part but it always worked up until now. #1 is a legitimate use case. You don't always know how big the file will end up so it's much more convenient to grow on demand, instead of having to bdrv_truncate() all the time. #2 is hacky but hard to solve without duplicating the bounce buffer and coroutine wrapping logic in bdrv_pread() (Kevin has suggested calling bdrv_co_readv() internally instead of bdrv_pread()). Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-08-22 12:13 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-05 8:11 [Qemu-devel] [PATCH] block: Produce zeros when protocols reading beyond end of file Asias He 2013-08-05 12:41 ` Kevin Wolf 2013-08-06 1:53 ` [Qemu-devel] [PATCH v2] " Asias He 2013-08-06 2:02 ` Fam Zheng 2013-08-06 2:38 ` Asias He 2013-08-07 8:04 ` Stefan Hajnoczi 2013-08-13 13:50 ` Stefan Hajnoczi 2013-08-22 12:13 ` Stefan Hajnoczi 2013-08-16 8:41 ` [Qemu-devel] [PATCH] " Stefan Hajnoczi 2013-08-19 6:36 ` Asias He 2013-08-19 12:09 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).