[Qemu-devel] [PATCH 0/2] introduce bitmap_try_new

[Qemu-devel] [PATCH 0/2] introduce bitmap_try_new

[Peter Lieven]

this adds a new helper to handle errors when allocation a bitmap and
also introduces its first user.

Peter Lieven (2):
  util: introduce bitmap_try_new
  block/iscsi: handle failure on malloc of the allocationmap

 block/iscsi.c         |   22 +++++++++++++++-------
 include/qemu/bitmap.h |    6 ++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] util: introduce bitmap_try_new

[Peter Lieven]

regular bitmap_new simply aborts if the memory allocation fails.
bitmap_try_new returns NULL on failure and allows for proper
error handling.

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

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 1babd5d..51b430f 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -94,6 +94,12 @@ static inline unsigned long *bitmap_new(long nbits)
     return g_malloc0(len);
 }
 
+static inline unsigned long *bitmap_try_new(long nbits)
+{
+    long len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
+    return g_try_malloc0(len);
+}
+
 static inline void bitmap_zero(unsigned long *dst, long nbits)
 {
     if (small_nbits(nbits)) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/2] util: introduce bitmap_try_new

[Eric Blake]

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

On 08/22/2014 03:26 AM, Peter Lieven wrote:
> regular bitmap_new simply aborts if the memory allocation fails.
> bitmap_try_new returns NULL on failure and allows for proper
> error handling.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  include/qemu/bitmap.h |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
> index 1babd5d..51b430f 100644
> --- a/include/qemu/bitmap.h
> +++ b/include/qemu/bitmap.h
> @@ -94,6 +94,12 @@ static inline unsigned long *bitmap_new(long nbits)
>      return g_malloc0(len);
>  }
>  
> +static inline unsigned long *bitmap_try_new(long nbits)
> +{
> +    long len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
> +    return g_try_malloc0(len);
> +}
> +

What you have works, but I personally would have reimplemented
bitmap_new as the first caller of bitmap_try_new in this patch, where
bitmap_new handles a NULL bitmap_try_new return by abort()ing, so that
it has the same behavior.  By routing the one function to use the other,
we are future-proofing: if initialization of a bitmap ever needs
modification, we only update bitmap_try_new, rather than copying the
updates to both functions.

-- 
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] [PATCH 1/2] util: introduce bitmap_try_new

[Peter Lieven]

Am 25.08.2014 um 17:09 schrieb Eric Blake:
> On 08/22/2014 03:26 AM, Peter Lieven wrote:
>> regular bitmap_new simply aborts if the memory allocation fails.
>> bitmap_try_new returns NULL on failure and allows for proper
>> error handling.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  include/qemu/bitmap.h |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
>> index 1babd5d..51b430f 100644
>> --- a/include/qemu/bitmap.h
>> +++ b/include/qemu/bitmap.h
>> @@ -94,6 +94,12 @@ static inline unsigned long *bitmap_new(long nbits)
>>      return g_malloc0(len);
>>  }
>>  
>> +static inline unsigned long *bitmap_try_new(long nbits)
>> +{
>> +    long len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);
>> +    return g_try_malloc0(len);
>> +}
>> +
> What you have works, but I personally would have reimplemented
> bitmap_new as the first caller of bitmap_try_new in this patch, where
> bitmap_new handles a NULL bitmap_try_new return by abort()ing, so that
> it has the same behavior.  By routing the one function to use the other,
> we are future-proofing: if initialization of a bitmap ever needs
> modification, we only update bitmap_try_new, rather than copying the
> updates to both functions.
>

Good point. What would be the right exit if we receive a NULL from
bitmap_try_new? abort() ?

Peter

Re: [Qemu-devel] [PATCH 1/2] util: introduce bitmap_try_new

[Eric Blake]

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

On 09/05/2014 02:00 PM, Peter Lieven wrote:

>>> +
>> What you have works, but I personally would have reimplemented
>> bitmap_new as the first caller of bitmap_try_new in this patch, where
>> bitmap_new handles a NULL bitmap_try_new return by abort()ing, so that
>> it has the same behavior.  By routing the one function to use the other,
>> we are future-proofing: if initialization of a bitmap ever needs
>> modification, we only update bitmap_try_new, rather than copying the
>> updates to both functions.
>>
> 
> Good point. What would be the right exit if we receive a NULL from
> bitmap_try_new? abort() ?

