[Qemu-devel] [RFC PATCH 0/3] block: add support for 4k logical blocks

[Qemu-devel] [RFC PATCH 0/3] block: add support for 4k logical blocks

[Paolo Bonzini]

This series adds support for 4k logical blocks by bouncing requests
unless the host's logical_block_size is also 4096.

Paolo Bonzini (3):
  block: add bdrv_get_alignment, use it
  raw: implement raw_get_alignment
  block: do not rely on the buffer alignment passed to the guest

 block.c           |   34 +++++++++++++++++++++++---
 block.h           |    3 +-
 block/raw-posix.c |   68 ++++++++++++++++++++++++++++++++++++++++++-----------
 block/raw-win32.c |   45 +++++++++++++++++++++++++++++++++++
 block_int.h       |    1 +
 hw/ide/core.c     |    2 +-
 hw/scsi-disk.c    |    2 +-
 hw/scsi-generic.c |    1 -
 hw/virtio-blk.c   |    2 +-
 9 files changed, 135 insertions(+), 23 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [RFC PATCH 1/3] block: add bdrv_get_alignment, use it

[Paolo Bonzini]

We cannot simply use the guest logical block size on the host; we need
to ask the protocol driver about the minimum required alignment.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c           |   26 ++++++++++++++++++++++++--
 block.h           |    1 +
 block/raw-posix.c |    3 ++-
 block_int.h       |    1 +
 4 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index d015887..aba92ad 100644
--- a/block.c
+++ b/block.c
@@ -476,7 +476,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
     bs->sg = 0;
     bs->open_flags = flags;
     bs->growable = 0;
-    bs->buffer_alignment = 512;
+    bs->buffer_alignment = 0;
 
     pstrcpy(bs->filename, sizeof(bs->filename), filename);
     bs->backing_file[0] = '\0';
@@ -1361,6 +1361,27 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
 }
 
 /**
+ * Required buffer alignment for a file or device. Return < 0 if error or unknown.
+ */
+int bdrv_get_alignment(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (bs->buffer_alignment == 0) {
+        if (drv->bdrv_get_alignment) {
+            bs->buffer_alignment = drv->bdrv_get_alignment(bs);
+        } else if (bs->file) {
+            bs->buffer_alignment = bdrv_get_alignment(bs->file);
+        } else {
+            bs->buffer_alignment = BDRV_SECTOR_SIZE;
+        }
+    }
+    return bs->buffer_alignment;
+}
+
+/**
  * Length of a file in bytes. Return < 0 if error or unknown.
  */
 int64_t bdrv_getlength(BlockDriverState *bs)
@@ -3025,7 +3046,8 @@ void bdrv_set_buffer_alignment(BlockDriverState *bs, int align)
 
 void *qemu_blockalign(BlockDriverState *bs, size_t size)
 {
-    return qemu_memalign((bs && bs->buffer_alignment) ? bs->buffer_alignment : 512, size);
+    int alignment = bs ? bdrv_get_alignment(bs) : 512;
+    return qemu_memalign(alignment, size);
 }
 
 void bdrv_set_dirty_tracking(BlockDriverState *bs, int enable)
diff --git a/block.h b/block.h
index a826059..5fa632c 100644
--- a/block.h
+++ b/block.h
@@ -294,6 +294,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
                     const char *base_filename, const char *base_fmt,
                     char *options, uint64_t img_size, int flags);
 
