[Qemu-devel] [PATCH for-2.1 0/2] raw-posix: Fail gracefully if no working alignment is found

[Qemu-devel] [PATCH for-2.1 0/2] raw-posix: Fail gracefully if no working alignment is found

[Kevin Wolf]

Kevin Wolf (2):
  block: Add Error argument to bdrv_refresh_limits()
  raw-posix: Fail gracefully if no working alignment is found

 block.c                   | 33 +++++++++++++++++++++++----------
 block/iscsi.c             |  3 +--
 block/qcow2.c             |  4 +---
 block/qed.c               |  4 +---
 block/raw-posix.c         | 39 ++++++++++++++++++++++++++++-----------
 block/raw_bsd.c           |  3 +--
 block/stream.c            |  2 +-
 block/vmdk.c              |  4 +---
 include/block/block.h     |  2 +-
 include/block/block_int.h |  2 +-
 10 files changed, 59 insertions(+), 37 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] block: Add Error argument to bdrv_refresh_limits()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 33 +++++++++++++++++++++++----------
 block/iscsi.c             |  3 +--
 block/qcow2.c             |  4 +---
 block/qed.c               |  4 +---
 block/raw-posix.c         |  4 +---
 block/raw_bsd.c           |  3 +--
 block/stream.c            |  2 +-
 block/vmdk.c              |  4 +---
 include/block/block.h     |  2 +-
 include/block/block_int.h |  2 +-
 10 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/block.c b/block.c
index 3e252a2..8cf519b 100644
--- a/block.c
+++ b/block.c
@@ -508,19 +508,24 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     return ret;
 }
 
