All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup
@ 2017-04-05 18:28 Jeff Cody
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function Jeff Cody
                   ` (8 more replies)
  0 siblings, 9 replies; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

This series does two things:

1.) Cleans up some of the logic behind setting the read_only flag
    for a BDS in the block layer, so that it is done consistently
    (and rules are applied consistently), and

2.) Adds .bdrv_reopen_prepare() implementation for RBD, so that block
    jobs can be run on backing chains that have rbd protocol nodes.

Jeff Cody (9):
  block: add bdrv_set_read_only() helper function
  block: do not set BDS read_only if copy_on_read enabled
  block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  block: code movement
  block: introduce bdrv_try_set_read_only()
  block: use bdrv_try_set_read_only() during reopen
  block/rbd - update variable names to more apt names
  block/rbd: do not blindly set bs->read_only
  block/rbd: Add support for reopen()

 block.c               | 56 +++++++++++++++++++++++++++++++++++---------
 block/bochs.c         |  5 +++-
 block/cloop.c         |  5 +++-
 block/dmg.c           |  6 ++++-
 block/rbd.c           | 65 +++++++++++++++++++++++++++++++++++++--------------
 block/vvfat.c         | 15 +++++++++---
 include/block/block.h |  2 ++
 7 files changed, 120 insertions(+), 34 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-07  9:06   ` Stefan Hajnoczi
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled Jeff Cody
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

We have a helper wrapper for checking for the BDS read_only flag,
add a helper wrapper to set the read_only flag as well.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 5 +++++
 block/bochs.c         | 2 +-
 block/cloop.c         | 2 +-
 block/dmg.c           | 2 +-
 block/rbd.c           | 2 +-
 block/vvfat.c         | 4 ++--
 include/block/block.h | 1 +
 7 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 927ba89..7b4c7ef 100644
--- a/block.c
+++ b/block.c
@@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+{
+    bs->read_only = read_only;
+}
+
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
                                                   const char *backing,
                                                   char *dest, size_t sz,
diff --git a/block/bochs.c b/block/bochs.c
index 516da56..bdc2831 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -110,7 +110,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bs->read_only = true; /* no write support yet */
+    bdrv_set_read_only(bs, true); /* no write support yet */
 
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
diff --git a/block/cloop.c b/block/cloop.c
index a6c7b9d..11f17c8 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -72,7 +72,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
 
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
diff --git a/block/dmg.c b/block/dmg.c
index a7d25fc..27ce4a6 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -420,7 +420,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     block_module_load_one("dmg-bz2");
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/rbd.c b/block/rbd.c
index 1ceeeb5..6ad2904 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -641,7 +641,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    bs->read_only = (s->snap != NULL);
+    bdrv_set_read_only(bs, (s->snap != NULL));
 
     qemu_opts_del(opts);
     return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index af5153d..d4ce6d7 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1157,7 +1157,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     s->current_cluster=0xffffffff;
 
     /* read only is the default for safety */
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
     s->qcow = NULL;
     s->qcow_filename = NULL;
     s->fat2 = NULL;
@@ -1173,7 +1173,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto fail;
         }
-        bs->read_only = false;
+        bdrv_set_read_only(bs, false);
     }
 
     bs->total_sectors = cyls * heads * secs;
diff --git a/include/block/block.h b/include/block/block.h
index 5149260..06c9032 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
+void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-05 19:16   ` John Snow
                     ` (2 more replies)
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only Jeff Cody
                   ` (6 subsequent siblings)
  8 siblings, 3 replies; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

A few block drivers will set the BDS read_only flag from their
.bdrv_open() function.  This means the bs->read_only flag could
be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
flag check occurs prior to the call to bdrv->bdrv_open().

This adds an error return to bdrv_set_read_only(), and an error will be
return if we try to set the BDS to read_only while copy_on_read is
enabled.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 10 +++++++++-
 block/bochs.c         |  5 ++++-
 block/cloop.c         |  5 ++++-
 block/dmg.c           |  6 +++++-
 block/rbd.c           |  6 +++++-
 block/vvfat.c         | 15 ++++++++++++---
 include/block/block.h |  2 +-
 7 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 7b4c7ef..f60d5ea 100644
--- a/block.c
+++ b/block.c
@@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
+    /* Do not set read_only if copy_on_read is enabled */
+    if (bs->copy_on_read && read_only) {
+        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
+                   bdrv_get_device_or_node_name(bs));
+        return -EINVAL;
+    }
+
     bs->read_only = read_only;
+    return 0;
 }
 
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index bdc2831..a759b6e 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bdrv_set_read_only(bs, true); /* no write support yet */
+    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
+    if (ret < 0) {
+        return ret;
+    }
 
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
diff --git a/block/cloop.c b/block/cloop.c
index 11f17c8..d6597fc 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bdrv_set_read_only(bs, true);
+    ret = bdrv_set_read_only(bs, true, errp);
+    if (ret < 0) {
+        return ret;
+    }
 
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
diff --git a/block/dmg.c b/block/dmg.c
index 27ce4a6..900ae5a 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
+    ret = bdrv_set_read_only(bs, true, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     block_module_load_one("dmg-bz2");
-    bdrv_set_read_only(bs, true);
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/rbd.c b/block/rbd.c
index 6ad2904..328e4a9 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    bdrv_set_read_only(bs, (s->snap != NULL));
+    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
+    if (r < 0) {
+        error_propagate(errp, local_err);
+        goto failed_open;
+    }
 
     qemu_opts_del(opts);
     return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index d4ce6d7..34a2854 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->current_cluster=0xffffffff;
 
-    /* read only is the default for safety */
-    bdrv_set_read_only(bs, true);
     s->qcow = NULL;
     s->qcow_filename = NULL;
     s->fat2 = NULL;
@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto fail;
         }
-        bdrv_set_read_only(bs, false);
+        ret = bdrv_set_read_only(bs, false, &local_err);
+        if (ret < 0) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
+    } else  {
+        /* read only is the default for safety */
+        ret = bdrv_set_read_only(bs, true, &local_err);
+        if (ret < 0) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
     }
 
     bs->total_sectors = cyls * heads * secs;
diff --git a/include/block/block.h b/include/block/block.h
index 06c9032..beb563a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function Jeff Cody
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-05 19:20   ` John Snow
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 4/9] block: code movement Jeff Cody
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of
the BDS 'read_only' state, but there are a few places where it
is ignored.  In the bdrv_set_read_only() helper, make sure to
honor the flag.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index f60d5ea..4a61ff0 100644
--- a/block.c
+++ b/block.c
@@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
         return -EINVAL;
     }
 
