With virtio-scsi, logical block size should never be larger than
physical block size. From an ATA/SCSI perspective, it makes no sense
to have the logical block size greater than the physical block size,
and it cannot even be effectively expressed in the command set. The
whole point of adding the physical block size to the ATA/SCSI command
set was to communicate a desire for a larger block size (than logical),
while maintaining backwards compatibility with legacy 512 byte block
size.

This was found by setting logical_block_size > physical_block_size in
the QEMU command line, and discovering that it confuses Windows VMs -
fsutil reports both physical and logical block sizes are equal to the
logical size.

Example QEMU option:

  -device scsi-hd,drive=drive-scsi0,id=disk1,bus=scsi.0,
          physical_block_size=512,logical_block_size=4096

Windows Server 2012 R2 VM:

  C:\Users\Administrator>fsutil fsinfo ntfsinfo F:
  ...
  Bytes Per Sector  :               4096
  Bytes Per Physical Sector :       4096
  Bytes Per Cluster :               4096
  Bytes Per FileRecord Segment    : 4096
  ...

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 hw/scsi/scsi-disk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 6e841fb..2a4f8c5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2329,6 +2329,14 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
 
     blkconf_serial(&s->qdev.conf, &s->serial);
     blkconf_blocksizes(&s->qdev.conf);
+
+    if (s->qdev.conf.logical_block_size >
+        s->qdev.conf.physical_block_size) {
+        error_setg(errp,
+                   "logical_block_size > physical_block_size not supported");
+        return;
+    }
+
     if (dev->type == TYPE_DISK) {
         blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
         if (err) {
-- 
1.8.3.1

On 16/10/2017 22:17, Mark Kanda wrote:
> With virtio-scsi, logical block size should never be larger than
> physical block size. From an ATA/SCSI perspective, it makes no sense
> to have the logical block size greater than the physical block size,
> and it cannot even be effectively expressed in the command set. The
> whole point of adding the physical block size to the ATA/SCSI command
> set was to communicate a desire for a larger block size (than logical),
> while maintaining backwards compatibility with legacy 512 byte block
> size.
> 
> This was found by setting logical_block_size > physical_block_size in
> the QEMU command line, and discovering that it confuses Windows VMs -
> fsutil reports both physical and logical block sizes are equal to the
> logical size.

It doesn't confuse Windows VMs; physical block size is expressed as a
number n such that physical_block_size = 2^n * logical_block_size.  So
if you specify an invalid configuration (which as you say "cannot be
expressed" in SBC) all QEMU can do is set n=0.

fsutil therefore prints exactly what QEMU says in READ CAPACITY. :)

I've adjusted a little bit the commit message and queued the patch, thanks!

Paolo

> Example QEMU option:
> 
>   -device scsi-hd,drive=drive-scsi0,id=disk1,bus=scsi.0,
>           physical_block_size=512,logical_block_size=4096
> 
> Windows Server 2012 R2 VM:
> 
>   C:\Users\Administrator>fsutil fsinfo ntfsinfo F:
>   ...
>   Bytes Per Sector  :               4096
>   Bytes Per Physical Sector :       4096
>   Bytes Per Cluster :               4096
>   Bytes Per FileRecord Segment    : 4096
>   ...
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  hw/scsi/scsi-disk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 6e841fb..2a4f8c5 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2329,6 +2329,14 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>  
>      blkconf_serial(&s->qdev.conf, &s->serial);
>      blkconf_blocksizes(&s->qdev.conf);
> +
> +    if (s->qdev.conf.logical_block_size >
> +        s->qdev.conf.physical_block_size) {
> +        error_setg(errp,
> +                   "logical_block_size > physical_block_size not supported");
> +        return;
> +    }
> +
>      if (dev->type == TYPE_DISK) {
>          blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
>          if (err) {
>