diff mbox

[2/2] block: align bounce buffers to page

Message ID 1430746944-27347-3-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev May 4, 2015, 1:42 p.m. UTC 
The following sequence
    int fd = open(argv[1], O_RDWR | O_CREAT | O_DIRECT, 0644);
    for (i = 0; i < 100000; i++)
            write(fd, buf, 4096);
performs 5% better if buf is aligned to 4096 bytes.

The difference is quite reliable.

On the other hand we do not want at the moment to enforce bounce
buffering if guest request is aligned to 512 bytes.

The patch changes default bounce buffer optimal alignment to
MAX(page size, 4k). 4k is chosen as maximal known sector size on real
HDD.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c           |  8 ++++----
 block/io.c        |  2 +-
 block/raw-posix.c | 14 ++++++++------
 3 files changed, 13 insertions(+), 11 deletions(-)

Comments

Stefan Hajnoczi May 11, 2015, 2:54 p.m. UTC | #1 
On Mon, May 04, 2015 at 04:42:24PM +0300, Denis V. Lunev wrote:
> @@ -726,7 +727,8 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  
>      raw_probe_alignment(bs, s->fd, errp);
>      bs->bl.min_mem_alignment = s->buf_align;
> -    bs->bl.opt_mem_alignment = s->buf_align;
> +    if (bs->bl.min_mem_alignment > bs->bl.opt_mem_alignment)
> +        bs->bl.opt_mem_alignment = bs->bl.min_mem_alignment;

QEMU coding style uses {} even when the if statement body is only one
line.
Eric Blake May 11, 2015, 3:32 p.m. UTC | #2 
On 05/11/2015 08:54 AM, Stefan Hajnoczi wrote:
> On Mon, May 04, 2015 at 04:42:24PM +0300, Denis V. Lunev wrote:
>> @@ -726,7 +727,8 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>  
>>      raw_probe_alignment(bs, s->fd, errp);
>>      bs->bl.min_mem_alignment = s->buf_align;
>> -    bs->bl.opt_mem_alignment = s->buf_align;
>> +    if (bs->bl.min_mem_alignment > bs->bl.opt_mem_alignment)
>> +        bs->bl.opt_mem_alignment = bs->bl.min_mem_alignment;
> 
> QEMU coding style uses {} even when the if statement body is only one
> line.

scripts/checkpatch.pl helps you check for this and other style issues.
Denis V. Lunev May 11, 2015, 3:40 p.m. UTC | #3 
On 11/05/15 18:32, Eric Blake wrote:
> On 05/11/2015 08:54 AM, Stefan Hajnoczi wrote:
>> On Mon, May 04, 2015 at 04:42:24PM +0300, Denis V. Lunev wrote:
>>> @@ -726,7 +727,8 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>>   
>>>       raw_probe_alignment(bs, s->fd, errp);
>>>       bs->bl.min_mem_alignment = s->buf_align;
>>> -    bs->bl.opt_mem_alignment = s->buf_align;
>>> +    if (bs->bl.min_mem_alignment > bs->bl.opt_mem_alignment)
>>> +        bs->bl.opt_mem_alignment = bs->bl.min_mem_alignment;
>> QEMU coding style uses {} even when the if statement body is only one
>> line.
> scripts/checkpatch.pl helps you check for this and other style issues.
>
yep, I have forgotten to run this before sending this time :(
diff mbox

Patch

diff --git a/block.c b/block.c
index e293907..325f727 100644
--- a/block.c
+++ b/block.c
@@ -106,8 +106,8 @@  int is_windows_drive(const char *filename)
 size_t bdrv_opt_mem_align(BlockDriverState *bs)
 {
     if (!bs || !bs->drv) {
-        /* 4k should be on the safe side */
-        return 4096;
+        /* page size or 4k (hdd sector size) should be on the safe side */
+        return MAX(4096, getpagesize());
     }
 
     return bs->bl.opt_mem_alignment;
@@ -116,8 +116,8 @@  size_t bdrv_opt_mem_align(BlockDriverState *bs)
 size_t bdrv_min_mem_align(BlockDriverState *bs)
 {
     if (!bs || !bs->drv) {
-        /* 4k should be on the safe side */
-        return 4096;
+        /* page size or 4k (hdd sector size) should be on the safe side */
+        return MAX(4096, getpagesize());
     }
 
     return bs->bl.min_mem_alignment;
diff --git a/block/io.c b/block/io.c
index 908a3d1..071652c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -205,7 +205,7 @@  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
         bs->bl.opt_mem_alignment = bs->file->bl.opt_mem_alignment;
     } else {
         bs->bl.min_mem_alignment = 512;
-        bs->bl.opt_mem_alignment = 512;
+        bs->bl.opt_mem_alignment = getpagesize();
     }
 
     if (bs->backing_hd) {
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 7083924..04f3d4e 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -301,6 +301,7 @@  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     char *buf;
+    size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
 
     /* For /dev/sg devices the alignment is not really used.
        With buffered I/O, we don't have any restrictions. */
@@ -330,9 +331,9 @@  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     /* If we could not get the sizes so far, we can only guess them */
     if (!s->buf_align) {
         size_t align;
-        buf = qemu_memalign(MAX_BLOCKSIZE, 2 * MAX_BLOCKSIZE);
-        for (align = 512; align <= MAX_BLOCKSIZE; align <<= 1) {
-            if (raw_is_io_aligned(fd, buf + align, MAX_BLOCKSIZE)) {
+        buf = qemu_memalign(max_align, 2 * max_align);
+        for (align = 512; align <= max_align; align <<= 1) {
+            if (raw_is_io_aligned(fd, buf + align, max_align)) {
                 s->buf_align = align;
                 break;
             }
@@ -342,8 +343,8 @@  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 
     if (!bs->request_alignment) {
         size_t align;
-        buf = qemu_memalign(s->buf_align, MAX_BLOCKSIZE);
-        for (align = 512; align <= MAX_BLOCKSIZE; align <<= 1) {
+        buf = qemu_memalign(s->buf_align, max_align);
+        for (align = 512; align <= max_align; align <<= 1) {
             if (raw_is_io_aligned(fd, buf, align)) {
                 bs->request_alignment = align;
                 break;
@@ -726,7 +727,8 @@  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 
     raw_probe_alignment(bs, s->fd, errp);
     bs->bl.min_mem_alignment = s->buf_align;
-    bs->bl.opt_mem_alignment = s->buf_align;
+    if (bs->bl.min_mem_alignment > bs->bl.opt_mem_alignment)
+        bs->bl.opt_mem_alignment = bs->bl.min_mem_alignment;
 }
 
 static int check_for_dasd(int fd)