I think that g_malloc() calls abort(), so yes, that sounds right.

-- 
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 --]

[Qemu-devel] [PATCH 2/2] block/iscsi: handle failure on malloc of the allocationmap

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index ed883c3..131357c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -325,6 +325,19 @@ static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors,
     return 1;
 }
 
+static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun)
+{
+    unsigned long *ptr;
+    ptr = bitmap_try_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
+                                                      iscsilun),
+                                      iscsilun->cluster_sectors));
+    if (ptr == NULL) {
+        error_report("iSCSI: could not initialize allocationmap. "
+                     "Out of memory.");
+    }
+    return ptr;
+}
+
 static void iscsi_allocationmap_set(IscsiLun *iscsilun, int64_t sector_num,
                                     int nb_sectors)
 {
@@ -1413,9 +1426,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
                                      iscsilun->block_size) >> BDRV_SECTOR_BITS;
         if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
-            iscsilun->allocationmap =
-                bitmap_new(DIV_ROUND_UP(bs->total_sectors,
-                                        iscsilun->cluster_sectors));
+            iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
         }
     }
 
@@ -1508,10 +1519,7 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 
     if (iscsilun->allocationmap != NULL) {
         g_free(iscsilun->allocationmap);
-        iscsilun->allocationmap =
-            bitmap_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
-                                                    iscsilun),
-                                    iscsilun->cluster_sectors));
+        iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
     }
 
     return 0;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: handle failure on malloc of the allocationmap

[Paolo Bonzini]

Il 22/08/2014 11:26, Peter Lieven ha scritto:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index ed883c3..131357c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -325,6 +325,19 @@ static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors,
>      return 1;
>  }
>  
> +static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun)
> +{
> +    unsigned long *ptr;
> +    ptr = bitmap_try_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
> +                                                      iscsilun),
> +                                      iscsilun->cluster_sectors));
> +    if (ptr == NULL) {
> +        error_report("iSCSI: could not initialize allocationmap. "
> +                     "Out of memory.");
> +    }
> +    return ptr;
> +}
> +
>  static void iscsi_allocationmap_set(IscsiLun *iscsilun, int64_t sector_num,
>                                      int nb_sectors)
>  {
> @@ -1413,9 +1426,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
>                                       iscsilun->block_size) >> BDRV_SECTOR_BITS;
>          if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
> -            iscsilun->allocationmap =
> -                bitmap_new(DIV_ROUND_UP(bs->total_sectors,
> -                                        iscsilun->cluster_sectors));
> +            iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
>          }
>      }

iscsi_open has an Error ** argument.  Please pass it to
iscsi_allocationmap_init and use error_setg instead of error_report.


> @@ -1508,10 +1519,7 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
>  
>      if (iscsilun->allocationmap != NULL) {
>          g_free(iscsilun->allocationmap);
> -        iscsilun->allocationmap =
> -            bitmap_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
> -                                                    iscsilun),
> -                                    iscsilun->cluster_sectors));
> +        iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);

Here you may have to use qerror_report_err, though I guess the failure
need not be fatal and you can leave the allocationmap set to NULL.

Paolo

>      }
>  
>      return 0;
> 

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: handle failure on malloc of the allocationmap

[Peter Lieven]

