block: optimize zero writes with bdrv_write_zeroes
RFC PATCH 1/1 block: optimize zero writes with bdrv_write_zeroes
this patch tries to optimize zero write requests
by automatically using bdrv_write_zeroes if it is
supported by the format.
i know that there is a lot of potential for discussion, but i would like to know what the others think.
this should significantly speed up file system initialization and should speed zero write test used to test backend storage performance.
the difference can simply be tested by e.g.
dd if=/dev/zero of=/dev/vdX bs=1M
--- block.c | 8 ++++++++ include/qemu-common.h | 1 + util/iov.c | 20 ++++++++++++++++++++ 3 files changed, 29 insertions(+)
block.c +8 -0
@@ -3145,6 +3145,14 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, ret =notifier_with_return_list_notify (&bs->before_write_notifiers, req); +if (!ret && !(flags &BDRV_REQ_ZERO_WRITE ) && + drv->bdrv_co_write_zeroes &&qemu_iovec_is_zero (qiov)) { + flags |=BDRV_REQ_ZERO_WRITE ; +/* if the device was not opened with discard=on the below flag +* is immediately cleared again in bdrv_co_do_write_zeroes */ + flags |=BDRV_REQ_MAY_UNMAP ; + } +if (ret <0 ) {/* Do nothing, write notifier decided to fail this request */ }else if (flags &BDRV_REQ_ZERO_WRITE ) {
include/qemu-common.h +1 -0
@@ -330,6 +330,7 @@ void qemu_iovec_concat(QEMUIOVector *dst,void qemu_iovec_concat_iov (QEMUIOVector *dst,struct iovec *src_iov,unsigned int src_cnt,size_t soffset,size_t sbytes); +bool qemu_iovec_is_zero (QEMUIOVector *qiov);void qemu_iovec_destroy (QEMUIOVector *qiov);void qemu_iovec_reset (QEMUIOVector *qiov); size_t qemu_iovec_to_buf(QEMUIOVector *qiov, size_t offset,
util/iov.c +20 -0
@@ -342,6 +342,26 @@ void qemu_iovec_concat(QEMUIOVector *dst,qemu_iovec_concat_iov (dst, src->iov, src->niov, soffset, sbytes); } +/* +* check if the contents of the iovecs is all zero +*/ +bool qemu_iovec_is_zero (QEMUIOVector *qiov) { +int i; +for (i =0 ; i < qiov->niov; i++) { +size_t offs = qiov->iov[i].iov_len & ~(4 *sizeof (long ) -1 ); +uint8_t *ptr = qiov->iov[i].iov_base; +if (offs && !buffer_is_zero (qiov->iov[i].iov_base, offs)) { +return false; + } +for (; offs < qiov->iov[i].iov_len; offs++) { +if (ptr[offs]) { +return false; + } + } + } +return true; +} +void qemu_iovec_destroy (QEMUIOVector *qiov) {assert (qiov->nalloc !=-1 );
Signature · 1 line
1.7.9.5
Quoted 10 lines · Peter Lieven
this patch tries to optimize zero write requests
by automatically usingbdrv_write_zeroesif it is
supported by the format.i know that there is a lot of potential for discussion, but i would like to know what the others think.
this should significantly speed up file system initialization and should speed zero write test used to test backend storage performance.
Quoted 6 lines
the difference can simply be tested by e.g.
dd if=/dev/zero of=/dev/vdX bs=1M
Signed-off-by Peter Lieven <[email protected]>---
With this patch, is is still possible to actually do zero fill? Prefill is usually writing zeroes too, but according to the semantic, bdrv_write_zeroes may just set L2 entry flag without allocating clusters, which won't satisfy that.
Thanks,
Fam
Am 22.02.2014 17:45, schrieb Fam Zheng:
Quoted 13 lines · Peter Lieven
Quoted 12 lines
this patch tries to optimize zero write requests
by automatically usingbdrv_write_zeroesif it is
supported by the format.i know that there is a lot of potential for discussion, but i would like to know what the others think.
this should significantly speed up file system initialization and should speed zero write test used to test backend storage performance.
the difference can simply be tested by e.g.
Quoted 8 lines
Quoted 4 lines
dd if=/dev/zero of=/dev/vdX bs=1M
Signed-off-by Peter Lieven <[email protected]>---
With this patch, is is still possible to actually do zero fill? Prefill is usually writing zeroes too, but according to the semantic,
bdrv_write_zeroesmay just set L2 entry flag without allocating clusters, which won't satisfy that.
Can you specify which operation you exactly mean? I don't think that there is a problem, but maybe it would be better to add a check for bs->file != NULL so the optimization takes only place for the format not for the protocol.
I meanwhile ran a short test which shows that there is potential for significant improvement.
I created a 60GB qcow2 container and formatted it with ext4. To immediately show the difference I disabled lazy inode table and lazy journal init (writing zero takes place immediately then).
Timing without the patch:
time mkfs.ext4 -E lazy_itable_init=0,lazy_journal_init=0 /dev/vda real 1m5.649s
user 0m0.416s
sys 0m3.148s
Timing with the patch:
time mkfs.ext4 -E lazy_itable_init=0,lazy_journal_init=0 /dev/vdX real 0m1.228s
user 0m0.116s
sys 0m0.732s
Container Size after Format without the patch: 1150615552 Byte (1097.3MB) Container Size after Format with the patch: 24645632 Byte (23.5MB)
Peter
Quoted 3 lines
Thanks,
Fam
Quoted 18 lines · Peter Lieven
Am 22.02.2014 17:45, schrieb Fam Zheng:
Quoted 17 lines · Peter Lieven
Quoted 16 lines
this patch tries to optimize zero write requests
by automatically usingbdrv_write_zeroesif it is
supported by the format.i know that there is a lot of potential for discussion, but i would like to know what the others think.
this should significantly speed up file system initialization and should speed zero write test used to test backend storage performance.
the difference can simply be tested by e.g.
dd if=/dev/zero of=/dev/vdX bs=1M
Signed-off-by Peter Lieven <[email protected]>---
Quoted 9 lines
Quoted 4 lines
With this patch, is is still possible to actually do zero fill? Prefill is usually writing zeroes too, but according to the semantic,
bdrv_write_zeroesmay just set L2 entry flag without allocating clusters, which won't satisfy that.Can you specify which operation you exactly mean? I don't think that there is a problem, but maybe it would be better to add a check for
bs->file!= NULL so the optimization takes only place for the format not for the protocol.
Previously, users can do
dd if=/dev/zero of=/dev/vdX
to force backend allocation and mapping. This is meaningful for later IO performance, but how long the dd takes doesn't matter as much, since it is a one time shot.
The same in your test case: yes, mkfs time may be improved, but it comes with tradeoff with later IO's slowness: when the real user data comes, the allocation still needs to be done.
I would do this in qcow2:
1. In qcow2_co_writev, allocate cluster regardless of if data is zero or not.
2. If data is zero, set QCOW2_OFLAG_ZERO in L2.
Fam
Il 24/02/2014 02:01, Fam Zheng ha scritto:
Quoted 5 lines
I would do this in qcow2:
1. In
qcow2_co_writev, allocate cluster regardless of if data is zero or not.
2. If data is zero, setQCOW2_OFLAG_ZEROin L2.
This is (or should be) bdrv_co_write_zeroes without BDRV_REQ_MAY_UNMAP.
Paolo
Quoted 8 lines · Paolo Bonzini
Il 24/02/2014 02:01, Fam Zheng ha scritto:
Quoted 5 lines
I would do this in qcow2:
1. In
qcow2_co_writev, allocate cluster regardless of if data is zero or not.
2. If data is zero, setQCOW2_OFLAG_ZEROin L2.This is (or should be)
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP.
But IIUC bdrv_co_write_zeroes without BDRV_REQ_MAY_UNMAP doesn't require cluster allocation if it's allocated yet, which is a bit different.
Fam
Il 24/02/2014 12:33, Fam Zheng ha scritto:
Quoted 4 lines
Quoted 1 line
This is (or should be)
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP.But IIUC
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAPdoesn't require cluster allocation if it's allocated yet, which is a bit different.
Yeah, that's why I wrote "or should be". Those are the intended semantics of bdrv_co_write_zeroes without BDRV_REQ_MAY_UNMAP: always allocate a cluster that will read as zeroes (allocating even if it does not necessarily write the zeroes).
For legacy reasons it may not be exactly what is implemented. I asked Kevin a couple of weeks ago and he sent a patch, but even he wasn't sure of what qcow2 was doing util he looked at the code. :)
Paolo
Quoted 6 lines · Paolo Bonzini
Il 24/02/2014 12:33, Fam Zheng ha scritto:
Quoted 4 lines
Quoted 1 line
This is (or should be)
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP.But IIUC
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAPdoesn't require cluster allocation if it's allocated yet, which is a bit different.
Quoted 9 lines
Yeah, that's why I wrote "or should be". Those are the intended semantics of
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP: always allocate a cluster that will read as zeroes (allocating even if it does not necessarily write the zeroes).For legacy reasons it may not be exactly what is implemented. I asked Kevin a couple of weeks ago and he sent a patch, but even he wasn't sure of what qcow2 was doing util he looked at the code. :)
I see. I could only tell in VMDK cluster doesn't have this "mapped and zeroed" state, so maybe we need some flexibility here and reduce assumption.
Fam
Am 24.02.2014 um 12:51 hat Paolo Bonzini geschrieben:
Quoted 10 lines
Il 24/02/2014 12:33, Fam Zheng ha scritto:
Quoted 4 lines
Quoted 1 line
This is (or should be)
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP.But IIUC
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAPdoesn't require cluster allocation if it's allocated yet, which is a bit different.Yeah, that's why I wrote "or should be". Those are the intended semantics of
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP: always allocate a cluster that will read as zeroes (allocating even if it does not necessarily write the zeroes).
Which would mean that there is no way to say "give me zeroes, and do it in the cheapest way possible". Because that would be to leave the allocation status as it is and just toggle the zero bit.
Quoted 3 lines
For legacy reasons it may not be exactly what is implemented. I asked Kevin a couple of weeks ago and he sent a patch, but even he wasn't sure of what qcow2 was doing util he looked at the code. :)
Even with my patch, it doesn't actually allocate the cluster, it just sets the flag.
Kevin
Il 24/02/2014 13:07, Kevin Wolf ha scritto:
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Yeah, that's why I wrote "or should be". Those are the intended semantics of
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP: always allocate a cluster that will read as zeroes (allocating even if it does not necessarily write the zeroes).Which would mean that there is no way to say "give me zeroes, and do it in the cheapest way possible". Because that would be to leave the allocation status as it is and just toggle the zero bit.
If bdrv_co_write_zeroes is SCSI's "WRITE SAME without UNMAP", then it must allocate. I think "give me zeroes and do it in the cheapest way possible" is BDRV_REQ_MAY_UNMAP (which *may* unmap but it doesn't have to).
That said, the really expensive part of unmapping is probably re-allocating the clusters on subsequent writes. The unmap itself isn't that expensive (even if you have to flush the refcount blocks before the L2 tables), is it?
Paolo
Am 24.02.2014 um 13:10 hat Paolo Bonzini geschrieben:
Quoted 5 lines
Il 24/02/2014 13:07, Kevin Wolf ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Yeah, that's why I wrote "or should be". Those are the intended semantics of
bdrv_co_write_zeroeswithoutBDRV_REQ_MAY_UNMAP: always allocate a cluster that will read as zeroes (allocating even if it does not necessarily write the zeroes).
Quoted 8 lines
Quoted 3 lines
Which would mean that there is no way to say "give me zeroes, and do it in the cheapest way possible". Because that would be to leave the allocation status as it is and just toggle the zero bit.
If
bdrv_co_write_zeroesis SCSI's "WRITE SAME without UNMAP", then it must allocate. I think "give me zeroes and do it in the cheapest way possible" isBDRV_REQ_MAY_UNMAP(which *may* unmap but it doesn't have to).
Hm, okay. So the intended behaviour for qcow2 is:
- without MAY_UNMAP: Preallocate the cluster and set the zero flag so that its content isn't valid
- with MAY_UNMAP: If the cluster is allocated, leave it allocated and set the zero flag; if it isn't, leave it unallocated and set the zero flag.
Quoted 4 lines
That said, the really expensive part of unmapping is probably re-allocating the clusters on subsequent writes. The unmap itself isn't that expensive (even if you have to flush the refcount blocks before the L2 tables), is it?
Flushes are pretty expensive. If you only have many discards in a row and then many allocations in a row, it's probably okay. Mixing them so that every other write is a discard might well kill performance.
Kevin
Am 22.02.2014 um 14:00 hat Peter Lieven geschrieben:
Quoted 7 lines
this patch tries to optimize zero write requests
by automatically usingbdrv_write_zeroesif it is
supported by the format.i know that there is a lot of potential for discussion, but i would like to know what the others think.
Quoted 8 lines
this should significantly speed up file system initialization and should speed zero write test used to test backend storage performance.
the difference can simply be tested by e.g.
dd if=/dev/zero of=/dev/vdX bs=1M
Signed-off-by Peter Lieven <[email protected]>
As you probably have expected, there's no way I can let the patch in in this form. The least you need to introduce is a boolean option to enable or disable the zero check. (The default would probably be disabled, but we can discuss this.)
Quoted 10 lines
block.c | 8 ++++++++ include/qemu-common.h | 1 + util/iov.c | 20 ++++++++++++++++++++ 3 files changed, 29 insertions(+)block.c +0
-0
block.c static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, · line 3153
Quoted 9 lines
block.c +6
-0@@ -3145,6 +3145,14 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, ret =notifier_with_return_list_notify (&bs->before_write_notifiers, req); +if (!ret && !(flags &BDRV_REQ_ZERO_WRITE ) && + drv->bdrv_co_write_zeroes &&qemu_iovec_is_zero (qiov)) { + flags |=BDRV_REQ_ZERO_WRITE ; +/* if the device was not opened with discard=on the below flag +* is immediately cleared again in bdrv_co_do_write_zeroes */ + flags |=BDRV_REQ_MAY_UNMAP ;
I'm not sure about this one. I think it is reasonable to expect that after an explicit write of a buffer filled with zeros the block is allocated.
In a simple qcow2-on-file case, we basically have three options for handling all-zero writes:
- Allocate the cluster on a qcow2 and file level and write literal zeros to it. No metadata updates involved in the next write to the cluster.
- Set the qcow2 zero flag, but leave the allocation in place. The next write in theory just needs to remove the zero flag (I think in practice we're doing an unnecessary COW) from the L2 table and that's it.
- Set the qcow2 zero flag and unmap the cluster on both the qcow2 and the filesystem layer. The next write causes new allocations in both layers, which means multiple metadata updates and possibly added fragmentation. The upside is that we use less disk space if there is no next write to this cluster.
I think it's pretty clear that the right behaviour depends on your use case and we can't find an one-size-fits-all solution.
block.c static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, · line 3158
Quoted 5 lines
block.c +2
-0+ } +if (ret <0 ) {/* Do nothing, write notifier decided to fail this request */ }else if (flags &BDRV_REQ_ZERO_WRITE ) {
Kevin
Quoted 13 lines · Kevin Wolf
Am 22.02.2014 um 14:00 hat Peter Lieven geschrieben:
Quoted 12 lines
this patch tries to optimize zero write requests
by automatically usingbdrv_write_zeroesif it is
supported by the format.i know that there is a lot of potential for discussion, but i would like to know what the others think.
this should significantly speed up file system initialization and should speed zero write test used to test backend storage performance.
the difference can simply be tested by e.g.
Quoted 7 lines
Quoted 3 lines
dd if=/dev/zero of=/dev/vdX bs=1M
Signed-off-by Peter Lieven <[email protected]>As you probably have expected, there's no way I can let the patch in in this form. The least you need to introduce is a boolean option to enable or disable the zero check. (The default would probably be disabled, but we can discuss this.)
I would have been really suprised *g*. As you and Fam already pointed out, the desired behaviour is heavily dependant on the use case.
I personally do not need this for QCOW2 but for iSCSI. Here the optimization is basically saved bandwidth since a zero write becomes a WRITESAME. Unless the user specifies unmap=on there is no change in what is written to disk.
I would be fine with a default off boolean variable. For my case it would also be sufficient to have boolean flag in the BlockDriver that indicates if this optimization is a good idea. For iSCSI I think it is. I think also for GlusterFS. In those both cases I basically saves bandwidth and let the backend storage more efficiently write zeroes if it is capable. A third use case would be a raw device on an SSD. In all cases if unmap=on it would additionally save disk space.
Quoted 35 lines
Quoted 11 lines
block.c | 8 ++++++++ include/qemu-common.h | 1 + util/iov.c | 20 ++++++++++++++++++++ 3 files changed, 29 insertions(+)block.c +0
-0block.c static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, · line 3153
Quoted 9 lines
block.c +6
-0@@ -3145,6 +3145,14 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, ret =notifier_with_return_list_notify (&bs->before_write_notifiers, req); +if (!ret && !(flags &BDRV_REQ_ZERO_WRITE ) && + drv->bdrv_co_write_zeroes &&qemu_iovec_is_zero (qiov)) { + flags |=BDRV_REQ_ZERO_WRITE ; +/* if the device was not opened with discard=on the below flag +* is immediately cleared again in bdrv_co_do_write_zeroes */ + flags |=BDRV_REQ_MAY_UNMAP ;I'm not sure about this one. I think it is reasonable to expect that after an explicit write of a buffer filled with zeros the block is allocated.
In a simple qcow2-on-file case, we basically have three options for handling all-zero writes:
- Allocate the cluster on a qcow2 and file level and write literal zeros to it. No metadata updates involved in the next write to the cluster.
- Set the qcow2 zero flag, but leave the allocation in place. The next write in theory just needs to remove the zero flag (I think in practice we're doing an unnecessary COW) from the L2 table and that's it.
Quoted 8 lines
- Set the qcow2 zero flag and unmap the cluster on both the qcow2 and the filesystem layer. The next write causes new allocations in both layers, which means multiple metadata updates and possibly added fragmentation. The upside is that we use less disk space if there is no next write to this cluster.
I think it's pretty clear that the right behaviour depends on your use case and we can't find an one-size-fits-all solution.
I wouldn't mind have this optimization only work on raw format for the moment.
Peter
Quoted 7 lines
block.c static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, · line 3158
Quoted 6 lines
block.c +2
-0+ } +if (ret <0 ) {/* Do nothing, write notifier decided to fail this request */ }else if (flags &BDRV_REQ_ZERO_WRITE ) {Kevin
Il 24/02/2014 11:26, Peter Lieven ha scritto:
Quoted 3 lines
I personally do not need this for QCOW2 but for iSCSI. Here the optimization is basically saved bandwidth since a zero write becomes a WRITESAME.
It saves bandwidth, but at the potential cost of extra host CPU utilization. I would be fine with having this automatically, but drv->bdrv_co_write_zeroes is not the right check because it is true for qcow2 and raw formats. Something using bdrv_get_info is probably better, because it would have fewer or no false positives.
Quoted 1 line
In all cases if unmap=on it would additionally save disk space.
It would also cause worse performance though. I think the automatic addition BDRV_REQ_MAY_UNMAP is what should be a separate option. Perhaps you can have a three-state option, detect-zeros=no/yes/unmap.
Paolo
Quoted 10 lines · Paolo Bonzini
Il 24/02/2014 11:26, Peter Lieven ha scritto:
Quoted 3 lines
I personally do not need this for QCOW2 but for iSCSI. Here the optimization is basically saved bandwidth since a zero write becomes a WRITESAME.
It saves bandwidth, but at the potential cost of extra host CPU utilization. I would be fine with having this automatically, but
drv->bdrv_co_write_zeroesis not the right check because it is true for qcow2 and raw formats. Something usingbdrv_get_infois probably better, because it would have fewer or no false positives.Quoted 1 line
In all cases if unmap=on it would additionally save disk space.
It would also cause worse performance though. I think the automatic addition
BDRV_REQ_MAY_UNMAPis what should be a separate option. Perhaps you can have a three-state option, detect-zeros=no/yes/unmap.
I think this is the best option to start with and default it to no for the beginning.
Peter
Quoted 10 lines · Paolo Bonzini
Il 24/02/2014 11:26, Peter Lieven ha scritto:
Quoted 3 lines
I personally do not need this for QCOW2 but for iSCSI. Here the optimization is basically saved bandwidth since a zero write becomes a WRITESAME.
It saves bandwidth, but at the potential cost of extra host CPU utilization. I would be fine with having this automatically, but
drv->bdrv_co_write_zeroesis not the right check because it is true for qcow2 and raw formats. Something usingbdrv_get_infois probably better, because it would have fewer or no false positives.Quoted 1 line
In all cases if unmap=on it would additionally save disk space.
It would also cause worse performance though. I think the automatic addition
BDRV_REQ_MAY_UNMAPis what should be a separate option. Perhaps you can have a three-state option, detect-zeros=no/yes/unmap.
What would be the desired way to store this flag in the BlockDriverState?
Peter
Am 24.02.2014 um 14:01 hat Peter Lieven geschrieben:
Quoted 13 lines · Paolo Bonzini
Quoted 12 lines
Il 24/02/2014 11:26, Peter Lieven ha scritto:
Quoted 3 lines
I personally do not need this for QCOW2 but for iSCSI. Here the optimization is basically saved bandwidth since a zero write becomes a WRITESAME.
It saves bandwidth, but at the potential cost of extra host CPU utilization. I would be fine with having this automatically, but
drv->bdrv_co_write_zeroesis not the right check because it is true for qcow2 and raw formats. Something usingbdrv_get_infois probably better, because it would have fewer or no false positives.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
In all cases if unmap=on it would additionally save disk space.
It would also cause worse performance though. I think the automatic addition
BDRV_REQ_MAY_UNMAPis what should be a separate option. Perhaps you can have a three-state option, detect-zeros=no/yes/unmap.What would be the desired way to store this flag in the BlockDriverState?
Some new enum field?
Kevin
Am 25.02.2014 14:41, schrieb Kevin Wolf:
Quoted 12 lines
Am 24.02.2014 um 14:01 hat Peter Lieven geschrieben:
Quoted 11 lines · Paolo Bonzini
Quoted 10 lines
Il 24/02/2014 11:26, Peter Lieven ha scritto:
Quoted 2 lines
I personally do not need this for QCOW2 but for iSCSI. Here the optimization is basically saved bandwidth since a zero write becomes a WRITESAME.
It saves bandwidth, but at the potential cost of extra host CPU utilization. I would be fine with having this automatically, but
drv->bdrv_co_write_zeroesis not the right check because it is true for qcow2 and raw formats. Something usingbdrv_get_infois probably better, because it would have fewer or no false positives.
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
In all cases if unmap=on it would additionally save disk space.
It would also cause worse performance though. I think the automatic addition
BDRV_REQ_MAY_UNMAPis what should be a separate option. Perhaps you can have a three-state option, detect-zeros=no/yes/unmap.What would be the desired way to store this flag in the BlockDriverState?
Some new enum field?
Is there an already implemented example where I can copy from? Its quite diffucult to search through all the involved functions when doing it for the first time.
Peter