+    /* Do not clear read_only if it is prohibited */
+    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+        error_setg(errp, "Node '%s' is read only",
+                   bdrv_get_device_or_node_name(bs));
+        return -EPERM;
+    }
+
     bs->read_only = read_only;
     return 0;
 }
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 4/9] block: code movement
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
                   ` (2 preceding siblings ...)
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-05 20:25   ` John Snow
  2017-04-07  9:16   ` Stefan Hajnoczi
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only() Jeff Cody
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

Move bdrv_is_read_only() up with its friends.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 4a61ff0..8bfe7f4 100644
--- a/block.c
+++ b/block.c
@@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+bool bdrv_is_read_only(BlockDriverState *bs)
+{
+    return bs->read_only;
+}
+
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     /* Do not set read_only if copy_on_read is enabled */
@@ -3367,11 +3372,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
     *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
 }
 
-bool bdrv_is_read_only(BlockDriverState *bs)
-{
-    return bs->read_only;
-}
-
 bool bdrv_is_sg(BlockDriverState *bs)
 {
     return bs->sg;
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
                   ` (3 preceding siblings ...)
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 4/9] block: code movement Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-05 20:28   ` John Snow
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen Jeff Cody
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

Introduce try function for setting read_only flags.  Will return < 0 on
error, with appropriate Error value set.  Does not alter any flags.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 14 +++++++++++++-
 include/block/block.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 8bfe7f4..ad958b9 100644
--- a/block.c
+++ b/block.c
@@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)
     return bs->read_only;
 }
 
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     /* Do not set read_only if copy_on_read is enabled */
     if (bs->copy_on_read && read_only) {
@@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
         return -EPERM;
     }
 
+    return 0;
+}
+
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+{
+    int ret = 0;
+
+    ret = bdrv_try_set_read_only(bs, read_only, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->read_only = read_only;
     return 0;
 }
diff --git a/include/block/block.h b/include/block/block.h
index beb563a..0049b57 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
+int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
                   ` (4 preceding siblings ...)
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only() Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-05 20:25   ` John Snow
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names Jeff Cody
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index ad958b9..3245fae 100644
--- a/block.c
+++ b/block.c
@@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     BlockDriver *drv;
     QemuOpts *opts;
     const char *value;
+    bool read_only;
 
     assert(reopen_state != NULL);
     assert(reopen_state->bs->drv != NULL);
@@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         qdict_put(reopen_state->options, "driver", qstring_from_str(value));
     }
 
-    /* if we are to stay read-only, do not allow permission change
-     * to r/w */
-    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
-        reopen_state->flags & BDRV_O_RDWR) {
-        error_setg(errp, "Node '%s' is read only",
-                   bdrv_get_device_or_node_name(reopen_state->bs));
+    /* If we are to stay read-only, do not allow permission change
+     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
+     * not set, or if the BDS still has copy_on_read enabled */
+    read_only = !(reopen_state->bs->open_flags & BDRV_O_RDWR);
+    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         goto error;
     }
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
                   ` (5 preceding siblings ...)
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-07  9:47   ` Stefan Hajnoczi
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only Jeff Cody
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen() Jeff Cody
  8 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

Update 'clientname' to be 'user', which tracks better with both
the QAPI and rados viarable naming.

Update 'name' to be 'image_name', as it indicates the rbd image.
Naming it 'image' would have been ideal, but we are using that for
the rados_image_t value returned by rbd_open().

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 328e4a9..c5e1aeb 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -94,7 +94,7 @@ typedef struct BDRVRBDState {
     rados_t cluster;
     rados_ioctx_t io_ctx;
     rbd_image_t image;
-    char *name;
+    char *image_name;
     char *snap;
 } BDRVRBDState;
 
@@ -350,7 +350,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     int64_t bytes = 0;
     int64_t objsize;
     int obj_order = 0;
-    const char *pool, *name, *conf, *clientname, *keypairs;
+    const char *pool, *image_name, *conf, *user, *keypairs;
     const char *secretid;
     rados_t cluster;
     rados_ioctx_t io_ctx;
@@ -393,11 +393,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
      */
     pool       = qdict_get_try_str(options, "pool");
     conf       = qdict_get_try_str(options, "conf");
-    clientname = qdict_get_try_str(options, "user");
-    name       = qdict_get_try_str(options, "image");
+    user       = qdict_get_try_str(options, "user");
+    image_name = qdict_get_try_str(options, "image");
     keypairs   = qdict_get_try_str(options, "=keyvalue-pairs");
 
-    ret = rados_create(&cluster, clientname);
+    ret = rados_create(&cluster, user);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error initializing");
         goto exit;
@@ -434,7 +434,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
         goto shutdown;
     }
 
-    ret = rbd_create(io_ctx, name, bytes, &obj_order);
+    ret = rbd_create(io_ctx, image_name, bytes, &obj_order);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error rbd create");
     }
@@ -540,7 +540,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
-    const char *pool, *snap, *conf, *clientname, *name, *keypairs;
+    const char *pool, *snap, *conf, *user, *image_name, *keypairs;
     const char *secretid;
     QemuOpts *opts;
     Error *local_err = NULL;
@@ -567,24 +567,24 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     pool           = qemu_opt_get(opts, "pool");
     conf           = qemu_opt_get(opts, "conf");
     snap           = qemu_opt_get(opts, "snapshot");
-    clientname     = qemu_opt_get(opts, "user");
-    name           = qemu_opt_get(opts, "image");
+    user           = qemu_opt_get(opts, "user");
+    image_name     = qemu_opt_get(opts, "image");
     keypairs       = qemu_opt_get(opts, "=keyvalue-pairs");
 
-    if (!pool || !name) {
+    if (!pool || !image_name) {
         error_setg(errp, "Parameters 'pool' and 'image' are required");
         r = -EINVAL;
         goto failed_opts;
     }
 
-    r = rados_create(&s->cluster, clientname);
+    r = rados_create(&s->cluster, user);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
         goto failed_opts;
     }
 
     s->snap = g_strdup(snap);
-    s->name = g_strdup(name);
+    s->image_name = g_strdup(image_name);
 
     /* try default location when conf=NULL, but ignore failure */
     r = rados_conf_read_file(s->cluster, conf);
@@ -635,9 +635,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_shutdown;
     }
 
-    r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
+    r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
-        error_setg_errno(errp, -r, "error reading header from %s", s->name);
+        error_setg_errno(errp, -r, "error reading header from %s",
+                         s->image_name);
         goto failed_open;
     }
 
@@ -655,7 +656,7 @@ failed_open:
 failed_shutdown:
     rados_shutdown(s->cluster);
     g_free(s->snap);
-    g_free(s->name);
+    g_free(s->image_name);
 failed_opts:
     qemu_opts_del(opts);
     g_free(mon_host);
@@ -669,7 +670,7 @@ static void qemu_rbd_close(BlockDriverState *bs)
     rbd_close(s->image);
     rados_ioctx_destroy(s->io_ctx);
     g_free(s->snap);
-    g_free(s->name);
+    g_free(s->image_name);
     rados_shutdown(s->cluster);
 }
 
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
                   ` (6 preceding siblings ...)
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-07  9:52   ` Stefan Hajnoczi
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen() Jeff Cody
  8 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

We do not want to just blindly set bs->read_only.  The only time
we need to set it with the current rbd driver, is if we are using an
rbd snapshot.  In that case, we must set it to read-only.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index c5e1aeb..35853c9 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -635,6 +635,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_shutdown;
     }
 
+    /* rbd_open is always r/w */
     r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
         error_setg_errno(errp, -r, "error reading header from %s",
@@ -642,10 +643,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
-    if (r < 0) {
-        error_propagate(errp, local_err);
-        goto failed_open;
+    /* If we are using an rbd snapshot, we must be r/o, otherwise
+     * leave as-is */
+    if (s->snap != NULL) {
+        r = bdrv_set_read_only(bs, true, &local_err);
+        if (r < 0) {
+            error_propagate(errp, local_err);
+            goto failed_open;
+        }
     }
 
     qemu_opts_del(opts);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen()
  2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
                   ` (7 preceding siblings ...)
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only Jeff Cody
@ 2017-04-05 18:28 ` Jeff Cody
  2017-04-07  9:53   ` Stefan Hajnoczi
  8 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 18:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, jsnow, stefanha, kwolf

