[Qemu-devel] [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Qemu-devel] [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a handful of bugfixes for scsi-generic
that where added into:

commit a4194b3f79a85e111f000788ddec05d465748851
Author: Hannes Reinecke <hare@suse.de>
Date:   Mon Nov 22 15:39:33 2010 -0800

    scsi: Use 'SCSIRequest' directly

this includes:

*) Fix incorrect errno usage in switch() statement within
   scsi_command_complete()

*) Remove bogus scsi_command_complete() for residual case
   within scsi_read_complete()

*) Remove incorrect '-' sign from return in scsi_send_command()

Tested with .37-rc2 TCM_Loop FILEIO backstores on KVM host into
Debian Lenny v2.6.26 KVM guest with an xfs filesystem mount.

Signed-off-by: Nicholas A. Bellinger nab@linux-iscsi.org>
---
 hw/scsi-generic.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 7d30115..dc277cc 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -146,13 +146,13 @@ static void scsi_command_complete(void *opaque, int ret)
 
     if (ret != 0) {
         switch(ret) {
-            case ENODEV:
+            case -ENODEV:
                 s->senselen = scsi_set_sense(s, SENSE_CODE(LUN_NOT_SUPPORTED));
                 break;
-            case EINVAL:
+            case -EINVAL:
                 s->senselen = scsi_set_sense(s, SENSE_CODE(INVALID_FIELD));
                 break;
-            case EBADR:
+            case -EBADR:
                 s->senselen = scsi_set_sense(s, SENSE_CODE(TARGET_FAILURE));
                 break;
             default:
@@ -230,12 +230,10 @@ static void scsi_read_complete(void * opaque, int ret)
         return;
     }
     len = r->io_header.dxfer_len - r->io_header.resid;
-    DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
+    DPRINTF("Data ready tag=0x%x remaining len=%d\n", r->req.tag, len);
 
     r->len = -1;
     r->req.bus->complete(&r->req, SCSI_REASON_DATA, len);
-    if (len == 0)
-        scsi_command_complete(r, 0);
 }
 
 /* Read more data from scsi device into buffer.  */
@@ -375,7 +373,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
     }
     scsi_req_fixup(&r->req);
 
-    DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", lun, tag,
+    DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", req->lun, req->tag,
             r->req.cmd.xfer, cmd[0]);
 
 #ifdef DEBUG_SCSI
@@ -414,7 +412,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
     r->len = r->req.cmd.xfer;
     if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
         r->len = 0;
-        return -r->req.cmd.xfer;
+        return r->req.cmd.xfer;
     }
 
     return r->req.cmd.xfer;
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Hannes Reinecke]

On 11/24/2010 09:18 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch adds a handful of bugfixes for scsi-generic
> that where added into:
> 
> commit a4194b3f79a85e111f000788ddec05d465748851
> Author: Hannes Reinecke <hare@suse.de>
> Date:   Mon Nov 22 15:39:33 2010 -0800
> 
>     scsi: Use 'SCSIRequest' directly
> 
> this includes:
> 
> *) Fix incorrect errno usage in switch() statement within
>    scsi_command_complete()
> 
> *) Remove bogus scsi_command_complete() for residual case
>    within scsi_read_complete()
> 
> *) Remove incorrect '-' sign from return in scsi_send_command()
> 
> Tested with .37-rc2 TCM_Loop FILEIO backstores on KVM host into
> Debian Lenny v2.6.26 KVM guest with an xfs filesystem mount.
> 
> Signed-off-by: Nicholas A. Bellinger nab@linux-iscsi.org>
> ---
>  hw/scsi-generic.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
> index 7d30115..dc277cc 100644
> --- a/hw/scsi-generic.c
> +++ b/hw/scsi-generic.c
> @@ -146,13 +146,13 @@ static void scsi_command_complete(void *opaque, int ret)
>  
>      if (ret != 0) {
>          switch(ret) {
> -            case ENODEV:
> +            case -ENODEV:
>                  s->senselen = scsi_set_sense(s, SENSE_CODE(LUN_NOT_SUPPORTED));
>                  break;
> -            case EINVAL:
> +            case -EINVAL:
>                  s->senselen = scsi_set_sense(s, SENSE_CODE(INVALID_FIELD));
>                  break;
> -            case EBADR:
> +            case -EBADR:
>                  s->senselen = scsi_set_sense(s, SENSE_CODE(TARGET_FAILURE));
>                  break;
>              default:
Oh. Correct. Although we could do a 'switch (-ret)' here.

> @@ -230,12 +230,10 @@ static void scsi_read_complete(void * opaque, int ret)
>          return;
>      }
>      len = r->io_header.dxfer_len - r->io_header.resid;
> -    DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
> +    DPRINTF("Data ready tag=0x%x remaining len=%d\n", r->req.tag, len);
>  
>      r->len = -1;
>      r->req.bus->complete(&r->req, SCSI_REASON_DATA, len);
> -    if (len == 0)
> -        scsi_command_complete(r, 0);
>  }
>  
>  /* Read more data from scsi device into buffer.  */