+int bdrv_get_alignment(BlockDriverState *bs);
 void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index a3de373..3482fc8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -301,10 +301,11 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
  */
 static int qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
 {
+    int alignment = bdrv_get_alignment(bs);
     int i;
 
     for (i = 0; i < qiov->niov; i++) {
-        if ((uintptr_t) qiov->iov[i].iov_base % bs->buffer_alignment) {
+        if ((uintptr_t) qiov->iov[i].iov_base % alignment) {
             return 0;
         }
     }
diff --git a/block_int.h b/block_int.h
index 77c0187..6caf063 100644
--- a/block_int.h
+++ b/block_int.h
@@ -113,6 +113,7 @@ struct BlockDriver {
 
     const char *protocol_name;
     int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
+    int (*bdrv_get_alignment)(BlockDriverState *bs);
     int64_t (*bdrv_getlength)(BlockDriverState *bs);
     int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
     int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
-- 
1.7.7.1

[Qemu-devel] [RFC PATCH 2/3] raw: implement raw_get_alignment

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/raw-posix.c |   65 ++++++++++++++++++++++++++++++++++++++++++----------
 block/raw-win32.c |   45 ++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 13 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 3482fc8..f6747ad 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -279,22 +279,56 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
     return raw_open_common(bs, filename, flags, 0);
 }
 
-/* XXX: use host sector size if necessary with:
+static int raw_get_alignment(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    unsigned int sector_size;
+    int ret;
+
+    ret = fd_open(bs);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* For block devices, try to get the actual sector size even if we
+     * do not need it, so that it can be passed down to the guest.
+     */
+#ifdef BLKSSZGET
+    if (ioctl(s->fd, BLKSSZGET, &sector_size)) {
+        return sector size;
+    }
+#endif
+#ifdef DKIOCGETBLOCKSIZE
+    if (ioctl(s->fd, DKIOCGETBLOCKSIZE, &sector_size)) {
+        return sector_size;
+    }
+#endif
 #ifdef DIOCGSECTORSIZE
-        {
-            unsigned int sectorsize = 512;
-            if (!ioctl(fd, DIOCGSECTORSIZE, &sectorsize) &&
-                sectorsize > bufsize)
-                bufsize = sectorsize;
-        }
+    if (ioctl(s->fd, DIOCGSECTORSIZE, &sector_size)) {
+        return sector_size;
+    }
 #endif
-#ifdef CONFIG_COCOA
-        uint32_t blockSize = 512;
-        if ( !ioctl( fd, DKIOCGETBLOCKSIZE, &blockSize ) && blockSize > bufsize) {
-            bufsize = blockSize;
+
+    /* If we could not get the size so far, we can only guess it if
+     * the file was opened with O_DIRECT.  If not, just return the
+     * minimal size.
+     *
+     * For /dev/sg devices the alignment is not really used, so return
+     * a dummy value for them too.
+     */
+    if (bs->sg || !s->aligned_buf) {
+        return 512;
+    }
+
+    for (sector_size = 512; sector_size < MAX_BLOCKSIZE; sector_size <<= 1) {
+        /* The buffer must be aligned to sector_size, but not sector_size*2.  */
+        if (pread(s->fd, s->aligned_buf + sector_size, sector_size, 0) >= 0) {
+            break;
         }
-#endif
-*/
+    }
+    return sector_size;
+}
+
 
 /*
  * Check if all memory in this vector is sector aligned.
@@ -642,6 +676,7 @@ static BlockDriver bdrv_file = {
 
     .bdrv_truncate = raw_truncate,
     .bdrv_getlength = raw_getlength,
+    .bdrv_get_alignment = raw_get_alignment,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
@@ -910,6 +945,7 @@ static BlockDriver bdrv_host_device = {
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength	= raw_getlength,
+    .bdrv_get_alignment = raw_get_alignment,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
@@ -1029,6 +1065,7 @@ static BlockDriver bdrv_host_floppy = {
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength	= raw_getlength,
+    .bdrv_get_alignment = raw_get_alignment,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
@@ -1128,6 +1165,7 @@ static BlockDriver bdrv_host_cdrom = {
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength     = raw_getlength,
+    .bdrv_get_alignment = raw_get_alignment,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
@@ -1247,6 +1285,7 @@ static BlockDriver bdrv_host_cdrom = {
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength     = raw_getlength,
+    .bdrv_get_alignment = raw_get_alignment,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
diff --git a/block/raw-win32.c b/block/raw-win32.c
index e4b0b75..f033037 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -96,6 +96,18 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
         overlapped |= FILE_FLAG_NO_BUFFERING;
     if (!(flags & BDRV_O_CACHE_WB))
         overlapped |= FILE_FLAG_WRITE_THROUGH;
+
+    if (filename[0] && filename[1] == ':') {
+        snprintf(s->drive_path, sizeof(s->drive_path), "%c:\\", filename[0]);
+    } else if (filename[0] == '\\' && filename[1] == '\\') {
+        s->drive_path[0] = 0;
+    } else {
+        /* Relative path.  */
+        char buf[MAX_PATH];
+        GetCurrentDirectory(MAX_PATH, buf);
+        snprintf(s->drive_path, sizeof(s->drive_path), "%c:\\", buf[0]);
+    }
+
     s->hfile = CreateFile(filename, access_flags,
                           FILE_SHARE_READ, NULL,
                           OPEN_EXISTING, overlapped, NULL);
@@ -184,6 +196,37 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset)
     return 0;
 }
 