This adds support for reopen in rbd, for changing between r/w and r/o.

Note, that this is only a flag change, but we will block a change from
r/o to r/w if we are using an RBD internal snapshot.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index 35853c9..6471f4f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -668,6 +668,26 @@ failed_opts:
     return r;
 }
 
+
+/* Since RBD is currently always opened R/W via the API,
+ * we just need to check if we are using a snapshot or not, in
+ * order to determine if we will allow it to be R/W */
+static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
+                                   BlockReopenQueue *queue, Error **errp)
+{
+    BDRVRBDState *s = state->bs->opaque;
+    int ret = 0;
+
+    if (s->snap && state->flags & BDRV_O_RDWR) {
+        error_setg(errp,
+                   "Cannot change node '%s' to r/w when using RBD snapshot",
+                   bdrv_get_device_or_node_name(state->bs));
+        ret = -EINVAL;
+    }
+
+    return ret;
+}
+
 static void qemu_rbd_close(BlockDriverState *bs)
 {
     BDRVRBDState *s = bs->opaque;
@@ -1074,6 +1094,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_parse_filename    = qemu_rbd_parse_filename,
     .bdrv_file_open         = qemu_rbd_open,
     .bdrv_close             = qemu_rbd_close,
+    .bdrv_reopen_prepare    = qemu_rbd_reopen_prepare,
     .bdrv_create            = qemu_rbd_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
     .bdrv_get_info          = qemu_rbd_getinfo,
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled Jeff Cody
@ 2017-04-05 19:16   ` John Snow
  2017-04-05 23:50     ` Jeff Cody
  2017-04-07  9:13   ` Stefan Hajnoczi
  2017-04-07  9:45   ` Stefan Hajnoczi
  2 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2017-04-05 19:16 UTC (permalink / raw)
  To: Jeff Cody, qemu-devel; +Cc: kwolf, stefanha, qemu-block



On 04/05/2017 02:28 PM, Jeff Cody wrote:
> A few block drivers will set the BDS read_only flag from their
> .bdrv_open() function.  This means the bs->read_only flag could
> be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
> flag check occurs prior to the call to bdrv->bdrv_open().
> 
> This adds an error return to bdrv_set_read_only(), and an error will be
> return if we try to set the BDS to read_only while copy_on_read is
> enabled.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 10 +++++++++-
>  block/bochs.c         |  5 ++++-
>  block/cloop.c         |  5 ++++-
>  block/dmg.c           |  6 +++++-
>  block/rbd.c           |  6 +++++-
>  block/vvfat.c         | 15 ++++++++++++---
>  include/block/block.h |  2 +-
>  7 files changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7b4c7ef..f60d5ea 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
> +    /* Do not set read_only if copy_on_read is enabled */
> +    if (bs->copy_on_read && read_only) {
> +        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",

COW?

> +                   bdrv_get_device_or_node_name(bs));
> +        return -EINVAL;
> +    }
> +
>      bs->read_only = read_only;
> +    return 0;
>  }
>  
>  void bdrv_get_full_backing_filename_from_filename(const char *backed,
> diff --git a/block/bochs.c b/block/bochs.c
> index bdc2831..a759b6e 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    bdrv_set_read_only(bs, true); /* no write support yet */
> +    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
> +    if (ret < 0) {
> +        return ret;
> +    }
>  
>      ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
>      if (ret < 0) {
> diff --git a/block/cloop.c b/block/cloop.c
> index 11f17c8..d6597fc 100644
> --- a/block/cloop.c
> +++ b/block/cloop.c
> @@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    bdrv_set_read_only(bs, true);
> +    ret = bdrv_set_read_only(bs, true, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
>  
>      /* read header */
>      ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
> diff --git a/block/dmg.c b/block/dmg.c
> index 27ce4a6..900ae5a 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> +    ret = bdrv_set_read_only(bs, true, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
>      block_module_load_one("dmg-bz2");
> -    bdrv_set_read_only(bs, true);
>  
>      s->n_chunks = 0;
>      s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> diff --git a/block/rbd.c b/block/rbd.c
> index 6ad2904..328e4a9 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_open;
>      }
>  
> -    bdrv_set_read_only(bs, (s->snap != NULL));
> +    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> +    if (r < 0) {
> +        error_propagate(errp, local_err);
> +        goto failed_open;
> +    }
>  
>      qemu_opts_del(opts);
>      return 0;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d4ce6d7..34a2854 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->current_cluster=0xffffffff;
>  
> -    /* read only is the default for safety */
> -    bdrv_set_read_only(bs, true);
>      s->qcow = NULL;
>      s->qcow_filename = NULL;
>      s->fat2 = NULL;
> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto fail;
>          }
> -        bdrv_set_read_only(bs, false);
> +        ret = bdrv_set_read_only(bs, false, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
> +    } else  {
> +        /* read only is the default for safety */
> +        ret = bdrv_set_read_only(bs, true, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
>      }
>  
>      bs->total_sectors = cyls * heads * secs;
> diff --git a/include/block/block.h b/include/block/block.h
> index 06c9032..beb563a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>                              int64_t sector_num, int nb_sectors, int *pnum);
>  
>  bool bdrv_is_read_only(BlockDriverState *bs);
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  bool bdrv_is_sg(BlockDriverState *bs);
>  bool bdrv_is_inserted(BlockDriverState *bs);
>  int bdrv_media_changed(BlockDriverState *bs);
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only Jeff Cody
@ 2017-04-05 19:20   ` John Snow
  2017-04-05 19:26     ` Eric Blake
  0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2017-04-05 19:20 UTC (permalink / raw)
  To: Jeff Cody, qemu-devel; +Cc: kwolf, stefanha, qemu-block



On 04/05/2017 02:28 PM, Jeff Cody wrote:
> The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of
> the BDS 'read_only' state, but there are a few places where it
> is ignored.  In the bdrv_set_read_only() helper, make sure to
> honor the flag.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block.c b/block.c
> index f60d5ea..4a61ff0 100644
> --- a/block.c
> +++ b/block.c
> @@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>          return -EINVAL;
>      }
>  
> +    /* Do not clear read_only if it is prohibited */
> +    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
> +        error_setg(errp, "Node '%s' is read only",
> +                   bdrv_get_device_or_node_name(bs));
> +        return -EPERM;
> +    }
> +
>      bs->read_only = read_only;
>      return 0;
>  }
> 

