[Qemu-devel] [PATCH 0/3] Remove ->bdrv_pread() internal block layer API

[Qemu-devel] [PATCH 0/3] Remove ->bdrv_pread() internal block layer API

[Avi Kivity]

The internal block layer API has a wart in the form of the
->bdrv_pread() and ->bdrv_pwrite() methods.  These duplicate
bdrv_read() and bdrv_write(), and are unnatural for a block
device in that they are not sector oriented.

One of the reasons for their existence is the scsi-generic host support.
That is also a wart, as scsi-generic is really a packet (command/response)
rather than a block (random access to sectors) API.  There is also the
wierd negative sector count == byte count thing.

This patchset addresses these warts by adding a new API for scsi-generic
and dropping ->bdrv_pread() and bdrv_pwrite().  While the improvement
falls short of spectacular, it does make further hacking on the block
layer easier as there are fewer callbacks to worry about.

Avi Kivity (3):
  Add specialized block driver scsi generic API
  Add internal scsi generic block API
  Drop internal bdrv_pread()/bdrv_pwrite() APIs

 block-raw-posix.c |   50 +++++++++++++++++++++++++--
 block-raw-win32.c |   20 +++++++----
 block.c           |   95 +++++++++++++++-------------------------------------
 block.h           |    8 ++++
 block_int.h       |   14 +++++--
 hw/scsi-generic.c |   39 ++++++++++++---------
 6 files changed, 126 insertions(+), 100 deletions(-)

[Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Avi Kivity]

When a scsi device is backed by a scsi generic device instead of an
ordinary host block device, the block API is abused in a couple of annoying
ways:

 - nb_sectors is negative, and specifies a byte count instead of a sector count
 - offset is ignored, since scsi-generic is essentially a packet protocol

This overloading makes hacking the block layer difficult.  Remove it by
introducing a new explicit API for scsi-generic devices.  The new API
is still backed by the old implementation, but at least the users are
insulated.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block.c           |   22 ++++++++++++++++++++++
 block.h           |    8 ++++++++
 hw/scsi-generic.c |   39 ++++++++++++++++++++++-----------------
 3 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 4f4bf7c..774ac2d 100644
--- a/block.c
+++ b/block.c
@@ -1594,3 +1594,25 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
         return drv->bdrv_ioctl(bs, req, buf);
     return -ENOTSUP;
 }
