| Message ID | 20101118144736.C183DF90AB@ochil.suse.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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.
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
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); } }
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(-)