[PATCH] blkmapd: Skip the SCSI ID if the data length is zero

[PATCH] blkmapd: Skip the SCSI ID if the data length is zero

[Kinglong Mee]

In vmware linux, the iscsi device contains more than one SCSI ID,
and, the second one's data length is zero.

blkmapd must skip it, otherwise, some device will be treat as the above one.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 utils/blkmapd/device-inq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/blkmapd/device-inq.c b/utils/blkmapd/device-inq.c
index 6b56b67..57fe7a3 100644
--- a/utils/blkmapd/device-inq.c
+++ b/utils/blkmapd/device-inq.c
@@ -198,7 +198,7 @@ struct bl_serial *bldev_read_serial(int fd, const char *filename)
 		dev_id = (struct bl_dev_id *)&(dev_root->data[pos]);
 		pos += (dev_id->len + devid_len);
 
-		if ((dev_id->ids & 0xf) < current_id)
+		if ((dev_id->ids & 0xf) < current_id || !dev_id->len)
 			continue;
 		switch (dev_id->ids & 0xf) {
 			/* We process SCSI ID with four ID cases: 0, 1, 2 and 3.
-- 
2.4.3

Re: [PATCH] blkmapd: Skip the SCSI ID if the data length is zero

[Christoph Hellwig]

On Mon, Jul 13, 2015 at 07:57:24AM +0800, Kinglong Mee wrote:
> In vmware linux, the iscsi device contains more than one SCSI ID,
> and, the second one's data length is zero.
> 
> blkmapd must skip it, otherwise, some device will be treat as the above one.
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>

Looks good, but can you please add a comment explaining why you're
skipping the zero-length EVPD page?

[PATCH v2] blkmapd: Skip the SCSI ID if data length is zero

[Kinglong Mee]

In vmware linux, the iscsi device contains more than one SCSI ID,
and, the second one's data length is zero.

If there are two iSCSI devices with the second SCSI ID's data length
is zero, the first iSCSI device will record with an invalid SCSI ID
as zero length, the second one will be treat as the first one for
the SCSI ID is zero length too.

It means the only the first iSCSI device is exist in blkmapd's cache,
the request for the second iSCSI device will failed as,

"blkmapd: Could not find disk for device" and,
"bl_resolve_deviceid failed to decode device: 2".

v2, update comments

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 utils/blkmapd/device-inq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/blkmapd/device-inq.c b/utils/blkmapd/device-inq.c
index 6b56b67..57fe7a3 100644
--- a/utils/blkmapd/device-inq.c
+++ b/utils/blkmapd/device-inq.c
@@ -198,7 +198,7 @@ struct bl_serial *bldev_read_serial(int fd, const char *filename)
 		dev_id = (struct bl_dev_id *)&(dev_root->data[pos]);
 		pos += (dev_id->len + devid_len);
 
-		if ((dev_id->ids & 0xf) < current_id)
+		if ((dev_id->ids & 0xf) < current_id || !dev_id->len)
 			continue;
 		switch (dev_id->ids & 0xf) {
 			/* We process SCSI ID with four ID cases: 0, 1, 2 and 3.
-- 
2.4.3

Re: [PATCH v2] blkmapd: Skip the SCSI ID if data length is zero

[Christoph Hellwig]

Hi Kinglong,

I meant to describe the rational as a comment in the code, not as
a more verbose commit log.

Re: [PATCH v2] blkmapd: Skip the SCSI ID if data length is zero

[Kinglong Mee]

On 7/13/2015 17:01, Christoph Hellwig wrote:
> Hi Kinglong,
> 
> I meant to describe the rational as a comment in the code, not as
> a more verbose commit log.

Sorry for my mistake, I will update it later.
Thanks again.

thanks,
Kinglong Mee

[PATCH v3] blkmapd: Skip the SCSI ID if data length is zero

[Kinglong Mee]

In vmware linux, the iscsi device contains more than one SCSI ID, 
and the second one's data length is zero.

If there are two iSCSI devices with the second SCSI ID's data length
is zero, the first iSCSI device will record with an invalid SCSI ID
as zero length, the second one will be treat as the first one for
the SCSI ID is zero length too.

It means the only the first iSCSI device is exist in blkmapd's cache,
the request for the second iSCSI device will failed as,

"blkmapd: Could not find disk for device" and,
"bl_resolve_deviceid failed to decode device: 2".

v2, update commit log
v3, add a comment in the code

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 utils/blkmapd/device-inq.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/utils/blkmapd/device-inq.c b/utils/blkmapd/device-inq.c
index 6b56b67..60fe2d3 100644
--- a/utils/blkmapd/device-inq.c
+++ b/utils/blkmapd/device-inq.c
@@ -198,6 +198,10 @@ struct bl_serial *bldev_read_serial(int fd, const char *filename)
 		dev_id = (struct bl_dev_id *)&(dev_root->data[pos]);
 		pos += (dev_id->len + devid_len);
 
+		/* Needs an SCSI ID with unique date not zero length */
+		if (!dev_id->len)
+			continue;
+
 		if ((dev_id->ids & 0xf) < current_id)
 			continue;
 		switch (dev_id->ids & 0xf) {
-- 
2.4.3

Re: [PATCH v3] blkmapd: Skip the SCSI ID if data length is zero

[Christoph Hellwig]

> +		/* Needs an SCSI ID with unique date not zero length */
> +		if (!dev_id->len)
> +			continue;

That explains what the code does, not why it does so.

How about:

		/*
		 * Some buggy targets (e.g. VMWare) export zero length
		 * EVPD pages, skip them to not confuse the device id
		 * cache.
		 */

[PATCH v4] blkmapd: Skip the SCSI ID if data length is zero

[Kinglong Mee]

In vmware linux, the iscsi device contains more than one SCSI ID,
and the second one's data length is zero.

If there are two iSCSI devices with the second SCSI ID's data length
is zero, the first iSCSI device will record with an invalid SCSI ID
as zero length, the second one will be treat as the first one for
the SCSI ID is zero length too.

It means the only the first iSCSI device is exist in blkmapd's cache,
the request for the second iSCSI device will failed as,

"blkmapd: Could not find disk for device" and,
"bl_resolve_deviceid failed to decode device: 2".

v2, update comments
v3, add a comment in the code
v4, update comment as Christoph's suggestion

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 utils/blkmapd/device-inq.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/utils/blkmapd/device-inq.c b/utils/blkmapd/device-inq.c
index 6b56b67..5c258bd 100644
--- a/utils/blkmapd/device-inq.c
+++ b/utils/blkmapd/device-inq.c
@@ -198,6 +198,13 @@ struct bl_serial *bldev_read_serial(int fd, const char *filename)
 		dev_id = (struct bl_dev_id *)&(dev_root->data[pos]);
 		pos += (dev_id->len + devid_len);
 
+		/* Some buggy targets (e.g. VMWare) export zero length
+		 * EVPD pages, skip them to not confuse the device id
+		 * cache.
+		 */
+		if (!dev_id->len)
+			continue;
+
 		if ((dev_id->ids & 0xf) < current_id)
 			continue;
 		switch (dev_id->ids & 0xf) {
-- 
2.4.3

Re: [PATCH v4] blkmapd: Skip the SCSI ID if data length is zero

[Steve Dickson]

Hello,

On 07/14/2015 10:11 AM, Kinglong Mee wrote:
> In vmware linux, the iscsi device contains more than one SCSI ID,
> and the second one's data length is zero.
> 
> If there are two iSCSI devices with the second SCSI ID's data length
> is zero, the first iSCSI device will record with an invalid SCSI ID
> as zero length, the second one will be treat as the first one for
> the SCSI ID is zero length too.
> 
> It means the only the first iSCSI device is exist in blkmapd's cache,
> the request for the second iSCSI device will failed as,
> 
> "blkmapd: Could not find disk for device" and,
> "bl_resolve_deviceid failed to decode device: 2".
> 
> v2, update comments
> v3, add a comment in the code
> v4, update comment as Christoph's suggestion
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  utils/blkmapd/device-inq.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/utils/blkmapd/device-inq.c b/utils/blkmapd/device-inq.c
> index 6b56b67..5c258bd 100644
> --- a/utils/blkmapd/device-inq.c
> +++ b/utils/blkmapd/device-inq.c
> @@ -198,6 +198,13 @@ struct bl_serial *bldev_read_serial(int fd, const char *filename)
>  		dev_id = (struct bl_dev_id *)&(dev_root->data[pos]);
>  		pos += (dev_id->len + devid_len);
>  
> +		/* Some buggy targets (e.g. VMWare) export zero length
> +		 * EVPD pages, skip them to not confuse the device id
> +		 * cache.
> +		 */
I'll take care of this comment... No need to re-post. 

steved.

> +		if (!dev_id->len)
> +			continue;
> +
>  		if ((dev_id->ids & 0xf) < current_id)
>  			continue;
>  		switch (dev_id->ids & 0xf) {
> 

Re: [PATCH v4] blkmapd: Skip the SCSI ID if data length is zero

[Steve Dickson]

On 07/14/2015 10:11 AM, Kinglong Mee wrote:
> In vmware linux, the iscsi device contains more than one SCSI ID,
> and the second one's data length is zero.
> 
> If there are two iSCSI devices with the second SCSI ID's data length
> is zero, the first iSCSI device will record with an invalid SCSI ID
> as zero length, the second one will be treat as the first one for
> the SCSI ID is zero length too.
> 
> It means the only the first iSCSI device is exist in blkmapd's cache,
> the request for the second iSCSI device will failed as,
> 
> "blkmapd: Could not find disk for device" and,
> "bl_resolve_deviceid failed to decode device: 2".
> 
> v2, update comments
> v3, add a comment in the code
> v4, update comment as Christoph's suggestion
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
Committed... 

steved.

> ---
>  utils/blkmapd/device-inq.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/utils/blkmapd/device-inq.c b/utils/blkmapd/device-inq.c
> index 6b56b67..5c258bd 100644
> --- a/utils/blkmapd/device-inq.c
> +++ b/utils/blkmapd/device-inq.c
> @@ -198,6 +198,13 @@ struct bl_serial *bldev_read_serial(int fd, const char *filename)
>  		dev_id = (struct bl_dev_id *)&(dev_root->data[pos]);
>  		pos += (dev_id->len + devid_len);
>  
> +		/* Some buggy targets (e.g. VMWare) export zero length
> +		 * EVPD pages, skip them to not confuse the device id
> +		 * cache.
> +		 */
> +		if (!dev_id->len)
> +			continue;
> +
>  		if ((dev_id->ids & 0xf) < current_id)
>  			continue;
>  		switch (dev_id->ids & 0xf) {
> 

Re: [PATCH v3] blkmapd: Skip the SCSI ID if data length is zero

[Steve Dickson]

On 07/14/2015 03:15 AM, Christoph Hellwig wrote:
>> +		/* Needs an SCSI ID with unique date not zero length */
>> +		if (!dev_id->len)
>> +			continue;
> 
> That explains what the code does, not why it does so.
> 
> How about:
> 
> 		/*
> 		 * Some buggy targets (e.g. VMWare) export zero length
> 		 * EVPD pages, skip them to not confuse the device id
> 		 * cache.
> 		 */
> 
I'm not a big fan of calling out vendors in comments. I'm thinking
dropping the "buggy" and "(e.g. VMWare)" would be appropriated.

steved.