Yes, that's correct (after a fashion).
Difference is that we're having to do one more callback into
scsi_read_data() with this hunk. But ok, ack.

> @@ -375,7 +373,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
>      }
>      scsi_req_fixup(&r->req);
>  
> -    DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", lun, tag,
> +    DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", req->lun, req->tag,
>              r->req.cmd.xfer, cmd[0]);
>  
>  #ifdef DEBUG_SCSI
> @@ -414,7 +412,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
>      r->len = r->req.cmd.xfer;
>      if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
>          r->len = 0;
> -        return -r->req.cmd.xfer;
> +        return r->req.cmd.xfer;
>      }
>  
>      return r->req.cmd.xfer;
NACK.
Returning a negative value here means we're about to execute a write.
Cf comment at the start of the function:

/* Execute a scsi command.  Returns the length of the data
   expected by the command.  This will be Positive for data
   transfers from the device (eg. disk reads), negative for
   transfers to the device (eg. disk writes),
   and zero if the command does not transfer any data.  */

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Nicholas A. Bellinger]

On Wed, 2010-11-24 at 09:57 +0100, Hannes Reinecke wrote:
> On 11/24/2010 09:18 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch adds a handful of bugfixes for scsi-generic
> > that where added into:
> > 
> > commit a4194b3f79a85e111f000788ddec05d465748851
> > Author: Hannes Reinecke <hare@suse.de>
> > Date:   Mon Nov 22 15:39:33 2010 -0800
> > 
> >     scsi: Use 'SCSIRequest' directly
> > 
> > this includes:
> > 
> > *) Fix incorrect errno usage in switch() statement within
> >    scsi_command_complete()
> > 
> > *) Remove bogus scsi_command_complete() for residual case
> >    within scsi_read_complete()
> > 
> > *) Remove incorrect '-' sign from return in scsi_send_command()
> > 
> > Tested with .37-rc2 TCM_Loop FILEIO backstores on KVM host into
> > Debian Lenny v2.6.26 KVM guest with an xfs filesystem mount.
> > 
> > Signed-off-by: Nicholas A. Bellinger nab@linux-iscsi.org>
> > ---
> >  hw/scsi-generic.c |   14 ++++++--------
> >  1 files changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
> > index 7d30115..dc277cc 100644
> > --- a/hw/scsi-generic.c
> > +++ b/hw/scsi-generic.c
> > @@ -146,13 +146,13 @@ static void scsi_command_complete(void *opaque, int ret)
> >  
> >      if (ret != 0) {
> >          switch(ret) {
> > -            case ENODEV:
> > +            case -ENODEV:
> >                  s->senselen = scsi_set_sense(s, SENSE_CODE(LUN_NOT_SUPPORTED));
> >                  break;
> > -            case EINVAL:
> > +            case -EINVAL:
> >                  s->senselen = scsi_set_sense(s, SENSE_CODE(INVALID_FIELD));
> >                  break;
> > -            case EBADR:
> > +            case -EBADR:
> >                  s->senselen = scsi_set_sense(s, SENSE_CODE(TARGET_FAILURE));
> >                  break;
> >              default:
> Oh. Correct. Although we could do a 'switch (-ret)' here.
> 
> > @@ -230,12 +230,10 @@ static void scsi_read_complete(void * opaque, int ret)
> >          return;
> >      }
> >      len = r->io_header.dxfer_len - r->io_header.resid;
> > -    DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
> > +    DPRINTF("Data ready tag=0x%x remaining len=%d\n", r->req.tag, len);
> >  
> >      r->len = -1;
> >      r->req.bus->complete(&r->req, SCSI_REASON_DATA, len);
> > -    if (len == 0)
> > -        scsi_command_complete(r, 0);
> >  }
> >  
> >  /* Read more data from scsi device into buffer.  */
> 
> Yes, that's correct (after a fashion).
> Difference is that we're having to do one more callback into
> scsi_read_data() with this hunk. But ok, ack.
> 
> > @@ -375,7 +373,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
> >      }
> >      scsi_req_fixup(&r->req);
> >  
> > -    DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", lun, tag,
> > +    DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", req->lun, req->tag,
> >              r->req.cmd.xfer, cmd[0]);
> >  
> >  #ifdef DEBUG_SCSI
> > @@ -414,7 +412,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
> >      r->len = r->req.cmd.xfer;
> >      if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
> >          r->len = 0;
> > -        return -r->req.cmd.xfer;
> > +        return r->req.cmd.xfer;
> >      }
> >  
> >      return r->req.cmd.xfer;
> NACK.
> Returning a negative value here means we're about to execute a write.
> Cf comment at the start of the function:
> 
> /* Execute a scsi command.  Returns the length of the data
>    expected by the command.  This will be Positive for data
>    transfers from the device (eg. disk reads), negative for
>    transfers to the device (eg. disk writes),
>    and zero if the command does not transfer any data.  */
> 

