| Message ID | 1414253919-3044-6-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On 2014-10-25 at 18:18, Peter Lieven wrote: > As Max pointed out there is a hidden cast from int64_t to int. > So use the newly introduced nb_sectors_lun2qemu for all > limits. > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/iscsi.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 1ae4add..85131b7 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) > > if (iscsilun->lbp.lbpu) { > if (iscsilun->bl.max_unmap < 0xffffffff) { > - bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, > - iscsilun); > + bs->bl.max_discard = nb_sectors_lun2qemu(iscsilun->bl.max_unmap, > + iscsilun); > } > - bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, > - iscsilun); > + bs->bl.discard_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, > + iscsilun); This looks wrong. I think an alignment should always be a power of two. The function however may return the unaligned INT_MAX. I think it should be capped to something like INT_MAX / 2 + 1 or just simply written out 0x40000000. > } > > if (iscsilun->bl.max_ws_len < 0xffffffff) { > - bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, > - iscsilun); > + bs->bl.max_write_zeroes = nb_sectors_lun2qemu(iscsilun->bl.max_ws_len, > + iscsilun); > } > if (iscsilun->lbp.lbpws) { > - bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, > - iscsilun); > + bs->bl.write_zeroes_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, > + iscsilun); Same here. > } > - bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len, > - iscsilun); > + bs->bl.opt_transfer_length = nb_sectors_lun2qemu(iscsilun->bl.opt_xfer_len, > + iscsilun); > } > > /* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in Max
On 27.10.2014 09:32, Max Reitz wrote: > On 2014-10-25 at 18:18, Peter Lieven wrote: >> As Max pointed out there is a hidden cast from int64_t to int. >> So use the newly introduced nb_sectors_lun2qemu for all >> limits. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> block/iscsi.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/block/iscsi.c b/block/iscsi.c >> index 1ae4add..85131b7 100644 >> --- a/block/iscsi.c >> +++ b/block/iscsi.c >> @@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) >> if (iscsilun->lbp.lbpu) { >> if (iscsilun->bl.max_unmap < 0xffffffff) { >> - bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, >> - iscsilun); >> + bs->bl.max_discard = nb_sectors_lun2qemu(iscsilun->bl.max_unmap, >> + iscsilun); >> } >> - bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >> - iscsilun); >> + bs->bl.discard_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, >> + iscsilun); > > This looks wrong. I think an alignment should always be a power of two. The function however may return the unaligned INT_MAX. I think it should be capped to something like INT_MAX / 2 + 1 or just simply written out 0x40000000. Good point, I would cap all limits to the highest power of two and use INT_MAX / 2 + 1 directly in nb_sectors_lun2qemu? Peter > >> } >> if (iscsilun->bl.max_ws_len < 0xffffffff) { >> - bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, >> - iscsilun); >> + bs->bl.max_write_zeroes = nb_sectors_lun2qemu(iscsilun->bl.max_ws_len, >> + iscsilun); >> } >> if (iscsilun->lbp.lbpws) { >> - bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >> - iscsilun); >> + bs->bl.write_zeroes_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, >> + iscsilun); > > Same here. > >> } >> - bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len, >> - iscsilun); >> + bs->bl.opt_transfer_length = nb_sectors_lun2qemu(iscsilun->bl.opt_xfer_len, >> + iscsilun); >> } >> /* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in > > Max
On 2014-10-27 at 09:35, Peter Lieven wrote: > On 27.10.2014 09:32, Max Reitz wrote: >> On 2014-10-25 at 18:18, Peter Lieven wrote: >>> As Max pointed out there is a hidden cast from int64_t to int. >>> So use the newly introduced nb_sectors_lun2qemu for all >>> limits. >>> >>> Signed-off-by: Peter Lieven <pl@kamp.de> >>> --- >>> block/iscsi.c | 20 ++++++++++---------- >>> 1 file changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/block/iscsi.c b/block/iscsi.c >>> index 1ae4add..85131b7 100644 >>> --- a/block/iscsi.c >>> +++ b/block/iscsi.c >>> @@ -1468,23 +1468,23 @@ static void >>> iscsi_refresh_limits(BlockDriverState *bs, Error **errp) >>> if (iscsilun->lbp.lbpu) { >>> if (iscsilun->bl.max_unmap < 0xffffffff) { >>> - bs->bl.max_discard = >>> sector_lun2qemu(iscsilun->bl.max_unmap, >>> - iscsilun); >>> + bs->bl.max_discard = >>> nb_sectors_lun2qemu(iscsilun->bl.max_unmap, >>> + iscsilun); >>> } >>> - bs->bl.discard_alignment = >>> sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> - iscsilun); >>> + bs->bl.discard_alignment = >>> nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> + iscsilun); >> >> This looks wrong. I think an alignment should always be a power of >> two. The function however may return the unaligned INT_MAX. I think >> it should be capped to something like INT_MAX / 2 + 1 or just simply >> written out 0x40000000. > > Good point, I would cap all limits to the highest power of two and use > INT_MAX / 2 + 1 directly in nb_sectors_lun2qemu? Seems good enough. I'd just like a comment in that function why you're doing that (in order to limit both alignments and "normal" lengths properly). I'm counting on nobody noticing that INT_MAX may be something different than 2^x - 1. *g* Max > Peter > >> >>> } >>> if (iscsilun->bl.max_ws_len < 0xffffffff) { >>> - bs->bl.max_write_zeroes = >>> sector_lun2qemu(iscsilun->bl.max_ws_len, >>> - iscsilun); >>> + bs->bl.max_write_zeroes = >>> nb_sectors_lun2qemu(iscsilun->bl.max_ws_len, >>> + iscsilun); >>> } >>> if (iscsilun->lbp.lbpws) { >>> - bs->bl.write_zeroes_alignment = >>> sector_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> - iscsilun); >>> + bs->bl.write_zeroes_alignment = >>> nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, >>> + iscsilun); >> >> Same here. >> >>> } >>> - bs->bl.opt_transfer_length = >>> sector_lun2qemu(iscsilun->bl.opt_xfer_len, >>> - iscsilun); >>> + bs->bl.opt_transfer_length = >>> nb_sectors_lun2qemu(iscsilun->bl.opt_xfer_len, >>> + iscsilun); >>> } >>> /* Since iscsi_open() ignores bdrv_flags, there is nothing to do >>> here in >> >> Max > >
diff --git a/block/iscsi.c b/block/iscsi.c index 1ae4add..85131b7 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) if (iscsilun->lbp.lbpu) { if (iscsilun->bl.max_unmap < 0xffffffff) { - bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap, - iscsilun); + bs->bl.max_discard = nb_sectors_lun2qemu(iscsilun->bl.max_unmap, + iscsilun); } - bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.discard_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, + iscsilun); } if (iscsilun->bl.max_ws_len < 0xffffffff) { - bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len, - iscsilun); + bs->bl.max_write_zeroes = nb_sectors_lun2qemu(iscsilun->bl.max_ws_len, + iscsilun); } if (iscsilun->lbp.lbpws) { - bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.write_zeroes_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran, + iscsilun); } - bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len, - iscsilun); + bs->bl.opt_transfer_length = nb_sectors_lun2qemu(iscsilun->bl.opt_xfer_len, + iscsilun); } /* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in
As Max pointed out there is a hidden cast from int64_t to int. So use the newly introduced nb_sectors_lun2qemu for all limits. Signed-off-by: Peter Lieven <pl@kamp.de> --- block/iscsi.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)