[Qemu-devel] [PATCH] send readcapacity10 when readcapacity16 failed

[Qemu-devel] [PATCH] send readcapacity10 when readcapacity16 failed

[Zhu Lingshan]

When play with Dell MD3000 target, for sure it
is a TYPE_DISK, but readcapacity16 would fail.
Then we find that readcapacity10 succeeded. It
looks like the target just support readcapacity10
even through it is a TYPE_DISK or have some
TYPE_ROM characteristics.

This patch can give a chance to send
readcapacity16 when readcapacity10 failed.
This patch is not harmful to original pathes

Signed-off-by: Zhu Lingshan <lszhu@suse.com>
---
 block/iscsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index bd1f1bf..c8d167f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1243,8 +1243,9 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
                     iscsilun->lbprz = !!rc16->lbprz;
                     iscsilun->use_16_for_rw = (rc16->returned_lba > 0xffffffff);
                 }
+                break;
             }
-            break;
+        //fall through to try readcapacity10 instead
         case TYPE_ROM:
             task = iscsi_readcapacity10_sync(iscsilun->iscsi, iscsilun->lun, 0, 0);
             if (task != NULL && task->status == SCSI_STATUS_GOOD) {
-- 
2.6.2

Re: [Qemu-devel] [Qemu-block] [PATCH] send readcapacity10 when readcapacity16 failed

[John Snow]

On 12/28/2015 10:32 PM, Zhu Lingshan wrote:
> When play with Dell MD3000 target, for sure it
> is a TYPE_DISK, but readcapacity16 would fail.
> Then we find that readcapacity10 succeeded. It
> looks like the target just support readcapacity10
> even through it is a TYPE_DISK or have some
> TYPE_ROM characteristics.
> 
> This patch can give a chance to send
> readcapacity16 when readcapacity10 failed.
> This patch is not harmful to original pathes
> 
> Signed-off-by: Zhu Lingshan <lszhu@suse.com>
> ---
>  block/iscsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index bd1f1bf..c8d167f 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1243,8 +1243,9 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
>                      iscsilun->lbprz = !!rc16->lbprz;
>                      iscsilun->use_16_for_rw = (rc16->returned_lba > 0xffffffff);
>                  }
> +                break;
>              }
> -            break;
> +        //fall through to try readcapacity10 instead
>          case TYPE_ROM:
>              task = iscsi_readcapacity10_sync(iscsilun->iscsi, iscsilun->lun, 0, 0);
>              if (task != NULL && task->status == SCSI_STATUS_GOOD) {
> 

For the uninitiated, why does readcapacity16 fail?

My gut feeling is that this is a hack, because:

- Either readcapacity16 should work, or
- We shouldn't be choosing 10/16 based on the target type to begin with

but I don't know much about iSCSI, so maybe You, Paolo or Peter could
fill me in.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH] send readcapacity10 when readcapacity16 failed

[ronnie sahlberg]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

MMC devices:
READ CAPACITY 10 support is mandatory.
No support for READ CAPACITY 16

SBC devices:
READ CAPACITY 10 is mandatory
READ CAPACITY 16 support is only required when you have thin provisioning
or protection information (or if the device is >2^32 blocks)
Almost all, but apparently not all, SBC devices support both.


For SBC devices you probably want to start with RC16 and only fallback to
RC10 if you get INVALID_OPCODE.
You start with RC16 since this is the way to discover if you have thin
provisioning or special protection information.

For MMC devices you could try the "try RC16 first and fallback to RC10" but
as probably almost no MMC devices support RC16 it makes little sense to do
so.



On Tue, Jan 5, 2016 at 11:42 AM, John Snow <jsnow@redhat.com> wrote:

