Re: [Qemu-devel] [PATCHv2 02/11] iscsi: read unmap info from block limits vpd page

[Peter Lieven]

Am 03.07.2013 um 05:43 schrieb ronnie sahlberg <address@hidden>:

> 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 <address@hidden> wrote:
>> Signed-off-by: Peter Lieven <address@hidden>
>> ---
>> 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
>>