Patchwork [12/16] scsi-generic: use plain ioctl

login
register
mail settings
Submitter Hannes Reinecke
Date Nov. 18, 2010, 2:47 p.m.
Message ID <20101118144736.C183DF90AB@ochil.suse.de>
Download mbox | patch
Permalink /patch/72115/
State New
Headers show

Comments

Hannes Reinecke - Nov. 18, 2010, 2:47 p.m.
aio_ioctl is emulated anyway and currently broken.
So better use 'normal' ioctl here as there are no benefits
on using the emulated async I/O call.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/scsi-generic.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)
Christoph Hellwig - Nov. 19, 2010, 6:39 p.m.
On Thu, Nov 18, 2010 at 03:47:36PM +0100, Hannes Reinecke wrote:
> 
> aio_ioctl is emulated anyway and currently broken.

What's broken about it currently?

> So better use 'normal' ioctl here as there are no benefits
> on using the emulated async I/O call.

There are huge benefits.  Without it the whole scsi command execution
happens synchronously in the qemu main loop, blocking guest execution.
Nicholas A. Bellinger - Nov. 20, 2010, 12:41 a.m.
On Fri, 2010-11-19 at 19:39 +0100, Christoph Hellwig wrote:
> On Thu, Nov 18, 2010 at 03:47:36PM +0100, Hannes Reinecke wrote:
> > 
> > aio_ioctl is emulated anyway and currently broken.
> 
> What's broken about it currently?

Mmmmmm, I do not recall this being broken in the first place..?  There
was a single issue with megasas+bdrv_aio_ioctl() with WinXP (that did
not appear with lsi53c895a) that was mentioned on the list earlier in
the year that required a patch to use bdev_ioctl(), but last I recall
Hannes had already fixed this in recent megasas.c code w/ 32-bit MSFT
guests.  Also, this is what I have been with scsi_generic.c and
scsi_bsg.c into TCM_loop in my v0.12.5 megasas tree, and I am not
observing any obvious issues with AIO IOCTLs for SG_IO/BSG into Linux
guests.

I will give AIO IOCTL ops a run with these on v2.6.37-rc2 lock-less KVM
host mode <-> TCM_Loop to verify against the v0.12.5 megasas tree.

> 
> > So better use 'normal' ioctl here as there are no benefits
> > on using the emulated async I/O call.
> 
> There are huge benefits.  Without it the whole scsi command execution
> happens synchronously in the qemu main loop, blocking guest execution.
> 

Indeed.  Using bdrv_ioctl() in execute_command() will very effectively
disable TCQ > 1 into the backend struct scsi_device.

Hannes, did you run into another scenario why you needed to do this for
megasas..? 

Other than this one piece the rest of the series looks very good.  Thank
you for putting these pieces together.

--nab
adq - Nov. 20, 2010, 1:25 a.m.
On 20 November 2010 00:41, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> On Fri, 2010-11-19 at 19:39 +0100, Christoph Hellwig wrote:
>> On Thu, Nov 18, 2010 at 03:47:36PM +0100, Hannes Reinecke wrote:
>> >
>> > aio_ioctl is emulated anyway and currently broken.
>>
>> What's broken about it currently?
>
> Mmmmmm, I do not recall this being broken in the first place..?  There
> was a single issue with megasas+bdrv_aio_ioctl() with WinXP (that did
> not appear with lsi53c895a) that was mentioned on the list earlier in
> the year that required a patch to use bdev_ioctl(), but last I recall
> Hannes had already fixed this in recent megasas.c code w/ 32-bit MSFT
> guests.  Also, this is what I have been with scsi_generic.c and
> scsi_bsg.c into TCM_loop in my v0.12.5 megasas tree, and I am not
> observing any obvious issues with AIO IOCTLs for SG_IO/BSG into Linux
> guests.
>
> I will give AIO IOCTL ops a run with these on v2.6.37-rc2 lock-less KVM
> host mode <-> TCM_Loop to verify against the v0.12.5 megasas tree.

Could this AIO ioctl breakage perhaps be the one I fixed here?
http://web.archiveorange.com/archive/v/1XS1vROmfC7dN9wYxsmt