OK, changint this back in megasas-upstream-v1 here along with the same
bug that was added to the the outgoing hw/scsi-bsg.c..

But yeah, this is a really confusing interface and seems like it really
should be fixed, right..?  Unless there is some legacy reason that it
makes it difficult to do so..?

--nab

[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Kevin Wolf]

Am 24.11.2010 09:57, schrieb Hannes Reinecke:
> On 11/24/2010 09:18 AM, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>
>> This patch adds a handful of bugfixes for scsi-generic
>> that where added into:
>>
>> commit a4194b3f79a85e111f000788ddec05d465748851
>> Author: Hannes Reinecke <hare@suse.de>
>> Date:   Mon Nov 22 15:39:33 2010 -0800
>>
>>     scsi: Use 'SCSIRequest' directly
>>
>> this includes:
>>
>> *) Fix incorrect errno usage in switch() statement within
>>    scsi_command_complete()
>>
>> *) Remove bogus scsi_command_complete() for residual case
>>    within scsi_read_complete()
>>
>> *) Remove incorrect '-' sign from return in scsi_send_command()
>>
>> Tested with .37-rc2 TCM_Loop FILEIO backstores on KVM host into
>> Debian Lenny v2.6.26 KVM guest with an xfs filesystem mount.
>>
>> Signed-off-by: Nicholas A. Bellinger nab@linux-iscsi.org>

Hannes, can you fold the necessary parts of this into the next version
of your series, so that we don't break things first and fix them only later?

Kevin

[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Hannes Reinecke]

On 11/24/2010 10:04 AM, Kevin Wolf wrote:
> Am 24.11.2010 09:57, schrieb Hannes Reinecke:
>> On 11/24/2010 09:18 AM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>>
>>> This patch adds a handful of bugfixes for scsi-generic
>>> that where added into:
>>>
>>> commit a4194b3f79a85e111f000788ddec05d465748851
>>> Author: Hannes Reinecke <hare@suse.de>
>>> Date:   Mon Nov 22 15:39:33 2010 -0800
>>>
>>>     scsi: Use 'SCSIRequest' directly
>>>
>>> this includes:
>>>
>>> *) Fix incorrect errno usage in switch() statement within
>>>    scsi_command_complete()
>>>
>>> *) Remove bogus scsi_command_complete() for residual case
>>>    within scsi_read_complete()
>>>
>>> *) Remove incorrect '-' sign from return in scsi_send_command()
>>>
>>> Tested with .37-rc2 TCM_Loop FILEIO backstores on KVM host into
>>> Debian Lenny v2.6.26 KVM guest with an xfs filesystem mount.
>>>
>>> Signed-off-by: Nicholas A. Bellinger nab@linux-iscsi.org>
> 
> Hannes, can you fold the necessary parts of this into the next version
> of your series, so that we don't break things first and fix them only later?
> 
I'll be folding the first one into my series.