+
+int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count)
+{
+    return bdrv_pwrite(bs, -1, buf, count);
+}
+
+int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count)
+{
+    return bdrv_pread(bs, -1, buf, count);
+}
+
+BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
+                                   BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return bdrv_aio_read(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
+
+BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
+                                    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return bdrv_aio_write(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
diff --git a/block.h b/block.h
index e1927dd..f06d98a 100644
--- a/block.h
+++ b/block.h
@@ -103,6 +103,14 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
                                  BlockDriverCompletionFunc *cb, void *opaque);
 void bdrv_aio_cancel(BlockDriverAIOCB *acb);
 
+/* sg packet commands */
+int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count);
+int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count);
+BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
+                                   BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
+                                    BlockDriverCompletionFunc *cb, void *opaque);
+
 int qemu_key_check(BlockDriverState *bs, const char *name);
 
 /* Ensure contents are flushed to disk.  */
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 5bf160a..53e8951 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -201,6 +201,7 @@ static int execute_command(BlockDriverState *bdrv,
                            SCSIRequest *r, int direction,
 			   BlockDriverCompletionFunc *complete)
 {
+    int ret;
 
     r->io_header.interface_id = 'S';
     r->io_header.dxfer_direction = direction;
@@ -214,25 +215,27 @@ static int execute_command(BlockDriverState *bdrv,
     r->io_header.usr_ptr = r;
     r->io_header.flags |= SG_FLAG_DIRECT_IO;
 
-    if (bdrv_pwrite(bdrv, -1, &r->io_header, sizeof(r->io_header)) == -1) {
+    ret = bdrv_sg_send_command(bdrv, &r->io_header, sizeof(r->io_header));
+    if (ret < 0) {
         BADF("execute_command: write failed ! (%d)\n", errno);
         return -1;
     }
     if (complete == NULL) {
         int ret;
         r->aiocb = NULL;
-        while ((ret = bdrv_pread(bdrv, -1, &r->io_header,
-                                           sizeof(r->io_header))) == -1 &&
-                      errno == EINTR);
-        if (ret == -1) {
+        while ((ret = bdrv_sg_recv_response(bdrv, &r->io_header,
+                                            sizeof(r->io_header))) < 0 &&
+               ret == -EINTR)
+            ;
+        if (ret < 0) {
             BADF("execute_command: read failed !\n");
             return -1;
         }
         return 0;
     }
 
-    r->aiocb = bdrv_aio_read(bdrv, 0, (uint8_t*)&r->io_header,
-                          -(int64_t)sizeof(r->io_header), complete, r);
+    r->aiocb = bdrv_sg_aio_read(bdrv, (uint8_t*)&r->io_header,
+                                sizeof(r->io_header), complete, r);
     if (r->aiocb == NULL) {
         BADF("execute_command: read failed !\n");
         return -1;
@@ -634,14 +637,15 @@ static int get_blocksize(BlockDriverState *bdrv)
     io_header.sbp = sensebuf;
     io_header.timeout = 6000; /* XXX */
 
-    ret = bdrv_pwrite(bdrv, -1, &io_header, sizeof(io_header));
-    if (ret == -1)
+    ret = bdrv_sg_send_command(bdrv, &io_header, sizeof(io_header));
+    if (ret < 0)
         return -1;
 
-    while ((ret = bdrv_pread(bdrv, -1, &io_header, sizeof(io_header))) == -1 &&
-           errno == EINTR);
+    while ((ret = bdrv_sg_recv_response(bdrv, &io_header, sizeof(io_header))) < 0 &&
+           ret == -EINTR)
+        ;
 
-    if (ret == -1)
+    if (ret < 0)
         return -1;
 
     return (buf[4] << 24) | (buf[5] << 16) | (buf[6] << 8) | buf[7];
@@ -671,14 +675,15 @@ static int get_stream_blocksize(BlockDriverState *bdrv)
     io_header.sbp = sensebuf;
     io_header.timeout = 6000; /* XXX */
 
-    ret = bdrv_pwrite(bdrv, -1, &io_header, sizeof(io_header));
-    if (ret == -1)
+    ret = bdrv_sg_send_command(bdrv, &io_header, sizeof(io_header));
+    if (ret < 0)
         return -1;
 
-    while ((ret = bdrv_pread(bdrv, -1, &io_header, sizeof(io_header))) == -1 &&
-           errno == EINTR);
+    while ((ret = bdrv_sg_recv_response(bdrv, &io_header, sizeof(io_header))) < 0 &&
+           ret == -EINTR)
+        ;
 
-    if (ret == -1)
+    if (ret < 0)
         return -1;
 
     return (buf[9] << 16) | (buf[10] << 8) | buf[11];
-- 
1.6.1.1

[Qemu-devel] [PATCH 2/3] Add internal scsi generic block API

[Avi Kivity]

Add an internal API for the generic block layer to send scsi generic commands
to block format driver.  This means block format drivers no longer need
to consider overloaded nb_sectors parameters.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block-raw-posix.c |   30 ++++++++++++++++++++++++++++++
 block.c           |    8 ++++----
 block_int.h       |   10 ++++++++++
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index 620791b..6b8adc4 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -1164,6 +1164,32 @@ static int raw_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 }
 #endif /* !linux */
 
+static int raw_sg_send_command(BlockDriverState *bs, void *buf, int count)
+{
+    return raw_pwrite(bs, -1, buf, count);
+}
+
+static int raw_sg_recv_response(BlockDriverState *bs, void *buf, int count)
+{
+    return raw_pread(bs, -1, buf, count);
+}
+
+static BlockDriverAIOCB *raw_sg_aio_read(BlockDriverState *bs,
+                                         void *buf, int count,
+                                         BlockDriverCompletionFunc *cb,
+                                         void *opaque)
+{
+    return raw_aio_read(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
+
+static BlockDriverAIOCB *raw_sg_aio_write(BlockDriverState *bs,
+                                          void *buf, int count,
+                                          BlockDriverCompletionFunc *cb,
+                                          void *opaque)
+{
+    return raw_aio_write(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
+
 BlockDriver bdrv_host_device = {
     "host_device",
     sizeof(BDRVRawState),
@@ -1193,4 +1219,8 @@ BlockDriver bdrv_host_device = {
     .bdrv_set_locked = raw_set_locked,
     /* generic scsi device */
     .bdrv_ioctl = raw_ioctl,
+    .bdrv_sg_send_command = raw_sg_send_command,
+    .bdrv_sg_recv_response = raw_sg_recv_response,
+    .bdrv_sg_aio_read = raw_sg_aio_read,
+    .bdrv_sg_aio_write = raw_sg_aio_write,
 };
diff --git a/block.c b/block.c
index 774ac2d..4aaea09 100644
--- a/block.c
+++ b/block.c
@@ -1597,22 +1597,22 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 
 int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count)
 {
-    return bdrv_pwrite(bs, -1, buf, count);
+    return bs->drv->bdrv_sg_send_command(bs, buf, count);
 }
 
 int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count)
 {
-    return bdrv_pread(bs, -1, buf, count);
+    return bs->drv->bdrv_sg_recv_response(bs, buf, count);
 }
 
 BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
                                    BlockDriverCompletionFunc *cb, void *opaque)
 {
-    return bdrv_aio_read(bs, 0, buf, -(int64_t)count, cb, opaque);
+    return bs->drv->bdrv_sg_aio_read(bs, buf, count, cb, opaque);
 }
 
 BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
                                     BlockDriverCompletionFunc *cb, void *opaque)
 {
-    return bdrv_aio_write(bs, 0, buf, -(int64_t)count, cb, opaque);
+    return bs->drv->bdrv_sg_aio_write(bs, buf, count, cb, opaque);
 }
diff --git a/block_int.h b/block_int.h
index e83fd2c..e4630f0 100644
--- a/block_int.h
+++ b/block_int.h
@@ -84,6 +84,16 @@ struct BlockDriver {
 
     /* to control generic scsi devices */
     int (*bdrv_ioctl)(BlockDriverState *bs, unsigned long int req, void *buf);
+    int (*bdrv_sg_send_command)(BlockDriverState *bs, void *buf, int count);
+    int (*bdrv_sg_recv_response)(BlockDriverState *bs, void *buf, int count);
+    BlockDriverAIOCB *(*bdrv_sg_aio_read)(BlockDriverState *bs,
+                                          void *buf, int count,
+                                          BlockDriverCompletionFunc *cb,
+                                          void *opaque);
+    BlockDriverAIOCB *(*bdrv_sg_aio_write)(BlockDriverState *bs,
+                                           void *buf, int count,
+                                           BlockDriverCompletionFunc *cb,
+                                           void *opaque);
 
     BlockDriverAIOCB *free_aiocb;
     struct BlockDriver *next;
-- 
1.6.1.1

[Qemu-devel] [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs

[Avi Kivity]

Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(), we can
drop the corresponding internal APIs, which overlap bdrv_read()/bdrv_write()
and, being byte oriented, are unnatural for a block device.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block-raw-posix.c |   20 +++++++++++---
 block-raw-win32.c |   20 ++++++++------
 block.c           |   73 ++++------------------------------------------------
 block_int.h       |    4 ---
 4 files changed, 34 insertions(+), 83 deletions(-)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index 6b8adc4..9b32615 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -353,6 +353,12 @@ static int raw_pread(BlockDriverState *bs, int64_t offset,
     return raw_pread_aligned(bs, offset, buf, count) + sum;
 }
 
+static int raw_read(BlockDriverState *bs, int64_t sector_num,
+                    uint8_t *buf, int nb_sectors)
+{
+    return raw_pread(bs, sector_num * 512, buf, (uint64_t)nb_sectors * 512);
+}
+
 /*
  * offset and count are in bytes and possibly not aligned. For files opened
  * with O_DIRECT, necessary alignments are ensured before calling
@@ -431,6 +437,12 @@ static int raw_pwrite(BlockDriverState *bs, int64_t offset,
     return raw_pwrite_aligned(bs, offset, buf, count) + sum;
 }
 
+static int raw_write(BlockDriverState *bs, int64_t sector_num,
+                     const uint8_t *buf, int nb_sectors)
+{
+    return raw_pwrite(bs, sector_num * 512, buf, (uint64_t)nb_sectors * 512);
+}
+
 #ifdef CONFIG_AIO
 /***********************************************************/
 /* Unix AIO using POSIX AIO */
@@ -828,8 +840,8 @@ BlockDriver bdrv_raw = {
     .aiocb_size = sizeof(RawAIOCB),
 #endif
 
-    .bdrv_pread = raw_pread,
-    .bdrv_pwrite = raw_pwrite,
+    .bdrv_read = raw_read,
+    .bdrv_write = raw_write,
     .bdrv_truncate = raw_truncate,
     .bdrv_getlength = raw_getlength,
 };
@@ -1208,8 +1220,8 @@ BlockDriver bdrv_host_device = {
     .aiocb_size = sizeof(RawAIOCB),
 #endif
 
-    .bdrv_pread = raw_pread,
-    .bdrv_pwrite = raw_pwrite,
+    .bdrv_read = raw_read,
+    .bdrv_write = raw_write,
     .bdrv_getlength = raw_getlength,
 
     /* removable device support */
diff --git a/block-raw-win32.c b/block-raw-win32.c
index 892f2d1..f37ab68 100644
--- a/block-raw-win32.c
+++ b/block-raw-win32.c
@@ -121,13 +121,15 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
     return 0;
 }
 
-static int raw_pread(BlockDriverState *bs, int64_t offset,
-                     uint8_t *buf, int count)
+static int raw_read(BlockDriverState *bs, int64_t sector_num,
+                    uint8_t *buf, int nb_sectors)
 {
     BDRVRawState *s = bs->opaque;
     OVERLAPPED ov;
     DWORD ret_count;
     int ret;
+    int64_t offset = sector_num * 512;
+    int count = nb_sectors * 512;
 
     memset(&ov, 0, sizeof(ov));
     ov.Offset = offset;
@@ -145,13 +147,15 @@ static int raw_pread(BlockDriverState *bs, int64_t offset,
     return ret_count;
 }
 
-static int raw_pwrite(BlockDriverState *bs, int64_t offset,
-                      const uint8_t *buf, int count)
+static int raw_write(BlockDriverState *bs, int64_t sector_num,
+                     const uint8_t *buf, int nb_sectors)
 {
     BDRVRawState *s = bs->opaque;
     OVERLAPPED ov;
     DWORD ret_count;
     int ret;
+    int64_t offset = sector_num * 512;
+    int count = nb_sectors * 512;
 
     memset(&ov, 0, sizeof(ov));
     ov.Offset = offset;
@@ -358,8 +362,8 @@ BlockDriver bdrv_raw = {
     .bdrv_aio_cancel = raw_aio_cancel,
     .aiocb_size = sizeof(RawAIOCB);
 #endif
-    .bdrv_pread = raw_pread,
-    .bdrv_pwrite = raw_pwrite,
+    .bdrv_read = raw_read,
+    .bdrv_write = raw_write,
     .bdrv_truncate = raw_truncate,
     .bdrv_getlength = raw_getlength,
 };
@@ -511,7 +515,7 @@ BlockDriver bdrv_host_device = {
     .bdrv_aio_cancel = raw_aio_cancel,
     .aiocb_size = sizeof(RawAIOCB);
 #endif
-    .bdrv_pread = raw_pread,
-    .bdrv_pwrite = raw_pwrite,
+    .bdrv_read = raw_read,
+    .bdrv_write = raw_write,
     .bdrv_getlength = raw_getlength,
 };
diff --git a/block.c b/block.c
index 4aaea09..5963d54 100644
--- a/block.c
+++ b/block.c
@@ -136,7 +136,7 @@ static void bdrv_register(BlockDriver *bdrv)
         bdrv->bdrv_aio_write = bdrv_aio_write_em;
         bdrv->bdrv_aio_cancel = bdrv_aio_cancel_em;
         bdrv->aiocb_size = sizeof(BlockDriverAIOCBSync);
-    } else if (!bdrv->bdrv_read && !bdrv->bdrv_pread) {
+    } else if (!bdrv->bdrv_read) {
         /* add synchronous IO emulation layer */
         bdrv->bdrv_read = bdrv_read_em;
         bdrv->bdrv_write = bdrv_write_em;
@@ -528,22 +528,7 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num,
     if (!drv)
         return -ENOMEDIUM;
 
-    if (drv->bdrv_pread) {
-        int ret, len;
-        len = nb_sectors * 512;
-        ret = drv->bdrv_pread(bs, sector_num * 512, buf, len);
-        if (ret < 0)
-            return ret;
-        else if (ret != len)
-            return -EINVAL;
-        else {
-	    bs->rd_bytes += (unsigned) len;
-	    bs->rd_ops ++;
-            return 0;
-	}
-    } else {
-        return drv->bdrv_read(bs, sector_num, buf, nb_sectors);
-    }
+    return drv->bdrv_read(bs, sector_num, buf, nb_sectors);
 }
 
 /* Return < 0 if error. Important errors are:
@@ -560,27 +545,11 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
         return -ENOMEDIUM;
     if (bs->read_only)
         return -EACCES;
-    if (drv->bdrv_pwrite) {
-        int ret, len, count = 0;
-        len = nb_sectors * 512;
-        do {
-            ret = drv->bdrv_pwrite(bs, sector_num * 512, buf, len - count);
-            if (ret < 0) {
-                printf("bdrv_write ret=%d\n", ret);
-                return ret;
-            }
-            count += ret;
-            buf += ret;
-        } while (count != len);
-        bs->wr_bytes += (unsigned) len;
-        bs->wr_ops ++;
-        return 0;
-    }
     return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
 }
 
-static int bdrv_pread_em(BlockDriverState *bs, int64_t offset,
-                         uint8_t *buf, int count1)
+int bdrv_pread(BlockDriverState *bs, int64_t offset,
+               void *buf, int count1)
 {
     uint8_t tmp_buf[SECTOR_SIZE];
     int len, nb_sectors, count;
@@ -623,8 +592,8 @@ static int bdrv_pread_em(BlockDriverState *bs, int64_t offset,
     return count1;
 }
 
-static int bdrv_pwrite_em(BlockDriverState *bs, int64_t offset,
-                          const uint8_t *buf, int count1)
+int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
+                const void *buf, int count1)
 {
     uint8_t tmp_buf[SECTOR_SIZE];
     int len, nb_sectors, count;
@@ -672,36 +641,6 @@ static int bdrv_pwrite_em(BlockDriverState *bs, int64_t offset,
 }
 
 /**
- * Read with byte offsets (needed only for file protocols)
- */
-int bdrv_pread(BlockDriverState *bs, int64_t offset,
-               void *buf1, int count1)
-{
-    BlockDriver *drv = bs->drv;
-
-    if (!drv)
-        return -ENOMEDIUM;
-    if (!drv->bdrv_pread)
-        return bdrv_pread_em(bs, offset, buf1, count1);
-    return drv->bdrv_pread(bs, offset, buf1, count1);
-}
-
-/**
- * Write with byte offsets (needed only for file protocols)
- */
-int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
-                const void *buf1, int count1)
-{
-    BlockDriver *drv = bs->drv;
-
-    if (!drv)
-        return -ENOMEDIUM;
-    if (!drv->bdrv_pwrite)
-        return bdrv_pwrite_em(bs, offset, buf1, count1);
-    return drv->bdrv_pwrite(bs, offset, buf1, count1);
-}
-
-/**
  * Truncate file to 'offset' bytes (needed only for file protocols)
  */
 int bdrv_truncate(BlockDriverState *bs, int64_t offset)
diff --git a/block_int.h b/block_int.h
index e4630f0..cc9966b 100644
--- a/block_int.h
+++ b/block_int.h
@@ -58,10 +58,6 @@ struct BlockDriver {
     int aiocb_size;
 
     const char *protocol_name;
-    int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
-                      uint8_t *buf, int count);
-    int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
-                       const uint8_t *buf, int count);
     int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
     int64_t (*bdrv_getlength)(BlockDriverState *bs);
     int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
-- 
1.6.1.1

[Qemu-devel] Re: [PATCH 0/3] Remove ->bdrv_pread() internal block layer API

[Anthony Liguori]

Avi Kivity wrote:
> The internal block layer API has a wart in the form of the
> ->bdrv_pread() and ->bdrv_pwrite() methods.  These duplicate
> bdrv_read() and bdrv_write(), and are unnatural for a block
> device in that they are not sector oriented.
>
> One of the reasons for their existence is the scsi-generic host support.
> That is also a wart, as scsi-generic is really a packet (command/response)
> rather than a block (random access to sectors) API.  There is also the
> wierd negative sector count == byte count thing.
>   

Great stuff, this has always bugged me.

Regards,

Anthony Liguori

> This patchset addresses these warts by adding a new API for scsi-generic
> and dropping ->bdrv_pread() and bdrv_pwrite().  While the improvement
> falls short of spectacular, it does make further hacking on the block
> layer easier as there are fewer callbacks to worry about.
>
> Avi Kivity (3):
>   Add specialized block driver scsi generic API
>   Add internal scsi generic block API
>   Drop internal bdrv_pread()/bdrv_pwrite() APIs
>
>  block-raw-posix.c |   50 +++++++++++++++++++++++++--
>  block-raw-win32.c |   20 +++++++----
>  block.c           |   95 +++++++++++++++-------------------------------------
>  block.h           |    8 ++++
>  block_int.h       |   14 +++++--
>  hw/scsi-generic.c |   39 ++++++++++++---------
>  6 files changed, 126 insertions(+), 100 deletions(-)
>
>   

[Qemu-devel] Re: [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs

[Anthony Liguori]

Avi Kivity wrote:
> Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(), we can
> drop the corresponding internal APIs, which overlap bdrv_read()/bdrv_write()
> and, being byte oriented, are unnatural for a block device.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
>
>  int bdrv_truncate(BlockDriverState *bs, int64_t offset)
> diff --git a/block_int.h b/block_int.h
> index e4630f0..cc9966b 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -58,10 +58,6 @@ struct BlockDriver {
>      int aiocb_size;
>  
>      const char *protocol_name;
> -    int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
> -                      uint8_t *buf, int count);
> -    int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
> -                       const uint8_t *buf, int count);
>   

$ grep -l bdrv_pwrite *.c hw/*.c
block.c
block-qcow2.c
block-qcow.c
block-raw-posix.c
block-raw-win32.c
block-vmdk.c
block-vpc.c
savevm.c
hw/scsi-generic.c

So there's a lot of users other than scsi-generic.  Usually, these 
callers are in the block layer to read/write metadata that isn't always 
block aligned.  Some buffer adjustment could fix savevm.c to ensure 
alignment.

These users are now relegated to using emulated pread/pwrite?  Won't 
that have a noticable impact on performance if updating small bits of 
metadata.  For instance, I think updating qcow2 refcounts would look bad 
since you have to read/write the full block to update 4 bytes of data.  
Granted it'll be cached, but...

Regards,

Anthony Liguori
>      int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
>      int64_t (*bdrv_getlength)(BlockDriverState *bs);
>      int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>   

[Qemu-devel] Re: [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(), 
>> we can
>> drop the corresponding internal APIs, which overlap 
>> bdrv_read()/bdrv_write()
>> and, being byte oriented, are unnatural for a block device.
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>
>>  int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>> diff --git a/block_int.h b/block_int.h
>> index e4630f0..cc9966b 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -58,10 +58,6 @@ struct BlockDriver {
>>      int aiocb_size;
>>  
>>      const char *protocol_name;
>> -    int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
>> -                      uint8_t *buf, int count);
>> -    int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
>> -                       const uint8_t *buf, int count);
>>   
>
> $ grep -l bdrv_pwrite *.c hw/*.c
> block.c
> block-qcow2.c
> block-qcow.c
> block-raw-posix.c
> block-raw-win32.c
> block-vmdk.c
> block-vpc.c
> savevm.c
> hw/scsi-generic.c
>
> So there's a lot of users other than scsi-generic.  Usually, these 
> callers are in the block layer to read/write metadata that isn't 
> always block aligned.  Some buffer adjustment could fix savevm.c to 
> ensure alignment.

It's more accurate to say that there are now no users that depend on the 
request size.  The other users will happily allow expansion of a small 
request to a sector.

> These users are now relegated to using emulated pread/pwrite?  Won't 
> that have a noticable impact on performance if updating small bits of 
> metadata.  For instance, I think updating qcow2 refcounts would look 
> bad since you have to read/write the full block to update 4 bytes of 
> data.  Granted it'll be cached, but...

I haven't measured but I'll bet the impact is unnoticable.  We're 
doubling the syscall count for writes, but the actual transfer (if 
cached) or the I/O (if uncached) will swamp that.  For reads, we're 
copying a bit more data, but that won't even tickle performance.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(), 
>> we can
>> drop the corresponding internal APIs, which overlap 
>> bdrv_read()/bdrv_write()
>> and, being byte oriented, are unnatural for a block device.
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>
>>  int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>> diff --git a/block_int.h b/block_int.h
>> index e4630f0..cc9966b 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -58,10 +58,6 @@ struct BlockDriver {
>>      int aiocb_size;
>>  
>>      const char *protocol_name;
>> -    int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
>> -                      uint8_t *buf, int count);
>> -    int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
>> -                       const uint8_t *buf, int count);
>>   
>
> $ grep -l bdrv_pwrite *.c hw/*.c
> block.c
> block-qcow2.c
> block-qcow.c
> block-raw-posix.c
> block-raw-win32.c
> block-vmdk.c
> block-vpc.c
> savevm.c
> hw/scsi-generic.c
>
> So there's a lot of users other than scsi-generic.  Usually, these 
> callers are in the block layer to read/write metadata that isn't 
> always block aligned.  Some buffer adjustment could fix savevm.c to 
> ensure alignment.

It's more accurate to say that there are now no users that depend on the 
request size.  The other users will happily allow expansion of a small 
request to a sector.

> These users are now relegated to using emulated pread/pwrite?  Won't 
> that have a noticable impact on performance if updating small bits of 
> metadata.  For instance, I think updating qcow2 refcounts would look 
> bad since you have to read/write the full block to update 4 bytes of 
> data.  Granted it'll be cached, but...

I haven't measured but I'll bet the impact is unnoticable.  We're 
doubling the syscall count for writes, but the actual transfer (if 
cached) or the I/O (if uncached) will swamp that.  For reads, we're 
copying a bit more data, but that won't even tickle performance.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Avi Kivity]

When a scsi device is backed by a scsi generic device instead of an
ordinary host block device, the block API is abused in a couple of annoying
ways:

 - nb_sectors is negative, and specifies a byte count instead of a sector count
 - offset is ignored, since scsi-generic is essentially a packet protocol

This overloading makes hacking the block layer difficult.  Remove it by
introducing a new explicit API for scsi-generic devices.  The new API
is still backed by the old implementation, but at least the users are
insulated.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block.c           |   22 ++++++++++++++++++++++
 block.h           |    8 ++++++++
 hw/scsi-generic.c |   39 ++++++++++++++++++++++-----------------
 3 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 1ef4ae5..39b27b2 100644
--- a/block.c
+++ b/block.c
@@ -1675,3 +1675,25 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
         return drv->bdrv_ioctl(bs, req, buf);
     return -ENOTSUP;
 }
+
+int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count)
+{
+    return bdrv_pwrite(bs, -1, buf, count);
+}
+
+int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count)
+{
+    return bdrv_pread(bs, -1, buf, count);
+}
+
+BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
+                                   BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return bdrv_aio_read(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
+
+BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
+                                    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    return bdrv_aio_write(bs, 0, buf, -(int64_t)count, cb, opaque);
+}
diff --git a/block.h b/block.h
index f6b14ee..b0c63ac 100644
--- a/block.h
+++ b/block.h
@@ -101,6 +101,14 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
                                  BlockDriverCompletionFunc *cb, void *opaque);
 void bdrv_aio_cancel(BlockDriverAIOCB *acb);
 
+/* sg packet commands */
+int bdrv_sg_send_command(BlockDriverState *bs, void *buf, int count);
+int bdrv_sg_recv_response(BlockDriverState *bs, void *buf, int count);
+BlockDriverAIOCB *bdrv_sg_aio_read(BlockDriverState *bs, void *buf, int count,
+                                   BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *bdrv_sg_aio_write(BlockDriverState *bs, void *buf, int count,
+                                    BlockDriverCompletionFunc *cb, void *opaque);
+
 /* Ensure contents are flushed to disk.  */
 void bdrv_flush(BlockDriverState *bs);
 void bdrv_flush_all(void);
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 5bf160a..53e8951 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -201,6 +201,7 @@ static int execute_command(BlockDriverState *bdrv,
                            SCSIRequest *r, int direction,
 			   BlockDriverCompletionFunc *complete)
 {
+    int ret;
 
     r->io_header.interface_id = 'S';
     r->io_header.dxfer_direction = direction;
@@ -214,25 +215,27 @@ static int execute_command(BlockDriverState *bdrv,
     r->io_header.usr_ptr = r;
     r->io_header.flags |= SG_FLAG_DIRECT_IO;
 
-    if (bdrv_pwrite(bdrv, -1, &r->io_header, sizeof(r->io_header)) == -1) {
+    ret = bdrv_sg_send_command(bdrv, &r->io_header, sizeof(r->io_header));
+    if (ret < 0) {
         BADF("execute_command: write failed ! (%d)\n", errno);
         return -1;
     }
     if (complete == NULL) {
         int ret;
         r->aiocb = NULL;
-        while ((ret = bdrv_pread(bdrv, -1, &r->io_header,
-                                           sizeof(r->io_header))) == -1 &&
-                      errno == EINTR);
-        if (ret == -1) {
+        while ((ret = bdrv_sg_recv_response(bdrv, &r->io_header,
+                                            sizeof(r->io_header))) < 0 &&
+               ret == -EINTR)
+            ;
+        if (ret < 0) {
             BADF("execute_command: read failed !\n");
             return -1;
         }
         return 0;
     }
 
-    r->aiocb = bdrv_aio_read(bdrv, 0, (uint8_t*)&r->io_header,
-                          -(int64_t)sizeof(r->io_header), complete, r);
+    r->aiocb = bdrv_sg_aio_read(bdrv, (uint8_t*)&r->io_header,
+                                sizeof(r->io_header), complete, r);
     if (r->aiocb == NULL) {
         BADF("execute_command: read failed !\n");
         return -1;
@@ -634,14 +637,15 @@ static int get_blocksize(BlockDriverState *bdrv)
     io_header.sbp = sensebuf;
     io_header.timeout = 6000; /* XXX */
 
-    ret = bdrv_pwrite(bdrv, -1, &io_header, sizeof(io_header));
-    if (ret == -1)
+    ret = bdrv_sg_send_command(bdrv, &io_header, sizeof(io_header));
+    if (ret < 0)
         return -1;
 
-    while ((ret = bdrv_pread(bdrv, -1, &io_header, sizeof(io_header))) == -1 &&
-           errno == EINTR);
+    while ((ret = bdrv_sg_recv_response(bdrv, &io_header, sizeof(io_header))) < 0 &&
+           ret == -EINTR)
+        ;
 
-    if (ret == -1)
+    if (ret < 0)
         return -1;
 
     return (buf[4] << 24) | (buf[5] << 16) | (buf[6] << 8) | buf[7];
@@ -671,14 +675,15 @@ static int get_stream_blocksize(BlockDriverState *bdrv)
     io_header.sbp = sensebuf;
     io_header.timeout = 6000; /* XXX */
 
-    ret = bdrv_pwrite(bdrv, -1, &io_header, sizeof(io_header));
-    if (ret == -1)
+    ret = bdrv_sg_send_command(bdrv, &io_header, sizeof(io_header));
+    if (ret < 0)
         return -1;
 
-    while ((ret = bdrv_pread(bdrv, -1, &io_header, sizeof(io_header))) == -1 &&
-           errno == EINTR);
+    while ((ret = bdrv_sg_recv_response(bdrv, &io_header, sizeof(io_header))) < 0 &&
+           ret == -EINTR)
+        ;
 
-    if (ret == -1)
+    if (ret < 0)
         return -1;
 
     return (buf[9] << 16) | (buf[10] << 8) | buf[11];
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Christoph Hellwig]

On Thu, Mar 12, 2009 at 02:57:09PM +0200, Avi Kivity wrote:
> When a scsi device is backed by a scsi generic device instead of an
> ordinary host block device, the block API is abused in a couple of annoying
> ways:
> 
>  - nb_sectors is negative, and specifies a byte count instead of a sector count
>  - offset is ignored, since scsi-generic is essentially a packet protocol
> 
> This overloading makes hacking the block layer difficult.  Remove it by
> introducing a new explicit API for scsi-generic devices.  The new API
> is still backed by the old implementation, but at least the users are
> insulated.

Getting rid of this is good, I found tons of issues in that area
with the I/O path exerciser I wrote last week.  Hower the way the new
API is designed somewhat gets in the way of my patch series to support
Gerd's native preadv/pwritev.

One thing I wonder is why we go through the block layer at all for SG.
It is actually backed by a char device that doesn't have semantics like
a block device at all, given that it can't be seekend and is accessed
at byte granularity.  We could just go directly to the native Linus
syscalls from scsi-generic.c (or a scsi-generic-linux.c if we want
some level of abstraction).  I'll cook up a patch trying that once I'm
back home.

Re: [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Avi Kivity]

Christoph Hellwig wrote:
> Hower the way the new
> API is designed somewhat gets in the way of my patch series to support
> Gerd's native preadv/pwritev.
>
>   

Can you point out specific issues?

> One thing I wonder is why we go through the block layer at all for SG.
> It is actually backed by a char device that doesn't have semantics like
> a block device at all, given that it can't be seekend and is accessed
> at byte granularity.  We could just go directly to the native Linus
> syscalls from scsi-generic.c (or a scsi-generic-linux.c if we want
> some level of abstraction).  I'll cook up a patch trying that once I'm
> back home

Using the block layer has the advantage of common setup, statistics, and 
management.  I agree that the actual data movement is horribly out of 
sync with the other format drivers.

Come to think of it, I don't see how statistics can work, so maybe it is 
a good idea to separate the two paths completely.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Christoph Hellwig]

On Sun, Mar 15, 2009 at 03:54:26PM +0200, Avi Kivity wrote:
> >Hower the way the new
> >API is designed somewhat gets in the way of my patch series to support
> >Gerd's native preadv/pwritev.
> >
> >  
> 
> Can you point out specific issues?

The patch series (now posted) makes all aio APIs use iovecs.  The
current non-iovec based SG API would require some nasty shims allocating
nested aiocbs.  Or we could just make the sg aio methods fake-ioveced
which is the solution I had before your split, always adding a
one-element iovec to the scsi-generic request structure.


> Using the block layer has the advantage of common setup, statistics, and 
> management.  I agree that the actual data movement is horribly out of 
> sync with the other format drivers.

Yeah, the setup is kinda interwinded with the block layer.  I wonder
whether the device-tree ideas will help sorting that out.

Note that I eventually want to support SG_IO on cdrom devices for
burning etc at which point we need to support scsi-generic even
if another driver is attached.  Note entirely sure how to handle
that yet.

Re: [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Avi Kivity]

Christoph Hellwig wrote:
> The patch series (now posted) makes all aio APIs use iovecs.  The
> current non-iovec based SG API would require some nasty shims allocating
> nested aiocbs.  Or we could just make the sg aio methods fake-ioveced
> which is the solution I had before your split, always adding a
> one-element iovec to the scsi-generic request structure.
>   

I'd go for having everything vectored.  It's how non-1962 hardware works.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Christoph Hellwig]

On Sun, Mar 15, 2009 at 04:57:53PM +0200, Avi Kivity wrote:
> Christoph Hellwig wrote:
> >The patch series (now posted) makes all aio APIs use iovecs.  The
> >current non-iovec based SG API would require some nasty shims allocating
> >nested aiocbs.  Or we could just make the sg aio methods fake-ioveced
> >which is the solution I had before your split, always adding a
> >one-element iovec to the scsi-generic request structure.
> >  
> 
> I'd go for having everything vectored.  It's how non-1962 hardware works.

Yeah, that's what I did for regular block I/O in my series.
scsi-generic is a little different beast, though.  qemu uses the legacy
(not sure if it's formally deprecated) read/write support on /dev/sg
which doen't support vectored I/O.  If we want to use vectored I/O
with sg we'd have to use the SG_IO ioctl (which also has the advantage
of working with all sd/sr/ide/etc block devices).  But supporting that
would probably require very different APIs then the current (aio-)
read/writeish one.

Re: [Qemu-devel] [PATCH 1/3] Add specialized block driver scsi generic API

[Jamie Lokier]

Avi Kivity wrote:
> I'd go for having everything vectored.  It's how non-1962 hardware works.

Hey, I'm working right now with a chip which has non-vectored IDE hardware!

(Yes, that bit sucks.)

-- Jamie