Conceptually straightforward.

looks like this might change behavior for... RBD and vvfat, right?
RBD is the subject of this series so we'll just assume that was broken
and stupid.

What's vvfat's story? It always set the read-only property to false
regardless of what you asked for?

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  2017-04-05 19:20   ` John Snow
@ 2017-04-05 19:26     ` Eric Blake
  2017-04-06  0:20       ` Jeff Cody
  0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2017-04-05 19:26 UTC (permalink / raw)
  To: John Snow, Jeff Cody, qemu-devel; +Cc: kwolf, qemu-block, stefanha

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

On 04/05/2017 02:20 PM, John Snow wrote:

> Conceptually straightforward.
> 
> looks like this might change behavior for... RBD and vvfat, right?
> RBD is the subject of this series so we'll just assume that was broken
> and stupid.
> 
> What's vvfat's story? It always set the read-only property to false
> regardless of what you asked for?

vvfat is even stupider than that - it has its own independent property
'rw' that determines whether to allow write operations, separate from
the inherited BDS readonly property.

-- 
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 --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 4/9] block: code movement
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 4/9] block: code movement Jeff Cody
@ 2017-04-05 20:25   ` John Snow
  2017-04-07  9:16   ` Stefan Hajnoczi
  1 sibling, 0 replies; 29+ messages in thread
From: John Snow @ 2017-04-05 20:25 UTC (permalink / raw)
  To: Jeff Cody, qemu-devel; +Cc: kwolf, stefanha, qemu-block



On 04/05/2017 02:28 PM, Jeff Cody wrote:
> Move bdrv_is_read_only() up with its friends.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4a61ff0..8bfe7f4 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> +bool bdrv_is_read_only(BlockDriverState *bs)
> +{
> +    return bs->read_only;
> +}
> +
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
>      /* Do not set read_only if copy_on_read is enabled */
> @@ -3367,11 +3372,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
>      *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
>  }
>  
> -bool bdrv_is_read_only(BlockDriverState *bs)
> -{
> -    return bs->read_only;
> -}
> -
>  bool bdrv_is_sg(BlockDriverState *bs)
>  {
>      return bs->sg;
> 

http://i.imgur.com/BrkMeS5.jpg

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen Jeff Cody
@ 2017-04-05 20:25   ` John Snow
  2017-04-06  0:27     ` Jeff Cody
  0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2017-04-05 20:25 UTC (permalink / raw)
  To: Jeff Cody, qemu-devel; +Cc: kwolf, stefanha, qemu-block



On 04/05/2017 02:28 PM, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ad958b9..3245fae 100644
> --- a/block.c
> +++ b/block.c
> @@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>      BlockDriver *drv;
>      QemuOpts *opts;
>      const char *value;
> +    bool read_only;
>  
>      assert(reopen_state != NULL);
>      assert(reopen_state->bs->drv != NULL);
> @@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          qdict_put(reopen_state->options, "driver", qstring_from_str(value));
>      }
>  
> -    /* if we are to stay read-only, do not allow permission change
> -     * to r/w */
> -    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> -        reopen_state->flags & BDRV_O_RDWR) {

So the current code checks reopen_state->flags & BDRV_O_RDWR;

> -        error_setg(errp, "Node '%s' is read only",
> -                   bdrv_get_device_or_node_name(reopen_state->bs));
> +    /* If we are to stay read-only, do not allow permission change
> +     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
> +     * not set, or if the BDS still has copy_on_read enabled */
> +    read_only = !(reopen_state->bs->open_flags & BDRV_O_RDWR);

And the proposed change checks reopen_state->bs->open_flags &
BDRV_O_RDWR. (It's negated again inside of bdrv_try_set_read_only.)

Both check against !(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR).

What's the functional difference of doing so, and is it intentional?

> +    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          goto error;
>      }
>  
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only() Jeff Cody
@ 2017-04-05 20:28   ` John Snow
  2017-04-06  0:23     ` Jeff Cody
  0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2017-04-05 20:28 UTC (permalink / raw)
  To: Jeff Cody, qemu-devel; +Cc: kwolf, stefanha, qemu-block



On 04/05/2017 02:28 PM, Jeff Cody wrote:
> Introduce try function for setting read_only flags.  Will return < 0 on
> error, with appropriate Error value set.  Does not alter any flags.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 14 +++++++++++++-
>  include/block/block.h |  1 +
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 8bfe7f4..ad958b9 100644
> --- a/block.c
> +++ b/block.c
> @@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)
>      return bs->read_only;
>  }
>  
> -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
>      /* Do not set read_only if copy_on_read is enabled */
>      if (bs->copy_on_read && read_only) {
> @@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>          return -EPERM;
>      }
>  
> +    return 0;
> +}
> +
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> +{
> +    int ret = 0;
> +
> +    ret = bdrv_try_set_read_only(bs, read_only, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
>      bs->read_only = read_only;
>      return 0;
>  }
> diff --git a/include/block/block.h b/include/block/block.h
> index beb563a..0049b57 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>                              int64_t sector_num, int nb_sectors, int *pnum);
>  
>  bool bdrv_is_read_only(BlockDriverState *bs);
> +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  bool bdrv_is_sg(BlockDriverState *bs);
>  bool bdrv_is_inserted(BlockDriverState *bs);
> 

Maybe I'm just green, but I find it weird that the function is named
try_x when we aren't actually trying to do anything, we're checking to
see if we can.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
  2017-04-05 19:16   ` John Snow