-int bdrv_refresh_limits(BlockDriverState *bs)
+void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BlockDriver *drv = bs->drv;
+    Error *local_err = NULL;
 
     memset(&bs->bl, 0, sizeof(bs->bl));
 
     if (!drv) {
-        return 0;
+        return;
     }
 
     /* Take some limits from the children as a default */
     if (bs->file) {
-        bdrv_refresh_limits(bs->file);
+        bdrv_refresh_limits(bs->file, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
         bs->bl.opt_transfer_length = bs->file->bl.opt_transfer_length;
         bs->bl.opt_mem_alignment = bs->file->bl.opt_mem_alignment;
     } else {
@@ -528,7 +533,11 @@ int bdrv_refresh_limits(BlockDriverState *bs)
     }
 
     if (bs->backing_hd) {
-        bdrv_refresh_limits(bs->backing_hd);
+        bdrv_refresh_limits(bs->backing_hd, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
         bs->bl.opt_transfer_length =
             MAX(bs->bl.opt_transfer_length,
                 bs->backing_hd->bl.opt_transfer_length);
@@ -539,10 +548,8 @@ int bdrv_refresh_limits(BlockDriverState *bs)
 
     /* Then let the driver override it */
     if (drv->bdrv_refresh_limits) {
-        return drv->bdrv_refresh_limits(bs);
+        drv->bdrv_refresh_limits(bs, errp);
     }
-
-    return 0;
 }
 
 /*
@@ -993,7 +1000,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         goto free_and_fail;
     }
 
-    bdrv_refresh_limits(bs);
+    bdrv_refresh_limits(bs, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto free_and_fail;
+    }
+
     assert(bdrv_opt_mem_align(bs) != 0);
     assert((bs->request_alignment != 0) || bs->sg);
     return 0;
@@ -1154,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
                     bs->backing_blocker);
 out:
-    bdrv_refresh_limits(bs);
+    bdrv_refresh_limits(bs, NULL);
 }
 
 /*
@@ -1778,7 +1791,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
                                               BDRV_O_CACHE_WB);
     reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
 
-    bdrv_refresh_limits(reopen_state->bs);
+    bdrv_refresh_limits(reopen_state->bs, NULL);
 }
 
 /*
diff --git a/block/iscsi.c b/block/iscsi.c
index f3e83e2..a7bb697 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1450,7 +1450,7 @@ static void iscsi_close(BlockDriverState *bs)
     memset(iscsilun, 0, sizeof(IscsiLun));
 }
 
-static int iscsi_refresh_limits(BlockDriverState *bs)
+static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     IscsiLun *iscsilun = bs->opaque;
 
@@ -1475,7 +1475,6 @@ static int iscsi_refresh_limits(BlockDriverState *bs)
     }
     bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len,
                                                  iscsilun);
-    return 0;
 }
 
 /* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in
diff --git a/block/qcow2.c b/block/qcow2.c
index b0faa69..445ead4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -855,13 +855,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 }
 
-static int qcow2_refresh_limits(BlockDriverState *bs)
+static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
 
     bs->bl.write_zeroes_alignment = s->cluster_sectors;
-
-    return 0;
 }
 
 static int qcow2_set_key(BlockDriverState *bs, const char *key)
diff --git a/block/qed.c b/block/qed.c
index cd4872b..7944832 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -528,13 +528,11 @@ out:
     return ret;
 }
 
-static int bdrv_qed_refresh_limits(BlockDriverState *bs)
+static void bdrv_qed_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVQEDState *s = bs->opaque;
 
     bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
-
-    return 0;
 }
 
 /* We have nothing to do for QED reopen, stubs just return
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2bcc73d..ef497b2 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -615,14 +615,12 @@ static void raw_reopen_abort(BDRVReopenState *state)
     state->opaque = NULL;
 }
 
-static int raw_refresh_limits(BlockDriverState *bs)
+static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
 
     raw_probe_alignment(bs);
     bs->bl.opt_mem_alignment = s->buf_align;
-
-    return 0;
 }
 
 static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 492f58d..f82f4c2 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -94,10 +94,9 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return bdrv_get_info(bs->file, bdi);
 }
 
-static int raw_refresh_limits(BlockDriverState *bs)
+static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     bs->bl = bs->file->bl;
-    return 0;
 }
 
 static int raw_truncate(BlockDriverState *bs, int64_t offset)
diff --git a/block/stream.c b/block/stream.c
index 34de8ba..cdea3e8 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -76,7 +76,7 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
         bdrv_unref(unused);
     }
 
-    bdrv_refresh_limits(top);
+    bdrv_refresh_limits(top, NULL);
 }
 
 static void coroutine_fn stream_run(void *opaque)
diff --git a/block/vmdk.c b/block/vmdk.c
index 27a78da..0517bba 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -938,7 +938,7 @@ fail:
 }
 
 
-static int vmdk_refresh_limits(BlockDriverState *bs)
+static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVVmdkState *s = bs->opaque;
     int i;
@@ -950,8 +950,6 @@ static int vmdk_refresh_limits(BlockDriverState *bs)
                     s->extents[i].cluster_sectors);
         }
     }
-
-    return 0;
 }
 
 static int get_whole_cluster(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index 32d3676..f08471d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -278,7 +278,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset);
 int64_t bdrv_getlength(BlockDriverState *bs);
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
-int bdrv_refresh_limits(BlockDriverState *bs);
+void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
 int bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f6c3bef..7b541a0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -240,7 +240,7 @@ struct BlockDriver {
     int (*bdrv_debug_resume)(BlockDriverState *bs, const char *tag);
     bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
 
-    int (*bdrv_refresh_limits)(BlockDriverState *bs);
+    void (*bdrv_refresh_limits)(BlockDriverState *bs, Error **errp);
 
     /*
      * Returns 1 if newly created images are guaranteed to contain only
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] block: Add Error argument to bdrv_refresh_limits()

[Eric Blake]

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

On 07/16/2014 09:48 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 33 +++++++++++++++++++++++----------
>  block/iscsi.c             |  3 +--
>  block/qcow2.c             |  4 +---
>  block/qed.c               |  4 +---
>  block/raw-posix.c         |  4 +---
>  block/raw_bsd.c           |  3 +--
>  block/stream.c            |  2 +-
>  block/vmdk.c              |  4 +---
>  include/block/block.h     |  2 +-
>  include/block/block_int.h |  2 +-
>  10 files changed, 32 insertions(+), 29 deletions(-)
> 

>      /* Take some limits from the children as a default */
>      if (bs->file) {
> -        bdrv_refresh_limits(bs->file);
> +        bdrv_refresh_limits(bs->file, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }

Nice that you are no longer ignoring failure from the child.


> @@ -1154,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>      bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
>                      bs->backing_blocker);
>  out:
> -    bdrv_refresh_limits(bs);
> +    bdrv_refresh_limits(bs, NULL);
>  }
>  
>  /*
> @@ -1778,7 +1791,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>                                                BDRV_O_CACHE_WB);
>      reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>  
> -    bdrv_refresh_limits(reopen_state->bs);
> +    bdrv_refresh_limits(reopen_state->bs, NULL);

> +++ b/block/stream.c
> @@ -76,7 +76,7 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
>          bdrv_unref(unused);
>      }
>  
> -    bdrv_refresh_limits(top);
> +    bdrv_refresh_limits(top, NULL);
>  }
>  

Should these three callers be concerned about failure?  If so, would
&error_abort be better than NULL?  But as for this patch, you are
preserving existing semantics, so you could save it for a later patch.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 1/2] block: Add Error argument to bdrv_refresh_limits()

[Kevin Wolf]

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

Am 16.07.2014 um 20:07 hat Eric Blake geschrieben:
> On 07/16/2014 09:48 AM, Kevin Wolf wrote:
> > @@ -1154,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> >      bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
> >                      bs->backing_blocker);
> >  out:
> > -    bdrv_refresh_limits(bs);
> > +    bdrv_refresh_limits(bs, NULL);
> >  }
> >  
> >  /*
> > @@ -1778,7 +1791,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> >                                                BDRV_O_CACHE_WB);
> >      reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
> >  
> > -    bdrv_refresh_limits(reopen_state->bs);
> > +    bdrv_refresh_limits(reopen_state->bs, NULL);
> 
> > +++ b/block/stream.c
> > @@ -76,7 +76,7 @@ static void close_unused_images(BlockDriverState *top, BlockDriverState *base,
> >          bdrv_unref(unused);
> >      }
> >  
> > -    bdrv_refresh_limits(top);
> > +    bdrv_refresh_limits(top, NULL);
> >  }
> >  
> 
> Should these three callers be concerned about failure?  If so, would
> &error_abort be better than NULL?  But as for this patch, you are
> preserving existing semantics, so you could save it for a later patch.

They probably should, but I couldn't figure out what they should to on
failure. Aborting qemu because a single block device fails isn't nice.
&error_abort has similar semantics as assert() for me ("This can't ever
happen"), and this is definitely not the case here as I/O errors can
happen anytime.

If anything, it's similar to a qcow2 image that has detected corruption
and therefore made the BDS unusable. But setting bs->drv = NULL within a
single block driver is already tricky (and so it was buggy at first),
doing it in block.c with BDSes of any driver sounds even worse.

This is the reason why I kept NULL here, even if it's not completely
right.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [PATCH 2/2] raw-posix: Fail gracefully if no working alignment is found

[Kevin Wolf]

If qemu couldn't find out what O_DIRECT alignment to use with a given
file, it would run into assert(bdrv_opt_mem_align(bs) != 0); in block.c
and confuse users. This adds a more descriptive error message for such
cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/raw-posix.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ef497b2..8e9758e 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -221,7 +221,7 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
-static void raw_probe_alignment(BlockDriverState *bs)
+static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     char *buf;
@@ -240,24 +240,24 @@ static void raw_probe_alignment(BlockDriverState *bs)
     s->buf_align = 0;
 
 #ifdef BLKSSZGET
-    if (ioctl(s->fd, BLKSSZGET, &sector_size) >= 0) {
+    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
         bs->request_alignment = sector_size;
     }
 #endif
 #ifdef DKIOCGETBLOCKSIZE
-    if (ioctl(s->fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
+    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
         bs->request_alignment = sector_size;
     }
 #endif
 #ifdef DIOCGSECTORSIZE
-    if (ioctl(s->fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
+    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
         bs->request_alignment = sector_size;
     }
 #endif
 #ifdef CONFIG_XFS
     if (s->is_xfs) {
         struct dioattr da;
-        if (xfsctl(NULL, s->fd, XFS_IOC_DIOINFO, &da) >= 0) {
+        if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
             bs->request_alignment = da.d_miniosz;
             /* The kernel returns wrong information for d_mem */
             /* s->buf_align = da.d_mem; */
@@ -270,7 +270,7 @@ static void raw_probe_alignment(BlockDriverState *bs)
         size_t align;
         buf = qemu_memalign(MAX_BLOCKSIZE, 2 * MAX_BLOCKSIZE);
         for (align = 512; align <= MAX_BLOCKSIZE; align <<= 1) {
-            if (pread(s->fd, buf + align, MAX_BLOCKSIZE, 0) >= 0) {
+            if (pread(fd, buf + align, MAX_BLOCKSIZE, 0) >= 0) {
                 s->buf_align = align;
                 break;
             }
@@ -282,13 +282,18 @@ static void raw_probe_alignment(BlockDriverState *bs)
         size_t align;
         buf = qemu_memalign(s->buf_align, MAX_BLOCKSIZE);
         for (align = 512; align <= MAX_BLOCKSIZE; align <<= 1) {
-            if (pread(s->fd, buf, align, 0) >= 0) {
+            if (pread(fd, buf, align, 0) >= 0) {
                 bs->request_alignment = align;
                 break;
             }
         }
         qemu_vfree(buf);
     }
+
+    if (!s->buf_align || !bs->request_alignment) {
+        error_setg(errp, "Could not find working O_DIRECT alignment. "
+                         "Try cache.direct=off.");
+    }
 }
 
 static void raw_parse_flags(int bdrv_flags, int *open_flags)
@@ -505,6 +510,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     BDRVRawState *s;
     BDRVRawReopenState *raw_s;
     int ret = 0;
+    Error *local_err = NULL;
 
     assert(state != NULL);
     assert(state->bs != NULL);
@@ -577,6 +583,19 @@ static int raw_reopen_prepare(BDRVReopenState *state,
             ret = -1;
         }
     }
+
+    /* Fail already reopen_prepare() if we can't get a working O_DIRECT
+     * alignment with the new fd. */
+    if (raw_s->fd != -1) {
+        raw_probe_alignment(state->bs, raw_s->fd, &local_err);
+        if (local_err) {
+            qemu_close(raw_s->fd);
+            raw_s->fd = -1;
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+        }
+    }
+
     return ret;
 }
 