The second one is actually a genuine error, and a rather old one to
boot. It went in with this commit:

commit 89c0f6438d16ebceccdcd096bbc0b5536146a443
Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Fri Oct 17 08:08:56 2008 +0000
Subject: scsi-generic: correct error management

So I'd rather have it submitted separately.
And a second opinion is _definitely_ required here.
nab, can you do the honours?

The third is bogus anyway.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Kevin Wolf]

Am 24.11.2010 11:16, schrieb Hannes Reinecke:
> The second one is actually a genuine error, and a rather old one to
> boot. It went in with this commit:
> 
> commit 89c0f6438d16ebceccdcd096bbc0b5536146a443
> Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Fri Oct 17 08:08:56 2008 +0000
> Subject: scsi-generic: correct error management
> 
> So I'd rather have it submitted separately.
> And a second opinion is _definitely_ required here.
> nab, can you do the honours?

The commit message of this commit says it's a workaround for a problem
with lsi:

> - when a read is aborted due to a mark/EOF/EOD/EOM, the len reported to
> controller can be 0. LSI controller emulation doesn't know how to manage
> this. A workaround found is to call the completion routine with
> SCSI_REASON_DONE just after calling it with SCSI_REASON_DATA with len=0.

Are you sure that it's not needed any more?

Kevin

[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Hannes Reinecke]

On 11/24/2010 11:34 AM, Kevin Wolf wrote:
> Am 24.11.2010 11:16, schrieb Hannes Reinecke:
>> The second one is actually a genuine error, and a rather old one to
>> boot. It went in with this commit:
>>
>> commit 89c0f6438d16ebceccdcd096bbc0b5536146a443
>> Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
>> Date:   Fri Oct 17 08:08:56 2008 +0000
>> Subject: scsi-generic: correct error management
>>
>> So I'd rather have it submitted separately.
>> And a second opinion is _definitely_ required here.
>> nab, can you do the honours?
> 
> The commit message of this commit says it's a workaround for a problem
> with lsi:
> 
>> - when a read is aborted due to a mark/EOF/EOD/EOM, the len reported to
>> controller can be 0. LSI controller emulation doesn't know how to manage
>> this. A workaround found is to call the completion routine with
>> SCSI_REASON_DONE just after calling it with SCSI_REASON_DATA with len=0.
> 
> Are you sure that it's not needed any more?
> 
Don't ask me. I didn't do the patch, and my knowledge of lsi HBA
internals is scanty.
Nic, can you comment here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Benjamin Herrenschmidt]

On Wed, 2010-11-24 at 11:46 +0100, Hannes Reinecke wrote:
> >> - when a read is aborted due to a mark/EOF/EOD/EOM, the len
> reported to
> >> controller can be 0. LSI controller emulation doesn't know how to
> manage
> >> this. A workaround found is to call the completion routine with
> >> SCSI_REASON_DONE just after calling it with SCSI_REASON_DATA with
> len=0.
> > 
> > Are you sure that it's not needed any more?
> > 
> Don't ask me. I didn't do the patch, and my knowledge of lsi HBA
> internals is scanty.
> Nic, can you comment here? 

Well, writing an HBA myself, it took me a while to figure out what I'm
supposed to expect from the layer :-)

So far tho, it appears that I can (at least with scsi-disk) rely on
always been eventually called with SCSI_REASON_DONE so my code (and
maybe the usb-msd code too, I haven't verified) relies on that to
complete requests... Is that incorrect ?

Cheers,
Ben.

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Gerd Hoffmann]

On 11/25/10 09:46, Benjamin Herrenschmidt wrote:

> So far tho, it appears that I can (at least with scsi-disk) rely on
> always been eventually called with SCSI_REASON_DONE so my code (and
> maybe the usb-msd code too, I haven't verified) relies on that to
> complete requests... Is that incorrect ?

Yes.

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Benjamin Herrenschmidt]