The patch is defintely in the latest git... it works fine for me with
my scsi-generic MMC command patches.
Nicholas A. Bellinger - Nov. 20, 2010, 8:23 a.m.
On Sat, 2010-11-20 at 01:25 +0000, adq wrote:
> On 20 November 2010 00:41, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> > On Fri, 2010-11-19 at 19:39 +0100, Christoph Hellwig wrote:
> >> On Thu, Nov 18, 2010 at 03:47:36PM +0100, Hannes Reinecke wrote:
> >> >
> >> > aio_ioctl is emulated anyway and currently broken.
> >>
> >> What's broken about it currently?
> >
> > Mmmmmm, I do not recall this being broken in the first place..?  There
> > was a single issue with megasas+bdrv_aio_ioctl() with WinXP (that did
> > not appear with lsi53c895a) that was mentioned on the list earlier in
> > the year that required a patch to use bdev_ioctl(), but last I recall
> > Hannes had already fixed this in recent megasas.c code w/ 32-bit MSFT
> > guests.  Also, this is what I have been with scsi_generic.c and
> > scsi_bsg.c into TCM_loop in my v0.12.5 megasas tree, and I am not
> > observing any obvious issues with AIO IOCTLs for SG_IO/BSG into Linux
> > guests.
> >
> > I will give AIO IOCTL ops a run with these on v2.6.37-rc2 lock-less KVM
> > host mode <-> TCM_Loop to verify against the v0.12.5 megasas tree.
> 
> Could this AIO ioctl breakage perhaps be the one I fixed here?
> http://web.archiveorange.com/archive/v/1XS1vROmfC7dN9wYxsmt
> 
> The patch is defintely in the latest git... it works fine for me with
> my scsi-generic MMC command patches.
> 

Interesting read, and thanks for the heads up on this bit..   I do not
personally recall running into any issues with TYPE_DISK w/ lsi53c895a
and AIO SG_IO into WinXP guests on v0.12.5 code.   After a quick double
check in the v0.12.5 megasas tree, the proper get_async_context_id() is
still present:

http://git.kernel.org/?p=virt/kvm/nab/qemu-kvm.git;a=blob;f=posix-aio-compat.c;h=ccdbf9a16d0ef1d7e57c87dbe43f318d4c7a5967;hb=HEAD#l560

So it appears this acb->async_context_id was incorrectly dropped during
v0.13 development, and with your fix commited into v0.13 mainline code
that Hannes should be able to safetly drop this the megasas series,
yes..?

Thank you for your comments!

--nab
adq - Nov. 20, 2010, 1:16 p.m.
On 20 November 2010 08:23, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> On Sat, 2010-11-20 at 01:25 +0000, adq wrote:
>> On 20 November 2010 00:41, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
>> > On Fri, 2010-11-19 at 19:39 +0100, Christoph Hellwig wrote:
>> >> On Thu, Nov 18, 2010 at 03:47:36PM +0100, Hannes Reinecke wrote:
>> >> >
>> >> > aio_ioctl is emulated anyway and currently broken.
>> >>
>> >> What's broken about it currently?
>> >
>> > Mmmmmm, I do not recall this being broken in the first place..?  There
>> > was a single issue with megasas+bdrv_aio_ioctl() with WinXP (that did
>> > not appear with lsi53c895a) that was mentioned on the list earlier in
>> > the year that required a patch to use bdev_ioctl(), but last I recall
>> > Hannes had already fixed this in recent megasas.c code w/ 32-bit MSFT
>> > guests.  Also, this is what I have been with scsi_generic.c and
>> > scsi_bsg.c into TCM_loop in my v0.12.5 megasas tree, and I am not
>> > observing any obvious issues with AIO IOCTLs for SG_IO/BSG into Linux
>> > guests.
>> >
>> > I will give AIO IOCTL ops a run with these on v2.6.37-rc2 lock-less KVM
>> > host mode <-> TCM_Loop to verify against the v0.12.5 megasas tree.
>>
>> Could this AIO ioctl breakage perhaps be the one I fixed here?
>> http://web.archiveorange.com/archive/v/1XS1vROmfC7dN9wYxsmt
>>
>> The patch is defintely in the latest git... it works fine for me with
>> my scsi-generic MMC command patches.
>>
>
> Interesting read, and thanks for the heads up on this bit..   I do not
> personally recall running into any issues with TYPE_DISK w/ lsi53c895a
> and AIO SG_IO into WinXP guests on v0.12.5 code.   After a quick double
> check in the v0.12.5 megasas tree, the proper get_async_context_id() is
> still present:
>
> http://git.kernel.org/?p=virt/kvm/nab/qemu-kvm.git;a=blob;f=posix-aio-compat.c;h=ccdbf9a16d0ef1d7e57c87dbe43f318d4c7a5967;hb=HEAD#l560
>
> So it appears this acb->async_context_id was incorrectly dropped during
> v0.13 development, and with your fix commited into v0.13 mainline code
> that Hannes should be able to safetly drop this the megasas series,
> yes..?
>
> Thank you for your comments!

