Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-03 Thread Peter Lieven
Am 02.09.2014 um 21:30 schrieb Peter Lieven p...@kamp.de: Looking at the code, is it possible that not the guest is causing trouble here, but multiwrite_merge code? From what I see the only limit it has when merging requests is the number of IOVs. Any thoughts? Mine are: a)

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-03 Thread Stefan Hajnoczi
On Wed, Sep 03, 2014 at 10:09:21AM +0200, Peter Lieven wrote: Am 02.09.2014 um 21:30 schrieb Peter Lieven p...@kamp.de: Looking at the code, is it possible that not the guest is causing trouble here, but multiwrite_merge code? From what I see the only limit it has when merging

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-03 Thread Peter Lieven
Am 03.09.2014 um 14:31 schrieb Stefan Hajnoczi stefa...@redhat.com: On Wed, Sep 03, 2014 at 10:09:21AM +0200, Peter Lieven wrote: Am 02.09.2014 um 21:30 schrieb Peter Lieven p...@kamp.de: Looking at the code, is it possible that not the guest is causing trouble here, but

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-03 Thread ronnie sahlberg
On Wed, Sep 3, 2014 at 1:09 AM, Peter Lieven p...@kamp.de wrote: Am 02.09.2014 um 21:30 schrieb Peter Lieven p...@kamp.de: Looking at the code, is it possible that not the guest is causing trouble here, but multiwrite_merge code? From what I see the only limit it has when merging

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-03 Thread Paolo Bonzini
Il 03/09/2014 16:17, ronnie sahlberg ha scritto: I think (a) would be best. But I would suggest some small modifications: Set the default max to something even smaller, like 256 sectors. This should not have much effect on throughput since the client/initiator can just send several

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-03 Thread ronnie sahlberg
On Wed, Sep 3, 2014 at 7:18 AM, Paolo Bonzini pbonz...@redhat.com wrote: Il 03/09/2014 16:17, ronnie sahlberg ha scritto: I think (a) would be best. But I would suggest some small modifications: Set the default max to something even smaller, like 256 sectors. This should not have much effect

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-03 Thread Peter Lieven
Am 03.09.2014 um 16:48 schrieb ronnie sahlberg ronniesahlb...@gmail.com: On Wed, Sep 3, 2014 at 7:18 AM, Paolo Bonzini pbonz...@redhat.com wrote: Il 03/09/2014 16:17, ronnie sahlberg ha scritto: I think (a) would be best. But I would suggest some small modifications: Set the default max

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-02 Thread ronnie sahlberg
That is one big request. I assume the device reports no limit in the VPD page so we can not state it is the guest/application going beyond the allowed limit? I am not entirely sure what meaning the target assigns to Protocol Error means here. It could be that ~100M is way higher than

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-02 Thread Peter Lieven
Am 02.09.2014 um 17:28 schrieb ronnie sahlberg: That is one big request. I assume the device reports no limit in the VPD page so we can not state it is the guest/application going beyond the allowed limit? Yes, my storage reports no limit, but afaik there is no way to tell the guest the limit

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-02 Thread Peter Lieven
Looking at the code, is it possible that not the guest is causing trouble here, but multiwrite_merge code? From what I see the only limit it has when merging requests is the number of IOVs. Any thoughts? Mine are: a) Introducing bs-bl.max_request_size and set merge = 0 if the result would

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-09-01 Thread Peter Lieven
On 17.06.2014 13:46, Paolo Bonzini wrote: Il 17/06/2014 13:37, Peter Lieven ha scritto: On 17.06.2014 13:15, Paolo Bonzini wrote: Il 17/06/2014 08:14, Peter Lieven ha scritto: BTW, while debugging a case with a bigger storage supplier I found that open-iscsi seems to do exactly this

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-17 Thread Peter Lieven
On 05.06.2014 11:12, Michael Tokarev wrote: 04.06.2014 18:00, ronnie sahlberg wrote: That would mean you get to use the 10 version of the cdb even for very large devices (as long as the IO is for blocks at the beginning of the device) and thus provide partial avoidance of this issue for those

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-17 Thread Paolo Bonzini
Il 17/06/2014 08:14, Peter Lieven ha scritto: BTW, while debugging a case with a bigger storage supplier I found that open-iscsi seems to do exactly this undeterministic behaviour. I have a 3TB LUN. If I access 2TB sectors it uses READ10/WRITE10 and if I go beyond 2TB it changes to

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-17 Thread Peter Lieven
On 17.06.2014 13:15, Paolo Bonzini wrote: Il 17/06/2014 08:14, Peter Lieven ha scritto: BTW, while debugging a case with a bigger storage supplier I found that open-iscsi seems to do exactly this undeterministic behaviour. I have a 3TB LUN. If I access 2TB sectors it uses READ10/WRITE10 and

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-17 Thread Peter Lieven
On 17.06.2014 13:46, Paolo Bonzini wrote: Il 17/06/2014 13:37, Peter Lieven ha scritto: On 17.06.2014 13:15, Paolo Bonzini wrote: Il 17/06/2014 08:14, Peter Lieven ha scritto: BTW, while debugging a case with a bigger storage supplier I found that open-iscsi seems to do exactly this

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-17 Thread Paolo Bonzini
Il 17/06/2014 13:37, Peter Lieven ha scritto: On 17.06.2014 13:15, Paolo Bonzini wrote: Il 17/06/2014 08:14, Peter Lieven ha scritto: BTW, while debugging a case with a bigger storage supplier I found that open-iscsi seems to do exactly this undeterministic behaviour. I have a 3TB LUN. If I

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-17 Thread Peter Lieven
On 17.06.2014 13:46, Paolo Bonzini wrote: Il 17/06/2014 13:37, Peter Lieven ha scritto: On 17.06.2014 13:15, Paolo Bonzini wrote: Il 17/06/2014 08:14, Peter Lieven ha scritto: BTW, while debugging a case with a bigger storage supplier I found that open-iscsi seems to do exactly this

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-05 Thread Michael Tokarev
04.06.2014 18:00, ronnie sahlberg wrote: That would mean you get to use the 10 version of the cdb even for very large devices (as long as the IO is for blocks at the beginning of the device) and thus provide partial avoidance of this issue for those large devices. That may make some bugs

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-05 Thread Peter Lieven
On 05.06.2014 11:12, Michael Tokarev wrote: 04.06.2014 18:00, ronnie sahlberg wrote: That would mean you get to use the 10 version of the cdb even for very large devices (as long as the IO is for blocks at the beginning of the device) and thus provide partial avoidance of this issue for those

[Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-04 Thread Peter Lieven
this patch changes the driver to uses 16 Byte CDBs for READ/WRITE only if the target requires 64bit lba addressing. On one hand this saves 6 bytes in each PDU on the other hand it seems that 10 Byte CDBs seems to be much better supported and tested as a recent issue I had with a major storage

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-04 Thread ronnie sahlberg
Looks good. As an alternative, you could do the 10 vs 16 decision based on the LBA instead of the size of the device : -if (use_16_for_ws) { + if (lba = 0x1) { iTask.task = iscsi_writesame16_task(iscsilun-iscsi, iscsilun-lun, lba,

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-04 Thread Peter Lieven
Am 04.06.2014 16:00, schrieb ronnie sahlberg: Looks good. As an alternative, you could do the 10 vs 16 decision based on the LBA instead of the size of the device : -if (use_16_for_ws) { + if (lba = 0x1) { iTask.task = iscsi_writesame16_task(iscsilun-iscsi,

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-04 Thread ronnie sahlberg
On Wed, Jun 4, 2014 at 7:43 AM, Peter Lieven p...@kamp.de wrote: Am 04.06.2014 16:00, schrieb ronnie sahlberg: Looks good. As an alternative, you could do the 10 vs 16 decision based on the LBA instead of the size of the device : -if (use_16_for_ws) { + if (lba = 0x1) {

Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

2014-06-04 Thread Paolo Bonzini
Il 04/06/2014 15:47, Peter Lieven ha scritto: this patch changes the driver to uses 16 Byte CDBs for READ/WRITE only if the target requires 64bit lba addressing. On one hand this saves 6 bytes in each PDU on the other hand it seems that 10 Byte CDBs seems to be much better supported and tested