Re: [PATCHv3 09/10] mpt3sas: always use first reserved smid for ioctl passthrough

[Hannes Reinecke]

On 02/21/2017 03:18 PM, Christoph Hellwig wrote:
> On Tue, Feb 21, 2017 at 01:27:08PM +0100, Hannes Reinecke wrote:
>> ioctl passthrough commands require a SCSIIO smid, but cannot
>> easily integrate with the block layer. But the driver already
>> has reserved some SCSIIO smids and we're only ever allowing
>> one ioctl command at a time we can use the first reserved smid
>> for ioctl commands.
>>
>> Signed-off-by: Hannes Reinecke 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_base.c | 11 ---
>>  drivers/scsi/mpt3sas/mpt3sas_ctl.c  | 10 ++
>>  2 files changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> index 3f9148c..0875e58 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> @@ -2432,7 +2432,9 @@ struct scsiio_tracker *
>>  ioc->scsi_lookup[i].cb_idx = 0xFF;
>>  ioc->scsi_lookup[i].scmd = NULL;
>>  ioc->scsi_lookup[i].direct_io = 0;
>> -list_add(>scsi_lookup[i].tracker_list, >free_list);
>> +if (i < ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT)
>> +list_add(>scsi_lookup[i].tracker_list,
>> + >free_list);
>>  spin_unlock_irqrestore(>scsi_lookup_lock, flags);
>>  
>>  _base_recovery_check(ioc);
>> @@ -5174,8 +5176,11 @@ struct scsiio_tracker *
>>  ioc->scsi_lookup[i].smid = smid;
>>  ioc->scsi_lookup[i].scmd = NULL;
>>  ioc->scsi_lookup[i].direct_io = 0;
>> -list_add_tail(>scsi_lookup[i].tracker_list,
>> ->free_list);
>> +if (i < ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT)
>> +list_add_tail(>scsi_lookup[i].tracker_list,
>> +  >free_list);
>> +else
>> +INIT_LIST_HEAD(>lookup[i].tracker_list);
> 
> Why the INIT_LIST_HEAD?  We never do a list_empty check on tracker_list,
> so it's rather pointless.
> 
Okay.

>> +smid = ioc->scsiio_depth - ioc->host->reserved_cmds;
> 
> ioc->host->reserved_cmds is never set at this point.  But given that only
> the smids < ioc->scsiio_depth are used it's doing the right thing
> if you just remove the "- ioc->host->reserved_cmds" entirely.
> 
Not quite; then it'll clash with the hi-priority smids which are
allocated after that.
But I can be using ioc->scsio_depth - INTERNAL_SCSIIO_CMDS_COUNT here.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCHv3 09/10] mpt3sas: always use first reserved smid for ioctl passthrough

[Christoph Hellwig]

On Tue, Feb 21, 2017 at 01:27:08PM +0100, Hannes Reinecke wrote:
> ioctl passthrough commands require a SCSIIO smid, but cannot
> easily integrate with the block layer. But the driver already
> has reserved some SCSIIO smids and we're only ever allowing
> one ioctl command at a time we can use the first reserved smid
> for ioctl commands.
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 11 ---
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c  | 10 ++
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
> b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 3f9148c..0875e58 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2432,7 +2432,9 @@ struct scsiio_tracker *
>   ioc->scsi_lookup[i].cb_idx = 0xFF;
>   ioc->scsi_lookup[i].scmd = NULL;
>   ioc->scsi_lookup[i].direct_io = 0;
> - list_add(>scsi_lookup[i].tracker_list, >free_list);
> + if (i < ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT)
> + list_add(>scsi_lookup[i].tracker_list,
> +  >free_list);
>   spin_unlock_irqrestore(>scsi_lookup_lock, flags);
>  
>   _base_recovery_check(ioc);
> @@ -5174,8 +5176,11 @@ struct scsiio_tracker *
>   ioc->scsi_lookup[i].smid = smid;
>   ioc->scsi_lookup[i].scmd = NULL;
>   ioc->scsi_lookup[i].direct_io = 0;
> - list_add_tail(>scsi_lookup[i].tracker_list,
> - >free_list);
> + if (i < ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT)
> + list_add_tail(>scsi_lookup[i].tracker_list,
> +   >free_list);
> + else
> + INIT_LIST_HEAD(>lookup[i].tracker_list);

Why the INIT_LIST_HEAD?  We never do a list_empty check on tracker_list,
so it's rather pointless.

> + smid = ioc->scsiio_depth - ioc->host->reserved_cmds;

ioc->host->reserved_cmds is never set at this point.  But given that only
the smids < ioc->scsiio_depth are used it's doing the right thing
if you just remove the "- ioc->host->reserved_cmds" entirely.

Otherwise this looks fine to me.

[PATCHv3 09/10] mpt3sas: always use first reserved smid for ioctl passthrough

[Hannes Reinecke]

ioctl passthrough commands require a SCSIIO smid, but cannot
easily integrate with the block layer. But the driver already
has reserved some SCSIIO smids and we're only ever allowing
one ioctl command at a time we can use the first reserved smid
for ioctl commands.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 11 ---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c  | 10 ++
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 3f9148c..0875e58 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2432,7 +2432,9 @@ struct scsiio_tracker *
ioc->scsi_lookup[i].cb_idx = 0xFF;
ioc->scsi_lookup[i].scmd = NULL;
ioc->scsi_lookup[i].direct_io = 0;
-   list_add(>scsi_lookup[i].tracker_list, >free_list);
+   if (i < ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT)
+   list_add(>scsi_lookup[i].tracker_list,
+>free_list);
spin_unlock_irqrestore(>scsi_lookup_lock, flags);
 
_base_recovery_check(ioc);
@@ -5174,8 +5176,11 @@ struct scsiio_tracker *
ioc->scsi_lookup[i].smid = smid;
ioc->scsi_lookup[i].scmd = NULL;
ioc->scsi_lookup[i].direct_io = 0;
-   list_add_tail(>scsi_lookup[i].tracker_list,
-   >free_list);
+   if (i < ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT)
+   list_add_tail(>scsi_lookup[i].tracker_list,
+ >free_list);
+   else
+   INIT_LIST_HEAD(>lookup[i].tracker_list);
}
 
/* hi-priority queue */
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c 
b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 02fe1c4..23e0ef1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -719,14 +719,8 @@ enum block_state {
goto out;
}
} else {
-
-   smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);
-   if (!smid) {
-   pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
-   ioc->name, __func__);
-   ret = -EAGAIN;
-   goto out;
-   }
+   /* Use first reserved smid for passthrough ioctls */
+   smid = ioc->scsiio_depth - ioc->host->reserved_cmds;
}
 
ret = 0;
-- 
1.8.5.6