[Qemu-devel] [PATCH 0/4] ISCSI: Passthough updates

[Qemu-devel] [PATCH 0/4] ISCSI: Passthough updates

[Ronnie Sahlberg]

Paolo, list

Please find a set of patches for iscsi.

1, The recent change to the eventsystem in iscsi is racy. If the async connect of the socket takes longer than almost instant, the shortcircuit trying to write directly to the socket may try to do so before the socket is established.
So we hcange the logic here and ONLY try the shortcircuit avoiding setting up POLLOUT if we have been fully logged in.

This avoids the race where we might crash if the socket is not yet connected.

2, Swithc to using READCAPACITY10  for MMC devices. Only SBC devices support READCAPACITY16 so far.

3, Switch to use READ10 for MMC devices instead of READ16. With this patch ISCSI and MMC devices now work correctly.

4, If the device is a SMC device, then force the device to be sg.
We dont support emulation as far as i know for SMV devices, so these device types would only work for sg or iscsi passthrough anyway.
The reason we need to forcer sg is that since we can not READ from a SMC device, we need to shortcircuit  find_image_format()  before it tries to do the bdrv_pread() trying to guess what format the image is.

With this patch I have verified I can access and manage a media changer via iscsi-passthrough.


regards
ronnie sahblerg

[Qemu-devel] [PATCH 1/4] ISCSI: Only call READCAPACITY16 for SBC devices

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index df0b6c8..39d75cb 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -727,13 +727,20 @@ iscsi_inquiry_cb(struct iscsi_context *iscsi, int status, void *command_data,
 
     scsi_free_scsi_task(task);
 
-    task = iscsi_readcapacity16_task(iscsi, itask->iscsilun->lun,
+    switch (itask->iscsilun->type) {
+    case TYPE_DISK:
+        task = iscsi_readcapacity16_task(iscsi, itask->iscsilun->lun,
                                    iscsi_readcapacity16_cb, opaque);
-    if (task == NULL) {
-        error_report("iSCSI: failed to send readcapacity16 command.");
-        itask->status   = 1;
+        if (task == NULL) {
+            error_report("iSCSI: failed to send readcapacity16 command.");
+            itask->status   = 1;
+            itask->complete = 1;
+            return;
+        }
+        break;
+    default:
+        itask->status   = 0;
         itask->complete = 1;
-        return;
     }
 }
 
-- 
1.7.3.1

[Qemu-devel] [PATCH 2/4] ISCSI: Use READCAPACITY10 for MMC devices

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 39d75cb..2ddb9e5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -700,6 +700,42 @@ iscsi_readcapacity16_cb(struct iscsi_context *iscsi, int status,
 }
 
 static void
+iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *itask = opaque;
+    struct scsi_readcapacity10 *rc10;
+    struct scsi_task *task = command_data;
+
+    if (status != 0) {
+        error_report("iSCSI: Failed to read capacity of iSCSI lun. %s",
+                     iscsi_get_error(iscsi));
+        itask->status   = 1;
+        itask->complete = 1;
+        scsi_free_scsi_task(task);
+        return;
+    }
+
+    rc10 = scsi_datain_unmarshall(task);
+    if (rc10 == NULL) {
+        error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
+        itask->status   = 1;
+        itask->complete = 1;
+        scsi_free_scsi_task(task);
+        return;
+    }
+
+    itask->iscsilun->block_size = rc10->block_size;
+    itask->iscsilun->num_blocks = rc10->lba + 1;
+    itask->bs->total_sectors    = itask->iscsilun->num_blocks *
+                               itask->iscsilun->block_size / BDRV_SECTOR_SIZE ;
+
+    itask->status   = 0;
+    itask->complete = 1;
+    scsi_free_scsi_task(task);
+}
+
+static void
 iscsi_inquiry_cb(struct iscsi_context *iscsi, int status, void *command_data,
                  void *opaque)
 {
@@ -738,6 +774,17 @@ iscsi_inquiry_cb(struct iscsi_context *iscsi, int status, void *command_data,
             return;
         }
         break;
+    case TYPE_ROM:
+        task = iscsi_readcapacity10_task(iscsi, itask->iscsilun->lun,
+                                   0, 0,
+                                   iscsi_readcapacity10_cb, opaque);
+        if (task == NULL) {
+            error_report("iSCSI: failed to send readcapacity16 command.");
+            itask->status   = 1;
+            itask->complete = 1;
+            return;
+        }
+        break;
     default:
         itask->status   = 0;
         itask->complete = 1;
-- 
1.7.3.1

[Qemu-devel] [PATCH 3/4] ISCSI: Only use READ16 for SBC devices. Use READ10 for other device types such as MMC

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 2ddb9e5..a015a52 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -379,14 +379,25 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
     memset(acb->task, 0, sizeof(struct scsi_task));
 
     acb->task->xfer_dir = SCSI_XFER_READ;
-    acb->task->cdb_size = 16;
-    acb->task->cdb[0]  = 0x88;
     lba = sector_qemu2lun(sector_num, iscsilun);
-    *(uint32_t *)&acb->task->cdb[2]  = htonl(lba >> 32);
-    *(uint32_t *)&acb->task->cdb[6]  = htonl(lba & 0xffffffff);
-    *(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors);
     acb->task->expxferlen = qemu_read_size;
 
+    switch (iscsilun->type) {
+    case TYPE_DISK:
+        acb->task->cdb_size = 16;
+        acb->task->cdb[0]  = 0x88;
+        *(uint32_t *)&acb->task->cdb[2]  = htonl(lba >> 32);
+        *(uint32_t *)&acb->task->cdb[6]  = htonl(lba & 0xffffffff);
+        *(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors);
+        break;
+    default:
+        acb->task->cdb_size = 10;
+        acb->task->cdb[0]  = 0x28;
+        *(uint32_t *)&acb->task->cdb[2] = htonl(lba);
+        *(uint16_t *)&acb->task->cdb[7] = htons(num_sectors);
+        break;
+    }
+    
     if (iscsi_scsi_command_async(iscsi, iscsilun->lun, acb->task,
                                  iscsi_aio_read16_cb,
                                  NULL,
-- 
1.7.3.1

[Qemu-devel] [PATCH 4/4] ISCSI: If the device we open is a SMC device, then force the use of sg. We dont have any medium changer emulation so only passthrough via real sg or scsi-generic via iscsi would work anyway.

[Ronnie Sahlberg]

Forcing sg also makes qemu skip trying to read from the device to guess the image format by reading from the device (find_image_format()).
SMC devices do not implement READ6/10/12/16  so it is noit possible to read from them.

With this patch I can successfully manage a SMC device wiht iscsi in passthrough mode.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index a015a52..9ce38b5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1032,6 +1032,15 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
     if (iscsi_url != NULL) {
         iscsi_destroy_url(iscsi_url);
     }
+
+    /* Medium changer. We dont have any emulation for this so this must
+       be sg ioctl compatible. We force it to be sg, otherwise qemu will try
+       to read from the device to guess the image format.
+     */
+    if (iscsilun->type == TYPE_MEDIUM_CHANGER) {
+            bs->sg = 1;
+    }
+
     return 0;
 
 failed:
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH 4/4] ISCSI: If the device we open is a SMC device, then force the use of sg. We dont have any medium changer emulation so only passthrough via real sg or scsi-generic via iscsi would work anyway.

[Paolo Bonzini]

Il 26/05/2012 06:56, Ronnie Sahlberg ha scritto:
> Forcing sg also makes qemu skip trying to read from the device to guess the image format by reading from the device (find_image_format()).
> SMC devices do not implement READ6/10/12/16  so it is noit possible to read from them.
> 
> With this patch I can successfully manage a SMC device wiht iscsi in passthrough mode.
> 
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  block/iscsi.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index a015a52..9ce38b5 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1032,6 +1032,15 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>      if (iscsi_url != NULL) {
>          iscsi_destroy_url(iscsi_url);
>      }
> +
> +    /* Medium changer. We dont have any emulation for this so this must
> +       be sg ioctl compatible. We force it to be sg, otherwise qemu will try
> +       to read from the device to guess the image format.
> +     */
> +    if (iscsilun->type == TYPE_MEDIUM_CHANGER) {
> +            bs->sg = 1;
> +    }
> +
>      return 0;
>  
>  failed:

Modified to also do the same for tapes, applied to scsi-next branch for 1.2.

Paolo

Re: [Qemu-devel] [PATCH 4/4] ISCSI: If the device we open is a SMC device, then force the use of sg. We dont have any medium changer emulation so only passthrough via real sg or scsi-generic via iscsi would work anyway.

[Andreas Färber]

Am 26.05.2012 09:36, schrieb Paolo Bonzini:
> Il 26/05/2012 06:56, Ronnie Sahlberg ha scritto:
>> Forcing sg also makes qemu skip trying to read from the device to guess the image format by reading from the device (find_image_format()).
>> SMC devices do not implement READ6/10/12/16  so it is noit possible to read from them.
>>
>> With this patch I can successfully manage a SMC device wiht iscsi in passthrough mode.
>>
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
[...]
> Modified to also do the same for tapes, applied to scsi-next branch for 1.2.

Paolo, it seems you haven't pushed scsi-next since then. I hope you've
also shortened the subject to a humanly bearable length?

Ronnie, please restrict commit message lines to at most 76 characters.
You can check by running `git log` in an 80-char-wide terminal. The
subject line is especially sensitive since this email subject is simply
unreadable. And Patchwork currently looks really ugly in WebKit due to
this patch subject.

https://live.gnome.org/Git/CommitMessages

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/4] ISCSI: If the device we open is a SMC device, then force the use of sg. We dont have any medium changer emulation so only passthrough via real sg or scsi-generic via iscsi would work anyway.

[Paolo Bonzini]

Il 27/05/2012 15:12, Andreas Färber ha scritto:
>> > Modified to also do the same for tapes, applied to scsi-next branch for 1.2.
> Paolo, it seems you haven't pushed scsi-next since then.

Yeah, I have a pending push request for scsi-next, so I'm waiting till
Anthony applies it before pushing 1.2-only patches (I wasn't expecting
parallel 1.1/1.2 development for SCSI).

> I hope you've
> also shortened the subject to a humanly bearable length?

Yes. :)

Paolo

Re: [Qemu-devel] [PATCH 4/4] ISCSI: If the device we open is a SMC device, then force the use of sg. We dont have any medium changer emulation so only passthrough via real sg or scsi-generic via iscsi would work anyway.

[ronnie sahlberg]

Paolo


I think I have seen a problem inside libiscsi that could be triggered
by the shortcut.

Can you remove this shortcut completely :

-    /* Try to write as much as we can to the socket
-     * without setting up an event.
-     * Only do this if we are completely logged in, so we know that
-     * the socket is in connected state.
-     */
-    if (iscsi_is_logged_in(iscsi)) {
-        if (iscsi_which_events(iscsi) & POLLOUT) {
-            iscsi_process_write(iscsilun);
-        }
-    }

I think there is a problem inside libiscsi if the socket becomes full
and is no longer writeable and we try to write via this shortcurcuit.
It will take a while until I can verify or fix that issue and before a
new version of libiscsi can be available
so I would feel most comfortable with if we just remove this
optimization from QEMU for now.

It can be added back later once libiscsi is fixed.



regards
ronnie sahlberg


On Mon, May 28, 2012 at 4:48 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 27/05/2012 15:12, Andreas Färber ha scritto:
>>> > Modified to also do the same for tapes, applied to scsi-next branch for 1.2.
>> Paolo, it seems you haven't pushed scsi-next since then.
>
> Yeah, I have a pending push request for scsi-next, so I'm waiting till
> Anthony applies it before pushing 1.2-only patches (I wasn't expecting
> parallel 1.1/1.2 development for SCSI).
>
>> I hope you've
>> also shortened the subject to a humanly bearable length?
>
> Yes. :)
>
> Paolo