@ 2017-04-05 23:50     ` Jeff Cody
  0 siblings, 0 replies; 29+ messages in thread
From: Jeff Cody @ 2017-04-05 23:50 UTC (permalink / raw)
  To: John Snow; +Cc: qemu-devel, kwolf, stefanha, qemu-block

On Wed, Apr 05, 2017 at 03:16:38PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > A few block drivers will set the BDS read_only flag from their
> > .bdrv_open() function.  This means the bs->read_only flag could
> > be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
> > flag check occurs prior to the call to bdrv->bdrv_open().
> > 
> > This adds an error return to bdrv_set_read_only(), and an error will be
> > return if we try to set the BDS to read_only while copy_on_read is
> > enabled.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c               | 10 +++++++++-
> >  block/bochs.c         |  5 ++++-
> >  block/cloop.c         |  5 ++++-
> >  block/dmg.c           |  6 +++++-
> >  block/rbd.c           |  6 +++++-
> >  block/vvfat.c         | 15 ++++++++++++---
> >  include/block/block.h |  2 +-
> >  7 files changed, 40 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 7b4c7ef..f60d5ea 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
> >      }
> >  }
> >  
> > -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >  {
> > +    /* Do not set read_only if copy_on_read is enabled */
> > +    if (bs->copy_on_read && read_only) {
> > +        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
> 
> COW?
>

Mooo! You are right, that should be COR (or better yet, I should just write
it out - copy on read).

> > +                   bdrv_get_device_or_node_name(bs));
> > +        return -EINVAL;
> > +    }
> > +
> >      bs->read_only = read_only;
> > +    return 0;
> >  }
> >  
> >  void bdrv_get_full_backing_filename_from_filename(const char *backed,
> > diff --git a/block/bochs.c b/block/bochs.c
> > index bdc2831..a759b6e 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
> >          return -EINVAL;
> >      }
> >  
> > -    bdrv_set_read_only(bs, true); /* no write support yet */
> > +    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> >  
> >      ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
> >      if (ret < 0) {
> > diff --git a/block/cloop.c b/block/cloop.c
> > index 11f17c8..d6597fc 100644
> > --- a/block/cloop.c
> > +++ b/block/cloop.c
> > @@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
> >          return -EINVAL;
> >      }
> >  
> > -    bdrv_set_read_only(bs, true);
> > +    ret = bdrv_set_read_only(bs, true, errp);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> >  
> >      /* read header */
> >      ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 27ce4a6..900ae5a 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -419,8 +419,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> >          return -EINVAL;
> >      }
> >  
> > +    ret = bdrv_set_read_only(bs, true, errp);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> >      block_module_load_one("dmg-bz2");
> > -    bdrv_set_read_only(bs, true);
> >  
> >      s->n_chunks = 0;
> >      s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> > diff --git a/block/rbd.c b/block/rbd.c
> > index 6ad2904..328e4a9 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >          goto failed_open;
> >      }
> >  
> > -    bdrv_set_read_only(bs, (s->snap != NULL));
> > +    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> > +    if (r < 0) {
> > +        error_propagate(errp, local_err);
> > +        goto failed_open;
> > +    }
> >  
> >      qemu_opts_del(opts);
> >      return 0;
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index d4ce6d7..34a2854 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >  
> >      s->current_cluster=0xffffffff;
> >  
> > -    /* read only is the default for safety */
> > -    bdrv_set_read_only(bs, true);
> >      s->qcow = NULL;
> >      s->qcow_filename = NULL;
> >      s->fat2 = NULL;
> > @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >          if (ret < 0) {
> >              goto fail;
> >          }
> > -        bdrv_set_read_only(bs, false);
> > +        ret = bdrv_set_read_only(bs, false, &local_err);
> > +        if (ret < 0) {
> > +            error_propagate(errp, local_err);
> > +            goto fail;
> > +        }
> > +    } else  {
> > +        /* read only is the default for safety */
> > +        ret = bdrv_set_read_only(bs, true, &local_err);
> > +        if (ret < 0) {
> > +            error_propagate(errp, local_err);
> > +            goto fail;
> > +        }
> >      }
> >  
> >      bs->total_sectors = cyls * heads * secs;
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 06c9032..beb563a 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> >                              int64_t sector_num, int nb_sectors, int *pnum);
> >  
> >  bool bdrv_is_read_only(BlockDriverState *bs);
> > -void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  bool bdrv_is_sg(BlockDriverState *bs);
> >  bool bdrv_is_inserted(BlockDriverState *bs);
> >  int bdrv_media_changed(BlockDriverState *bs);
> > 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  2017-04-05 19:26     ` Eric Blake
@ 2017-04-06  0:20       ` Jeff Cody
  2017-04-06  0:55         ` Jeff Cody
  0 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-06  0:20 UTC (permalink / raw)
  To: Eric Blake; +Cc: John Snow, qemu-devel, kwolf, qemu-block, stefanha

