[Qemu-devel] [PATCH v2 0/3] scsi-generic and BLKSECTGET

[Qemu-devel] [PATCH v2 0/3] scsi-generic and BLKSECTGET

[Eric Farman]

In the Linux kernel, I see two (three) places where the BLKSECTGET ioctl is
handled:

(1) block/(compat_)ioctl.c -- (compat_)blkdev_ioctl
(2) drivers/scsi/sg.c -- sg_ioctl

The former has been around forever[1], and returns a short value measured in
sectors.  A sector is generally assumed to be 512 bytes.

The latter has been around for slightly less than forever[2], and returns an
int that measures the value in bytes.  A change to return the block count
was brought up a few years ago[3] and nacked.

As a convenient example, if I use the blockdev tool to drive the ioctl to a
SCSI disk and its scsi-generic equivalent, I get different results:

  # lsscsi -g
  [0:0:8:1077166114]disk    IBM      2107900          .217  /dev/sda /dev/sg0
  # blockdev --getmaxsect /dev/sda
  2560
  # blockdev --getmaxsect /dev/sg0
  20

Now, the value for /dev/sda looks "correct" to me.

  # cd /sys/devices/css0/0.0.0125/0.0.1f69/host0/rport-0\:0-8/
  # cd target0\:0\:8/0\:0\:8\:1077166114/
  # cat block/sda/queue/max_sectors_kb
  1280
  # cat block/sda/queue/hw_sector_size
  512

And the math checks out:

  max_sectors_kb * 1024 / hw_sector_size == getmaxsect
  -OR-
  1280 * 1024 / 512 = 2560

For /dev/sg0, it appears the answer is coming from the sg_ioctl result
which is already multiplied by the block size, and then looking at only the
upper half (short) of the returned big-endian fullword:

  (1280 * 1024 / 512) * 512 = 1310720 = x00140000 => x0014 = 20

The reason for all this?  Well, QEMU recently added a BLKSECTGET ioctl
call[4] which we see during guest boot.  This code presumes the value is in
blocks/sectors, and converts it to bytes[5].  Not that this matters, because
the short/int discrepancy gives us "zero" on s390x.

Also, that code doesn't execute for scsi-generic devices, so the conversion
to bytes is correct, but I'd like to extend this code to interrogate
scsi-generic devices as well.  This is important because libvirt converts
a specified virtio-scsi device to its /dev/sgX address for the QEMU
commandline.

So, do I have to code around the different field sizes (int vs short) as
well as scaling (bytes vs blocks)?  Obviously doable, but looking at the
resulting commits, I find myself feeling a little ill.

Changes:
 v1->v2:
  - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]

[1] The initial kernel git commit
[2] kernel commit 44ec95425c1d9dce6e4638c29e4362cfb44814e7
[3] https://lkml.org/lkml/2012/6/27/78
[4] qemu commit 6f6071745bd0366221f5a0160ed7d18d0e38b9f7
[5] qemu commit 5def6b80e1eca696c1fc6099e7f4d36729686402 

Eric Farman (3):
  hw/scsi: Fix debug message of cdb structure in scsi-generic
  block: Fix target variable of BLKSECTGET ioctl
  block: get max_transfer limit for char (scsi-generic) devices

 block/file-posix.c     | 17 +++++++++++------
 hw/scsi/scsi-generic.c |  5 +++--
 include/block/block.h  |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.8.4

[Qemu-devel] [PATCH v2 1/3] hw/scsi: Fix debug message of cdb structure in scsi-generic

[Eric Farman]

When running with debug enabled, the scsi-generic cdb that is
dumped skips byte 0 of the command, which is the opcode.  This
makes identifying which command is being issued/completed a
little difficult.  Example:

  0x00 0x00 0x01 0x00 0x00
  scsi-generic: scsi_read_data 0x0
  scsi-generic: Data ready tag=0x0 len=164
  scsi-generic: scsi_read_data 0x0
  scsi-generic: Command complete 0x0x10a42c60 tag=0x0 status=0

