introduce max_transfer_length
cover introduce max_transfer_length
This series adds the basics for introducing a maximum transfer length to the block layer. Its main purpose is currently avoiding that a multiwrite_merge exceeds the max_xfer_len of an attached iSCSI LUN. This is a required bug fix.
Discussed reporting of this maximum in the SCSI Disk Inquiry Emulation of the Block Limits VPD is currently not added as we do not import any of the other limits there. This has to be addresses in a seperate series.
Peter Lieven (4):
BlockLimits: introduce max_transfer_length
block: immediate cancel oversized read/write requests block/iscsi: set max_transfer_length
block: avoid creating oversized writes in multiwrite_merge
block.c | 23 +++++++++++++++++++++++ block/iscsi.c | 12 ++++++++++-- include/block/block_int.h | 3 +++ 3 files changed, 36 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5
PATCH 1/4 BlockLimits: introduce max_transfer_length
--- block.c | 4 ++++ include/block/block_int.h | 3 +++ 2 files changed, 7 insertions(+)
block.c +4 -0
@@ -529,6 +529,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)return ; } bs->bl.opt_transfer_length = bs->file->bl.opt_transfer_length; + bs->bl.max_transfer_length = bs->file->bl.max_transfer_length; bs->bl.opt_mem_alignment = bs->file->bl.opt_mem_alignment; }else { bs->bl.opt_mem_alignment =512 ;
@@ -543,6 +544,9 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp) bs->bl.opt_transfer_length =MAX (bs->bl.opt_transfer_length, bs->backing_hd->bl.opt_transfer_length); + bs->bl.max_transfer_length = +MIN (bs->bl.max_transfer_length, + bs->backing_hd->bl.max_transfer_length); bs->bl.opt_mem_alignment =MAX (bs->bl.opt_mem_alignment, bs->backing_hd->bl.opt_mem_alignment);
include/block/block_int.h +3 -0
@@ -288,6 +288,9 @@ typedef struct BlockLimits {/* optimal transfer length in sectors */ int opt_transfer_length; +/* maximal transfer length in sectors */ +int max_transfer_length; +/* memory alignment so that no bounce buffer is needed */ size_t opt_mem_alignment; } BlockLimits;
Signature · 1 line
1.7.9.5
PATCH 2/4 block: immediately cancel oversized read/write requests
In reply to Peter Lieven--- block.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
block.c +14 -0
@@ -3215,6 +3215,13 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,return -EINVAL ; } +if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("read of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_preadv (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }
@@ -3507,6 +3514,13 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,return -EINVAL ; } +if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("write of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_pwritev (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 41 lines · Peter Lieven
Signed-off-by Peter Lieven <[email protected]>--- block.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)block.c +14
-0@@ -3215,6 +3215,13 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,return -EINVAL ; } +if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("read of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_preadv (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }@@ -3507,6 +3514,13 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,return -EINVAL ; } +if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("write of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_pwritev (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }Signature · 1 line
1.7.9.5
Look like you are changing the coroutine version.
Some hardware like virtio-blk uses the AIO version of read and writes. What would happen if all the block drivers down the chain are AIO enabled ?
Best regards
Benoît
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 08/09/2014 15:44, Benoît Canet ha scritto:
Quoted 30 lines
Quoted 14 lines
Quoted 14 lines
block.c +7
-0+if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("read of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_preadv (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }@@ -3507,6 +3514,13 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,return -EINVAL ; }Quoted 14 lines
Quoted 14 lines
block.c +7
-0+if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("write of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_pwritev (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }Signature · 1 line
1.7.9.5Look like you are changing the coroutine version.
Quoted 2 lines
Some hardware like virtio-blk uses the AIO version of read and writes. What would happen if all the block drivers down the chain are AIO enabled ?
The AIO version still goes through bdrv_co_do_readv/writev.
However, error_report is not something you can use for guest-accessible error messages, unless you want your logs to fill up with error messages. :)
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 31 lines · Paolo Bonzini
Il 08/09/2014 15:44, Benoît Canet ha scritto:
Quoted 30 lines
Quoted 14 lines
Quoted 14 lines
block.c +7
-0+if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("read of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_preadv (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }@@ -3507,6 +3514,13 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,return -EINVAL ; }Quoted 14 lines
Quoted 14 lines
block.c +7
-0+if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("write of %d sectors at sector %ld exceeds device max" +" transfer length of %d sectors." , nb_sectors, sector_num, + bs->bl.max_transfer_length); +return -EINVAL ; + } +return bdrv_co_do_pwritev (bs, sector_num <<BDRV_SECTOR_BITS , nb_sectors <<BDRV_SECTOR_BITS , qiov, flags); }Signature · 1 line
1.7.9.5Look like you are changing the coroutine version.
Quoted 6 lines
Quoted 2 lines
Some hardware like virtio-blk uses the AIO version of read and writes. What would happen if all the block drivers down the chain are AIO enabled ?
The AIO version still goes through bdrv_co_do_readv/writev.
However,
error_reportis not something you can use for guest-accessible error messages, unless you want your logs to fill up with error messages. :)
So you would not throw an error msg here?
Peter
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 08/09/2014 15:56, Peter Lieven ha scritto:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Look like you are changing the coroutine version.
Some hardware like virtio-blk uses the AIO version of read and writes. What would happen if all the block drivers down the chain are AIO enabled ?
Quoted 7 lines
Quoted 5 lines
The AIO version still goes through bdrv_co_do_readv/writev.
However,
error_reportis not something you can use for guest-accessible error messages, unless you want your logs to fill up with error messages. :)So you would not throw an error msg here?
No, though a trace could be useful.
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 6 lines · Paolo Bonzini
Il 08/09/2014 15:56, Peter Lieven ha scritto:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Look like you are changing the coroutine version.
Some hardware like virtio-blk uses the AIO version of read and writes. What would happen if all the block drivers down the chain are AIO enabled ?
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
The AIO version still goes through bdrv_co_do_readv/writev.
However,
error_reportis not something you can use for guest-accessible error messages, unless you want your logs to fill up with error messages. :)So you would not throw an error msg here?
No, though a trace could be useful.
Is there a howto somewhere how to implement that?
Then I would send a v2 if there are no other complaints.
Whats your opinion changed the max_xfer_len to 0xffff regardsless of use_16_for_rw in iSCSI?
Peter
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 08/09/2014 16:35, Peter Lieven ha scritto:
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
messages. :)
So you would not throw an error msg here?
No, though a trace could be useful.
Is there a howto somewhere how to implement that?
Try commit 4ac4458076e1aaf3b01a6361154117df20e22215.
Quoted 2 lines
Whats your opinion changed the
max_xfer_lento0xffffregardsless ofuse_16_for_rwin iSCSI?
If you implemented request splitting in the block layer, it would be okay to force max_xfer_len to 0xffff.
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 6 lines · Paolo Bonzini
Il 08/09/2014 16:35, Peter Lieven ha scritto:
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
messages. :)
So you would not throw an error msg here?
No, though a trace could be useful.
Is there a howto somewhere how to implement that?
Try commit
4ac4458076e1aaf3b01a6361154117df20e22215.
Thanks for the pointer.
Quoted 5 lines
Quoted 3 lines
Whats your opinion changed the
max_xfer_lento0xffffregardsless ofuse_16_for_rwin iSCSI?If you implemented request splitting in the block layer, it would be okay to force
max_xfer_lento0xffff.
Unfortunately, I currently have no time for that. It will include some shuffling with qiovs that has to be properly tested.
Regarding iSCSI: In fact currently the limit is 0xffff for all iSCSI Targets < 2TB. So I thought that its not obvious at all why a > 2TB target can handle bigger requests.
To the root cause of this patch multiwrite_merge I still have some thoughts:
- why are we merging requests for raw (especially host devices and/or iSCSI?) The original patch from Kevin was to mitigate a QCOW2 performance regression. For iSCSI the qiov concats are destroying all the zerocopy efforts we made.
- should we only merge requests within the same cluster?
- why is there no multiread_merge?
Peter
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Am 08.09.2014 um 16:54 hat Peter Lieven geschrieben:
Quoted 10 lines · Paolo Bonzini
Quoted 5 lines
Il 08/09/2014 16:35, Peter Lieven ha scritto:
Quoted 2 lines
Whats your opinion changed the
max_xfer_lento0xffffregardsless ofuse_16_for_rwin iSCSI?If you implemented request splitting in the block layer, it would be okay to force
max_xfer_lento0xffff.Unfortunately, I currently have no time for that. It will include some shuffling with qiovs that has to be properly tested.
Quoted 6 lines
Regarding iSCSI: In fact currently the limit is
0xfffffor all iSCSI Targets < 2TB. So I thought that its not obvious at all why a > 2TB target can handle bigger requests.To the root cause of this patch
multiwrite_mergeI still have some thoughts:
- why are we merging requests for raw (especially host devices and/or iSCSI?) The original patch from Kevin was to mitigate a QCOW2 performance regression.
The problem wasn't in qcow2, though, it just became more apparent there because lots of small requests are deadly to performance during cluster allocation. Installation simply took ages compared to IDE. If you do a real benchmark, you'll probably see (smaller) differences with raw, too.
The core problem is virtio-blk getting much smaller requests. IIRC, I got an average of 128k-512k per request for IDE and something as poor as 4k-32k for virtio-blk. If I read this thread correctly, you're saying that this is still the case. I suspect there is something wrong with the guest driver, but somebody would have to dig into that.
Quoted 1 line
For iSCSI the qiov concats are destroying all the zerocopy efforts we made.
If this is true, it just means that the iscsi driver sucks for vectored I/O and needs to be fixed.
Quoted 1 line
- should we only merge requests within the same cluster?
Does it hurt to merge everything we can? The block driver needs to be able to take things apart anyway, the large request could come from somewhere else (guest, block job, builtin NBD server, etc.)
Quoted 1 line
- why is there no
multiread_merge?
Because nobody implemented it. :-)
As I said above, writes hurt a lot because of qcow2 cluster allocation. Reads are probably losing some performance as well (someone would need to check this), but processing them separately isn't quite as painful.
Kevin
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 21 lines · Kevin Wolf
Am 08.09.2014 um 16:54 hat Peter Lieven geschrieben:
Quoted 9 lines · Paolo Bonzini
Quoted 5 lines
Il 08/09/2014 16:35, Peter Lieven ha scritto:
Quoted 2 lines
Whats your opinion changed the
max_xfer_lento0xffffregardsless ofuse_16_for_rwin iSCSI?If you implemented request splitting in the block layer, it would be okay to force
max_xfer_lento0xffff.Unfortunately, I currently have no time for that. It will include some shuffling with qiovs that has to be properly tested.
Quoted 6 lines
Regarding iSCSI: In fact currently the limit is
0xfffffor all iSCSI Targets < 2TB. So I thought that its not obvious at all why a > 2TB target can handle bigger requests.To the root cause of this patch
multiwrite_mergeI still have some thoughts:
- why are we merging requests for raw (especially host devices and/or iSCSI?) The original patch from Kevin was to mitigate a QCOW2 performance regression.The problem wasn't in qcow2, though, it just became more apparent there because lots of small requests are deadly to performance during cluster allocation. Installation simply took ages compared to IDE. If you do a real benchmark, you'll probably see (smaller) differences with raw, too.
Quoted 5 lines
The core problem is virtio-blk getting much smaller requests. IIRC, I got an average of 128k-512k per request for IDE and something as poor as 4k-32k for virtio-blk. If I read this thread correctly, you're saying that this is still the case. I suspect there is something wrong with the guest driver, but somebody would have to dig into that.
This seems to be the case. I will check if this disappears with most recent kernels virtio-blk versions.
Quoted 4 lines
Quoted 2 lines
For iSCSI the qiov concats are destroying all the zerocopy efforts we made.
If this is true, it just means that the iscsi driver sucks for vectored I/O and needs to be fixed.
It works quite well, I misunderstood the iovec_concat routine at first. I thought it was copying payload data around. The overhead for bilding the qiov structure only should be neglectible.
Quoted 5 lines
Quoted 2 lines
- should we only merge requests within the same cluster?
Does it hurt to merge everything we can? The block driver needs to be able to take things apart anyway, the large request could come from somewhere else (guest, block job, builtin NBD server, etc.)
I was just thinking. It was unclear what the original intention was.
My concern was that merging too much will increase latency for the specific I/O even if it increases throughput.
Quoted 3 lines
Quoted 2 lines
- why is there no
multiread_merge?Because nobody implemented it. :-)
Maybe its worth looking at this. Taking the multiwrite_merge stuff as a basis it shouldn't be too hard.
Quoted 4 lines
As I said above, writes hurt a lot because of qcow2 cluster allocation. Reads are probably losing some performance as well (someone would need to check this), but processing them separately isn't quite as painful.
I will try to sort that out.
Peter
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Am 23.09.2014 um 10:55 hat Peter Lieven geschrieben:
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
- should we only merge requests within the same cluster?
Does it hurt to merge everything we can? The block driver needs to be able to take things apart anyway, the large request could come from somewhere else (guest, block job, builtin NBD server, etc.)
I was just thinking. It was unclear what the original intention was.
I hope it's clearer now. Anyway, for qcow2 merging even across clusters does help a bit if those clusters are physically contiguous.
Quoted 2 lines
My concern was that merging too much will increase latency for the specific I/O even if it increases throughput.
That's probably a valid concern. Perhaps we should add a set of options to enable or disable certain request manipulations that the block layer can do. This includes request merging, but also alignment adjustments, zero detection (which already has an option) etc.
Quoted 5 lines
Quoted 2 lines
Quoted 1 line
- why is there no
multiread_merge?Because nobody implemented it. :-)
Maybe its worth looking at this. Taking the
multiwrite_mergestuff as a basis it shouldn't be too hard.
Yes, it should be doable. We need numbers, though, before merging something like this.
Quoted 5 lines
Quoted 3 lines
As I said above, writes hurt a lot because of qcow2 cluster allocation. Reads are probably losing some performance as well (someone would need to check this), but processing them separately isn't quite as painful.
I will try to sort that out.
Great, thanks!
Kevin
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
In reply to Paolo BonziniQuoted 8 lines · Paolo Bonzini
Il 08/09/2014 16:35, Peter Lieven ha scritto:
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
messages. :)
So you would not throw an error msg here?
No, though a trace could be useful.
Is there a howto somewhere how to implement that?
Quoted 7 lines
Try commit
4ac4458076e1aaf3b01a6361154117df20e22215.Quoted 2 lines
Whats your opinion changed the
max_xfer_lento0xffffregardsless ofuse_16_for_rwin iSCSI?If you implemented request splitting in the block layer, it would be okay to force
max_xfer_lento0xffff.
I think it should be OK if that is a different series. This only affects the iSCSI transport since it is the only transport so far to record or report a max transfer length.
If a guest generates these big requests(i.e. not multiwrite) we already fail them due to the READ10 limitations. We would just fail the request at a different layer in the stack with these patches.
What I would like to see would also be to report these limitations to the guest itself to prevent it from generating too large I/Os.
I am willing to do that part once the initial max_xfer_len ones are finished.
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 08/09/2014 17:13, ronnie sahlberg ha scritto:
Quoted 3 lines
What I would like to see would also be to report these limitations to the guest itself to prevent it from generating too large I/Os.
That's difficult because you don't want a backend change (e.g. from local storage to iSCSI) to change the vital product data in the guest.
That's why we have splitting code for discard, and why we would have to add it for read/write too.
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 5 lines · Paolo Bonzini
Il 08/09/2014 17:13, ronnie sahlberg ha scritto:
Quoted 2 lines
What I would like to see would also be to report these limitations to the guest itself to prevent it from generating too large I/Os.
That's difficult because you don't want a backend change (e.g. from local storage to iSCSI) to change the vital product data in the guest.
I hadn't that in mind....
Quoted 3 lines
That's why we have splitting code for discard, and why we would have to add it for read/write too.
Why should a guest generate such big requests. Afaik the reported
limit for e.g. virtio-blk is 1024 sectors (reported through blockdev --getmaxsect /dev/vda). I think it was only the multiwrite_merge code causing trouble here.
Peter
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 08/09/2014 17:18, Peter Lieven ha scritto:
Quoted 5 lines
Quoted 3 lines
That's why we have splitting code for discard, and why we would have to add it for read/write too.
Why should a guest generate such big requests.
When copying data, gparted will try using very large I/O sizes. Of course if something breaks it will just use a smaller size, but it would make performance worse.
I tried now (with local storage, not virtual---but with such large block sizes it's disk bound anyway, one request can take 0.1 seconds to execute) and a 2 MB block size is 20% slower than 16 MB block size on your usual 3.5" rotational SATA disk.
Paolo
Quoted 4 lines
Afaik the reported
limit for e.g. virtio-blk is 1024 sectors (reported through blockdev --getmaxsect/dev/vda).
I think it was only themultiwrite_mergecode causing trouble here.
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 11 lines
Am 08.09.2014 um 17:27 schrieb Paolo Bonzini <[email protected]>:
Il 08/09/2014 17:18, Peter Lieven ha scritto:
Quoted 5 lines
Quoted 3 lines
That's why we have splitting code for discard, and why we would have to add it for read/write too.
Why should a guest generate such big requests.
Quoted 9 lines
When copying data, gparted will try using very large I/O sizes. Of course if something breaks it will just use a smaller size, but it would make performance worse.
I tried now (with local storage, not virtual---but with such large block sizes it's disk bound anyway, one request can take 0.1 seconds to execute) and a 2 MB block size is 20% slower than 16 MB block size on your usual 3.5" rotational SATA disk.
can you share with what command exactly you ran these tests?
i tried myself and found that without multiwrite_merge i was not able to create a request bigger than 0xffff sectors from inside linux.
Peter
Quoted 8 lines
Paolo
Quoted 4 lines
Afaik the reported
limit for e.g. virtio-blk is 1024 sectors (reported through blockdev --getmaxsect/dev/vda).
I think it was only themultiwrite_mergecode causing trouble here.
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 08/09/2014 18:18, Peter Lieven ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
When copying data, gparted will try using very large I/O sizes. Of course if something breaks it will just use a smaller size, but it would make performance worse.
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
I tried now (with local storage, not virtual---but with such large block sizes it's disk bound anyway, one request can take 0.1 seconds to execute) and a 2 MB block size is 20% slower than 16 MB block size on your usual 3.5" rotational SATA disk.
can you share with what command exactly you ran these tests?
i tried myself and found that without
multiwrite_mergei was not able to create a request bigger than0xffffsectors from inside linux.
On a different machine:
$ time dd if=/dev/zero of=test bs=16777216 count=30 oflag=direct real 0m13.497s user 0m0.001s sys 0m0.541s
$ time dd if=/dev/zero of=test2 bs=1048576 count=480 oflag=direct real 0m15.835s user 0m0.005s sys 0m0.770s
The bigger block size is 17% faster; for disk-to-disk copy:
$ time dd if=test of=test3 bs=16777216 count=30 iflag=direct oflag=direct real 0m26.075s user 0m0.001s sys 0m0.678s
$ time dd if=test2 of=test4 bs=1048576 count=480 iflag=direct oflag=direct real 0m45.210s user 0m0.005s sys 0m1.145s
The bigger block size is 73% faster.
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Am 08.09.2014 um 18:29 schrieb Paolo Bonzini:
Quoted 32 lines
Il 08/09/2014 18:18, Peter Lieven ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
When copying data, gparted will try using very large I/O sizes. Of course if something breaks it will just use a smaller size, but it would make performance worse.
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
I tried now (with local storage, not virtual---but with such large block sizes it's disk bound anyway, one request can take 0.1 seconds to execute) and a 2 MB block size is 20% slower than 16 MB block size on your usual 3.5" rotational SATA disk.
can you share with what command exactly you ran these tests?
i tried myself and found that without
multiwrite_mergei was not able to create a request bigger than0xffffsectors from inside linux.On a different machine:
$ time dd if=/dev/zero of=test bs=16777216 count=30 oflag=direct real 0m13.497s user 0m0.001s sys 0m0.541s$ time dd if=/dev/zero of=test2 bs=1048576 count=480 oflag=direct real 0m15.835s user 0m0.005s sys 0m0.770sThe bigger block size is 17% faster; for disk-to-disk copy:
$ time dd if=test of=test3 bs=16777216 count=30 iflag=direct oflag=direct real 0m26.075s user 0m0.001s sys 0m0.678s
Quoted 6 lines
$ time dd if=test2 of=test4 bs=1048576 count=480 iflag=direct oflag=direct real 0m45.210s user 0m0.005s sys 0m1.145sThe bigger block size is 73% faster.
I perfectly believe that 16MB blocksize is faster than 2MB. That is true for iSCSI as well.
However, I do not see requests of this size coming in via virtio-blk when I ran:
dd if=/dev/zero of=test bs=16777216 count=30 oflag=direct.
As you can see from the multiwrite_merge trace the merging has never been stopped because of the max_transfer_length. The question is, why are the I/O requests not coming in as specified?
multiwrite_merge: num_reqs 15 -> 1iscsi_co_writev: sector_num 0 nb_sectors 15360 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 17 -> 1iscsi_co_writev: sector_num 15360 nb_sectors 17408 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 32768 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 44032 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 63488 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 12 -> 1iscsi_co_writev: sector_num 65536 nb_sectors 12288 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 18 -> 1iscsi_co_writev: sector_num 77824 nb_sectors 18432 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 96256 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 12 -> 1iscsi_co_writev: sector_num 98304 nb_sectors 12288 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 110592 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 1 -> 1iscsi_co_writev: sector_num 130048 nb_sectors 1024 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 131072 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 142336 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 161792 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 12 -> 1iscsi_co_writev: sector_num 163840 nb_sectors 12288 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 176128 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 1 -> 1iscsi_co_writev: sector_num 195584 nb_sectors 1024 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 7 -> 1iscsi_co_writev: sector_num 196608 nb_sectors 7168 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 18 -> 1iscsi_co_writev: sector_num 203776 nb_sectors 18432 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 7 -> 1iscsi_co_writev: sector_num 222208 nb_sectors 7168 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 229376 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 240640 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 260096 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 262144 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 18 -> 1iscsi_co_writev: sector_num 273408 nb_sectors 18432 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 3 -> 1iscsi_co_writev: sector_num 291840 nb_sectors 3072 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 12 -> 1iscsi_co_writev: sector_num 294912 nb_sectors 12288 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 307200 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 1 -> 1iscsi_co_writev: sector_num 326656 nb_sectors 1024 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 327680 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 338944 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 358400 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 360448 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 371712 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 391168 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 393216 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 14 -> 1iscsi_co_writev: sector_num 404480 nb_sectors 14336 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 7 -> 1iscsi_co_writev: sector_num 418816 nb_sectors 7168 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 425984 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 20 -> 1iscsi_co_writev: sector_num 437248 nb_sectors 20480 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 1 -> 1iscsi_co_writev: sector_num 457728 nb_sectors 1024 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 458752 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 18 -> 1iscsi_co_writev: sector_num 470016 nb_sectors 18432 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 3 -> 1iscsi_co_writev: sector_num 488448 nb_sectors 3072 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 491520 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 502784 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 522240 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 524288 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 18 -> 1iscsi_co_writev: sector_num 535552 nb_sectors 18432 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 3 -> 1iscsi_co_writev: sector_num 553984 nb_sectors 3072 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 557056 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 568320 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 587776 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 589824 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 601088 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 620544 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 12 -> 1iscsi_co_writev: sector_num 622592 nb_sectors 12288 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 18 -> 1iscsi_co_writev: sector_num 634880 nb_sectors 18432 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 653312 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 655360 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 666624 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 686080 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 688128 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 19 -> 1iscsi_co_writev: sector_num 699392 nb_sectors 19456 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 718848 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 9 -> 1iscsi_co_writev: sector_num 720896 nb_sectors 9216 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 18 -> 1iscsi_co_writev: sector_num 730112 nb_sectors 18432 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 5 -> 1iscsi_co_writev: sector_num 748544 nb_sectors 5120 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 12 -> 1iscsi_co_writev: sector_num 753664 nb_sectors 12288 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 7 -> 1iscsi_co_writev: sector_num 765952 nb_sectors 7168 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 6 -> 1iscsi_co_writev: sector_num 773120 nb_sectors 6144 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 7 -> 1iscsi_co_writev: sector_num 779264 nb_sectors 7168 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 786432 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 14 -> 1iscsi_co_writev: sector_num 797696 nb_sectors 14336 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 5 -> 1iscsi_co_writev: sector_num 812032 nb_sectors 5120 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 817152 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 8 -> 1iscsi_co_writev: sector_num 819200 nb_sectors 8192 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 13 -> 1iscsi_co_writev: sector_num 827392 nb_sectors 13312 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 6 -> 1iscsi_co_writev: sector_num 840704 nb_sectors 6144 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 5 -> 1iscsi_co_writev: sector_num 846848 nb_sectors 5120 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 851968 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 14 -> 1iscsi_co_writev: sector_num 863232 nb_sectors 14336 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 6 -> 1iscsi_co_writev: sector_num 877568 nb_sectors 6144 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 1 -> 1iscsi_co_writev: sector_num 883712 nb_sectors 1024 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 884736 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 896000 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 5 -> 1iscsi_co_writev: sector_num 907264 nb_sectors 5120 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 5 -> 1iscsi_co_writev: sector_num 912384 nb_sectors 5120 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 11 -> 1iscsi_co_writev: sector_num 917504 nb_sectors 11264 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 14 -> 1iscsi_co_writev: sector_num 928768 nb_sectors 14336 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 5 -> 1iscsi_co_writev: sector_num 943104 nb_sectors 5120 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 2 -> 1iscsi_co_writev: sector_num 948224 nb_sectors 2048 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 12 -> 1iscsi_co_writev: sector_num 950272 nb_sectors 12288 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 15 -> 1iscsi_co_writev: sector_num 962560 nb_sectors 15360 bs->bl.max_transfer_length 65535multiwrite_merge: num_reqs 5 -> 1iscsi_co_writev: sector_num 977920 nb_sectors 5120 bs->bl.max_transfer_length 65535
Peter
Quoted 2 lines
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 12/09/2014 13:43, Peter Lieven ha scritto:
Quoted 3 lines
As you can see from the
multiwrite_mergetrace the merging has never been stopped because of themax_transfer_length. The question is, why are the I/O requests not coming in as specified?
I think that's because of the filesystem. Try writing directly to a device.
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Quoted 4 lines · Paolo Bonzini
Il 12/09/2014 13:43, Peter Lieven ha scritto:
Quoted 2 lines
As you can see from the
multiwrite_mergetrace the merging has never been stopped because of themax_transfer_length. The question is, why are the I/O requests not coming in as specified?I think that's because of the filesystem. Try writing directly to a device.
thats what I did...
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Il 18/09/2014 16:16, Peter Lieven ha scritto:
Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
As you can see from the
multiwrite_mergetrace the merging has never been stopped because of
themax_transfer_length. The question is, why are the I/O requests not coming in as specified?I think that's because of the filesystem. Try writing directly to a device.
thats what I did...
Ah, I saw of=test.
Paolo
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
Am 18.09.2014 um 16:17 schrieb Paolo Bonzini <[email protected]>:
Quoted 6 lines
Il 18/09/2014 16:16, Peter Lieven ha scritto:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
As you can see from the
multiwrite_mergetrace the merging has never been stopped because of
themax_transfer_length. The question is, why are the I/O requests not coming in as specified?
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
I think that's because of the filesystem. Try writing directly to a device.
thats what I did...
Ah, I saw of=test.
It was definitely /dev/vda. Any ideas? Maybe something in the virtio drivers?
Peter
Re: [Qemu-devel] [PATCH 2/4] block: immediately cancel oversized read/write requests
In reply to ronnie sahlbergQuoted 20 lines · ronnie sahlberg
Quoted 11 lines · Paolo Bonzini
Il 08/09/2014 16:35, Peter Lieven ha scritto:
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
messages. :)
So you would not throw an error msg here?
No, though a trace could be useful.
Is there a howto somewhere how to implement that?
Try commit
4ac4458076e1aaf3b01a6361154117df20e22215.Quoted 2 lines
Whats your opinion changed the
max_xfer_lento0xffffregardsless ofuse_16_for_rwin iSCSI?If you implemented request splitting in the block layer, it would be okay to force
max_xfer_lento0xffff.I think it should be OK if that is a different series. This only affects the iSCSI transport since it is the only transport so far to record or report a max transfer length.
If a guest generates these big requests(i.e. not multiwrite) we already fail them due to the READ10 limitations. We would just fail the request at a different layer in the stack with these patches.
Quoted 3 lines
What I would like to see would also be to report these limitations to the guest itself to prevent it from generating too large I/Os.
I am willing to do that part once the initialmax_xfer_lenones are finished.
Yes, I also think this approach is straightforward. We should avoid big requests at the beginning and not fix them at the end. I had a look it shouldn't be too hard. There are default values for virtio-scsi HD inquiry emulation which are set if no cmdline values are specified. It should be possible to feed them from bs->bl if set.
Peter
PATCH 3/4 block/iscsi: set max_transfer_length
In reply to Peter Lieventhe limit of 0xffffff for 16 byte CDBs is intentional to avoid overflows on 32-bit architectures.
--- block/iscsi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
block/iscsi.c +10 -2
@@ -1455,10 +1455,18 @@ static void iscsi_close(BlockDriverState *bs)static void iscsi_refresh_limits (BlockDriverState *bs,Error **errp) { -IscsiLun *iscsilun = bs->opaque; -/* We don't actually refresh here, but just return data queried in * iscsi_open(): iscsi targets don't change their limits. */ + +IscsiLun *iscsilun = bs->opaque; +uint32_t max_xfer_len = iscsilun->use_16_for_rw ?0xffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =sector_lun2qemu (max_xfer_len, iscsilun); +if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
Signature · 1 line
1.7.9.5
PATCH 4/4 block: avoid creating oversized writes in multiwrite_merge
In reply to Peter Lieven--- block.c | 5 +++++ 1 file changed, 5 insertions(+)
block.c +5 -0
@@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, merge =0 ; } +if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors + + reqs[i].nb_sectors > bs->bl.max_transfer_length) { + merge =0 ; + } +if (merge) {size_t size;QEMUIOVector *qiov =g_malloc0 (sizeof (*qiov));
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Il 05/09/2014 18:51, Peter Lieven ha scritto:
Quoted 12 lines
Signed-off-by Peter Lieven <[email protected]>--- block.c | 5 +++++ 1 file changed, 5 insertions(+)block.c +0
-0@@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, merge =0 ;
block.c static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, · line 4564
Quoted 10 lines
block.c +5
-0} +if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors + + reqs[i].nb_sectors > bs->bl.max_transfer_length) { + merge =0 ; + } +if (merge) {size_t size;QEMUIOVector *qiov =g_malloc0 (sizeof (*qiov));
So I think if we treat it just as a hint for multiwrite, we can avoid writing code to split oversized requests. They always worked so far, we can certainly wait until we have a real bugfix.
Can you drop patch 2 and resend the rest?
Thanks,
Paolo
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Am 18.09.2014 um 16:13 schrieb Paolo Bonzini <[email protected]>:
Quoted 14 lines
Il 05/09/2014 18:51, Peter Lieven ha scritto:
Quoted 13 lines
Signed-off-by Peter Lieven <[email protected]>---
block.c| 5 +++++1 file changed, 5 insertions(+)block.c +0
-0@@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, merge =0 ; }
Quoted 13 lines
block.c static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, · line 4564
Quoted 9 lines
block.c +5
-0+if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors + + reqs[i].nb_sectors > bs->bl.max_transfer_length) { + merge =0 ; + } +if (merge) {size_t size;QEMUIOVector *qiov =g_malloc0 (sizeof (*qiov));So I think if we treat it just as a hint for multiwrite, we can avoid writing code to split oversized requests. They always worked so far, we can certainly wait until we have a real bug fix.
I would not treat this as a hint. I would use it in cases where we definitely know an absolute hard limit for I/O request size. Otherwise the value for bs->bl.max_transfer_length should be 0.
If there comes in an oversized request we fail it as early as possible and regarding the multi write code we avoid that it accidentally generates an oversized request.
Peter
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Il 19/09/2014 00:56, Peter Lieven ha scritto:
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
So I think if we treat it just as a hint for multiwrite, we can avoid writing code to split oversized requests. They always worked so far, we can certainly wait until we have a real bug fix.
I would not treat this as a hint. I would use it in cases where we definitely know an absolute hard limit for I/O request size. Otherwise the value for
bs->bl.max_transfer_lengthshould be 0.If there comes in an oversized request we fail it as early as possible
That's the part that I'd rather not touch, at least not without doing request splitting.
Paolo
and regarding
Quoted 1 line
the multi write code we avoid that it accidentally generates an oversized request.
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Quoted 11 lines · Paolo Bonzini
Il 19/09/2014 00:56, Peter Lieven ha scritto:
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
So I think if we treat it just as a hint for multiwrite, we can avoid writing code to split oversized requests. They always worked so far, we can certainly wait until we have a real bug fix.
I would not treat this as a hint. I would use it in cases where we definitely know an absolute hard limit for I/O request size. Otherwise the value for
bs->bl.max_transfer_lengthshould be 0.If there comes in an oversized request we fail it as early as possible
That's the part that I'd rather not touch, at least not without doing request splitting.
This series aims not at touching default behaviour. The default for max_transfer_length is 0 (no limit). max_transfer_length is a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack. Currently, we only have a limit for iSCSI. Without Patch 2 it would fail after we have send the command to the target. And without Patch 4 it may happen that multiwrite_merge traps the into the limit.
Maybe I should adjust the description of max_transfer_length?
Peter
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.
Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since we never had a bug report about this, I'm not pushing to implement splitting. However, patch 2 is still wrong.
Patch 4 instead is fixing a real bug, so it's very much welcome.
Paolo
Quoted 7 lines
Currently, we only
have a limit for iSCSI. Without Patch 2 it would fail after we have send the command to
the target. And without Patch 4 it may happen thatmultiwrite_mergetraps the into the limit.Maybe I should adjust the description of
max_transfer_length?
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Quoted 7 lines · Paolo Bonzini
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.
Peter
Quoted 15 lines
Since we never had a bug report about this, I'm not pushing to implement splitting. However, patch 2 is still wrong.
Patch 4 instead is fixing a real bug, so it's very much welcome.
Paolo
Quoted 7 lines
Currently, we only
have a limit for iSCSI. Without Patch 2 it would fail after we have send the command to
the target. And without Patch 4 it may happen thatmultiwrite_mergetraps the into the limit.Maybe I should adjust the description of
max_transfer_length?
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 9 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Quoted 8 lines
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.
You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (qemu_iovec_concat and friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.
Kevin
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Quoted 13 lines · Kevin Wolf
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 12 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
Quoted 8 lines
Quoted 4 lines
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (
qemu_iovec_concatand friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.
Thanks for that pointer. Looking at qemu_iovec_concat this seems to be a nobrainer.
I will first send an updated series dropping Patch 2 and will then send a follow-up that splits requests.
Peter
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
In reply to Kevin WolfQuoted 13 lines · Kevin Wolf
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 12 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
Quoted 8 lines
Quoted 4 lines
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (
qemu_iovec_concatand friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.
Might it be as easy as this?
static int coroutine_fn bdrv_co_do_readv(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov, BdrvRequestFlags flags)
{
if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) {
return -EINVAL;
}
if (bs->bl.max_transfer_length &&
nb_sectors > bs->bl.max_transfer_length) {
int ret = 0;
QEMUIOVector *qiov2 = NULL;
size_t soffset = 0;
trace_bdrv_co_do_readv_toobig(bs, sector_num, nb_sectors,bs->bl.max_transfer_length);
qemu_iovec_init(qiov2, qiov->niov);
while (nb_sectors > bs->bl.max_transfer_length && !ret) {
qemu_iovec_reset(qiov2);
qemu_iovec_concat(qiov2, qiov, soffset,
bs->bl.max_transfer_length << BDRV_SECTOR_BITS);
ret = bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS,
bs->bl.max_transfer_length << BDRV_SECTOR_BITS,
qiov2, flags);
soffset += bs->bl.max_transfer_length << BDRV_SECTOR_BITS;
sector_num += bs->bl.max_transfer_length;
nb_sectors -= bs->bl.max_transfer_length;
}
qemu_iovec_destroy(qiov2);
if (ret) {
return ret;
}
}
return bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS,
nb_sectors << BDRV_SECTOR_BITS, qiov, flags);
}Quoted 2 lines
Kevin
Signature · 16 lines
Mit freundlichen Grüßen Peter Lieven ........................................................... KAMP Netzwerkdienste GmbH Vestische Str. 89-91 | 46117 Oberhausen Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40 [email protected] | http://www.kamp.de Geschäftsführer: Heiner Lante | Michael Lante Amtsgericht Duisburg | HRB Nr. 12154 USt-Id-Nr.: DE 120607556 ...........................................................
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Am 23.09.2014 um 11:32 hat Peter Lieven geschrieben:
Quoted 18 lines · Kevin Wolf
Quoted 17 lines
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 16 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.
Quoted 6 lines
Quoted 4 lines
You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (
qemu_iovec_concatand friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.Might it be as easy as this?
This is completely untested, right? :-)
But ignoring bugs, the principle looks right.
Quoted 12 lines
static int
coroutine_fnbdrv_co_do_readv(BlockDriverState *bs,
int64_tsector_num, intnb_sectors, QEMUIOVector *qiov, BdrvRequestFlags flags){ if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) { return -EINVAL; } if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { int ret = 0; QEMUIOVector *qiov2 = NULL;
Make it "QEMUIOVector qiov2;" on the stack.
Quoted 6 lines
size_tsoffset = 0;trace_bdrv_co_do_readv_toobig(bs,
sector_num,nb_sectors,bs->bl.max_transfer_length);qemu_iovec_init(qiov2, qiov->niov);
And &qiov2 here, then this doesn't crash with a NULL dereference.
Quoted 7 lines
while (nb_sectors > bs->bl.max_transfer_length && !ret) {
qemu_iovec_reset(qiov2);
qemu_iovec_concat(qiov2, qiov, soffset,
bs->bl.max_transfer_length << BDRV_SECTOR_BITS);
ret = bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS,
bs->bl.max_transfer_length << BDRV_SECTOR_BITS,
qiov2, flags);Quoted 8 lines
soffset += bs->bl.max_transfer_length << BDRV_SECTOR_BITS;
sector_num += bs->bl.max_transfer_length;
nb_sectors -= bs->bl.max_transfer_length;
}
qemu_iovec_destroy(qiov2);
if (ret) {
return ret;
}The error check needs to be immediately after the assignment of ret, otherwise the next loop iteration can overwrite an error with a success (and if it didn't, it would still do useless I/O because the request as a whole would fail anyway).
Quoted 4 lines
}
return bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS,
nb_sectors << BDRV_SECTOR_BITS, qiov, flags);qiov doesn't work here for the splitting case. You need the remaining part, not the whole original qiov.
Kevin
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Quoted 19 lines · Kevin Wolf
Am 23.09.2014 um 11:32 hat Peter Lieven geschrieben:
Quoted 18 lines · Kevin Wolf
Quoted 17 lines
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 16 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines
You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (
qemu_iovec_concatand friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.Might it be as easy as this?
This is completely untested, right? :-)
Yes :-)
I was just unsure if the general approach is right.
Quoted 40 lines
But ignoring bugs, the principle looks right.
Quoted 12 lines
static int
coroutine_fnbdrv_co_do_readv(BlockDriverState *bs,
int64_tsector_num, intnb_sectors, QEMUIOVector *qiov, BdrvRequestFlags flags){ if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) { return -EINVAL; } if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { int ret = 0; QEMUIOVector *qiov2 = NULL;Make it "QEMUIOVector qiov2;" on the stack.
Quoted 6 lines
size_tsoffset = 0;trace_bdrv_co_do_readv_toobig(bs,
sector_num,nb_sectors,bs->bl.max_transfer_length);qemu_iovec_init(qiov2, qiov->niov);And &qiov2 here, then this doesn't crash with a NULL dereference.
Quoted 7 lines
while (nb_sectors > bs->bl.max_transfer_length && !ret) { qemu_iovec_reset(qiov2); qemu_iovec_concat(qiov2, qiov, soffset, bs->bl.max_transfer_length << BDRV_SECTOR_BITS); ret = bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS, bs->bl.max_transfer_length << BDRV_SECTOR_BITS, qiov2, flags);Quoted 8 lines
soffset += bs->bl.max_transfer_length << BDRV_SECTOR_BITS; sector_num += bs->bl.max_transfer_length; nb_sectors -= bs->bl.max_transfer_length; } qemu_iovec_destroy(qiov2); if (ret) { return ret; }
Quoted 4 lines
The error check needs to be immediately after the assignment of ret, otherwise the next loop iteration can overwrite an error with a success (and if it didn't, it would still do useless I/O because the request as a whole would fail anyway).
There is a && !ret in the loop condition. I wanted to avoid copying the destroy part.
BTW, is it !ret or ret < 0 ?
Quoted 7 lines
Quoted 5 lines
} return bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS, nb_sectors << BDRV_SECTOR_BITS, qiov, flags);qiov doesn't work here for the splitting case. You need the remaining part, not the whole original qiov.
Right ;-)
Peter
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Am 23.09.2014 um 11:52 hat Peter Lieven geschrieben:
Quoted 24 lines · Kevin Wolf
Quoted 23 lines
Am 23.09.2014 um 11:32 hat Peter Lieven geschrieben:
Quoted 22 lines · Kevin Wolf
Quoted 21 lines
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 8 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Quoted 8 lines
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (
qemu_iovec_concatand friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.
Quoted 5 lines
Quoted 2 lines
Quoted 1 line
Might it be as easy as this?
This is completely untested, right? :-)
Yes :-)
I was just unsure if the general approach is right.
Looks alright to me.
Quoted 39 lines
Quoted 39 lines
But ignoring bugs, the principle looks right.
Quoted 12 lines
static int
coroutine_fnbdrv_co_do_readv(BlockDriverState *bs,
int64_tsector_num, intnb_sectors, QEMUIOVector *qiov, BdrvRequestFlags flags){ if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) { return -EINVAL; } if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { int ret = 0; QEMUIOVector *qiov2 = NULL;Make it "QEMUIOVector qiov2;" on the stack.
Quoted 6 lines
size_tsoffset = 0;trace_bdrv_co_do_readv_toobig(bs,
sector_num,nb_sectors,bs->bl.max_transfer_length);qemu_iovec_init(qiov2, qiov->niov);And &qiov2 here, then this doesn't crash with a NULL dereference.
Quoted 15 lines
while (nb_sectors > bs->bl.max_transfer_length && !ret) { qemu_iovec_reset(qiov2); qemu_iovec_concat(qiov2, qiov, soffset, bs->bl.max_transfer_length << BDRV_SECTOR_BITS); ret = bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS, bs->bl.max_transfer_length << BDRV_SECTOR_BITS, qiov2, flags); soffset += bs->bl.max_transfer_length << BDRV_SECTOR_BITS; sector_num += bs->bl.max_transfer_length; nb_sectors -= bs->bl.max_transfer_length; } qemu_iovec_destroy(qiov2); if (ret) { return ret; }
Quoted 6 lines
Quoted 4 lines
The error check needs to be immediately after the assignment of ret, otherwise the next loop iteration can overwrite an error with a success (and if it didn't, it would still do useless I/O because the request as a whole would fail anyway).
There is a && !ret in the loop condition. I wanted to avoid copying the destroy part.
Ah, yes, clever. I missed that. Maybe too clever then. ;-)
Quoted 1 line
BTW, is it !ret or ret < 0 ?
It only ever returns 0 or negative, so both are equivalent. I prefer checks for ret < 0, but that's a matter of style rather than correctness.
Another problem I just noticed is that this is not the only caller of bdrv_co_do_preadv(), so you're not splitting all requests. The synchronous bdrv_read/write/pread/pwrite/pwritev functions all don't get the functionality this way.
Perhaps you should be doing it inside bdrv_co_do_preadv(), before the call to bdrv_aligned_preadv(). Might even be more correct if it can happen that the alignment adjustment increases a request too much to fit in bl.max_transfer_length.
Kevin
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Quoted 87 lines · Kevin Wolf
Am 23.09.2014 um 11:52 hat Peter Lieven geschrieben:
Quoted 20 lines · Kevin Wolf
Quoted 19 lines
Am 23.09.2014 um 11:32 hat Peter Lieven geschrieben:
Quoted 18 lines · Kevin Wolf
Quoted 17 lines
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 16 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines
You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (
qemu_iovec_concatand friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.Might it be as easy as this?
This is completely untested, right? :-)
Yes :-)
I was just unsure if the general approach is right.Looks alright to me.
Quoted 39 lines
Quoted 39 lines
But ignoring bugs, the principle looks right.
Quoted 12 lines
static int
coroutine_fnbdrv_co_do_readv(BlockDriverState *bs,
int64_tsector_num, intnb_sectors, QEMUIOVector *qiov, BdrvRequestFlags flags){ if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) { return -EINVAL; } if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { int ret = 0; QEMUIOVector *qiov2 = NULL;Make it "QEMUIOVector qiov2;" on the stack.
Quoted 6 lines
size_tsoffset = 0;trace_bdrv_co_do_readv_toobig(bs,
sector_num,nb_sectors,bs->bl.max_transfer_length);qemu_iovec_init(qiov2, qiov->niov);And &qiov2 here, then this doesn't crash with a NULL dereference.
Quoted 15 lines
while (nb_sectors > bs->bl.max_transfer_length && !ret) { qemu_iovec_reset(qiov2); qemu_iovec_concat(qiov2, qiov, soffset, bs->bl.max_transfer_length << BDRV_SECTOR_BITS); ret = bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS, bs->bl.max_transfer_length << BDRV_SECTOR_BITS, qiov2, flags); soffset += bs->bl.max_transfer_length << BDRV_SECTOR_BITS; sector_num += bs->bl.max_transfer_length; nb_sectors -= bs->bl.max_transfer_length; } qemu_iovec_destroy(qiov2); if (ret) { return ret; }Quoted 5 lines
Quoted 4 lines
The error check needs to be immediately after the assignment of ret, otherwise the next loop iteration can overwrite an error with a success (and if it didn't, it would still do useless I/O because the request as a whole would fail anyway).
There is a && !ret in the loop condition. I wanted to avoid copying the destroy part.
Ah, yes, clever. I missed that. Maybe too clever then. ;-)
Quoted 1 line
BTW, is it !ret or ret < 0 ?
It only ever returns 0 or negative, so both are equivalent. I prefer checks for ret < 0, but that's a matter of style rather than correctness.
Another problem I just noticed is that this is not the only caller of
bdrv_co_do_preadv(), so you're not splitting all requests. The synchronous bdrv_read/write/pread/pwrite/pwritev functions all don't get the functionality this way.
Quoted 4 lines
Perhaps you should be doing it inside
bdrv_co_do_preadv(), before the call tobdrv_aligned_preadv(). Might even be more correct if it can happen that the alignment adjustment increases a request too much to fit in bl.max_transfer_length.
If I do it this way can I use the same req Object for all splitted requests?
Peter
Re: [Qemu-devel] [PATCH 4/4] block: avoid creating oversized writes in multiwrite_merge
Am 30.09.2014 um 09:26 hat Peter Lieven geschrieben:
Quoted 88 lines · Kevin Wolf
Quoted 87 lines
Am 23.09.2014 um 11:52 hat Peter Lieven geschrieben:
Quoted 20 lines · Kevin Wolf
Quoted 19 lines
Am 23.09.2014 um 11:32 hat Peter Lieven geschrieben:
Quoted 18 lines · Kevin Wolf
Quoted 17 lines
Am 23.09.2014 um 08:15 hat Peter Lieven geschrieben:
Quoted 16 lines · Paolo Bonzini
Quoted 7 lines
Il 22/09/2014 11:43, Peter Lieven ha scritto:
Quoted 3 lines
This series aims not at touching default behaviour. The default for
max_transfer_lengthis 0 (no limit).max_transfer_lengthis a limit that MUST be satisfied otherwise the request will fail. And Patch 2 aims at catching this fail earlier in the stack.Understood. But the right fix is to avoid that backend limits transpire into guest ABI, not to catch the limits earlier. So the right fix would be to implement request splitting.
Since you proposed to add traces for this would you leave those in? And since iSCSI is the only user of this at the moment would you go for implementing this check in the iSCSI block layer?
As for the split logic would you think it is enough to build new qiov's out of the too big iov without copying the contents? This would work as long as a single iov inside the qiov is not bigger the
max_transfer_length. Otherwise I would need to allocate temporary buffers and copy around.Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines
You can split single iovs, too. There are functions that make this very easy, they copy a sub-qiov with a byte granularity offset and length (
qemu_iovec_concatand friends). qcow2 uses the same to split requests at (fragmented) cluster boundaries.Might it be as easy as this?
This is completely untested, right? :-)
Yes :-)
I was just unsure if the general approach is right.Looks alright to me.
Quoted 39 lines
Quoted 39 lines
But ignoring bugs, the principle looks right.
Quoted 12 lines
static int
coroutine_fnbdrv_co_do_readv(BlockDriverState *bs,
int64_tsector_num, intnb_sectors, QEMUIOVector *qiov, BdrvRequestFlags flags){ if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) { return -EINVAL; } if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { int ret = 0; QEMUIOVector *qiov2 = NULL;Make it "QEMUIOVector qiov2;" on the stack.
Quoted 6 lines
size_tsoffset = 0;trace_bdrv_co_do_readv_toobig(bs,
sector_num,nb_sectors,bs->bl.max_transfer_length);qemu_iovec_init(qiov2, qiov->niov);And &qiov2 here, then this doesn't crash with a NULL dereference.
Quoted 15 lines
while (nb_sectors > bs->bl.max_transfer_length && !ret) { qemu_iovec_reset(qiov2); qemu_iovec_concat(qiov2, qiov, soffset, bs->bl.max_transfer_length << BDRV_SECTOR_BITS); ret = bdrv_co_do_preadv(bs, sector_num << BDRV_SECTOR_BITS, bs->bl.max_transfer_length << BDRV_SECTOR_BITS, qiov2, flags); soffset += bs->bl.max_transfer_length << BDRV_SECTOR_BITS; sector_num += bs->bl.max_transfer_length; nb_sectors -= bs->bl.max_transfer_length; } qemu_iovec_destroy(qiov2); if (ret) { return ret; }Quoted 5 lines
Quoted 4 lines
The error check needs to be immediately after the assignment of ret, otherwise the next loop iteration can overwrite an error with a success (and if it didn't, it would still do useless I/O because the request as a whole would fail anyway).
There is a && !ret in the loop condition. I wanted to avoid copying the destroy part.
Ah, yes, clever. I missed that. Maybe too clever then. ;-)
Quoted 1 line
BTW, is it !ret or ret < 0 ?
It only ever returns 0 or negative, so both are equivalent. I prefer checks for ret < 0, but that's a matter of style rather than correctness.
Another problem I just noticed is that this is not the only caller of
bdrv_co_do_preadv(), so you're not splitting all requests. The synchronous bdrv_read/write/pread/pwrite/pwritev functions all don't get the functionality this way.
Quoted 7 lines
Quoted 4 lines
Perhaps you should be doing it inside
bdrv_co_do_preadv(), before the call tobdrv_aligned_preadv(). Might even be more correct if it can happen that the alignment adjustment increases a request too much to fit in bl.max_transfer_length.If I do it this way can I use the same req Object for all splitted requests?
That's a good question. I think as long as you process the parts of the split request one after another, reusing the same req object should be safe. If you were to process them in parallel, though, I wouldn't be as sure about it (which you probably don't want because it complicates things :-)).
The probably most obviously correct way to handle things would be to have one tracked_request_begin/end() for the whole request and then call bdrv_aligned_preadv() multiple times in between. Otherwise you'd have to serialise each part individually etc.
Kevin
Looks good to me.
(minor question is just why not let default max be 0xffff for both 10 and 16 CDBs ?)
Quoted 23 lines · Peter Lieven
This series adds the basics for introducing a maximum transfer length to the block layer. Its main purpose is currently avoiding that a
multiwrite_mergeexceeds themax_xfer_lenof an attached iSCSI LUN. This is a required bug fix.Discussed reporting of this maximum in the SCSI Disk Inquiry Emulation of the Block Limits VPD is currently not added as we do not import any of the other limits there. This has to be addresses in a seperate series.
Peter Lieven (4):
BlockLimits: introducemax_transfer_length
block: immediate cancel oversized read/write requests block/iscsi: setmax_transfer_length
block: avoid creating oversized writes inmultiwrite_mergeblock.c | 23 +++++++++++++++++++++++ block/iscsi.c | 12 ++++++++++-- include/block/block_int.h | 3 +++ 3 files changed, 36 insertions(+), 2 deletions(-)--
1.7.9.5
Am 05.09.2014 um 19:05 schrieb ronnie sahlberg:
Quoted 4 lines
Looks good to me.
(minor question is just why not let default max be
0xfffffor both 10 and 16 CDBs ?)
You are right. I was looking at the technical limit, but in fact it doesn't make sense to have different limits. Its ridiculous to say, you wan't to do big I/O then you need a target thats bigger than 2TB ;-)
Peter
Quoted 25 lines · Peter Lieven
Quoted 23 lines
This series adds the basics for introducing a maximum transfer length to the block layer. Its main purpose is currently avoiding that a
multiwrite_mergeexceeds themax_xfer_lenof an attached iSCSI LUN. This is a required bug fix.Discussed reporting of this maximum in the SCSI Disk Inquiry Emulation of the Block Limits VPD is currently not added as we do not import any of the other limits there. This has to be addresses in a seperate series.
Peter Lieven (4):
BlockLimits: introducemax_transfer_length
block: immediate cancel oversized read/write requests block/iscsi: setmax_transfer_length
block: avoid creating oversized writes inmultiwrite_mergeblock.c | 23 +++++++++++++++++++++++ block/iscsi.c | 12 ++++++++++-- include/block/block_int.h | 3 +++ 3 files changed, 36 insertions(+), 2 deletions(-)--
1.7.9.5
ronnie sahlberg
Reviewed-byFeel free to add a
to the patches.
Quoted 38 lines · Peter Lieven
Am 05.09.2014 um 19:05 schrieb ronnie sahlberg:
Quoted 4 lines
Looks good to me.
(minor question is just why not let default max be
0xfffffor both 10 and 16 CDBs ?)You are right. I was looking at the technical limit, but in fact it doesn't make sense to have different limits. Its ridiculous to say, you wan't to do big I/O then you need a target thats bigger than 2TB ;-)
Peter
Quoted 25 lines · Peter Lieven
Quoted 23 lines
This series adds the basics for introducing a maximum transfer length to the block layer. Its main purpose is currently avoiding that a
multiwrite_mergeexceeds themax_xfer_lenof an attached iSCSI LUN. This is a required bug fix.Discussed reporting of this maximum in the SCSI Disk Inquiry Emulation of the Block Limits VPD is currently not added as we do not import any of the other limits there. This has to be addresses in a seperate series.
Peter Lieven (4):
BlockLimits: introducemax_transfer_length
block: immediate cancel oversized read/write requests block/iscsi: setmax_transfer_length
block: avoid creating oversized writes inmultiwrite_mergeblock.c | 23 +++++++++++++++++++++++ block/iscsi.c | 12 ++++++++++-- include/block/block_int.h | 3 +++ 3 files changed, 36 insertions(+), 2 deletions(-)--
1.7.9.5