On Wed, Apr 05, 2017 at 02:26:53PM -0500, Eric Blake wrote:
> On 04/05/2017 02:20 PM, John Snow wrote:
> 
> > Conceptually straightforward.
> > 
> > looks like this might change behavior for... RBD and vvfat, right?
> > RBD is the subject of this series so we'll just assume that was broken
> > and stupid.
> > 

Yes on RBD, and that change is intentional.

> > What's vvfat's story? It always set the read-only property to false
> > regardless of what you asked for?
> 
> vvfat is even stupider than that - it has its own independent property
> 'rw' that determines whether to allow write operations, separate from
> the inherited BDS readonly property.
>

Yes, it is very odd.  But if we have copy_on_read enabled, or explicitly set
the block device to read-only via QAPI or -drive, I think that those should
take precedence.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()
  2017-04-05 20:28   ` John Snow
@ 2017-04-06  0:23     ` Jeff Cody
  2017-04-07  9:18       ` Stefan Hajnoczi
  0 siblings, 1 reply; 29+ messages in thread
From: Jeff Cody @ 2017-04-06  0:23 UTC (permalink / raw)
  To: John Snow; +Cc: qemu-devel, kwolf, stefanha, qemu-block

On Wed, Apr 05, 2017 at 04:28:44PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > Introduce try function for setting read_only flags.  Will return < 0 on
> > error, with appropriate Error value set.  Does not alter any flags.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c               | 14 +++++++++++++-
> >  include/block/block.h |  1 +
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index 8bfe7f4..ad958b9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)
> >      return bs->read_only;
> >  }
> >  
> > -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >  {
> >      /* Do not set read_only if copy_on_read is enabled */
> >      if (bs->copy_on_read && read_only) {
> > @@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >          return -EPERM;
> >      }
> >  
> > +    return 0;
> > +}
> > +
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +{
> > +    int ret = 0;
> > +
> > +    ret = bdrv_try_set_read_only(bs, read_only, errp);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> >      bs->read_only = read_only;
> >      return 0;
> >  }
> > diff --git a/include/block/block.h b/include/block/block.h
> > index beb563a..0049b57 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> >                              int64_t sector_num, int nb_sectors, int *pnum);
> >  
> >  bool bdrv_is_read_only(BlockDriverState *bs);
> > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  bool bdrv_is_sg(BlockDriverState *bs);
> >  bool bdrv_is_inserted(BlockDriverState *bs);
> > 
> 
> Maybe I'm just green, but I find it weird that the function is named
> try_x when we aren't actually trying to do anything, we're checking to
> see if we can.

Yeah, not a good name.  Maybe bdrv_set_read_only_test() or
bdrv_test_set_read_only()?

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen
  2017-04-05 20:25   ` John Snow
