[Qemu-devel] [PATCHv4 0/4] introduce max_transfer_length

[Qemu-devel] [PATCHv4 0/4] introduce max_transfer_length

[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_merge exceeds the max_xfer_len of an attached iSCSI LUN.
This is a required bug fix.

Splitting up requests according to the max_transfer_length will follow
in a later series.

v3->v4: introduce MIN_NON_ZERO to correctly calculate minimum of 2 limits.
v2->v3: remove Patch 2 completely [Paolo]
v1->v2: do not throw errors but generate trace events in Patch 2 [Paolo]

Peter Lieven (4):
  util: introduce MIN_NON_ZERO
  BlockLimits: introduce max_transfer_length
  block/iscsi: set max_transfer_length
  block: avoid creating oversized writes in multiwrite_merge

 block.c                   |    9 +++++++++
 block/iscsi.c             |   12 ++++++++++--
 include/block/block_int.h |    3 +++
 include/qemu/osdep.h      |    4 ++++
 4 files changed, 26 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCHv4 1/4] util: introduce MIN_NON_ZERO

[Peter Lieven]

at least in block layer we have the case of limits being defined for a
BlockDriverState. However, in this context often zero (0) has the special
meanining of undefined which means no limit. If two of those limits are
combined and the minimum is needed the minimum function should only return
zero if both parameters are zero.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 include/qemu/osdep.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 1565404..9a238df 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -68,6 +68,10 @@ typedef signed int              int_fast16_t;
 #define MAX(a, b) (((a) > (b)) ? (a) : (b))
 #endif
 
+#ifndef MIN_NON_ZERO
+#define MIN_NON_ZERO(a, b) ((!!(a) && (a) < (b)) ? (a) : (b))
+#endif
+
 #ifndef ROUND_UP
 #define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
 #endif
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv4 1/4] util: introduce MIN_NON_ZERO

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]

On 10/16/2014 01:54 AM, Peter Lieven wrote:
> at least in block layer we have the case of limits being defined for a
> BlockDriverState. However, in this context often zero (0) has the special
> meanining of undefined which means no limit. If two of those limits are
> combined and the minimum is needed the minimum function should only return
> zero if both parameters are zero.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  include/qemu/osdep.h |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 1565404..9a238df 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -68,6 +68,10 @@ typedef signed int              int_fast16_t;
>  #define MAX(a, b) (((a) > (b)) ? (a) : (b))
>  #endif
>  
> +#ifndef MIN_NON_ZERO
> +#define MIN_NON_ZERO(a, b) ((!!(a) && (a) < (b)) ? (a) : (b))

'(a) && expr' is already forcing the evaluation of (a) as a boolean;
thus rendering the '!!(a)' conversion to boolean redundant.

The patch is correct as is, so I'll leave a positive review, but if you
have a reason to respin, making it two characters shorter is probably
worth it.  Maybe it's also worth a comment that this macro is only
designed to work on unsigned values.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCHv4 1/4] util: introduce MIN_NON_ZERO

[Max Reitz]

On 2014-10-16 at 09:54, Peter Lieven wrote:
> at least in block layer we have the case of limits being defined for a
> BlockDriverState. However, in this context often zero (0) has the special
> meanining of undefined which means no limit. If two of those limits are
> combined and the minimum is needed the minimum function should only return
> zero if both parameters are zero.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   include/qemu/osdep.h |    4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 1565404..9a238df 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -68,6 +68,10 @@ typedef signed int              int_fast16_t;
>   #define MAX(a, b) (((a) > (b)) ? (a) : (b))
>   #endif
>   
> +#ifndef MIN_NON_ZERO
> +#define MIN_NON_ZERO(a, b) ((!!(a) && (a) < (b)) ? (a) : (b))

I contrast to Eric I'd even like (a) != 0 instead of !!(a). I normally 
use just "foo" instead of "foo != 0" or "foo != NULL", but in this case 
I'd like (a) != 0 (or (a) > 0) because of the name "NON_ZERO".

Now you have three alternatives to choose from. You're welcome. :-)

Anyway, I'm fine with any, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +#endif
> +
>   #ifndef ROUND_UP
>   #define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
>   #endif

[Qemu-devel] [PATCHv4 2/4] BlockLimits: introduce max_transfer_length

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c                   |    4 ++++
 include/block/block_int.h |    3 +++
 2 files changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 27533f3..0fbf916 100644
--- a/block.c
+++ b/block.c
@@ -536,6 +536,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;
@@ -550,6 +551,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_NON_ZERO(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);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d86a6c..b13a10a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -289,6 +289,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;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv4 2/4] BlockLimits: introduce max_transfer_length

[Max Reitz]

On 2014-10-16 at 09:54, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block.c                   |    4 ++++
>   include/block/block_int.h |    3 +++
>   2 files changed, 7 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCHv4 3/4] block/iscsi: set max_transfer_length

[Peter Lieven]

