| Message ID | 1372338695-411-3-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
max_unmap : If the target does not return an explicit limit for max_unmap it will return 0xffffffff which means "no limit". I think you should add a check for max_unmap and clamp it down to something sane. Maybe a maximum of 128M ? Same for bdc, that should also be checked and clamped down to sane values. On Thu, Jun 27, 2013 at 11:11 PM, Peter Lieven <pl@kamp.de> wrote: > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/iscsi.c | 80 ++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 56 insertions(+), 24 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index a38a1bf..2e2455d 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -54,6 +54,8 @@ typedef struct IscsiLun { > uint8_t lbpu; > uint8_t lbpws; > uint8_t lbpws10; > + uint32_t max_unmap; > + uint32_t max_unmap_bdc; > } IscsiLun; > > typedef struct IscsiAIOCB { > @@ -1007,6 +1009,37 @@ static QemuOptsList runtime_opts = { > }, > }; > > +static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, > + int lun, int evpd, int pc) { > + int full_size; > + struct scsi_task *task = NULL; > + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64); > + if (task == NULL || task->status != SCSI_STATUS_GOOD) { > + goto fail; > + } > + full_size = scsi_datain_getfullsize(task); > + if (full_size > task->datain.size) { > + scsi_free_scsi_task(task); > + > + /* we need more data for the full list */ > + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size); > + if (task == NULL || task->status != SCSI_STATUS_GOOD) { > + goto fail; > + } > + } > + > + return task; > + > +fail: > + error_report("iSCSI: Inquiry command failed : %s", > + iscsi_get_error(iscsi)); > + if (task) { > + scsi_free_scsi_task(task); > + return NULL; > + } > + return NULL; > +} > + > /* > * We support iscsi url's on the form > * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun> > @@ -1139,33 +1172,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) > > if (iscsilun->lbpme) { > struct scsi_inquiry_logical_block_provisioning *inq_lbp; > - int full_size; > - > - task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1, > - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING, > - 64); > - if (task == NULL || task->status != SCSI_STATUS_GOOD) { > - error_report("iSCSI: Inquiry command failed : %s", > - iscsi_get_error(iscsilun->iscsi)); > + task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, > + SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING); > + if (task == NULL) { > ret = -EINVAL; > goto out; > } > - full_size = scsi_datain_getfullsize(task); > - if (full_size > task->datain.size) { > - scsi_free_scsi_task(task); > - > - /* we need more data for the full list */ > - task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1, > - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING, > - full_size); > - if (task == NULL || task->status != SCSI_STATUS_GOOD) { > - error_report("iSCSI: Inquiry command failed : %s", > - iscsi_get_error(iscsilun->iscsi)); > - ret = -EINVAL; > - goto out; > - } > - } > - > inq_lbp = scsi_datain_unmarshall(task); > if (inq_lbp == NULL) { > error_report("iSCSI: failed to unmarshall inquiry datain blob"); > @@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) > iscsilun->lbpu = inq_lbp->lbpu; > iscsilun->lbpws = inq_lbp->lbpws; > iscsilun->lbpws10 = inq_lbp->lbpws10; > + scsi_free_scsi_task(task); > + task = NULL; > + } > + > + if (iscsilun->lbpu) { > + struct scsi_inquiry_block_limits *inq_bl; > + task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, > + SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS); > + if (task == NULL) { > + ret = -EINVAL; > + goto out; > + } > + inq_bl = scsi_datain_unmarshall(task); > + if (inq_bl == NULL) { > + error_report("iSCSI: failed to unmarshall inquiry datain blob"); > + ret = -EINVAL; > + goto out; > + } > + iscsilun->max_unmap = inq_bl->max_unmap; > + iscsilun->max_unmap_bdc = inq_bl->max_unmap_bdc; > } > > #if defined(LIBISCSI_FEATURE_NOP_COUNTER) > -- > 1.7.9.5 >
Am 03.07.2013 um 05:43 schrieb ronnie sahlberg <ronniesahlberg@gmail.com>: > max_unmap : > > If the target does not return an explicit limit for max_unmap it will > return 0xffffffff which means "no limit". > thanks for the remark. i wasn't aware. > I think you should add a check for max_unmap and clamp it down to > something sane. > Maybe a maximum of 128M ? okay, i personally would tent to less (32MB or 64MB). > > Same for bdc, that should also be checked and clamped down to sane values. BDC is not used. I had an implementation that sent multiple descriptors out, but at least for my storage the maximum unmap counts not for each descriptors, but for all together. So in this case we do not need the field at all. I forgot to remove it. discard and write_zeroes will both only send one request up to max_unmap in size. apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1? I have read in the specs something that the target might unmap the blocks or not touch them at all. Maybe you have more information. Peter > > > On Thu, Jun 27, 2013 at 11:11 PM, Peter Lieven <pl@kamp.de> wrote: >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> block/iscsi.c | 80 ++++++++++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 56 insertions(+), 24 deletions(-) >> >> diff --git a/block/iscsi.c b/block/iscsi.c >> index a38a1bf..2e2455d 100644 >> --- a/block/iscsi.c >> +++ b/block/iscsi.c >> @@ -54,6 +54,8 @@ typedef struct IscsiLun { >> uint8_t lbpu; >> uint8_t lbpws; >> uint8_t lbpws10; >> + uint32_t max_unmap; >> + uint32_t max_unmap_bdc; >> } IscsiLun; >> >> typedef struct IscsiAIOCB { >> @@ -1007,6 +1009,37 @@ static QemuOptsList runtime_opts = { >> }, >> }; >> >> +static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, >> + int lun, int evpd, int pc) { >> + int full_size; >> + struct scsi_task *task = NULL; >> + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64); >> + if (task == NULL || task->status != SCSI_STATUS_GOOD) { >> + goto fail; >> + } >> + full_size = scsi_datain_getfullsize(task); >> + if (full_size > task->datain.size) { >> + scsi_free_scsi_task(task); >> + >> + /* we need more data for the full list */ >> + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size); >> + if (task == NULL || task->status != SCSI_STATUS_GOOD) { >> + goto fail; >> + } >> + } >> + >> + return task; >> + >> +fail: >> + error_report("iSCSI: Inquiry command failed : %s", >> + iscsi_get_error(iscsi)); >> + if (task) { >> + scsi_free_scsi_task(task); >> + return NULL; >> + } >> + return NULL; >> +} >> + >> /* >> * We support iscsi url's on the form >> * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun> >> @@ -1139,33 +1172,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) >> >> if (iscsilun->lbpme) { >> struct scsi_inquiry_logical_block_provisioning *inq_lbp; >> - int full_size; >> - >> - task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1, >> - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING, >> - 64); >> - if (task == NULL || task->status != SCSI_STATUS_GOOD) { >> - error_report("iSCSI: Inquiry command failed : %s", >> - iscsi_get_error(iscsilun->iscsi)); >> + task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, >> + SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING); >> + if (task == NULL) { >> ret = -EINVAL; >> goto out; >> } >> - full_size = scsi_datain_getfullsize(task); >> - if (full_size > task->datain.size) { >> - scsi_free_scsi_task(task); >> - >> - /* we need more data for the full list */ >> - task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1, >> - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING, >> - full_size); >> - if (task == NULL || task->status != SCSI_STATUS_GOOD) { >> - error_report("iSCSI: Inquiry command failed : %s", >> - iscsi_get_error(iscsilun->iscsi)); >> - ret = -EINVAL; >> - goto out; >> - } >> - } >> - >> inq_lbp = scsi_datain_unmarshall(task); >> if (inq_lbp == NULL) { >> error_report("iSCSI: failed to unmarshall inquiry datain blob"); >> @@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) >> iscsilun->lbpu = inq_lbp->lbpu; >> iscsilun->lbpws = inq_lbp->lbpws; >> iscsilun->lbpws10 = inq_lbp->lbpws10; >> + scsi_free_scsi_task(task); >> + task = NULL; >> + } >> + >> + if (iscsilun->lbpu) { >> + struct scsi_inquiry_block_limits *inq_bl; >> + task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, >> + SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS); >> + if (task == NULL) { >> + ret = -EINVAL; >> + goto out; >> + } >> + inq_bl = scsi_datain_unmarshall(task); >> + if (inq_bl == NULL) { >> + error_report("iSCSI: failed to unmarshall inquiry datain blob"); >> + ret = -EINVAL; >> + goto out; >> + } >> + iscsilun->max_unmap = inq_bl->max_unmap; >> + iscsilun->max_unmap_bdc = inq_bl->max_unmap_bdc; >> } >> >> #if defined(LIBISCSI_FEATURE_NOP_COUNTER) >> -- >> 1.7.9.5 >>
Il 03/07/2013 23:23, Peter Lieven ha scritto: > BDC is not used. I had an implementation that sent multiple descriptors out, but > at least for my storage the maximum unmap counts not for each descriptors, but for all > together. So in this case we do not need the field at all. I forgot to remove it. > > discard and write_zeroes will both only send one request up to max_unmap in size. > > apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1? Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero payload, but I suspect there may be buggy targets here. > I have read in the specs something that the target might unmap the blocks or not touch them at all. > Maybe you have more information. That's even true of UNMAP itself, actually. :) The storage can always "upgrade" a block from unmapped to anchored and from anchored to allocated, so UNMAP can be a no-op and still comply with the standard. Paolo
Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <pbonzini@redhat.com>: > Il 03/07/2013 23:23, Peter Lieven ha scritto: >> BDC is not used. I had an implementation that sent multiple descriptors out, but >> at least for my storage the maximum unmap counts not for each descriptors, but for all >> together. So in this case we do not need the field at all. I forgot to remove it. >> >> discard and write_zeroes will both only send one request up to max_unmap in size. >> >> apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1? > > Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ > to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero > payload, but I suspect there may be buggy targets here. > >> I have read in the specs something that the target might unmap the blocks or not touch them at all. >> Maybe you have more information. > > That's even true of UNMAP itself, actually. :) > > The storage can always "upgrade" a block from unmapped to anchored and > from anchored to allocated, so UNMAP can be a no-op and still comply > with the standard. My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads as zero afterwards? Regardless if the target decides to "upgrade" the block or do not unmap the block? Peter
Il 04/07/2013 23:07, Peter Lieven ha scritto: > > Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <pbonzini@redhat.com>: > >> Il 03/07/2013 23:23, Peter Lieven ha scritto: >>> BDC is not used. I had an implementation that sent multiple descriptors out, but >>> at least for my storage the maximum unmap counts not for each descriptors, but for all >>> together. So in this case we do not need the field at all. I forgot to remove it. >>> >>> discard and write_zeroes will both only send one request up to max_unmap in size. >>> >>> apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1? >> >> Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ >> to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero >> payload, but I suspect there may be buggy targets here. >> >>> I have read in the specs something that the target might unmap the blocks or not touch them at all. >>> Maybe you have more information. >> >> That's even true of UNMAP itself, actually. :) >> >> The storage can always "upgrade" a block from unmapped to anchored and >> from anchored to allocated, so UNMAP can be a no-op and still comply >> with the standard. > > My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads > as zero afterwards? Regardless if the target decides to "upgrade" the block or do > not unmap the block? I would be very surprised, but if you are worried about that, it definitely won't happen with WRITE_SAME. Paolo
The device MIGHT map or anchor the blocks after the unmap but it may only do so if the blocks that become mapped are all zero. So I think you can safely assume that if lbprz==1 then it will always read back as zero no matter what happens internally in the target. Either the block becomes unmapped/deallocated and then it will read back as zero, or the blocks is automatically re-mapped to anchored/mapped again but this can only happen if the mapped block is all zero so once again it will still read back as all zero. === 4.7.3.5 Autonomous LBA transitions A device server may perform the following actions at any time: a) transition any deallocated LBA to mapped; b) transition any anchored LBA to mapped; or c) transition any deallocated LBA to anchored. If the LBPRZ bit in the READ CAPACITY (16) parameter data (see 5.16.2) is set to one, and a mapped LBA references a logical block that contains: a) user data with all bits set to zero; and Working Draft SCSI Block Commands – 3 (SBC-3) 27T10/BSR INCITS 514 Revision 35d 15 May 2013 b) protection information, if any, set to FFFF_FFFF_FFFF_FFFFh, then the device server may transition that mapped LBA to anchored or deallocated at any time. The logical block provisioning st On Thu, Jul 4, 2013 at 2:07 PM, Peter Lieven <pl@kamp.de> wrote: > > Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <pbonzini@redhat.com>: > >> Il 03/07/2013 23:23, Peter Lieven ha scritto: >>> BDC is not used. I had an implementation that sent multiple descriptors out, but >>> at least for my storage the maximum unmap counts not for each descriptors, but for all >>> together. So in this case we do not need the field at all. I forgot to remove it. >>> >>> discard and write_zeroes will both only send one request up to max_unmap in size. >>> >>> apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1? >> >> Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ >> to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero >> payload, but I suspect there may be buggy targets here. >> >>> I have read in the specs something that the target might unmap the blocks or not touch them at all. >>> Maybe you have more information. >> >> That's even true of UNMAP itself, actually. :) >> >> The storage can always "upgrade" a block from unmapped to anchored and >> from anchored to allocated, so UNMAP can be a no-op and still comply >> with the standard. > > My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads > as zero afterwards? Regardless if the target decides to "upgrade" the block or do > not unmap the block? > > Peter >
ok, to sum up you see no potential problem with my patch to optimize write zeroes by unmap iff lbprz==1 and lbpme == 1 ? the alternative would be to use writesame16 and sent a zero block. this would allow an optimization also if lbprz == 0. in this case i would not set the unmap bit. peter Am 05.07.2013 um 09:11 schrieb ronnie sahlberg <ronniesahlberg@gmail.com>: > The device MIGHT map or anchor the blocks after the unmap but it may > only do so if the blocks that become mapped are all zero. > > So I think you can safely assume that if lbprz==1 then it will always > read back as zero no matter what happens internally in the target. > > Either the block becomes unmapped/deallocated and then it will read > back as zero, > or the blocks is automatically re-mapped to anchored/mapped again > but this can only happen if the mapped block is all zero so once again > it will still read back as all zero. > > === > > 4.7.3.5 Autonomous LBA transitions > A device server may perform the following actions at any time: > a) transition any deallocated LBA to mapped; > b) transition any anchored LBA to mapped; or > c) transition any deallocated LBA to anchored. > If the LBPRZ bit in the READ CAPACITY (16) parameter data (see 5.16.2) > is set to one, and a mapped LBA > references a logical block that contains: > a) user data with all bits set to zero; and > Working Draft SCSI Block Commands – 3 (SBC-3) > 27T10/BSR INCITS 514 Revision 35d > 15 May 2013 > b) protection information, if any, set to FFFF_FFFF_FFFF_FFFFh, > then the device server may transition that mapped LBA to anchored or > deallocated at any time. > The logical block provisioning st > > On Thu, Jul 4, 2013 at 2:07 PM, Peter Lieven <pl@kamp.de> wrote: >> >> Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <pbonzini@redhat.com>: >> >>> Il 03/07/2013 23:23, Peter Lieven ha scritto: >>>> BDC is not used. I had an implementation that sent multiple descriptors out, but >>>> at least for my storage the maximum unmap counts not for each descriptors, but for all >>>> together. So in this case we do not need the field at all. I forgot to remove it. >>>> >>>> discard and write_zeroes will both only send one request up to max_unmap in size. >>>> >>>> apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1? >>> >>> Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ >>> to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero >>> payload, but I suspect there may be buggy targets here. >>> >>>> I have read in the specs something that the target might unmap the blocks or not touch them at all. >>>> Maybe you have more information. >>> >>> That's even true of UNMAP itself, actually. :) >>> >>> The storage can always "upgrade" a block from unmapped to anchored and >>> from anchored to allocated, so UNMAP can be a no-op and still comply >>> with the standard. >> >> My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads >> as zero afterwards? Regardless if the target decides to "upgrade" the block or do >> not unmap the block? >> >> Peter >>
Right. I don't see any problem with your patch. On Sat, Jul 6, 2013 at 3:15 PM, Peter Lieven <pl@kamp.de> wrote: > ok, to sum up you see no potential problem with my patch to optimize write zeroes by > unmap iff lbprz==1 and lbpme == 1 ? ACK > > the alternative would be to use writesame16 and sent a zero block. this would allow > an optimization also if lbprz == 0. in this case i would not set the unmap bit. > > peter > > > Am 05.07.2013 um 09:11 schrieb ronnie sahlberg <ronniesahlberg@gmail.com>: > >> The device MIGHT map or anchor the blocks after the unmap but it may >> only do so if the blocks that become mapped are all zero. >> >> So I think you can safely assume that if lbprz==1 then it will always >> read back as zero no matter what happens internally in the target. >> >> Either the block becomes unmapped/deallocated and then it will read >> back as zero, >> or the blocks is automatically re-mapped to anchored/mapped again >> but this can only happen if the mapped block is all zero so once again >> it will still read back as all zero. >> >> === >> >> 4.7.3.5 Autonomous LBA transitions >> A device server may perform the following actions at any time: >> a) transition any deallocated LBA to mapped; >> b) transition any anchored LBA to mapped; or >> c) transition any deallocated LBA to anchored. >> If the LBPRZ bit in the READ CAPACITY (16) parameter data (see 5.16.2) >> is set to one, and a mapped LBA >> references a logical block that contains: >> a) user data with all bits set to zero; and >> Working Draft SCSI Block Commands – 3 (SBC-3) >> 27T10/BSR INCITS 514 Revision 35d >> 15 May 2013 >> b) protection information, if any, set to FFFF_FFFF_FFFF_FFFFh, >> then the device server may transition that mapped LBA to anchored or >> deallocated at any time. >> The logical block provisioning st >> >> On Thu, Jul 4, 2013 at 2:07 PM, Peter Lieven <pl@kamp.de> wrote: >>> >>> Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <pbonzini@redhat.com>: >>> >>>> Il 03/07/2013 23:23, Peter Lieven ha scritto: >>>>> BDC is not used. I had an implementation that sent multiple descriptors out, but >>>>> at least for my storage the maximum unmap counts not for each descriptors, but for all >>>>> together. So in this case we do not need the field at all. I forgot to remove it. >>>>> >>>>> discard and write_zeroes will both only send one request up to max_unmap in size. >>>>> >>>>> apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1? >>>> >>>> Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ >>>> to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero >>>> payload, but I suspect there may be buggy targets here. >>>> >>>>> I have read in the specs something that the target might unmap the blocks or not touch them at all. >>>>> Maybe you have more information. >>>> >>>> That's even true of UNMAP itself, actually. :) >>>> >>>> The storage can always "upgrade" a block from unmapped to anchored and >>>> from anchored to allocated, so UNMAP can be a no-op and still comply >>>> with the standard. >>> >>> My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads >>> as zero afterwards? Regardless if the target decides to "upgrade" the block or do >>> not unmap the block? >>> >>> Peter >>> >
Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/iscsi.c | 80 ++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 56 insertions(+), 24 deletions(-) > @@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) > iscsilun->lbpu = inq_lbp->lbpu; > iscsilun->lbpws = inq_lbp->lbpws; > iscsilun->lbpws10 = inq_lbp->lbpws10; > + scsi_free_scsi_task(task); > + task = NULL; > + } > + > + if (iscsilun->lbpu) { > + struct scsi_inquiry_block_limits *inq_bl; > + task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, > + SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS); > + if (task == NULL) { > + ret = -EINVAL; > + goto out; > + } > + inq_bl = scsi_datain_unmarshall(task); > + if (inq_bl == NULL) { > + error_report("iSCSI: failed to unmarshall inquiry datain blob"); > + ret = -EINVAL; > + goto out; > + } > + iscsilun->max_unmap = inq_bl->max_unmap; > + iscsilun->max_unmap_bdc = inq_bl->max_unmap_bdc; > } How about scsi_free_scsi_task() here as well? It's caught at the end of the function, but I think it's nicer to free it in the block that uses it locally. Kevin
Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/iscsi.c | 80 ++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 56 insertions(+), 24 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index a38a1bf..2e2455d 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -54,6 +54,8 @@ typedef struct IscsiLun { > uint8_t lbpu; > uint8_t lbpws; > uint8_t lbpws10; > + uint32_t max_unmap; > + uint32_t max_unmap_bdc; > } IscsiLun; > > typedef struct IscsiAIOCB { > @@ -1007,6 +1009,37 @@ static QemuOptsList runtime_opts = { > }, > }; > > +static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, > + int lun, int evpd, int pc) { Oops, forgot to add the comment in the other mail... This isn't valid coding style, the brace belongs on a line of its own. Kevin
diff --git a/block/iscsi.c b/block/iscsi.c index a38a1bf..2e2455d 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -54,6 +54,8 @@ typedef struct IscsiLun { uint8_t lbpu; uint8_t lbpws; uint8_t lbpws10; + uint32_t max_unmap; + uint32_t max_unmap_bdc; } IscsiLun; typedef struct IscsiAIOCB { @@ -1007,6 +1009,37 @@ static QemuOptsList runtime_opts = { }, }; +static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi, + int lun, int evpd, int pc) { + int full_size; + struct scsi_task *task = NULL; + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64); + if (task == NULL || task->status != SCSI_STATUS_GOOD) { + goto fail; + } + full_size = scsi_datain_getfullsize(task); + if (full_size > task->datain.size) { + scsi_free_scsi_task(task); + + /* we need more data for the full list */ + task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size); + if (task == NULL || task->status != SCSI_STATUS_GOOD) { + goto fail; + } + } + + return task; + +fail: + error_report("iSCSI: Inquiry command failed : %s", + iscsi_get_error(iscsi)); + if (task) { + scsi_free_scsi_task(task); + return NULL; + } + return NULL; +} + /* * We support iscsi url's on the form * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun> @@ -1139,33 +1172,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) if (iscsilun->lbpme) { struct scsi_inquiry_logical_block_provisioning *inq_lbp; - int full_size; - - task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1, - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING, - 64); - if (task == NULL || task->status != SCSI_STATUS_GOOD) { - error_report("iSCSI: Inquiry command failed : %s", - iscsi_get_error(iscsilun->iscsi)); + task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, + SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING); + if (task == NULL) { ret = -EINVAL; goto out; } - full_size = scsi_datain_getfullsize(task); - if (full_size > task->datain.size) { - scsi_free_scsi_task(task); - - /* we need more data for the full list */ - task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1, - SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING, - full_size); - if (task == NULL || task->status != SCSI_STATUS_GOOD) { - error_report("iSCSI: Inquiry command failed : %s", - iscsi_get_error(iscsilun->iscsi)); - ret = -EINVAL; - goto out; - } - } - inq_lbp = scsi_datain_unmarshall(task); if (inq_lbp == NULL) { error_report("iSCSI: failed to unmarshall inquiry datain blob"); @@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) iscsilun->lbpu = inq_lbp->lbpu; iscsilun->lbpws = inq_lbp->lbpws; iscsilun->lbpws10 = inq_lbp->lbpws10; + scsi_free_scsi_task(task); + task = NULL; + } + + if (iscsilun->lbpu) { + struct scsi_inquiry_block_limits *inq_bl; + task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1, + SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS); + if (task == NULL) { + ret = -EINVAL; + goto out; + } + inq_bl = scsi_datain_unmarshall(task); + if (inq_bl == NULL) { + error_report("iSCSI: failed to unmarshall inquiry datain blob"); + ret = -EINVAL; + goto out; + } + iscsilun->max_unmap = inq_bl->max_unmap; + iscsilun->max_unmap_bdc = inq_bl->max_unmap_bdc; } #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
Signed-off-by: Peter Lieven <pl@kamp.de> --- block/iscsi.c | 80 ++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 24 deletions(-)