@ 2017-04-06  0:27     ` Jeff Cody
  0 siblings, 0 replies; 29+ messages in thread
From: Jeff Cody @ 2017-04-06  0:27 UTC (permalink / raw)
  To: John Snow; +Cc: qemu-devel, kwolf, stefanha, qemu-block

On Wed, Apr 05, 2017 at 04:25:39PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index ad958b9..3245fae 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
> >      BlockDriver *drv;
> >      QemuOpts *opts;
> >      const char *value;
> > +    bool read_only;
> >  
> >      assert(reopen_state != NULL);
> >      assert(reopen_state->bs->drv != NULL);
> > @@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
> >          qdict_put(reopen_state->options, "driver", qstring_from_str(value));
> >      }
> >  
> > -    /* if we are to stay read-only, do not allow permission change
> > -     * to r/w */
> > -    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> > -        reopen_state->flags & BDRV_O_RDWR) {
> 
> So the current code checks reopen_state->flags & BDRV_O_RDWR;
> 
> > -        error_setg(errp, "Node '%s' is read only",
> > -                   bdrv_get_device_or_node_name(reopen_state->bs));
> > +    /* If we are to stay read-only, do not allow permission change
> > +     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
> > +     * not set, or if the BDS still has copy_on_read enabled */
> > +    read_only = !(reopen_state->bs->open_flags & BDRV_O_RDWR);
> 
> And the proposed change checks reopen_state->bs->open_flags &
> BDRV_O_RDWR. (It's negated again inside of bdrv_try_set_read_only.)
> 
> Both check against !(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR).
> 
> What's the functional difference of doing so, and is it intentional?
>

Good catch; unintentional.  That line should read:

+    read_only = !(reopen_state->flags & BDRV_O_RDWR);

The functional difference is we would be testing against the open_flags of
the current BS to see if we are requesting r/w or r/o, rather than checking
against the reopen requested flags.

> > +    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> >          goto error;
> >      }
> >  
> > 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  2017-04-06  0:20       ` Jeff Cody
@ 2017-04-06  0:55         ` Jeff Cody
  0 siblings, 0 replies; 29+ messages in thread
From: Jeff Cody @ 2017-04-06  0:55 UTC (permalink / raw)
  To: Eric Blake; +Cc: John Snow, qemu-devel, kwolf, qemu-block, stefanha

On Wed, Apr 05, 2017 at 08:20:28PM -0400, Jeff Cody wrote:
> On Wed, Apr 05, 2017 at 02:26:53PM -0500, Eric Blake wrote:
> > On 04/05/2017 02:20 PM, John Snow wrote:
> > 
> > > Conceptually straightforward.
> > > 
> > > looks like this might change behavior for... RBD and vvfat, right?
> > > RBD is the subject of this series so we'll just assume that was broken
> > > and stupid.
> > > 
> 
> Yes on RBD, and that change is intentional.
> 
> > > What's vvfat's story? It always set the read-only property to false
> > > regardless of what you asked for?
> > 
> > vvfat is even stupider than that - it has its own independent property
> > 'rw' that determines whether to allow write operations, separate from
> > the inherited BDS readonly property.
> >
> 
> Yes, it is very odd.  But if we have copy_on_read enabled, or explicitly set
> the block device to read-only via QAPI or -drive, I think that those should
> take precedence.
>

I meant to add an example - currently, with this drive commandline:

"-drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on"

The drive is not read-only, but writable.  That seems broken.

After this patch, this ends up throwing an error now (which I think is a
logical thing to do):

qemu-system-x86_64: -drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on: Node '#block238' is read only

How this affects vvfat (and rbd) should be documented in the commit message,
however, so I should ammend that if we keep this behavior.

One other possible option is to treat the vvfat 'rw' option as meaning
"enable writes, iff the block device is writable".  With that
interpretation, we could do something different in the above scenario:
silently fail to set bs->read_only to 'true' in the vvfat driver, and keep
it r/o.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function Jeff Cody
@ 2017-04-07  9:06   ` Stefan Hajnoczi
  0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:06 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow, kwolf

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

On Wed, Apr 05, 2017 at 02:28:43PM -0400, Jeff Cody wrote:
> We have a helper wrapper for checking for the BDS read_only flag,
> add a helper wrapper to set the read_only flag as well.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 5 +++++
>  block/bochs.c         | 2 +-
>  block/cloop.c         | 2 +-
>  block/dmg.c           | 2 +-
>  block/rbd.c           | 2 +-
>  block/vvfat.c         | 4 ++--
>  include/block/block.h | 1 +
>  7 files changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled Jeff Cody
  2017-04-05 19:16   ` John Snow
@ 2017-04-07  9:13   ` Stefan Hajnoczi
  2017-04-07  9:45   ` Stefan Hajnoczi
  2 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:13 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow, kwolf

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

On Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:

Minor comments but:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/block.c b/block.c
> index 7b4c7ef..f60d5ea 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
> +    /* Do not set read_only if copy_on_read is enabled */
> +    if (bs->copy_on_read && read_only) {
> +        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",

Users might be puzzled by "COR".  The -drive option is called
"copy-on-read" so spelling it out is clearer than using an acronym.

> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto fail;
>          }
> -        bdrv_set_read_only(bs, false);
> +        ret = bdrv_set_read_only(bs, false, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }

read_only = false by default.  There's no need to set it now that you've
moved the bdrv_set_read_only(bs, true) call.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 4/9] block: code movement
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 4/9] block: code movement Jeff Cody
  2017-04-05 20:25   ` John Snow
@ 2017-04-07  9:16   ` Stefan Hajnoczi
  1 sibling, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:16 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow, kwolf

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

On Wed, Apr 05, 2017 at 02:28:46PM -0400, Jeff Cody wrote:
> Move bdrv_is_read_only() up with its friends.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()
  2017-04-06  0:23     ` Jeff Cody