the limit of 0xffffff for 16 byte CDBs is intentional to
avoid overflows on 32-bit architectures.

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 3a01de0..c873d13 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1449,10 +1449,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,
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv4 3/4] block/iscsi: set max_transfer_length

[Max Reitz]

On 2014-10-16 at 09:54, Peter Lieven wrote:
> the limit of 0xffffff for 16 byte CDBs is intentional to
> avoid overflows on 32-bit architectures.

How is it related to 32 bit? I somehow feel like it has to do something 
with the result of sector_lun2qemu() which involves block_size...

> Signed-off-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>   block/iscsi.c |   12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 3a01de0..c873d13 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1449,10 +1449,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,

Re: [Qemu-devel] [PATCHv4 3/4] block/iscsi: set max_transfer_length

[Peter Lieven]

Am 23.10.2014 um 13:18 schrieb Max Reitz:
> On 2014-10-16 at 09:54, Peter Lieven wrote:
>> the limit of 0xffffff for 16 byte CDBs is intentional to
>> avoid overflows on 32-bit architectures.
>
> How is it related to 32 bit? I somehow feel like it has to do something with the result of sector_lun2qemu() which involves block_size...

iscsilun->bl.max_xfer_len is 32-bit unsigned while nb_sectors is usually signed. Furthermore as you suspected nb_sectors is always 512Byte sectors
while iscsilun->block_size can be 4k or even more.

I will change the code to set max_xfer_len to 0xffffffff and limit the output of sector_lun2qemu() to INT_MAX in the bs->bl.max_transfer_length case.

However, in real life you will never want to have a transfer of 0x3fffffff blocks, won't you?

Peter


>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> ---
>>   block/iscsi.c |   12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 3a01de0..c873d13 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1449,10 +1449,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,
>

Re: [Qemu-devel] [PATCHv4 3/4] block/iscsi: set max_transfer_length

[Max Reitz]

On 2014-10-25 at 17:16, Peter Lieven wrote:
> Am 23.10.2014 um 13:18 schrieb Max Reitz:
>> On 2014-10-16 at 09:54, Peter Lieven wrote:
>>> the limit of 0xffffff for 16 byte CDBs is intentional to
>>> avoid overflows on 32-bit architectures.
>> How is it related to 32 bit? I somehow feel like it has to do something with the result of sector_lun2qemu() which involves block_size...
> iscsilun->bl.max_xfer_len is 32-bit unsigned while nb_sectors is usually signed. Furthermore as you suspected nb_sectors is always 512Byte sectors
> while iscsilun->block_size can be 4k or even more.
>
> I will change the code to set max_xfer_len to 0xffffffff and limit the output of sector_lun2qemu() to INT_MAX in the bs->bl.max_transfer_length case.
>
> However, in real life you will never want to have a transfer of 0x3fffffff blocks, won't you?

I'd be fine with a single block, but QEMU guests matter more than me. :-)

I was just wondering how you ended up with 2^24 - 1 as it would be (2^32 
- 1) / 256. I do remember IDE doing 16-bit-transfers, so I could imagine 
how this works out with 512 byte sector size, but this isn't IDE, so...

(sorry, it's been a while since I worked with the SCSI protocol myself)

Max

> Peter
>
>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>> ---
>>>    block/iscsi.c |   12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index 3a01de0..c873d13 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -1449,10 +1449,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,

[Qemu-devel] [PATCHv4 4/4] block: avoid creating oversized writes in multiwrite_merge

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block.c b/block.c
index 0fbf916..9ad2287 100644
--- a/block.c
+++ b/block.c
@@ -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));
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv4 4/4] block: avoid creating oversized writes in multiwrite_merge

[Max Reitz]

On 2014-10-16 at 09:54, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>   block.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/block.c b/block.c
> index 0fbf916..9ad2287 100644
> --- a/block.c
> +++ b/block.c
> @@ -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));

Reviewed-by: Max Reitz <mreitz@redhat.com>

I feel like we should respect max_transfer_length in more than just this 
function, though. Every block device (or block driver) that sets a 
maximum transfer length should check requests against it as well, so we 
don't need to duplicate the same tests in the block layer functions, but 
maybe if at some point there are no things left to be done on the block 
layer, we could do that.

Max

Re: [Qemu-devel] [PATCHv4 4/4] block: avoid creating oversized writes in multiwrite_merge

[Peter Lieven]

Am 23.10.2014 um 13:23 schrieb Max Reitz:
> On 2014-10-16 at 09:54, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> ---
>>   block.c |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 0fbf916..9ad2287 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -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));
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> I feel like we should respect max_transfer_length in more than just this function, though. Every block device (or block driver) that sets a maximum transfer length should check requests against it as well, so we don't need to duplicate the same tests in the block layer functions, but maybe if at some point there are no things left to be done on the block layer, we could do that.

I have on my todo to add a splitting logic to block.c to 

bdrv_co_do_preadv() and bdrv_co_do_pwritev(), but I agreed with Kevin to introduce this
*after* 2.2.

Peter