+static int raw_get_alignment(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    DWORD sectorsPerCluster, freeClusters, totalClusters, count;
+    DISK_GEOMETRY_EX dg;
+    BOOL status;
+
+    dg.Geometry.BytesPerSector = 512;
+    switch(s->type) {
+    case FTYPE_CD:
+        return 2048;
+    case FTYPE_HARDDISK:
+        status = DeviceIoControl(s->hfile, IOCTL_DISK_GET_DRIVE_GEOMETRY_EX,
+                                 NULL, 0, &dg, sizeof(dg), &count, NULL);
+        if (status != 0) {
+            break;
+        }
+        /* fall through */
+    case FTYPE_FILE:
+        if (s->drive_path[0]) {
+            GetDiskFreeSpace(s->drive_path, &sectorsPerCluster,
+                             &dg.Geometry.BytesPerSector,
+                             &freeClusters, &totalClusters);
+        }
+        break;
+    default:
+        return -EIO;
+    }
+    return dg.Geometry.BytesPerSector;
+}
+
 static int64_t raw_getlength(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
@@ -288,6 +331,7 @@ static BlockDriver bdrv_file = {
 
     .bdrv_truncate	= raw_truncate,
     .bdrv_getlength	= raw_getlength,
+    .bdrv_get_alignment	= raw_get_alignment,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 
@@ -418,6 +462,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_co_flush_to_disk  = raw_flush,
 
     .bdrv_getlength	= raw_getlength,
+    .bdrv_get_alignment	= raw_get_alignment,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
 };
-- 
1.7.7.1

[Qemu-devel] [RFC PATCH 3/3] block: do not rely on the buffer alignment passed to the guest

[Paolo Bonzini]

The guest alignment should not override the host's.  Instead, just use
the bdrv_set_buffer_alignment calls to print a warning about possible
performance degradations due to insufficiently-aligned guest buffers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c           |    8 ++++++--
 block.h           |    2 +-
 hw/ide/core.c     |    2 +-
 hw/scsi-disk.c    |    2 +-
 hw/scsi-generic.c |    1 -
 hw/virtio-blk.c   |    2 +-
 6 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index aba92ad..cd11344 100644
--- a/block.c
+++ b/block.c
@@ -3039,9 +3039,13 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
     return NULL;
 }
 
-void bdrv_set_buffer_alignment(BlockDriverState *bs, int align)
+void bdrv_check_buffer_alignment(BlockDriverState *bs, int align)
 {
-    bs->buffer_alignment = align;
+    if ((bs->open_flags & BDRV_O_NOCACHE) && bdrv_get_alignment(bs) > align) {
+        error_report("Host block size is %d, guest block size is %d.\n"
+		     "cache=none may cause performance degradation.",
+		     bdrv_get_alignment(bs), align);
+    }
 }
 
 void *qemu_blockalign(BlockDriverState *bs, size_t size)
diff --git a/block.h b/block.h
index 5fa632c..386284c 100644
--- a/block.h
+++ b/block.h
@@ -295,7 +295,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
                     char *options, uint64_t img_size, int flags);
 
 int bdrv_get_alignment(BlockDriverState *bs);
-void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
+void bdrv_check_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
 
 #define BDRV_SECTORS_PER_DIRTY_CHUNK 2048
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 93a1a68..3b8cdb2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1863,7 +1863,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
     s->smart_selftest_count = 0;
     if (kind == IDE_CD) {
         bdrv_set_dev_ops(bs, &ide_cd_block_ops, s);
-        bdrv_set_buffer_alignment(bs, 2048);
+        bdrv_check_buffer_alignment(bs, 2048);
     } else {
         if (!bdrv_is_inserted(s->bs)) {
             error_report("Device needs media, but drive is empty");
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 673948c..946d670 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1553,7 +1553,7 @@ static int scsi_initfn(SCSIDevice *dev)
     if (s->removable) {
         bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_cd_block_ops, s);
     }
-    bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
+    bdrv_check_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
 
     bdrv_iostatus_enable(s->qdev.conf.bs);
     add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 9594cc1..c681a37 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -188,7 +188,6 @@ static void scsi_read_complete(void * opaque, int ret)
             s->blocksize = ldl_be_p(&r->buf[8]);
             s->max_lba = ldq_be_p(&r->buf[0]);
         }
-        bdrv_set_buffer_alignment(s->conf.bs, s->blocksize);
 
         scsi_req_data(&r->req, len);
         if (!r->req.io_canceled) {
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 91c92b2..ddc8b15 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -624,7 +624,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
     register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
                     virtio_blk_save, virtio_blk_load, s);
     bdrv_set_dev_ops(s->bs, &virtio_block_ops, s);
-    bdrv_set_buffer_alignment(s->bs, conf->logical_block_size);
+    bdrv_check_buffer_alignment(s->bs, conf->logical_block_size);
 
     bdrv_iostatus_enable(s->bs);
     add_boot_device_path(conf->bootindex, dev, "/disk@0,0");