>
>
> On 12/28/2015 10:32 PM, Zhu Lingshan wrote:
> > When play with Dell MD3000 target, for sure it
> > is a TYPE_DISK, but readcapacity16 would fail.
> > Then we find that readcapacity10 succeeded. It
> > looks like the target just support readcapacity10
> > even through it is a TYPE_DISK or have some
> > TYPE_ROM characteristics.
> >
> > This patch can give a chance to send
> > readcapacity16 when readcapacity10 failed.
> > This patch is not harmful to original pathes
> >
> > Signed-off-by: Zhu Lingshan <lszhu@suse.com>
> > ---
> >  block/iscsi.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index bd1f1bf..c8d167f 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -1243,8 +1243,9 @@ static void iscsi_readcapacity_sync(IscsiLun
> *iscsilun, Error **errp)
> >                      iscsilun->lbprz = !!rc16->lbprz;
> >                      iscsilun->use_16_for_rw = (rc16->returned_lba >
> 0xffffffff);
> >                  }
> > +                break;
> >              }
> > -            break;
> > +        //fall through to try readcapacity10 instead
> >          case TYPE_ROM:
> >              task = iscsi_readcapacity10_sync(iscsilun->iscsi,
> iscsilun->lun, 0, 0);
> >              if (task != NULL && task->status == SCSI_STATUS_GOOD) {
> >
>
> For the uninitiated, why does readcapacity16 fail?
>
> My gut feeling is that this is a hack, because:
>
> - Either readcapacity16 should work, or
> - We shouldn't be choosing 10/16 based on the target type to begin with
>
> but I don't know much about iSCSI, so maybe You, Paolo or Peter could
> fill me in.
>
> --js
>

[-- Attachment #2: Type: text/html, Size: 3424 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] send readcapacity10 when readcapacity16 failed

[John Snow]

On 01/05/2016 02:57 PM, ronnie sahlberg wrote:
> MMC devices:
> READ CAPACITY 10 support is mandatory.
> No support for READ CAPACITY 16
> 
> SBC devices:
> READ CAPACITY 10 is mandatory
> READ CAPACITY 16 support is only required when you have thin
> provisioning or protection information (or if the device is >2^32 blocks)
> Almost all, but apparently not all, SBC devices support both.
> 
> 
> For SBC devices you probably want to start with RC16 and only fallback
> to RC10 if you get INVALID_OPCODE.
> You start with RC16 since this is the way to discover if you have thin
> provisioning or special protection information.
> 
> For MMC devices you could try the "try RC16 first and fallback to RC10"
> but as probably almost no MMC devices support RC16 it makes little sense
> to do so.
> 
> 

Ronnie: Thanks for the explanation!

Zhu: In light of this, can the patch be reworked slightly to explicitly
check *why* READCAPACITY16 failed and only attempt the READCAPACITY10 as
a fallback if it receives INVALID_OPCODE?

If it fails for any other reason it's probably best to report the error
and let QEMU decide what to do about it.

I suppose caching a flag that lets us know to go straight to
READ_CAPACITY10 is not worthwhile because this command is not likely to
be issued very often.

Thanks,
--js

> 
> On Tue, Jan 5, 2016 at 11:42 AM, John Snow <jsnow@redhat.com
> <mailto:jsnow@redhat.com>> wrote:
> 
> 
> 
>     On 12/28/2015 10:32 PM, Zhu Lingshan wrote:
>     > When play with Dell MD3000 target, for sure it
>     > is a TYPE_DISK, but readcapacity16 would fail.
>     > Then we find that readcapacity10 succeeded. It
>     > looks like the target just support readcapacity10
>     > even through it is a TYPE_DISK or have some
>     > TYPE_ROM characteristics.
>     >
>     > This patch can give a chance to send
>     > readcapacity16 when readcapacity10 failed.
>     > This patch is not harmful to original pathes
>     >
>     > Signed-off-by: Zhu Lingshan <lszhu@suse.com <mailto:lszhu@suse.com>>
>     > ---
>     >  block/iscsi.c | 3 ++-
>     >  1 file changed, 2 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/block/iscsi.c b/block/iscsi.c
>     > index bd1f1bf..c8d167f 100644
>     > --- a/block/iscsi.c
>     > +++ b/block/iscsi.c
>     > @@ -1243,8 +1243,9 @@ static void iscsi_readcapacity_sync(IscsiLun
>     *iscsilun, Error **errp)
>     >                      iscsilun->lbprz = !!rc16->lbprz;
>     >                      iscsilun->use_16_for_rw = (rc16->returned_lba
>     > 0xffffffff);
>     >                  }
>     > +                break;
>     >              }
>     > -            break;
>     > +        //fall through to try readcapacity10 instead
>     >          case TYPE_ROM:
>     >              task = iscsi_readcapacity10_sync(iscsilun->iscsi,
>     iscsilun->lun, 0, 0);
>     >              if (task != NULL && task->status == SCSI_STATUS_GOOD) {
>     >
> 
>     For the uninitiated, why does readcapacity16 fail?
> 
>     My gut feeling is that this is a hack, because:
> 
>     - Either readcapacity16 should work, or
>     - We shouldn't be choosing 10/16 based on the target type to begin with
> 
>     but I don't know much about iSCSI, so maybe You, Paolo or Peter could
>     fill me in.
> 
>     --js
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH] send readcapacity10 when readcapacity16 failed