Re: [Qemu-devel] [PATCH 4/4] ISCSI: If the device we open is a SMC device, then force the use of sg. We dont have any medium changer emulation so only passthrough via real sg or scsi-generic via iscsi would work anyway.

[Paolo Bonzini]

Il 28/05/2012 13:55, ronnie sahlberg ha scritto:
> Paolo
> 
> 
> I think I have seen a problem inside libiscsi that could be triggered
> by the shortcut.
> 
> Can you remove this shortcut completely :
> 
> -    /* Try to write as much as we can to the socket
> -     * without setting up an event.
> -     * Only do this if we are completely logged in, so we know that
> -     * the socket is in connected state.
> -     */
> -    if (iscsi_is_logged_in(iscsi)) {
> -        if (iscsi_which_events(iscsi) & POLLOUT) {
> -            iscsi_process_write(iscsilun);
> -        }
> -    }
> 
> I think there is a problem inside libiscsi if the socket becomes full
> and is no longer writeable and we try to write via this shortcurcuit.
> It will take a while until I can verify or fix that issue and before a
> new version of libiscsi can be available
> so I would feel most comfortable with if we just remove this
> optimization from QEMU for now.
> 
> It can be added back later once libiscsi is fixed.

Done.  Rebased scsi-next, new commit is
f4dfa67f04037c1b1a8f4e4ddc944c5ab308f35b.

