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 <address@hidden
> <mailto:address@hidden>> 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 <address@hidden <mailto:address@hidden>>
>     > ---
>     >  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
> 
>