[Paolo Bonzini]

On 06/01/2016 18:57, John Snow wrote:
> Ronnie: Thanks for the explanation!
> 
> Zhu: In light of this, can the patch be reworked slightly to explicitly
> check *why* READCAPACITY16 failed and only attempt the READCAPACITY10 as
> a fallback if it receives INVALID_OPCODE?
> 
> If it fails for any other reason it's probably best to report the error
> and let QEMU decide what to do about it.

Any other failure probably would happen for READ CAPACITY(10) as well, so
it's okay to ignore it for READ CAPACITY(16).

Zhu's patch matches what Linux does by default, it seems okay.  The only
change needed is to retry READ CAPACITY(16) if there is a UNIT ATTENTION
sense:

+            if (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
+                && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+                break;
+            }

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH] send readcapacity10 when readcapacity16 failed

[Peter Lieven]

Am 07.01.2016 um 11:07 schrieb Paolo Bonzini:
>
> On 06/01/2016 18:57, John Snow wrote:
>> Ronnie: Thanks for the explanation!
>>
>> Zhu: In light of this, can the patch be reworked slightly to explicitly
>> check *why* READCAPACITY16 failed and only attempt the READCAPACITY10 as
>> a fallback if it receives INVALID_OPCODE?
>>
>> If it fails for any other reason it's probably best to report the error
>> and let QEMU decide what to do about it.
> Any other failure probably would happen for READ CAPACITY(10) as well, so
> it's okay to ignore it for READ CAPACITY(16).
>
> Zhu's patch matches what Linux does by default, it seems okay.  The only
> change needed is to retry READ CAPACITY(16) if there is a UNIT ATTENTION
> sense:
>
> +            if (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
> +                && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
> +                break;
> +            }
>
> Paolo

Paolo, Ronnie, do you know what Readcapacity(10) returns if the target
blocks count is greater than what can be described in 32bit?

Anyway, is there a new version of this patch? I would also like to have a look
before it is commited.

Thanks,
Peter

Re: [Qemu-devel] [Qemu-block] [PATCH] send readcapacity10 when readcapacity16 failed

[Paolo Bonzini]

On 11/01/2016 09:49, Peter Lieven wrote:
> > +            if (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
> > +                && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
> > +                break;
> > +            }
>
> Paolo, Ronnie, do you know what Readcapacity(10) returns if the target
> blocks count is greater than what can be described in 32bit?

Yes, it returns 0xFFFFFFFF.

> Anyway, is there a new version of this patch? I would also like to have a look
> before it is commited.

See https://github.com/bonzini/qemu/commit/1efcbda37.

Paolo

Re: [Qemu-devel] [PATCH] send readcapacity10 when readcapacity16 failed

[Paolo Bonzini]

On 29/12/2015 04:32, Zhu Lingshan wrote:
> When play with Dell MD3000 target, for sure it
> is a TYPE_DISK, but readcapacity16 would fail.
> Then we find that readcapacity10 succeeded. It
> looks like the target just support readcapacity10
> even through it is a TYPE_DISK or have some
> TYPE_ROM characteristics.
> 
> This patch can give a chance to send
> readcapacity16 when readcapacity10 failed.
> This patch is not harmful to original pathes
> 
> Signed-off-by: Zhu Lingshan <lszhu@suse.com>
> ---
>  block/iscsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index bd1f1bf..c8d167f 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1243,8 +1243,9 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
>                      iscsilun->lbprz = !!rc16->lbprz;
>                      iscsilun->use_16_for_rw = (rc16->returned_lba > 0xffffffff);
>                  }
> +                break;
>              }
> -            break;
> +        //fall through to try readcapacity10 instead
>          case TYPE_ROM:
>              task = iscsi_readcapacity10_sync(iscsilun->iscsi, iscsilun->lun, 0, 0);
>              if (task != NULL && task->status == SCSI_STATUS_GOOD) {
> 

Queued, thanks.

Paolo