Paolo

Re: [Qemu-devel] [PATCH 0/4] ISCSI: Passthough updates

[Paolo Bonzini]

Il 26/05/2012 06:56, Ronnie Sahlberg ha scritto:
> Paolo, list
> 
> Please find a set of patches for iscsi.
> 
> 1, The recent change to the eventsystem in iscsi is racy. If the
> async connect of the socket takes longer than almost instant, the
> shortcircuit trying to write directly to the socket may try to do so
> before the socket is established. So we hcange the logic here and
> ONLY try the shortcircuit avoiding setting up POLLOUT if we have been
> fully logged in.
> 
> This avoids the race where we might crash if the socket is not yet
> connected.
> 
> 2, Swithc to using READCAPACITY10  for MMC devices. Only SBC devices
> support READCAPACITY16 so far.
> 
> 3, Switch to use READ10 for MMC devices instead of READ16. With this
> patch ISCSI and MMC devices now work correctly.
> 
> 4, If the device is a SMC device, then force the device to be sg. We
> dont support emulation as far as i know for SMV devices, so these
> device types would only work for sg or iscsi passthrough anyway. The
> reason we need to forcer sg is that since we can not READ from a SMC
> device, we need to shortcircuit  find_image_format()  before it tries
> to do the bdrv_pread() trying to guess what format the image is.
> 
> With this patch I have verified I can access and manage a media
> changer via iscsi-passthrough.

Applied 1-2 to scsi-next branch for 1.1, 3 for 1.2.

Paolo