@ 2017-04-07  9:18       ` Stefan Hajnoczi
  0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:18 UTC (permalink / raw)
  To: Jeff Cody; +Cc: John Snow, qemu-devel, kwolf, qemu-block

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

On Wed, Apr 05, 2017 at 08:23:12PM -0400, Jeff Cody wrote:
> On Wed, Apr 05, 2017 at 04:28:44PM -0400, John Snow wrote:
> > On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > > diff --git a/include/block/block.h b/include/block/block.h
> > > index beb563a..0049b57 100644
> > > --- a/include/block/block.h
> > > +++ b/include/block/block.h
> > > @@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> > >                              int64_t sector_num, int nb_sectors, int *pnum);
> > >  
> > >  bool bdrv_is_read_only(BlockDriverState *bs);
> > > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> > >  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> > >  bool bdrv_is_sg(BlockDriverState *bs);
> > >  bool bdrv_is_inserted(BlockDriverState *bs);
> > > 
> > 
> > Maybe I'm just green, but I find it weird that the function is named
> > try_x when we aren't actually trying to do anything, we're checking to
> > see if we can.
> 
> Yeah, not a good name.  Maybe bdrv_set_read_only_test() or
> bdrv_test_set_read_only()?

This one reads nicely:

  if (bdrv_can_set_read_only(...)) {
      ...
  }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled Jeff Cody
  2017-04-05 19:16   ` John Snow
  2017-04-07  9:13   ` Stefan Hajnoczi
@ 2017-04-07  9:45   ` Stefan Hajnoczi
  2 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:45 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow, kwolf

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

On Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:
> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto fail;
>          }
> -        bdrv_set_read_only(bs, false);
> +        ret = bdrv_set_read_only(bs, false, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }

I realized later in the series why you are doing this.

The error code path introduces a resource leak: enable_write_target()
has already been called and isn't cleaned up by the fail label.

It would be cleaner to check that bs is writable before calling
enable_write_target().

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names Jeff Cody
@ 2017-04-07  9:47   ` Stefan Hajnoczi
  0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:47 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow, kwolf

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

On Wed, Apr 05, 2017 at 02:28:49PM -0400, Jeff Cody wrote:
> Update 'clientname' to be 'user', which tracks better with both
> the QAPI and rados viarable naming.
> 
> Update 'name' to be 'image_name', as it indicates the rbd image.
> Naming it 'image' would have been ideal, but we are using that for
> the rados_image_t value returned by rbd_open().
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only Jeff Cody
@ 2017-04-07  9:52   ` Stefan Hajnoczi
  0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:52 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow, kwolf

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

On Wed, Apr 05, 2017 at 02:28:50PM -0400, Jeff Cody wrote:
> We do not want to just blindly set bs->read_only.  The only time
> we need to set it with the current rbd driver, is if we are using an
> rbd snapshot.  In that case, we must set it to read-only.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index c5e1aeb..35853c9 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -635,6 +635,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_shutdown;
>      }
>  
> +    /* rbd_open is always r/w */
>      r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
>      if (r < 0) {
>          error_setg_errno(errp, -r, "error reading header from %s",
> @@ -642,10 +643,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_open;
>      }
>  
> -    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> -    if (r < 0) {
> -        error_propagate(errp, local_err);
> -        goto failed_open;
> +    /* If we are using an rbd snapshot, we must be r/o, otherwise
> +     * leave as-is */
> +    if (s->snap != NULL) {
> +        r = bdrv_set_read_only(bs, true, &local_err);
> +        if (r < 0) {
> +            error_propagate(errp, local_err);
> +            goto failed_open;
> +        }

This patch series breaks git-bisect(1) for -drive
file.driver=rbd,file.read-only=on since bdrv_set_read_only(bs, false,
&local_err) fails.

Please squash this patch where you added the bdrv_set_read_only() errp
argument.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen()
  2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen() Jeff Cody
@ 2017-04-07  9:53   ` Stefan Hajnoczi
  0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2017-04-07  9:53 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, jsnow, kwolf

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

On Wed, Apr 05, 2017 at 02:28:51PM -0400, Jeff Cody wrote:
> This adds support for reopen in rbd, for changing between r/w and r/o.
> 
> Note, that this is only a flag change, but we will block a change from
> r/o to r/w if we are using an RBD internal snapshot.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2017-04-07  9:53 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-05 18:28 [Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup Jeff Cody
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function Jeff Cody
2017-04-07  9:06   ` Stefan Hajnoczi
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled Jeff Cody
2017-04-05 19:16   ` John Snow
2017-04-05 23:50     ` Jeff Cody
2017-04-07  9:13   ` Stefan Hajnoczi
2017-04-07  9:45   ` Stefan Hajnoczi
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only Jeff Cody
2017-04-05 19:20   ` John Snow
2017-04-05 19:26     ` Eric Blake
2017-04-06  0:20       ` Jeff Cody
2017-04-06  0:55         ` Jeff Cody
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 4/9] block: code movement Jeff Cody
2017-04-05 20:25   ` John Snow
2017-04-07  9:16   ` Stefan Hajnoczi
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only() Jeff Cody
2017-04-05 20:28   ` John Snow
2017-04-06  0:23     ` Jeff Cody
2017-04-07  9:18       ` Stefan Hajnoczi
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen Jeff Cody
2017-04-05 20:25   ` John Snow
2017-04-06  0:27     ` Jeff Cody
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names Jeff Cody
2017-04-07  9:47   ` Stefan Hajnoczi
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only Jeff Cody
2017-04-07  9:52   ` Stefan Hajnoczi
2017-04-05 18:28 ` [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen() Jeff Cody
2017-04-07  9:53   ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.