Improve this by adding a message prior to the loop, similar to
what exists for scsi-disk.  Clean up a few other messages to be
more explicit of what is being represented.  Example:

  scsi-generic: Command: data=0x12 0x00 0x00 0x01 0x00 0x00
  scsi-generic: scsi_read_data tag=0x0
  scsi-generic: Data ready tag=0x0 len=164
  scsi-generic: scsi_read_data tag=0x0
  scsi-generic: Command complete 0x0x10a452d0 tag=0x0 status=0

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 hw/scsi/scsi-generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7a588a7..92f091a 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -246,7 +246,7 @@ static void scsi_read_data(SCSIRequest *req)
     SCSIDevice *s = r->req.dev;
     int ret;
 
-    DPRINTF("scsi_read_data 0x%x\n", req->tag);
+    DPRINTF("scsi_read_data tag=0x%x\n", req->tag);
 
     /* The request is used as the AIO opaque value, so add a ref.  */
     scsi_req_ref(&r->req);
@@ -294,7 +294,7 @@ static void scsi_write_data(SCSIRequest *req)
     SCSIDevice *s = r->req.dev;
     int ret;
 
-    DPRINTF("scsi_write_data 0x%x\n", req->tag);
+    DPRINTF("scsi_write_data tag=0x%x\n", req->tag);
     if (r->len == 0) {
         r->len = r->buflen;
         scsi_req_data(&r->req, r->len);
@@ -329,6 +329,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
     int ret;
 
 #ifdef DEBUG_SCSI
+    DPRINTF("Command: data=0x%02x", cmd[0]);
     {
         int i;
         for (i = 1; i < r->req.cmd.len; i++) {
-- 
2.8.4

[Qemu-devel] [PATCH v2 2/3] block: Fix target variable of BLKSECTGET ioctl

[Eric Farman]

Commit 6f607174 introduced a routine to call the kernel BLKSECTGET
ioctl, which stores the result back to user space.  However, the
size of the data returned depends on the routine handling the ioctl.
The (compat_)blkdev_ioctl returns a short, while sg_ioctl returns
an int.  Thus, on big-endian systems, we can find ourselves
accidentally shifting the result to a much larger value.
(On s390x, a short is 16 bits while an int is 32 bits.)

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 block/file-posix.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 28b47d9..2115155 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
     state->opaque = NULL;
 }
 
-static int hdev_get_max_transfer_length(int fd)
+static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
 {
 #ifdef BLKSECTGET
     int max_sectors = 0;
-    if (ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
+    short max_sectors_short = 0;
+    if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
         return max_sectors;
+    } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors_short) == 0) {
+        return max_sectors_short;
     } else {
         return -errno;
     }
@@ -672,7 +675,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 
     if (!fstat(s->fd, &st)) {
         if (S_ISBLK(st.st_mode)) {
-            int ret = hdev_get_max_transfer_length(s->fd);
+            int ret = hdev_get_max_transfer_length(bs, s->fd);
             if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
                 bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
             }
-- 
2.8.4

[Qemu-devel] [PATCH v2 3/3] block: get max_transfer limit for char (scsi-generic) devices

[Eric Farman]

Commit 6f607174 introduced a routine to get the maximum number
of bytes for a single I/O transfer for block devices, however
scsi generic devices are character devices, not block.  Add
a condition for this, such that scsi generic devices can view
the same data.

Some tweaking of data is required, because the block and sg
ioctls return values in different scales (sectors versus
bytes).  So adjust hdev_get_max_transfer_length such that it
always returns a value in bytes.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 block/file-posix.c    | 10 ++++++----
 include/block/block.h |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2115155..94068ca 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
     int max_sectors = 0;
     short max_sectors_short = 0;
     if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
+        /* sg returns a value in bytes */
         return max_sectors;
     } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors_short) == 0) {
-        return max_sectors_short;
+        /* block returns a value in sectors */
+        return max_sectors_short << BDRV_SECTOR_BITS;
     } else {
         return -errno;
     }
