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

[Qemu-devel] [PATCH v3 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.

Changes:
 v2->v3:
  - Move byte/sector conversions to patch 2 [Fam Zheng]
  - Rename "max_sectors" when holding a byte value [Fam Zheng]
 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     | 19 +++++++++++--------
 hw/scsi/scsi-generic.c |  5 +++--
 include/block/block.h  |  1 +
 3 files changed, 15 insertions(+), 10 deletions(-)

-- 
2.8.4

[Qemu-devel] [PATCH v3 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 v3 2/3] block: Fix target variable of BLKSECTGET ioctl

[Eric Farman]

Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
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.)

Also, the two ioctl handlers return values in different scales (block
returns sectors, while sg returns bytes), so some tweaking of the outputs
is required such that hdev_get_max_transfer_length returns a value in a
consistent set of units.

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

diff --git a/block/file-posix.c b/block/file-posix.c
index 28b47d9..9f83725 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) {
-        return max_sectors;
+    int max_bytes = 0;
+    short max_sectors = 0;
+    if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
+        return max_bytes;
+    } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
+        return max_sectors << BDRV_SECTOR_BITS;
     } else {
         return -errno;
     }
@@ -672,9 +675,9 @@ 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);
-            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
-                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
+            int ret = hdev_get_max_transfer_length(bs, s->fd);
+            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

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

[Eric Farman]

We can get the maximum number of bytes for a single I/O transfer
from the BLKSECTGET ioctl, but we only perform this for block
devices.  scsi-generic devices are represented as character devices,
and so do not issue this today.  Update this, so that virtio-scsi
devices using the scsi-generic interface can return the same data.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9f83725..2134e0e 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -674,7 +674,7 @@ 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_BYTES) {
                 bs->bl.max_transfer = pow2floor(ret);
-- 
2.8.4

Re: [Qemu-devel] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Fam Zheng]

On Fri, 01/20 17:25, Eric Farman wrote:
> Changes:
>  v2->v3:
>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>  v1->v2:
>   - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Paolo Bonzini]

On 22/01/2017 15:29, Fam Zheng wrote:
> On Fri, 01/20 17:25, Eric Farman wrote:
>> Changes:
>>  v2->v3:
>>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>>  v1->v2:
>>   - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Looks good, who's merging this? :)

Paolo

Re: [Qemu-devel] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Fam Zheng]

On Tue, 01/24 12:23, Paolo Bonzini wrote:
> 
> 
> On 22/01/2017 15:29, Fam Zheng wrote:
> > On Fri, 01/20 17:25, Eric Farman wrote:
> >> Changes:
> >>  v2->v3:
> >>   - Move byte/sector conversions to patch 2 [Fam Zheng]
> >>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
> >>  v1->v2:
> >>   - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> Looks good, who's merging this? :)

All I know is not me :) Adding Kevin and Max who maintain block/file-posix.c.

Fam

Re: [Qemu-devel] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Eric Farman]

On 01/24/2017 07:09 AM, Fam Zheng wrote:
> On Tue, 01/24 12:23, Paolo Bonzini wrote:
>>
>>
>> On 22/01/2017 15:29, Fam Zheng wrote:
>>> On Fri, 01/20 17:25, Eric Farman wrote:
>>>> Changes:
>>>>  v2->v3:
>>>>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>>>>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>>>>  v1->v2:
>>>>   - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]
>>>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>
>> Looks good, who's merging this? :)
>
> All I know is not me :) Adding Kevin and Max who maintain block/file-posix.c.

Also not me :) But thank you for the review and consideration of this!

  - Eric

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[John Snow]

On 01/24/2017 07:09 AM, Fam Zheng wrote:
> On Tue, 01/24 12:23, Paolo Bonzini wrote:
>>
>>
>> On 22/01/2017 15:29, Fam Zheng wrote:
>>> On Fri, 01/20 17:25, Eric Farman wrote:
>>>> Changes:
>>>>  v2->v3:
>>>>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>>>>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>>>>  v1->v2:
>>>>   - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]
>>>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>
>> Looks good, who's merging this? :)
> 
> All I know is not me :) Adding Kevin and Max who maintain block/file-posix.c.
> 
> Fam
> 

Kevin's traveling for DevConf.cz, so how about Max? :)

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Max Reitz]

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

On 31.01.2017 12:37, John Snow wrote:
> 
> 
> On 01/24/2017 07:09 AM, Fam Zheng wrote:
>> On Tue, 01/24 12:23, Paolo Bonzini wrote:
>>>
>>>
>>> On 22/01/2017 15:29, Fam Zheng wrote:
>>>> On Fri, 01/20 17:25, Eric Farman wrote:
>>>>> Changes:
>>>>>  v2->v3:
>>>>>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>>>>>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>>>>>  v1->v2:
>>>>>   - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]
>>>>
>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>
>>> Looks good, who's merging this? :)
>>
>> All I know is not me :) Adding Kevin and Max who maintain block/file-posix.c.
>>
>> Fam
>>
> 
> Kevin's traveling for DevConf.cz, so how about Max? :)

Did Kevin tell you to write that? If so, tell him he's got to merge all
of my patches once he's back. :-P

Ooooh, I see, he's trying to get me to merge so many patches I'll have
to send my own pull request. Cleverrr. Verrry clever. ;-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

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

[Max Reitz]

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