On Thu, 2010-11-25 at 09:50 +0100, Gerd Hoffmann wrote:
> On 11/25/10 09:46, Benjamin Herrenschmidt wrote:
> 
> > So far tho, it appears that I can (at least with scsi-disk) rely on
> > always been eventually called with SCSI_REASON_DONE so my code (and
> > maybe the usb-msd code too, I haven't verified) relies on that to
> > complete requests... Is that incorrect ?
> 
> Yes.

Well, so far it works :-) But I suppose I must be lucky.. I must admit
that it's very unclear how that SCSI "stack" is meant to be used from
the HBA standpoint.

Right now, I've somewhat come up with:

  - client request occurs
  - call device send_command()
  - if result is 0, assume my complete() was called with 
    SCSI_REASON_DONE
  - else, use sign of result for transfer direction, store the
    absolute value as the total expected transfer len and call
    the device scsi_data_read()/write() and wait for complete()
  - when complete() is called:
    - if SCSI_REASON_DONE, complete client request
    - else perform the client "DMA" for "arg" bytes
    - call the device scsi_data_read()/write() again

So far it seems to work with scsi-disk but maybe I miss something in
which case I would very much enjoy being corrected :-)

Cheers,
Ben.

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Hannes Reinecke]

On 11/25/2010 10:01 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2010-11-25 at 09:50 +0100, Gerd Hoffmann wrote:
>> On 11/25/10 09:46, Benjamin Herrenschmidt wrote:
>>
>>> So far tho, it appears that I can (at least with scsi-disk) rely on
>>> always been eventually called with SCSI_REASON_DONE so my code (and
>>> maybe the usb-msd code too, I haven't verified) relies on that to
>>> complete requests... Is that incorrect ?
>>
>> Yes.
> 
> Well, so far it works :-) But I suppose I must be lucky.. I must admit
> that it's very unclear how that SCSI "stack" is meant to be used from
> the HBA standpoint.
> 
> Right now, I've somewhat come up with:
> 
>   - client request occurs
>   - call device send_command()
>   - if result is 0, assume my complete() was called with 
>     SCSI_REASON_DONE
>   - else, use sign of result for transfer direction, store the
>     absolute value as the total expected transfer len and call
>     the device scsi_data_read()/write() and wait for complete()
>   - when complete() is called:
>     - if SCSI_REASON_DONE, complete client request
>     - else perform the client "DMA" for "arg" bytes
>     - call the device scsi_data_read()/write() again
> 
No, this is exactly as I'm expecting the SCSI layer to work.
So from the light of this the patch to scsi-generic is valid.
And it really looks like papering over a bug in the lsi HBA code.

Gerd?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Benjamin Herrenschmidt]

On Thu, 2010-11-25 at 10:06 +0100, Hannes Reinecke wrote:
> No, this is exactly as I'm expecting the SCSI layer to work.
> So from the light of this the patch to scsi-generic is valid.
> And it really looks like papering over a bug in the lsi HBA code.

Ok. I have no special case tho for a complete() coming for data with 0
in arg. I will just skip the DMA part and call read/write again until I
get SCSI_REASON_DONE.

Cheers,
Ben.

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Gerd Hoffmann]

>> Right now, I've somewhat come up with:
>>
>>    - client request occurs
>>    - call device send_command()
>>    - if result is 0, assume my complete() was called with
>>      SCSI_REASON_DONE
>>    - else, use sign of result for transfer direction, store the
>>      absolute value as the total expected transfer len and call
>>      the device scsi_data_read()/write() and wait for complete()
>>    - when complete() is called:
>>      - if SCSI_REASON_DONE, complete client request
>>      - else perform the client "DMA" for "arg" bytes
>>      - call the device scsi_data_read()/write() again
>>
> No, this is exactly as I'm expecting the SCSI layer to work.

Yes.

> So from the light of this the patch to scsi-generic is valid.

Yes.

> And it really looks like papering over a bug in the lsi HBA code.

Indeed.  If the patch breaks lsi I'd agree that it is lsi's fault. 
Anyone tested whenever current upstream lsi actually breaks with the 
patch applied?

I've attempted to convert lsi to use iovecs a while ago, and *that* 
version definitively had problems with short transfers, so maybe this is 
a leftover from those days where scsi hacking was based on that branch ...

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Benjamin Herrenschmidt]