No problem. Oh, that bug was /definitely/ in the released 0.12.x
series code; I spotted it when trying to get scsi-generic to work on
arch linux, which was using 0.12.x.
Hannes Reinecke - Nov. 22, 2010, 7:21 a.m.
On 11/20/2010 02:25 AM, adq wrote:
> On 20 November 2010 00:41, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
>> On Fri, 2010-11-19 at 19:39 +0100, Christoph Hellwig wrote:
>>> On Thu, Nov 18, 2010 at 03:47:36PM +0100, Hannes Reinecke wrote:
>>>>
>>>> aio_ioctl is emulated anyway and currently broken.
>>>
>>> What's broken about it currently?
>>
>> Mmmmmm, I do not recall this being broken in the first place..?  There
>> was a single issue with megasas+bdrv_aio_ioctl() with WinXP (that did
>> not appear with lsi53c895a) that was mentioned on the list earlier in
>> the year that required a patch to use bdev_ioctl(), but last I recall
>> Hannes had already fixed this in recent megasas.c code w/ 32-bit MSFT
>> guests.  Also, this is what I have been with scsi_generic.c and
>> scsi_bsg.c into TCM_loop in my v0.12.5 megasas tree, and I am not
>> observing any obvious issues with AIO IOCTLs for SG_IO/BSG into Linux
>> guests.
>>
>> I will give AIO IOCTL ops a run with these on v2.6.37-rc2 lock-less KVM
>> host mode <-> TCM_Loop to verify against the v0.12.5 megasas tree.
> 
> Could this AIO ioctl breakage perhaps be the one I fixed here?
> http://web.archiveorange.com/archive/v/1XS1vROmfC7dN9wYxsmt
> 
> The patch is defintely in the latest git... it works fine for me with
> my scsi-generic MMC command patches.

Ah. Yes, this looks like it. I'll give it a spin; I've made the
original patch against an older git rev so I might've missed that one.

Cheers,

Hannes

Patch

diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index de37d78..6250ce5 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -147,12 +147,8 @@  static void scsi_command_complete(void *opaque, int ret)
 /* Cancel a pending data transfer.  */
 static void scsi_cancel_io(SCSIRequest *req)
 {
-    SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
-
+    /* Nothing to do; cannot abort ioctls :-( */
     DPRINTF("Cancel tag=0x%x\n", req->tag);
-    if (r->req.aiocb)
-        bdrv_aio_cancel(r->req.aiocb);
-    r->req.aiocb = NULL;
 }
 
 static int execute_command(BlockDriverState *bdrv,
@@ -160,6 +156,7 @@  static int execute_command(BlockDriverState *bdrv,
 			   BlockDriverCompletionFunc *complete)
 {
     SCSIGenericState *s = DO_UPCAST(SCSIGenericState, qdev, r->req.dev);
+    int ret;
 
     r->io_header.interface_id = 'S';
     r->io_header.dxfer_direction = direction;
@@ -173,13 +170,12 @@  static int execute_command(BlockDriverState *bdrv,
     r->io_header.usr_ptr = r;
     r->io_header.flags |= SG_FLAG_DIRECT_IO;
 
-    r->req.aiocb = bdrv_aio_ioctl(bdrv, SG_IO, &r->io_header, complete, r);
-    if (r->req.aiocb == NULL) {
-        BADF("execute_command: read failed !\n");
-        return -1;
-    }
+    DPRINTF("SG_IO tag=0x%x dxfer=%d iov=%d\n", r->req.tag,
+            r->io_header.dxfer_len, r->io_header.iovec_count);
 
-    return 0;
+    ret = bdrv_ioctl(bdrv, SG_IO, &r->io_header);
+    complete(r, ret);
+    return ret;
 }
 
 static void scsi_read_complete(void * opaque, int ret)
@@ -448,9 +444,6 @@  static void scsi_generic_purge_requests(SCSIGenericState *s)
 
     while (!QTAILQ_EMPTY(&s->qdev.requests)) {
         r = DO_UPCAST(SCSIGenericReq, req, QTAILQ_FIRST(&s->qdev.requests));
-        if (r->req.aiocb) {
-            bdrv_aio_cancel(r->req.aiocb);
-        }
         scsi_remove_request(&r->req);
     }
 }