Re: [PATCH] scsi-sg: pass flag to inhibit setting LUN

["Jiří Pinkava" <j-pi@seznam.cz>]

Dne 3.1.2014 21:10, Douglas Gilbert napsal(a):
> On 14-01-02 08:19 AM, Jiří Pinkava wrote:
>> Hi,
>>
>> This patch implements support for inhibiting setting LUN number
>> for SCSI custom command send via /dev/sgX with ioctl(.., SG_IO, ...)
>> call.
>>
>> This solves problems with some devices which claim support of
>> SCSI v1/v2, but for some special purposes use custom commands
>> which does not conform with standard message format.
>> (Like mine Pentax digital single-lens reflex camera)
>> It does not affect devices with SCSI v3 or latter.
>>
>> For this purpose was earlier introduced
>> SG_FLAG_LUN_INHIBIT / SG_FLAG_UNUSED_LUN_INHIBIT (glibc/kernel) flag,
>> but the implementation was lost in some earlier code refactor.
>>
>> The only possible issue I see is current implementation supports only
>> SG driver but there is few more code paths where the similar ioctl can
>> be invoked
>> (eg. anny SCSI device?). I'm not sure about, feel free to say anything
>> about it.
>
> Hi,
> SG_FLAG_LUN_INHIBIT was added to the sg driver around 15
> years ago. The code to centralize the masking in scsi.c
> and remove the "LUN inhibit" capability arrived about
> 10 years ago. A handful of people have complained about
> it since. An you are the first to do something about it!
>
> I have a few comments about the code (see below) but the
> logic seems okay to me. That said I would be very surprised
> if this is allowed back in the kernel. If you manage to do
> that then I'll be studying your technique (because many of my
> patches to the sg driver are not accepted).
>
> So you can takes that as an "ack" with comments and good luck.
> I'll let others judge.
>
>> ---
>>   drivers/scsi/scsi.c       | 3 ++-
>>   drivers/scsi/sg.c         | 3 +++
>>   include/linux/blk_types.h | 2 ++
>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index fe0bcb1..f6d5fc9 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -693,7 +693,8 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
>>        * If SCSI-2 or lower, store the LUN value in cmnd.
>>        */
>>       if (cmd->device->scsi_level <= SCSI_2 &&
>> -        cmd->device->scsi_level != SCSI_UNKNOWN) {
>> +        cmd->device->scsi_level != SCSI_UNKNOWN &&
>> +        !(cmd->request->cmd_flags & REQ_LUN_INHIBIT)) {
>>           cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
>>                      (cmd->device->lun << 5 & 0xe0);
>>       }
>
> The above being time sensitive code I was thinking a switch
> might make it clearer and give the compiler more chances to
> optimize:
>       switch(cmd->device->scsi_level) {
>       case SCSI_2:
>       case SCSI_1_CCS:
>       case SCSI_1:
>               if (!(cmd->request->cmd_flags & REQ_LUN_INHIBIT)) {
>                       cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
>                                      (cmd->device->lun << 5 & 0xe0);
>               }
>               break;
>       default:
>               ;
>       }
>
> Hmm, can a switch's default be made "likely"?

In switch can be used only __branch_check__ (E, C) or
__builtin_expect(E, C), which
have too many '_' in name and I'm too afraid to use them.

switch (__builtin_expect(var, const)) {
...

if there is such a pressure for speed, unlikely(cmd->device->scsi_level
<= SCSI_2)
might be used, but i does not have statistics about how much modern
high-speed devices report itself as scsi_level <= SCSI_2 or scsi_level
== SCSI_UNKNOWN.
(suppose the rate is pretty low).

The switch solution look more elegant.

>
> More generally folks, with SAS SSDs available that can read at
> 1100 MB/sec we may want to spend time looking at the fast
> paths through the SCSI subsystem.
>
>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index df5e961..8e09015 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -1663,6 +1663,9 @@ static int sg_start_req(Sg_request *srp, unsigned
>> char *cmd)
>>       rq->sense = srp->sense_b;
>>       rq->retries = SG_DEFAULT_RETRIES;
>>
>> +    if (hp->flags & SG_FLAG_UNUSED_LUN_INHIBIT)
>> +        rq->cmd_flags |=  REQ_LUN_INHIBIT;
>> +
>>       if ((dxfer_len <= 0) || (dxfer_dir == SG_DXFER_NONE))
>>           return 0;
>>
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index 238ef0e..35d436b 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -178,6 +178,7 @@ enum rq_flag_bits {
>>       __REQ_MIXED_MERGE,    /* merge of different types, fail
>> separately */
>>       __REQ_KERNEL,         /* direct IO to kernel pages */
>>       __REQ_PM,        /* runtime pm request */
>> +    __REQ_LUN_INHIBIT,    /* pass through for
>> SG_FLAG_UNUSED_LUN_INHIBIT flag */
>>       __REQ_END,        /* last of chain of requests */
>>       __REQ_NR_BITS,        /* stops here */
>>   };
>> @@ -230,6 +231,7 @@ enum rq_flag_bits {
>>   #define REQ_SECURE        (1ULL << __REQ_SECURE)
>>   #define REQ_KERNEL        (1ULL << __REQ_KERNEL)
>>   #define REQ_PM            (1ULL << __REQ_PM)
>> +#define REQ_LUN_INHIBIT    (1ULL << __REQ_LUN_INHIBIT)
>>   #define REQ_END            (1ULL << __REQ_END)
>>
>>   #endif /* __LINUX_BLK_TYPES_H */
>>
>
> And I think the SG_FLAG_LUN_INHIBIT define should be re-instated
> to sg.h . For backward compatibility (for the last 10 years)
> please leave the SG_FLAG_UNUSED_LUN_INHIBIT define there. Both
> defines should have the same value.
>
> Doug Gilbert
>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html