> > The commit message of this commit says it's a workaround for a problem
> > with lsi:
> > 
> >> - when a read is aborted due to a mark/EOF/EOD/EOM, the len reported to
> >> controller can be 0. LSI controller emulation doesn't know how to manage
> >> this. A workaround found is to call the completion routine with
> >> SCSI_REASON_DONE just after calling it with SCSI_REASON_DATA with len=0.
> > 
> > Are you sure that it's not needed any more?
> > 
> Don't ask me. I didn't do the patch, and my knowledge of lsi HBA
> internals is scanty.
> Nic, can you comment here?

Back to this topic...

On my current WIP code, currently based on qemu upstream commit
f711df67d611e4762966a249742a5f7499e19f99, I've just observed the
following behaviour:

Use -cdrom /dev/cdrom (which points to /dev/sr0)

No disk in the drive, scsi-disk kicks in. test-unit-ready properly says
there's no medium, so far so good.

Now, my (buggy) guest code tries to do a READ_10. At this point, qemu
hangs. What happens is that my scsi complete callback get called with
reason 1 (DATA) and arg 0. I then perform no data copy, and call back
sdev->info->read_data() which eventually calls me back again with reason
1 and arg 0, etc... in a loop.

I haven't had a chance yet to look at what's happening in the guts of
scsi-disk, but here, either I'm supposed to special case DATA with 0
bytes requested in my HBA driver, or there's a bug in scsi-disk.

I can work around it by doing a cancel_io in that case and then
completing the request is if reason had been 0 (DONE) but that's of
course just a band-aid.

I'll let you know what I find out when I get a chance to look at this
again (hopefully soon).

Cheers,
Ben.

[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Nicholas A. Bellinger]

On Wed, 2010-11-24 at 11:16 +0100, Hannes Reinecke wrote:
> On 11/24/2010 10:04 AM, Kevin Wolf wrote:
> > Am 24.11.2010 09:57, schrieb Hannes Reinecke:
> >> On 11/24/2010 09:18 AM, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger <nab@linux-iscsi.org>
> >>>
> >>> This patch adds a handful of bugfixes for scsi-generic
> >>> that where added into:
> >>>
> >>> commit a4194b3f79a85e111f000788ddec05d465748851
> >>> Author: Hannes Reinecke <hare@suse.de>
> >>> Date:   Mon Nov 22 15:39:33 2010 -0800
> >>>
> >>>     scsi: Use 'SCSIRequest' directly
> >>>
> >>> this includes:
> >>>
> >>> *) Fix incorrect errno usage in switch() statement within
> >>>    scsi_command_complete()
> >>>
> >>> *) Remove bogus scsi_command_complete() for residual case
> >>>    within scsi_read_complete()
> >>>
> >>> *) Remove incorrect '-' sign from return in scsi_send_command()
> >>>
> >>> Tested with .37-rc2 TCM_Loop FILEIO backstores on KVM host into
> >>> Debian Lenny v2.6.26 KVM guest with an xfs filesystem mount.
> >>>
> >>> Signed-off-by: Nicholas A. Bellinger nab@linux-iscsi.org>
> > 
> > Hannes, can you fold the necessary parts of this into the next version
> > of your series, so that we don't break things first and fix them only later?
> > 
> I'll be folding the first one into my series.
> 
> The second one is actually a genuine error, and a rather old one to
> boot. It went in with this commit:
> 
> commit 89c0f6438d16ebceccdcd096bbc0b5536146a443
> Author: aurel32 <aurel32@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Fri Oct 17 08:08:56 2008 +0000
> Subject: scsi-generic: correct error management
> 
> So I'd rather have it submitted separately.
> And a second opinion is _definitely_ required here.
> nab, can you do the honours?
> 

Yep, so it appears that commit 89c0f6438d16 did introduce the bogus
'double complete' in scsi_read_complete, which I think was intended to
handle residual counts for TYPE_TYPE...

 /* Cancel a pending data transfer.  */
@@ -251,6 +257,8 @@ static void scsi_read_complete(void * opaque, int ret)

     r->len = -1;
     s->completion(s->opaque, SCSI_REASON_DATA, r->tag, len);
+    if (len == 0)
+        scsi_command_complete(r, 0);
 }