@@ -619,7 +638,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
 
-    raw_probe_alignment(bs);
+    raw_probe_alignment(bs, s->fd, errp);
     bs->bl.opt_mem_alignment = s->buf_align;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] raw-posix: Fail gracefully if no working alignment is found

[Eric Blake]

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

On 07/16/2014 09:48 AM, Kevin Wolf wrote:
> If qemu couldn't find out what O_DIRECT alignment to use with a given
> file, it would run into assert(bdrv_opt_mem_align(bs) != 0); in block.c
> and confuse users. This adds a more descriptive error message for such
> cases.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/raw-posix.c | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH for-2.1 0/2] raw-posix: Fail gracefully if no working alignment is found

[Stefan Hajnoczi]

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

On Wed, Jul 16, 2014 at 05:48:15PM +0200, Kevin Wolf wrote:
> Kevin Wolf (2):
>   block: Add Error argument to bdrv_refresh_limits()
>   raw-posix: Fail gracefully if no working alignment is found
> 
>  block.c                   | 33 +++++++++++++++++++++++----------
>  block/iscsi.c             |  3 +--
>  block/qcow2.c             |  4 +---
>  block/qed.c               |  4 +---
>  block/raw-posix.c         | 39 ++++++++++++++++++++++++++++-----------
>  block/raw_bsd.c           |  3 +--
>  block/stream.c            |  2 +-
>  block/vmdk.c              |  4 +---
>  include/block/block.h     |  2 +-
>  include/block/block_int.h |  2 +-
>  10 files changed, 59 insertions(+), 37 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]