On 25.08.2014 12:37, Paolo Bonzini wrote:
> Il 22/08/2014 11:26, Peter Lieven ha scritto:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/iscsi.c |   22 +++++++++++++++-------
>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index ed883c3..131357c 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -325,6 +325,19 @@ static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors,
>>       return 1;
>>   }
>>   
>> +static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun)
>> +{
>> +    unsigned long *ptr;
>> +    ptr = bitmap_try_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>> +                                                      iscsilun),
>> +                                      iscsilun->cluster_sectors));
>> +    if (ptr == NULL) {
>> +        error_report("iSCSI: could not initialize allocationmap. "
>> +                     "Out of memory.");
>> +    }
>> +    return ptr;
>> +}
>> +
>>   static void iscsi_allocationmap_set(IscsiLun *iscsilun, int64_t sector_num,
>>                                       int nb_sectors)
>>   {
>> @@ -1413,9 +1426,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>           iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
>>                                        iscsilun->block_size) >> BDRV_SECTOR_BITS;
>>           if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
>> -            iscsilun->allocationmap =
>> -                bitmap_new(DIV_ROUND_UP(bs->total_sectors,
>> -                                        iscsilun->cluster_sectors));
>> +            iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
>>           }
>>       }
> iscsi_open has an Error ** argument.  Please pass it to
> iscsi_allocationmap_init and use error_setg instead of error_report.

I could pass the Error argument and use error_report only if the pointer is null.

>
>
>> @@ -1508,10 +1519,7 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
>>   
>>       if (iscsilun->allocationmap != NULL) {
>>           g_free(iscsilun->allocationmap);
>> -        iscsilun->allocationmap =
>> -            bitmap_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>> -                                                    iscsilun),
>> -                                    iscsilun->cluster_sectors));
>> +        iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
> Here you may have to use qerror_report_err, though I guess the failure
> need not be fatal and you can leave the allocationmap set to NULL.

That was the plan. I would also not fail on iscsi_open or would you?

Peter

>
> Paolo
>
>>       }
>>   
>>       return 0;
>>


-- 

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
   pl@kamp.de | 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 2/2] block/iscsi: handle failure on malloc of the allocationmap

[Paolo Bonzini]

Il 25/08/2014 13:10, Peter Lieven ha scritto:
> On 25.08.2014 12:37, Paolo Bonzini wrote:
>> Il 22/08/2014 11:26, Peter Lieven ha scritto:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>   block/iscsi.c |   22 +++++++++++++++-------
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index ed883c3..131357c 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -325,6 +325,19 @@ static bool is_request_lun_aligned(int64_t
>>> sector_num, int nb_sectors,
>>>       return 1;
>>>   }
>>>   +static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun)
>>> +{
>>> +    unsigned long *ptr;
>>> +    ptr =
>>> bitmap_try_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>>> +                                                      iscsilun),
>>> +                                      iscsilun->cluster_sectors));
>>> +    if (ptr == NULL) {
>>> +        error_report("iSCSI: could not initialize allocationmap. "
>>> +                     "Out of memory.");
>>> +    }
>>> +    return ptr;
>>> +}
>>> +
>>>   static void iscsi_allocationmap_set(IscsiLun *iscsilun, int64_t
>>> sector_num,
>>>                                       int nb_sectors)
>>>   {
>>> @@ -1413,9 +1426,7 @@ static int iscsi_open(BlockDriverState *bs,
>>> QDict *options, int flags,
>>>           iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
>>>                                        iscsilun->block_size) >>
>>> BDRV_SECTOR_BITS;
>>>           if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
>>> -            iscsilun->allocationmap =
>>> -                bitmap_new(DIV_ROUND_UP(bs->total_sectors,
>>> -                                        iscsilun->cluster_sectors));
>>> +            iscsilun->allocationmap =
>>> iscsi_allocationmap_init(iscsilun);
>>>           }
>>>       }
>> iscsi_open has an Error ** argument.  Please pass it to
>> iscsi_allocationmap_init and use error_setg instead of error_report.
> 
> I could pass the Error argument and use error_report only if the pointer
> is null.

No, NULL means "I don't care about errors, or I don't care about precise
error messages and will use the return value to check for errors".

>>> @@ -1508,10 +1519,7 @@ static int iscsi_truncate(BlockDriverState
>>> *bs, int64_t offset)
>>>         if (iscsilun->allocationmap != NULL) {
>>>           g_free(iscsilun->allocationmap);
>>> -        iscsilun->allocationmap =
>>> -           
>>> bitmap_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>>> -                                                    iscsilun),
>>> -                                    iscsilun->cluster_sectors));
>>> +        iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
>> Here you may have to use qerror_report_err, though I guess the failure
>> need not be fatal and you can leave the allocationmap set to NULL.
> 
> That was the plan. I would also not fail on iscsi_open or would you?