I am currently under the assumption for this and bsg_read_complete that
s->completion(..., len) is handling the residual count back to block.

Is this correct..?

--nab

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Benjamin Herrenschmidt]

> Yep, so it appears that commit 89c0f6438d16 did introduce the bogus
> 'double complete' in scsi_read_complete, which I think was intended to
> handle residual counts for TYPE_TYPE...
> 
>  /* Cancel a pending data transfer.  */
> @@ -251,6 +257,8 @@ static void scsi_read_complete(void * opaque, int ret)
> 
>      r->len = -1;
>      s->completion(s->opaque, SCSI_REASON_DATA, r->tag, len);
> +    if (len == 0)
> +        scsi_command_complete(r, 0);
>  }
> 
> I am currently under the assumption for this and bsg_read_complete that
> s->completion(..., len) is handling the residual count back to block.
> 
> Is this correct..?

So I just debugged a crash where loading my vscsi driver kills qemu
(segfault) after trying to complete a command twice with scsi-generic.

Removing the above hunk fixes it. So this is a genuine fix that should
be applied (asap even :-)

I still have an odd problem with scsi-disk.c where reading from an
empty cdrom drive crashes it, I'll debug that later.

Cheers,
Ben.

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Nicholas A. Bellinger]

On Tue, 2010-12-21 at 12:49 +1100, Benjamin Herrenschmidt wrote:
> > Yep, so it appears that commit 89c0f6438d16 did introduce the bogus
> > 'double complete' in scsi_read_complete, which I think was intended to
> > handle residual counts for TYPE_TYPE...
> > 
> >  /* Cancel a pending data transfer.  */
> > @@ -251,6 +257,8 @@ static void scsi_read_complete(void * opaque, int ret)
> > 
> >      r->len = -1;
> >      s->completion(s->opaque, SCSI_REASON_DATA, r->tag, len);
> > +    if (len == 0)
> > +        scsi_command_complete(r, 0);
> >  }
> > 
> > I am currently under the assumption for this and bsg_read_complete that
> > s->completion(..., len) is handling the residual count back to block.
> > 
> > Is this correct..?
> 
> So I just debugged a crash where loading my vscsi driver kills qemu
> (segfault) after trying to complete a command twice with scsi-generic.
> 
> Removing the above hunk fixes it. So this is a genuine fix that should
> be applied (asap even :-)
> 

Hi Ben,

Thanks for verifying this one.  Kevin, please make sure this original
patch to drop the bogus double complete gets picked up.

Best Regards,

--nab

> I still have an odd problem with scsi-disk.c where reading from an
> empty cdrom drive crashes it, I'll debug that later.
> 
> Cheers,
> Ben.
> 
> 

Re: [Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Kevin Wolf]

Am 23.12.2010 22:58, schrieb Nicholas A. Bellinger:
> On Tue, 2010-12-21 at 12:49 +1100, Benjamin Herrenschmidt wrote:
>>> Yep, so it appears that commit 89c0f6438d16 did introduce the bogus
>>> 'double complete' in scsi_read_complete, which I think was intended to
>>> handle residual counts for TYPE_TYPE...
>>>
>>>  /* Cancel a pending data transfer.  */
>>> @@ -251,6 +257,8 @@ static void scsi_read_complete(void * opaque, int ret)
>>>
>>>      r->len = -1;
>>>      s->completion(s->opaque, SCSI_REASON_DATA, r->tag, len);
>>> +    if (len == 0)
>>> +        scsi_command_complete(r, 0);
>>>  }
>>>
>>> I am currently under the assumption for this and bsg_read_complete that
>>> s->completion(..., len) is handling the residual count back to block.
>>>
>>> Is this correct..?
>>
>> So I just debugged a crash where loading my vscsi driver kills qemu
>> (segfault) after trying to complete a command twice with scsi-generic.
>>
>> Removing the above hunk fixes it. So this is a genuine fix that should
>> be applied (asap even :-)
>>
> 
> Hi Ben,
> 
> Thanks for verifying this one.  Kevin, please make sure this original
> patch to drop the bogus double complete gets picked up.

Wasn't the original patch NACKed by Hannes in parts? Can you re-post a
patch that includes only this specific fix?

Kevin