v2 of this series only fix the format type of max_lun:
drivers/scsi/ibmvscsi/ibmvscsi.c:2298:4:
warning: format '%d' expects argument of type 'int',
but argument 4 has type 'u64 {aka long long unsigned int}' [-Wformat=]
"Maximum ID: %d Maximum LUN: %d Maximum Channel: %d\n",
^
Laurent Vivier (3):
ibmvscsi: make parameters max_id and max_channel read-only
ibmvscsi: display default value for max_id, max_lun and max_channel.
ibmvscsi: Allow to configure maximum LUN
drivers/scsi/ibmvscsi/ibmvscsi.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
--
2.1.0
The value of the parameter is never re-read by the driver,
so a new value is ignored. Let know the user he
can't modify it by removing writable attribute.
Signed-off-by: Laurent Vivier <[email protected]>
Reviewed-by: Brian King <[email protected]>
Acked-by: Tyrel Datwyler <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 6a41c36..3e76490 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -105,9 +105,9 @@ MODULE_AUTHOR("Dave Boutcher");
MODULE_LICENSE("GPL");
MODULE_VERSION(IBMVSCSI_VERSION);
-module_param_named(max_id, max_id, int, S_IRUGO | S_IWUSR);
+module_param_named(max_id, max_id, int, S_IRUGO);
MODULE_PARM_DESC(max_id, "Largest ID value for each channel");
-module_param_named(max_channel, max_channel, int, S_IRUGO | S_IWUSR);
+module_param_named(max_channel, max_channel, int, S_IRUGO);
MODULE_PARM_DESC(max_channel, "Largest channel value");
module_param_named(init_timeout, init_timeout, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(init_timeout, "Initialization timeout in seconds");
--
2.1.0
As devices with values greater than that are silently ignored,
this gives some hints to the sys admin to know why he doesn't see
his devices...
Signed-off-by: Laurent Vivier <[email protected]>
Reviewed-by: Brian King <[email protected]>
Acked-by: Tyrel Datwyler <[email protected]>
---
v2: fix format of max_lun (u64): use %llu
drivers/scsi/ibmvscsi/ibmvscsi.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 3e76490..04de287 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -106,9 +106,9 @@ MODULE_LICENSE("GPL");
MODULE_VERSION(IBMVSCSI_VERSION);
module_param_named(max_id, max_id, int, S_IRUGO);
-MODULE_PARM_DESC(max_id, "Largest ID value for each channel");
+MODULE_PARM_DESC(max_id, "Largest ID value for each channel [Default=64]");
module_param_named(max_channel, max_channel, int, S_IRUGO);
-MODULE_PARM_DESC(max_channel, "Largest channel value");
+MODULE_PARM_DESC(max_channel, "Largest channel value [Default=3]");
module_param_named(init_timeout, init_timeout, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(init_timeout, "Initialization timeout in seconds");
module_param_named(max_requests, max_requests, int, S_IRUGO);
@@ -2294,6 +2294,10 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
host->max_channel = max_channel;
host->max_cmd_len = 16;
+ dev_info(dev,
+ "Maximum ID: %d Maximum LUN: %llu Maximum Channel: %d\n",
+ host->max_id, host->max_lun, host->max_channel);
+
if (scsi_add_host(hostdata->host, hostdata->dev))
goto add_host_failed;
--
2.1.0
QEMU allows until 32 LUNs.
Signed-off-by: Laurent Vivier <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 04de287..4480d3e 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -84,6 +84,7 @@
*/
static int max_id = 64;
static int max_channel = 3;
+static int max_lun = 8;
static int init_timeout = 300;
static int login_timeout = 60;
static int info_timeout = 30;
@@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
module_param_named(client_reserve, client_reserve, int, S_IRUGO );
MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
+module_param(max_lun, int, S_IRUGO);
+MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
struct ibmvscsi_host_data *hostdata);
@@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
goto init_pool_failed;
}
- host->max_lun = 8;
+ host->max_lun = max_lun;
host->max_id = max_id;
host->max_channel = max_channel;
host->max_cmd_len = 16;
--
2.1.0
On 04/11/2015 11:20, Laurent Vivier wrote:
> QEMU allows until 32 LUNs.
>
> Signed-off-by: Laurent Vivier <[email protected]>
I forget the:
Reviewed-by: Brian King <[email protected]>
Acked-by: Tyrel Datwyler <[email protected]>
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 04de287..4480d3e 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -84,6 +84,7 @@
> */
> static int max_id = 64;
> static int max_channel = 3;
> +static int max_lun = 8;
> static int init_timeout = 300;
> static int login_timeout = 60;
> static int info_timeout = 30;
> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
> +module_param(max_lun, int, S_IRUGO);
> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>
> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
> struct ibmvscsi_host_data *hostdata);
> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
> goto init_pool_failed;
> }
>
> - host->max_lun = 8;
> + host->max_lun = max_lun;
> host->max_id = max_id;
> host->max_channel = max_channel;
> host->max_cmd_len = 16;
>
On 11/04/2015 11:20 AM, Laurent Vivier wrote:
> QEMU allows until 32 LUNs.
>
> Signed-off-by: Laurent Vivier <[email protected]>
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 04de287..4480d3e 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -84,6 +84,7 @@
> */
> static int max_id = 64;
> static int max_channel = 3;
> +static int max_lun = 8;
> static int init_timeout = 300;
> static int login_timeout = 60;
> static int info_timeout = 30;
> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
> +module_param(max_lun, int, S_IRUGO);
> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>
> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
> struct ibmvscsi_host_data *hostdata);
> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
> goto init_pool_failed;
> }
>
> - host->max_lun = 8;
> + host->max_lun = max_lun;
> host->max_id = max_id;
> host->max_channel = max_channel;
> host->max_cmd_len = 16;
>
Please, don't do this.
'max_lun' should only be set if the HBA / transport has some hard
limitations on the number of bytes it can use.
Otherwise the scanning algorithm in scsi_scan.c should do the
correct thing, independent on the 'max_lun' setting.
If qemu has some issues here someone should rather fix qemu ...
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +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)
On 04/11/2015 12:16, Hannes Reinecke wrote:
> On 11/04/2015 11:20 AM, Laurent Vivier wrote:
>> QEMU allows until 32 LUNs.
>>
>> Signed-off-by: Laurent Vivier <[email protected]>
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 04de287..4480d3e 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -84,6 +84,7 @@
>> */
>> static int max_id = 64;
>> static int max_channel = 3;
>> +static int max_lun = 8;
>> static int init_timeout = 300;
>> static int login_timeout = 60;
>> static int info_timeout = 30;
>> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>> +module_param(max_lun, int, S_IRUGO);
>> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>>
>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>> struct ibmvscsi_host_data *hostdata);
>> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>> goto init_pool_failed;
>> }
>>
>> - host->max_lun = 8;
>> + host->max_lun = max_lun;
>> host->max_id = max_id;
>> host->max_channel = max_channel;
>> host->max_cmd_len = 16;
>>
> Please, don't do this.
>
> 'max_lun' should only be set if the HBA / transport has some hard
> limitations on the number of bytes it can use.
> Otherwise the scanning algorithm in scsi_scan.c should do the
> correct thing, independent on the 'max_lun' setting.
So you are saying we can remove the line ?
> If qemu has some issues here someone should rather fix qemu ...
There is no issue with QEMU. QEMU can manage more than "8" LUNs, and
we'd like to.
Laurent
On 11/04/2015 12:46 PM, Laurent Vivier wrote:
>
>
> On 04/11/2015 12:16, Hannes Reinecke wrote:
>> On 11/04/2015 11:20 AM, Laurent Vivier wrote:
>>> QEMU allows until 32 LUNs.
>>>
>>> Signed-off-by: Laurent Vivier <[email protected]>
>>> ---
>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>> index 04de287..4480d3e 100644
>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>> @@ -84,6 +84,7 @@
>>> */
>>> static int max_id = 64;
>>> static int max_channel = 3;
>>> +static int max_lun = 8;
>>> static int init_timeout = 300;
>>> static int login_timeout = 60;
>>> static int info_timeout = 30;
>>> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>>> +module_param(max_lun, int, S_IRUGO);
>>> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>>>
>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>> struct ibmvscsi_host_data *hostdata);
>>> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>>> goto init_pool_failed;
>>> }
>>>
>>> - host->max_lun = 8;
>>> + host->max_lun = max_lun;
>>> host->max_id = max_id;
>>> host->max_channel = max_channel;
>>> host->max_cmd_len = 16;
>>>
>> Please, don't do this.
>>
>> 'max_lun' should only be set if the HBA / transport has some hard
>> limitations on the number of bytes it can use.
>> Otherwise the scanning algorithm in scsi_scan.c should do the
>> correct thing, independent on the 'max_lun' setting.
>
> So you are saying we can remove the line ?
>
Ho-hum. In principle you could, as ibmvscsi is using SRP internally,
which does support 64 bit LUNs.
However, due to some weird design decisions there is
the function 'lun_from_dev()', which mangles the incoming LUN number
into something ... else.
Which leaves only 4 bits free for the actual LUN number, requiring
you to use max_lun = 16.
Personally I would just do away with that function and use the
incoming LUN numbers as is.
Brian? Any comments?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +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)
On 11/04/2015 07:02 AM, Hannes Reinecke wrote:
> On 11/04/2015 12:46 PM, Laurent Vivier wrote:
>>
>>
>> On 04/11/2015 12:16, Hannes Reinecke wrote:
>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote:
>>>> QEMU allows until 32 LUNs.
>>>>
>>>> Signed-off-by: Laurent Vivier <[email protected]>
>>>> ---
>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>> index 04de287..4480d3e 100644
>>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>> @@ -84,6 +84,7 @@
>>>> */
>>>> static int max_id = 64;
>>>> static int max_channel = 3;
>>>> +static int max_lun = 8;
>>>> static int init_timeout = 300;
>>>> static int login_timeout = 60;
>>>> static int info_timeout = 30;
>>>> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>>>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>>>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>>>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>>>> +module_param(max_lun, int, S_IRUGO);
>>>> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>>>>
>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>>> struct ibmvscsi_host_data *hostdata);
>>>> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>>>> goto init_pool_failed;
>>>> }
>>>>
>>>> - host->max_lun = 8;
>>>> + host->max_lun = max_lun;
>>>> host->max_id = max_id;
>>>> host->max_channel = max_channel;
>>>> host->max_cmd_len = 16;
>>>>
>>> Please, don't do this.
>>>
>>> 'max_lun' should only be set if the HBA / transport has some hard
>>> limitations on the number of bytes it can use.
>>> Otherwise the scanning algorithm in scsi_scan.c should do the
>>> correct thing, independent on the 'max_lun' setting.
>>
>> So you are saying we can remove the line ?
>>
> Ho-hum. In principle you could, as ibmvscsi is using SRP internally,
> which does support 64 bit LUNs.
>
> However, due to some weird design decisions there is
> the function 'lun_from_dev()', which mangles the incoming LUN number
> into something ... else.
> Which leaves only 4 bits free for the actual LUN number, requiring
> you to use max_lun = 16.
>
> Personally I would just do away with that function and use the
> incoming LUN numbers as is.
I pulled out my copy of SAM, what lun_from_dev is doing is translating
the incoming bus / id / LUN to a LUN in the logical unit addressing format,
as defined in 4.6.9 of SAM-4.
--------------------------------------
|Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 |
--------------------------------------
| n | (10b) | Target |
--------------------------------------
| n+1| Bus | LUN |
--------------------------------------
So this means, in the current implementation, we have 6 bits for target (max=63),
3 bits for bus (max=7), and 5 bits for LUN (max=31).
It might not be a bad idea to enforce these limits on the module parameters. Otherwise
we'll have a mess when we run through lun_from_dev...
As far as eliminating lun_from_dev and just passing the LUN through, I don't think we
can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are
doing the translation above.
-Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
On 05/11/2015 00:06, Brian King wrote:
> On 11/04/2015 07:02 AM, Hannes Reinecke wrote:
>> On 11/04/2015 12:46 PM, Laurent Vivier wrote:
>>>
>>>
>>> On 04/11/2015 12:16, Hannes Reinecke wrote:
>>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote:
>>>>> QEMU allows until 32 LUNs.
>>>>>
>>>>> Signed-off-by: Laurent Vivier <[email protected]>
>>>>> ---
>>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>> index 04de287..4480d3e 100644
>>>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>> @@ -84,6 +84,7 @@
>>>>> */
>>>>> static int max_id = 64;
>>>>> static int max_channel = 3;
>>>>> +static int max_lun = 8;
>>>>> static int init_timeout = 300;
>>>>> static int login_timeout = 60;
>>>>> static int info_timeout = 30;
>>>>> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>>>>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>>>>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>>>>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>>>>> +module_param(max_lun, int, S_IRUGO);
>>>>> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>>>>>
>>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>>>> struct ibmvscsi_host_data *hostdata);
>>>>> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>>>>> goto init_pool_failed;
>>>>> }
>>>>>
>>>>> - host->max_lun = 8;
>>>>> + host->max_lun = max_lun;
>>>>> host->max_id = max_id;
>>>>> host->max_channel = max_channel;
>>>>> host->max_cmd_len = 16;
>>>>>
>>>> Please, don't do this.
>>>>
>>>> 'max_lun' should only be set if the HBA / transport has some hard
>>>> limitations on the number of bytes it can use.
>>>> Otherwise the scanning algorithm in scsi_scan.c should do the
>>>> correct thing, independent on the 'max_lun' setting.
>>>
>>> So you are saying we can remove the line ?
>>>
>> Ho-hum. In principle you could, as ibmvscsi is using SRP internally,
>> which does support 64 bit LUNs.
>>
>> However, due to some weird design decisions there is
>> the function 'lun_from_dev()', which mangles the incoming LUN number
>> into something ... else.
>> Which leaves only 4 bits free for the actual LUN number, requiring
>> you to use max_lun = 16.
>>
>> Personally I would just do away with that function and use the
>> incoming LUN numbers as is.
>
> I pulled out my copy of SAM, what lun_from_dev is doing is translating
> the incoming bus / id / LUN to a LUN in the logical unit addressing format,
> as defined in 4.6.9 of SAM-4.
>
> --------------------------------------
> |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 |
> --------------------------------------
> | n | (10b) | Target |
> --------------------------------------
> | n+1| Bus | LUN |
> --------------------------------------
>
> So this means, in the current implementation, we have 6 bits for target (max=63),
> 3 bits for bus (max=7), and 5 bits for LUN (max=31).
>
> It might not be a bad idea to enforce these limits on the module parameters. Otherwise
> we'll have a mess when we run through lun_from_dev...
>
> As far as eliminating lun_from_dev and just passing the LUN through, I don't think we
> can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are
> doing the translation above.
So what I understand is this patch is OK but I have to check the given
value is less than 32 (the same max value as for QEMU) ?
Laurent
On 11/05/2015 12:06 AM, Brian King wrote:
> On 11/04/2015 07:02 AM, Hannes Reinecke wrote:
>> On 11/04/2015 12:46 PM, Laurent Vivier wrote:
>>>
>>>
>>> On 04/11/2015 12:16, Hannes Reinecke wrote:
>>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote:
>>>>> QEMU allows until 32 LUNs.
>>>>>
>>>>> Signed-off-by: Laurent Vivier <[email protected]>
>>>>> ---
>>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>> index 04de287..4480d3e 100644
>>>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>> @@ -84,6 +84,7 @@
>>>>> */
>>>>> static int max_id = 64;
>>>>> static int max_channel = 3;
>>>>> +static int max_lun = 8;
>>>>> static int init_timeout = 300;
>>>>> static int login_timeout = 60;
>>>>> static int info_timeout = 30;
>>>>> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>>>>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>>>>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>>>>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>>>>> +module_param(max_lun, int, S_IRUGO);
>>>>> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>>>>>
>>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>>>> struct ibmvscsi_host_data *hostdata);
>>>>> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>>>>> goto init_pool_failed;
>>>>> }
>>>>>
>>>>> - host->max_lun = 8;
>>>>> + host->max_lun = max_lun;
>>>>> host->max_id = max_id;
>>>>> host->max_channel = max_channel;
>>>>> host->max_cmd_len = 16;
>>>>>
>>>> Please, don't do this.
>>>>
>>>> 'max_lun' should only be set if the HBA / transport has some hard
>>>> limitations on the number of bytes it can use.
>>>> Otherwise the scanning algorithm in scsi_scan.c should do the
>>>> correct thing, independent on the 'max_lun' setting.
>>>
>>> So you are saying we can remove the line ?
>>>
>> Ho-hum. In principle you could, as ibmvscsi is using SRP internally,
>> which does support 64 bit LUNs.
>>
>> However, due to some weird design decisions there is
>> the function 'lun_from_dev()', which mangles the incoming LUN number
>> into something ... else.
>> Which leaves only 4 bits free for the actual LUN number, requiring
>> you to use max_lun = 16.
>>
>> Personally I would just do away with that function and use the
>> incoming LUN numbers as is.
>
> I pulled out my copy of SAM, what lun_from_dev is doing is translating
> the incoming bus / id / LUN to a LUN in the logical unit addressing format,
> as defined in 4.6.9 of SAM-4.
>
> --------------------------------------
> |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 |
> --------------------------------------
> | n | (10b) | Target |
> --------------------------------------
> | n+1| Bus | LUN |
> --------------------------------------
>
> So this means, in the current implementation, we have 6 bits for target (max=63),
> 3 bits for bus (max=7), and 5 bits for LUN (max=31).
>
> It might not be a bad idea to enforce these limits on the module parameters. Otherwise
> we'll have a mess when we run through lun_from_dev...
>
Correct. With the current implementation we need to set max_lun to 31.
> As far as eliminating lun_from_dev and just passing the LUN through, I don't think we
> can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are
> doing the translation above.
>
? How can that be ?
REPORT LUNs will give you a list of available LUNs _from that target_.
As the SCSI stack now moved to handle full 64bit LUNs the values
from REPORT LUNs will be used as-is (well, byte-swapped, but still).
Can you test with the attached patch?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +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)
On 11/05/2015 02:51 AM, Hannes Reinecke wrote:
> On 11/05/2015 12:06 AM, Brian King wrote:
>> On 11/04/2015 07:02 AM, Hannes Reinecke wrote:
>>> On 11/04/2015 12:46 PM, Laurent Vivier wrote:
>>>>
>>>>
>>>> On 04/11/2015 12:16, Hannes Reinecke wrote:
>>>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote:
>>>>>> QEMU allows until 32 LUNs.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <[email protected]>
>>>>>> ---
>>>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>>> index 04de287..4480d3e 100644
>>>>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>>> @@ -84,6 +84,7 @@
>>>>>> */
>>>>>> static int max_id = 64;
>>>>>> static int max_channel = 3;
>>>>>> +static int max_lun = 8;
>>>>>> static int init_timeout = 300;
>>>>>> static int login_timeout = 60;
>>>>>> static int info_timeout = 30;
>>>>>> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>>>>>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>>>>>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>>>>>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>>>>>> +module_param(max_lun, int, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>>>>>>
>>>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>>>>> struct ibmvscsi_host_data *hostdata);
>>>>>> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>>>>>> goto init_pool_failed;
>>>>>> }
>>>>>>
>>>>>> - host->max_lun = 8;
>>>>>> + host->max_lun = max_lun;
>>>>>> host->max_id = max_id;
>>>>>> host->max_channel = max_channel;
>>>>>> host->max_cmd_len = 16;
>>>>>>
>>>>> Please, don't do this.
>>>>>
>>>>> 'max_lun' should only be set if the HBA / transport has some hard
>>>>> limitations on the number of bytes it can use.
>>>>> Otherwise the scanning algorithm in scsi_scan.c should do the
>>>>> correct thing, independent on the 'max_lun' setting.
>>>>
>>>> So you are saying we can remove the line ?
>>>>
>>> Ho-hum. In principle you could, as ibmvscsi is using SRP internally,
>>> which does support 64 bit LUNs.
>>>
>>> However, due to some weird design decisions there is
>>> the function 'lun_from_dev()', which mangles the incoming LUN number
>>> into something ... else.
>>> Which leaves only 4 bits free for the actual LUN number, requiring
>>> you to use max_lun = 16.
>>>
>>> Personally I would just do away with that function and use the
>>> incoming LUN numbers as is.
>>
>> I pulled out my copy of SAM, what lun_from_dev is doing is translating
>> the incoming bus / id / LUN to a LUN in the logical unit addressing format,
>> as defined in 4.6.9 of SAM-4.
>>
>> --------------------------------------
>> |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 |
>> --------------------------------------
>> | n | (10b) | Target |
>> --------------------------------------
>> | n+1| Bus | LUN |
>> --------------------------------------
>>
>> So this means, in the current implementation, we have 6 bits for target (max=63),
>> 3 bits for bus (max=7), and 5 bits for LUN (max=31).
>>
>> It might not be a bad idea to enforce these limits on the module parameters. Otherwise
>> we'll have a mess when we run through lun_from_dev...
>>
> Correct. With the current implementation we need to set max_lun to 31.
>
>> As far as eliminating lun_from_dev and just passing the LUN through, I don't think we
>> can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are
>> doing the translation above.
>>
> ? How can that be ?
I was just going by what sg_luns returned on a VSCSI disk:
[root@powerio-blue-lp6 ~]# lsscsi
[0:0:1:0] disk AIX VDASD 0001 /dev/sda
[0:0:2:0] cd/dvd AIX VOPTA /dev/sr0
[0:0:3:0] disk AIX VDASD 0001 /dev/sdb
[0:0:4:0] disk AIX VDASD 0001 /dev/sdc
[0:0:7:0] cd/dvd AIX VOPTA /dev/sr1
[root@powerio-blue-lp6 ~]# sg_luns /dev/sda
Lun list length = 8 which imples 1 lun entry
Report luns [select_report=0x0]:
0000000000000000
[root@powerio-blue-lp6 ~]# sg_luns /dev/sdb
Lun list length = 8 which imples 1 lun entry
Report luns [select_report=0x0]:
0000000000000000
This is without your patch, but I don't think there should be any filtering of
report luns even in this case. Its what the VIOS is returning that is at
issue here.
Thanks,
Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
On 11/05/2015 02:26 AM, Laurent Vivier wrote:
>
>
> On 05/11/2015 00:06, Brian King wrote:
>> On 11/04/2015 07:02 AM, Hannes Reinecke wrote:
>>> On 11/04/2015 12:46 PM, Laurent Vivier wrote:
>>>>
>>>>
>>>> On 04/11/2015 12:16, Hannes Reinecke wrote:
>>>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote:
>>>>>> QEMU allows until 32 LUNs.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <[email protected]>
>>>>>> ---
>>>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++-
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>>> index 04de287..4480d3e 100644
>>>>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>>>>> @@ -84,6 +84,7 @@
>>>>>> */
>>>>>> static int max_id = 64;
>>>>>> static int max_channel = 3;
>>>>>> +static int max_lun = 8;
>>>>>> static int init_timeout = 300;
>>>>>> static int login_timeout = 60;
>>>>>> static int info_timeout = 30;
>>>>>> @@ -117,6 +118,8 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>>>>>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>>>>>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>>>>>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>>>>>> +module_param(max_lun, int, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(max_lun, "Maximum LUN value [Default=8]");
>>>>>>
>>>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>>>>> struct ibmvscsi_host_data *hostdata);
>>>>>> @@ -2289,7 +2292,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>>>>>> goto init_pool_failed;
>>>>>> }
>>>>>>
>>>>>> - host->max_lun = 8;
>>>>>> + host->max_lun = max_lun;
>>>>>> host->max_id = max_id;
>>>>>> host->max_channel = max_channel;
>>>>>> host->max_cmd_len = 16;
>>>>>>
>>>>> Please, don't do this.
>>>>>
>>>>> 'max_lun' should only be set if the HBA / transport has some hard
>>>>> limitations on the number of bytes it can use.
>>>>> Otherwise the scanning algorithm in scsi_scan.c should do the
>>>>> correct thing, independent on the 'max_lun' setting.
>>>>
>>>> So you are saying we can remove the line ?
>>>>
>>> Ho-hum. In principle you could, as ibmvscsi is using SRP internally,
>>> which does support 64 bit LUNs.
>>>
>>> However, due to some weird design decisions there is
>>> the function 'lun_from_dev()', which mangles the incoming LUN number
>>> into something ... else.
>>> Which leaves only 4 bits free for the actual LUN number, requiring
>>> you to use max_lun = 16.
>>>
>>> Personally I would just do away with that function and use the
>>> incoming LUN numbers as is.
>>
>> I pulled out my copy of SAM, what lun_from_dev is doing is translating
>> the incoming bus / id / LUN to a LUN in the logical unit addressing format,
>> as defined in 4.6.9 of SAM-4.
>>
>> --------------------------------------
>> |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 |
>> --------------------------------------
>> | n | (10b) | Target |
>> --------------------------------------
>> | n+1| Bus | LUN |
>> --------------------------------------
>>
>> So this means, in the current implementation, we have 6 bits for target (max=63),
>> 3 bits for bus (max=7), and 5 bits for LUN (max=31).
>>
>> It might not be a bad idea to enforce these limits on the module parameters. Otherwise
>> we'll have a mess when we run through lun_from_dev...
>>
>> As far as eliminating lun_from_dev and just passing the LUN through, I don't think we
>> can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are
>> doing the translation above.
>
> So what I understand is this patch is OK but I have to check the given
> value is less than 32 (the same max value as for QEMU) ?
As far as I'm concerned, yes.
Thanks,
Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center