| Message ID | 1408699567-6940-3-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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; >
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; >>
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
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
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
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;
Signed-off-by: Peter Lieven <pl@kamp.de> --- block/iscsi.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)