@@ -674,10 +676,10 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     struct stat st;
 
     if (!fstat(s->fd, &st)) {
-        if (S_ISBLK(st.st_mode)) {
+        if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
             int ret = hdev_get_max_transfer_length(bs, s->fd);
-            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
-                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
+            if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
+                bs->bl.max_transfer = pow2floor(ret);
             }
         }
     }
diff --git a/include/block/block.h b/include/block/block.h
index 8b0dcda..4e81f20 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -116,6 +116,7 @@ typedef struct HDGeometry {
 
 #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
                                      INT_MAX >> BDRV_SECTOR_BITS)
+#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
 
 /*
  * Allocation status flags
-- 
2.8.4

Re: [Qemu-devel] [PATCH v2 3/3] block: get max_transfer limit for char (scsi-generic) devices

[Fam Zheng]

On Thu, 01/19 21:51, Eric Farman wrote:
> Commit 6f607174 introduced a routine to get the maximum number
> of bytes for a single I/O transfer for block devices, however
> scsi generic devices are character devices, not block.  Add
> a condition for this, such that scsi generic devices can view
> the same data.
> 
> Some tweaking of data is required, because the block and sg
> ioctls return values in different scales (sectors versus
> bytes).  So adjust hdev_get_max_transfer_length such that it
> always returns a value in bytes.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  block/file-posix.c    | 10 ++++++----
>  include/block/block.h |  1 +
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 2115155..94068ca 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
>      int max_sectors = 0;
>      short max_sectors_short = 0;
>      if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
> +        /* sg returns a value in bytes */
>          return max_sectors;

The variable name is now misleading, maybe use "bytes" instead?

>      } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors_short) == 0) {
> -        return max_sectors_short;
> +        /* block returns a value in sectors */
> +        return max_sectors_short << BDRV_SECTOR_BITS;
>      } else {
>          return -errno;
>      }
> @@ -674,10 +676,10 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>      struct stat st;
>  
>      if (!fstat(s->fd, &st)) {
> -        if (S_ISBLK(st.st_mode)) {
> +        if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
>              int ret = hdev_get_max_transfer_length(bs, s->fd);
> -            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
> -                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
> +            if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> +                bs->bl.max_transfer = pow2floor(ret);
>              }
>          }
>      }
> diff --git a/include/block/block.h b/include/block/block.h
> index 8b0dcda..4e81f20 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -116,6 +116,7 @@ typedef struct HDGeometry {
>  
>  #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>                                       INT_MAX >> BDRV_SECTOR_BITS)
> +#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>  
>  /*
>   * Allocation status flags
> -- 
> 2.8.4
> 
> 

Re: [Qemu-devel] [PATCH v2 3/3] block: get max_transfer limit for char (scsi-generic) devices

[Fam Zheng]

On Fri, 01/20 17:31, Fam Zheng wrote:
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 2115155..94068ca 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
> >      int max_sectors = 0;
> >      short max_sectors_short = 0;
> >      if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
> > +        /* sg returns a value in bytes */
> >          return max_sectors;
> 
> The variable name is now misleading, maybe use "bytes" instead?

BTW patch 2 should already make the function return bytes consistently.  Doing
it here is meaningless code churn.

Fam

Re: [Qemu-devel] [PATCH v2 3/3] block: get max_transfer limit for char (scsi-generic) devices

[Eric Farman]

On 01/20/2017 04:53 AM, Fam Zheng wrote:
> On Fri, 01/20 17:31, Fam Zheng wrote:
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index 2115155..94068ca 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
>>>      int max_sectors = 0;
>>>      short max_sectors_short = 0;
>>>      if (bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
>>> +        /* sg returns a value in bytes */
>>>          return max_sectors;
>>
>> The variable name is now misleading, maybe use "bytes" instead?
>
> BTW patch 2 should already make the function return bytes consistently.  Doing
> it here is meaningless code churn.

Excellent points.  Spinning v3 now.

  - Eric