-- 
1.7.7.1

Re: [Qemu-devel] [RFC PATCH 0/3] block: add support for 4k logical blocks

[Mark Wu]

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

I don't understand this series very well.  So could you help me answer the
following questions?
1. This patch aims to add 4k logical support for qemu running on a host
with 4k logical block size, right? For guest, we can use
logical_block_size=4096 to achieve that even on a host with the
logical_block_size of 512. Am I right?

2. Can we just call bdrv_get_alignment in bdrv_open_common once and use the
stored buffer_alignment for future usage instead of always
calling bdrv_get_alignment in qemu_blockalign/qiov_is_aligned.  What's the
benefit of the dynamic way?

Thanks.

2011/11/24 Paolo Bonzini <pbonzini@redhat.com>

> This series adds support for 4k logical blocks by bouncing requests
> unless the host's logical_block_size is also 4096.
>
> Paolo Bonzini (3):
>  block: add bdrv_get_alignment, use it
>  raw: implement raw_get_alignment
>  block: do not rely on the buffer alignment passed to the guest
>
>  block.c           |   34 +++++++++++++++++++++++---
>  block.h           |    3 +-
>  block/raw-posix.c |   68
> ++++++++++++++++++++++++++++++++++++++++++-----------
>  block/raw-win32.c |   45 +++++++++++++++++++++++++++++++++++
>  block_int.h       |    1 +
>  hw/ide/core.c     |    2 +-
>  hw/scsi-disk.c    |    2 +-
>  hw/scsi-generic.c |    1 -
>  hw/virtio-blk.c   |    2 +-
>  9 files changed, 135 insertions(+), 23 deletions(-)
>
> --
> 1.7.7.1
>
>
>

[-- Attachment #2: Type: text/html, Size: 2439 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/3] block: add support for 4k logical blocks

[Paolo Bonzini]

On 11/25/2011 08:26 AM, Mark Wu wrote:
> 1. This patch aims to add 4k logical support for qemu running on a host
> with 4k logical block size, right?

No, it adds support for 512b logical block sizes running on a host with 
4k logical block size and cache=none.  This is suboptimal as it requires 
bounce buffers, but it can happen with migration and until libvirt 
provides a knob for the guest's logical block size.

> For guest, we can use
> logical_block_size=4096 to achieve that even on a host with the
> logical_block_size of 512. Am I right?

Yes.

> 2. Can we just call bdrv_get_alignment in bdrv_open_common once and use
> the stored buffer_alignment for future usage instead of always
> calling bdrv_get_alignment in qemu_blockalign/qiov_is_aligned.  What's
> the benefit of the dynamic way?

Yes, I think it's possible.

Paolo

Re: [Qemu-devel] [RFC PATCH 0/3] block: add support for 4k logical blocks

[Christoph Hellwig]

On Fri, Nov 25, 2011 at 09:27:43AM +0100, Paolo Bonzini wrote:
> No, it adds support for 512b logical block sizes running on a host with 4k 
> logical block size and cache=none.  This is suboptimal as it requires 
> bounce buffers, but it can happen with migration and until libvirt provides 
> a knob for the guest's logical block size.

It's also highly dangerous becausw the fs may assume a sector is written
atomically, but now it isn't.  If at all I would recommend to only support
this case read only.

Re: [Qemu-devel] [RFC PATCH 0/3] block: add support for 4k logical blocks

[Mark Wu]

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

Ahh, I got it!
Thanks.

2011/11/25 Paolo Bonzini <pbonzini@redhat.com>

> On 11/25/2011 08:26 AM, Mark Wu wrote:
>
>> 1. This patch aims to add 4k logical support for qemu running on a host
>> with 4k logical block size, right?
>>
>
> No, it adds support for 512b logical block sizes running on a host with 4k
> logical block size and cache=none.  This is suboptimal as it requires
> bounce buffers, but it can happen with migration and until libvirt provides
> a knob for the guest's logical block size.
>
>
>  For guest, we can use
>> logical_block_size=4096 to achieve that even on a host with the
>> logical_block_size of 512. Am I right?
>>
>
> Yes.
>
>
>  2. Can we just call bdrv_get_alignment in bdrv_open_common once and use
>> the stored buffer_alignment for future usage instead of always
>> calling bdrv_get_alignment in qemu_blockalign/qiov_is_**aligned.  What's
>> the benefit of the dynamic way?
>>
>
> Yes, I think it's possible.
>
> Paolo
>

[-- Attachment #2: Type: text/html, Size: 1736 bytes --]