On 20.01.2017 17:25, Eric Farman wrote:
> Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
> 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.)
> 
> Also, the two ioctl handlers return values in different scales (block
> returns sectors, while sg returns bytes), so some tweaking of the outputs
> is required such that hdev_get_max_transfer_length returns a value in a
> consistent set of units.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  block/file-posix.c    | 17 ++++++++++-------
>  include/block/block.h |  1 +
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 28b47d9..9f83725 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) {
> -        return max_sectors;
> +    int max_bytes = 0;
> +    short max_sectors = 0;
> +    if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
> +        return max_bytes;
> +    } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
> +        return max_sectors << BDRV_SECTOR_BITS;
>      } else {
>          return -errno;
>      }
> @@ -672,9 +675,9 @@ 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);
> -            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
> -                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
> +            int ret = hdev_get_max_transfer_length(bs, s->fd);
> +            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)

I'd rather like it the other way round (i.e.

#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX)
#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >>
BDRV_SECTOR_BITS)

), but I don't suppose I can interest you in a respin, can I? Would it
be OK with you if I changed you commit when I apply it?

Max

>  
>  /*
>   * Allocation status flags
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[John Snow]

On 01/31/2017 06:47 PM, Max Reitz wrote:
> On 31.01.2017 12:37, John Snow wrote:
>>
>>
>> On 01/24/2017 07:09 AM, Fam Zheng wrote:
>>> On Tue, 01/24 12:23, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 22/01/2017 15:29, Fam Zheng wrote:
>>>>> On Fri, 01/20 17:25, Eric Farman wrote:
>>>>>> Changes:
>>>>>>  v2->v3:
>>>>>>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>>>>>>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>>>>>>  v1->v2:
>>>>>>   - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]
>>>>>
>>>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>>>
>>>> Looks good, who's merging this? :)
>>>
>>> All I know is not me :) Adding Kevin and Max who maintain block/file-posix.c.
>>>
>>> Fam
>>>
>>
>> Kevin's traveling for DevConf.cz, so how about Max? :)
> 
> Did Kevin tell you to write that? If so, tell him he's got to merge all
> of my patches once he's back. :-P
> 

He didn't, and I am unaware of whatever your current arrangements are!

> Ooooh, I see, he's trying to get me to merge so many patches I'll have
> to send my own pull request. Cleverrr. Verrry clever. ;-)
> 
> Max
> 

I guess you two will sort it out between yourselves...

--js

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

[Eric Farman]

On 01/31/2017 07:25 PM, Max Reitz wrote:
> On 20.01.2017 17:25, Eric Farman wrote:
>> Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
>> 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.)
>>
>> Also, the two ioctl handlers return values in different scales (block
>> returns sectors, while sg returns bytes), so some tweaking of the outputs
>> is required such that hdev_get_max_transfer_length returns a value in a
>> consistent set of units.
>>
>> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
>> ---
>>  block/file-posix.c    | 17 ++++++++++-------
>>  include/block/block.h |  1 +
>>  2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 28b47d9..9f83725 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) {
>> -        return max_sectors;
>> +    int max_bytes = 0;
>> +    short max_sectors = 0;
>> +    if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
>> +        return max_bytes;
>> +    } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
>> +        return max_sectors << BDRV_SECTOR_BITS;
>>      } else {
>>          return -errno;
>>      }
>> @@ -672,9 +675,9 @@ 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);
>> -            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
>> -                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
>> +            int ret = hdev_get_max_transfer_length(bs, s->fd);
>> +            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)
>
> I'd rather like it the other way round (i.e.
>
> #define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX)
> #define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >>
> BDRV_SECTOR_BITS)
>
> ), but I don't suppose I can interest you in a respin, can I? Would it
> be OK with you if I changed you commit when I apply it?
>
> Max


That's far cleaner looking, so absolutely go ahead.  Thank you!

  - Eric

>
>>
>>  /*
>>   * Allocation status flags
>>
>
>

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Max Reitz]

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

On 20.01.2017 17:25, Eric Farman wrote:
> 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.
>
> Changes:
>  v2->v3:
>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>  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     | 19 +++++++++++--------
>  hw/scsi/scsi-generic.c |  5 +++--
>  include/block/block.h  |  1 +
>  3 files changed, 15 insertions(+), 10 deletions(-)

Thank you, I've applied the series to my block tree (with the macro
definitions in patch 2 switched as proposed):

https://github.com/XanClic/qemu/commits/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Max Reitz]

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

On 01.02.2017 20:55, Max Reitz wrote:
> On 20.01.2017 17:25, Eric Farman wrote:
>> 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.
>>
>> Changes:
>>  v2->v3:
>>   - Move byte/sector conversions to patch 2 [Fam Zheng]
>>   - Rename "max_sectors" when holding a byte value [Fam Zheng]
>>  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     | 19 +++++++++++--------
>>  hw/scsi/scsi-generic.c |  5 +++--
>>  include/block/block.h  |  1 +
>>  3 files changed, 15 insertions(+), 10 deletions(-)
> 
> Thank you, I've applied the series to my block tree (with the macro
> definitions in patch 2 switched as proposed):
> 
> https://github.com/XanClic/qemu/commits/block

Noticed now that Paolo has already merged this. Well, then out it goes.
Too bad the swap of BDRV_REQUEST_MAX_BYTES and BDRV_REQUEST_MAX_SECTORS
is not in his tree.

Well, I'll make it an own patch then.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v3 0/3] scsi-generic and BLKSECTGET

[Paolo Bonzini]

On 08/02/2017 23:33, Max Reitz wrote:
> Noticed now that Paolo has already merged this. Well, then out it goes.
> Too bad the swap of BDRV_REQUEST_MAX_BYTES and BDRV_REQUEST_MAX_SECTORS
> is not in his tree.
> 
> Well, I'll make it an own patch then.

Sorry, that was by mistake. :(

Paolo