Hmm, good question.  If so, the patch is correct with error_report.  I
thought a failure in iscsi_open would be less surprising.  The question
then is, do we need an option to disable allocationmap queries?

Paolo

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: handle failure on malloc of the allocationmap

[Peter Lieven]

On 25.08.2014 13:20, Paolo Bonzini wrote:
> Il 25/08/2014 13:10, Peter Lieven ha scritto:
>> On 25.08.2014 12:37, Paolo Bonzini wrote:
>>> Il 22/08/2014 11:26, Peter Lieven ha scritto:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>    block/iscsi.c |   22 +++++++++++++++-------
>>>>    1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>> index ed883c3..131357c 100644
>>>> --- a/block/iscsi.c
>>>> +++ b/block/iscsi.c
>>>> @@ -325,6 +325,19 @@ static bool is_request_lun_aligned(int64_t
>>>> sector_num, int nb_sectors,
>>>>        return 1;
>>>>    }
>>>>    +static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun)
>>>> +{
>>>> +    unsigned long *ptr;
>>>> +    ptr =
>>>> bitmap_try_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>>>> +                                                      iscsilun),
>>>> +                                      iscsilun->cluster_sectors));
>>>> +    if (ptr == NULL) {
>>>> +        error_report("iSCSI: could not initialize allocationmap. "
>>>> +                     "Out of memory.");
>>>> +    }
>>>> +    return ptr;
>>>> +}
>>>> +
>>>>    static void iscsi_allocationmap_set(IscsiLun *iscsilun, int64_t
>>>> sector_num,
>>>>                                        int nb_sectors)
>>>>    {
>>>> @@ -1413,9 +1426,7 @@ static int iscsi_open(BlockDriverState *bs,
>>>> QDict *options, int flags,
>>>>            iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
>>>>                                         iscsilun->block_size) >>
>>>> BDRV_SECTOR_BITS;
>>>>            if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
>>>> -            iscsilun->allocationmap =
>>>> -                bitmap_new(DIV_ROUND_UP(bs->total_sectors,
>>>> -                                        iscsilun->cluster_sectors));
>>>> +            iscsilun->allocationmap =
>>>> iscsi_allocationmap_init(iscsilun);
>>>>            }
>>>>        }
>>> iscsi_open has an Error ** argument.  Please pass it to
>>> iscsi_allocationmap_init and use error_setg instead of error_report.
>> I could pass the Error argument and use error_report only if the pointer
>> is null.
> No, NULL means "I don't care about errors, or I don't care about precise
> error messages and will use the return value to check for errors".
>
>>>> @@ -1508,10 +1519,7 @@ static int iscsi_truncate(BlockDriverState
>>>> *bs, int64_t offset)
>>>>          if (iscsilun->allocationmap != NULL) {
>>>>            g_free(iscsilun->allocationmap);
>>>> -        iscsilun->allocationmap =
>>>> -
>>>> bitmap_new(DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks,
>>>> -                                                    iscsilun),
>>>> -                                    iscsilun->cluster_sectors));
>>>> +        iscsilun->allocationmap = iscsi_allocationmap_init(iscsilun);
>>> Here you may have to use qerror_report_err, though I guess the failure
>>> need not be fatal and you can leave the allocationmap set to NULL.
>> That was the plan. I would also not fail on iscsi_open or would you?
> Hmm, good question.  If so, the patch is correct with error_report.  I
> thought a failure in iscsi_open would be less surprising.  The question
> then is, do we need an option to disable allocationmap queries?

We have cache=none... Otherwise if allocationmap is NULL the code
can already handle it.

Peter

Re: [Qemu-devel] [PATCH 2/2] block/iscsi: handle failure on malloc of the allocationmap

[Paolo Bonzini]

Il 25/08/2014 13:26, Peter Lieven ha scritto:
>>>
>> Hmm, good question.  If so, the patch is correct with error_report.  I
>> thought a failure in iscsi_open would be less surprising.  The question
>> then is, do we need an option to disable allocationmap queries?
> 
> We have cache=none...

Doh, right.  Then I think iscsi_open should fail if it cannot allocate
the map.

Paolo