Re: [Qemu-devel] [PATCH v2 0/7] dma-helpers, scsi-block: use SG_IO for all I/O on scsi-block

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

On 23/05/16 13:54, Paolo Bonzini wrote:

> scsi-block uses the block layer for reads and writes in order to avoid
> allocating bounce buffers as big as the transferred data.  We know how
> to split a large transfer to multiple reads and writes, and thus we can
> use scsi-disk.c's existing code to do I/O in multiple chunks (for non-s/g
> SCSI hosts) or through the DMA helpers (for s/g SCSI hosts).
> 
> Unfortunately, this has the side effect of eating the SCSI status except
> in the very few cases where we can convert an errno code back to a SCSI
> status.  It puts a big wrench in persistent reservations support in the
> guest, for example.
> 
> Luckily, splitting a large transfer into multiple SBC commands is just as
> easy, and this is what the last patch does.  It takes the original CDB,
> patches in a modified starting sector and sector count, and executes the
> SCSI command through blk_aio_ioctl.  It is also easy to pass a QEMUIOVector
> to SG_IO, so that s/g SCSI hosts keep the performance.
> 
> This rebases the patches on top of Eric's changes for byte-based
> BlockBackend access and fixes a few bugs I knew about in the RFC.
> 
> Patches 1, 5 and 6 are new.
> 
> Paolo
> 
> Paolo Bonzini (7):
>   dma-helpers: change interface to byte-based
>   dma-helpers: change BlockBackend to opaque value in DMAIOFunc
>   scsi-disk: introduce a common base class
>   scsi-disk: introduce dma_readv and dma_writev
>   scsi-disk: add need_fua_emulation to SCSIDiskClass
>   scsi-disk: introduce scsi_disk_req_check_error
>   scsi-block: always use SG_IO
> 
>  dma-helpers.c        |  54 +++++--
>  hw/block/nvme.c      |   6 +-
>  hw/ide/ahci.c        |   6 +-
>  hw/ide/core.c        |  20 ++-
>  hw/ide/internal.h    |   6 +-
>  hw/ide/macio.c       |   2 +-
>  hw/scsi/scsi-disk.c  | 409 +++++++++++++++++++++++++++++++++++++--------------
>  include/sysemu/dma.h |  20 +--
>  trace-events         |   2 +-
>  9 files changed, 371 insertions(+), 154 deletions(-)

Hi Paolo,

I thought I'd give this patchset a spin with a view to seeing whether I
could switch the macio device back to the now byte-based dma-helpers,
but came up with a couple of compile errors. Attached is the minor diff
I applied in order to get a successful compile (apologies for not being
inline, however I couldn't get my mail client to stop wrapping incorrectly).


ATB,

Mark.


[-- Attachment #2: dma-helpers-compile-fix.patch --]
[-- Type: text/x-diff, Size: 1050 bytes --]

diff --git a/dma-helpers.c b/dma-helpers.c
index 92c5d55..b521d84 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -192,7 +192,7 @@ static const AIOCBInfo dma_aiocb_info = {
 };
 
 BlockAIOCB *dma_blk_io(AioContext *ctx,
-    QEMUSGList *sg, uint64_t opaque,
+    QEMUSGList *sg, uint64_t offset,
     DMAIOFunc *io_func, void *io_func_opaque,
     BlockCompletionFunc *cb,
     void *opaque, DMADirection dir)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 02faa3a..f7a23fc 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -540,7 +540,6 @@ static void scsi_write_data(SCSIRequest *req)
     if (r->req.sg) {
         dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_WRITE);
         r->req.resid -= r->req.sg->size;
-                                     scsi_dma_complete, r);
         r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk),
                                   r->req.sg, r->sector << BDRV_SECTOR_BITS,
                                   sdc->dma_writev, r, scsi_dma_complete, r,