[Qemu-devel] [PATCH 0/4] rbd cleanup and settings precedence fixes

[Qemu-devel] [PATCH 0/4] rbd cleanup and settings precedence fixes

[Josh Durgin]

Patches 1 and 2 are simple cleanups. 3 and 4 fix the precedence of
cache options and ceph settings. The cache option precedence in
particular is important due to the potential for misconfigurations
(ceph.conf setting rbd_cache=true, qemu setting cache=none) to
accidentally put data at risk.

Josh Durgin (4):
  rbd: remove unused constants and fields
  MAINTAINERS: update email address
  rbd: make qemu's cache setting override any ceph setting
  rbd: fix ceph settings precedence

 MAINTAINERS |  2 +-
 block/rbd.c | 64 +++++++++++++++++++++++++++++++++----------------------------
 2 files changed, 36 insertions(+), 30 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] rbd: remove unused constants and fields

[Josh Durgin]

RBDAIOCB.status was only used for cancel, which was removed in
7691e24dbebb46658e89b3f950fda6ec78bbb823.

RBDAIOCB.sector_num was never used.

RADOSCB.done and rcbid were never used.

RBD_FD* are obsolete since the pipe was removed in
e04fb07fd1676e9facd7f3f878c1bbe03bccd26b.

Signed-off-by: Josh Durgin <jdurgin@redhat.com>
---
 block/rbd.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index fbe87e0..50b5f6b 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -74,25 +74,18 @@ typedef struct RBDAIOCB {
     QEMUIOVector *qiov;
     char *bounce;
     RBDAIOCmd cmd;
-    int64_t sector_num;
     int error;
     struct BDRVRBDState *s;
-    int status;
 } RBDAIOCB;
 
 typedef struct RADOSCB {
-    int rcbid;
     RBDAIOCB *acb;
     struct BDRVRBDState *s;
-    int done;
     int64_t size;
     char *buf;
     int64_t ret;
 } RADOSCB;
 
-#define RBD_FD_READ 0
-#define RBD_FD_WRITE 1
-
 typedef struct BDRVRBDState {
     rados_t cluster;
     rados_ioctx_t io_ctx;
@@ -405,7 +398,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
     }
     qemu_vfree(acb->bounce);
     acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
-    acb->status = 0;
 
     qemu_aio_unref(acb);
 }
@@ -621,7 +613,6 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
     acb->error = 0;
     acb->s = s;
     acb->bh = NULL;
-    acb->status = -EINPROGRESS;
 
     if (cmd == RBD_AIO_WRITE) {
         qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
@@ -633,7 +624,6 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
     size = nb_sectors * BDRV_SECTOR_SIZE;
 
     rcb = g_new(RADOSCB, 1);
-    rcb->done = 0;
     rcb->acb = acb;
     rcb->buf = buf;
     rcb->s = acb->s;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/4] rbd: remove unused constants and fields

[Jeff Cody]

On Wed, Jun 10, 2015 at 08:28:43PM -0700, Josh Durgin wrote:
> RBDAIOCB.status was only used for cancel, which was removed in
> 7691e24dbebb46658e89b3f950fda6ec78bbb823.
> 
> RBDAIOCB.sector_num was never used.
> 
> RADOSCB.done and rcbid were never used.
> 
> RBD_FD* are obsolete since the pipe was removed in
> e04fb07fd1676e9facd7f3f878c1bbe03bccd26b.
> 
> Signed-off-by: Josh Durgin <jdurgin@redhat.com>
> ---
>  block/rbd.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index fbe87e0..50b5f6b 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -74,25 +74,18 @@ typedef struct RBDAIOCB {
>      QEMUIOVector *qiov;
>      char *bounce;
>      RBDAIOCmd cmd;
> -    int64_t sector_num;
>      int error;
>      struct BDRVRBDState *s;
> -    int status;
>  } RBDAIOCB;
>  
>  typedef struct RADOSCB {
> -    int rcbid;
>      RBDAIOCB *acb;
>      struct BDRVRBDState *s;
> -    int done;
>      int64_t size;
>      char *buf;
>      int64_t ret;
>  } RADOSCB;
>  
> -#define RBD_FD_READ 0
> -#define RBD_FD_WRITE 1
> -
>  typedef struct BDRVRBDState {
>      rados_t cluster;
>      rados_ioctx_t io_ctx;
> @@ -405,7 +398,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
>      }
>      qemu_vfree(acb->bounce);
>      acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
> -    acb->status = 0;
>  
>      qemu_aio_unref(acb);
>  }
> @@ -621,7 +613,6 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
>      acb->error = 0;
>      acb->s = s;
>      acb->bh = NULL;
> -    acb->status = -EINPROGRESS;
>  
>      if (cmd == RBD_AIO_WRITE) {
>          qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
> @@ -633,7 +624,6 @@ static BlockAIOCB *rbd_start_aio(BlockDriverState *bs,
>      size = nb_sectors * BDRV_SECTOR_SIZE;
>  
>      rcb = g_new(RADOSCB, 1);
> -    rcb->done = 0;
>      rcb->acb = acb;
>      rcb->buf = buf;
>      rcb->s = acb->s;
> -- 
> 1.9.1
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH 2/4] MAINTAINERS: update email address

[Josh Durgin]

The old one still works for now, but will not work indefinitely.

Signed-off-by: Josh Durgin <jdurgin@redhat.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ed8215..34eeb88 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1135,7 +1135,7 @@ S: Supported
 F: block/vmdk.c
 
 RBD
-M: Josh Durgin <josh.durgin@inktank.com>
+M: Josh Durgin <jdurgin@redhat.com>
 M: Jeff Cody <jcody@redhat.com>
 L: qemu-block@nongnu.org
 S: Supported
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/4] MAINTAINERS: update email address

[Jeff Cody]

On Wed, Jun 10, 2015 at 08:28:44PM -0700, Josh Durgin wrote:
> The old one still works for now, but will not work indefinitely.
> 
> Signed-off-by: Josh Durgin <jdurgin@redhat.com>
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4ed8215..34eeb88 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1135,7 +1135,7 @@ S: Supported
>  F: block/vmdk.c
>  
>  RBD
> -M: Josh Durgin <josh.durgin@inktank.com>
> +M: Josh Durgin <jdurgin@redhat.com>
>  M: Jeff Cody <jcody@redhat.com>
>  L: qemu-block@nongnu.org
>  S: Supported
> -- 
> 1.9.1
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH 3/4] rbd: make qemu's cache setting override any ceph setting

[Josh Durgin]

To be safe, when cache=none is used ceph settings should not be able
to override it to turn on caching. This was previously possible with
rbd_cache=true in the rbd device configuration or a ceph configuration
file. Similarly, rbd settings could have turned off caching when qemu
requested it, although this would just be a performance problem.

Fix this by changing rbd's cache setting to match qemu after all other
ceph settings have been applied.

Signed-off-by: Josh Durgin <jdurgin@redhat.com>
---
 block/rbd.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 50b5f6b..00d027d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -460,6 +460,18 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         s->snap = g_strdup(snap_buf);
     }
 
+    if (strstr(conf, "conf=") == NULL) {
+        /* try default location, but ignore failure */
+        rados_conf_read_file(s->cluster, NULL);
+    }
+
+    if (conf[0] != '\0') {
+        r = qemu_rbd_set_conf(s->cluster, conf, errp);
+        if (r < 0) {
+            goto failed_shutdown;
+        }
+    }
+
     /*
      * Fallback to more conservative semantics if setting cache
      * options fails. Ignore errors from setting rbd_cache because the
@@ -473,18 +485,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         rados_conf_set(s->cluster, "rbd_cache", "true");
     }
 
-    if (strstr(conf, "conf=") == NULL) {
-        /* try default location, but ignore failure */
-        rados_conf_read_file(s->cluster, NULL);
-    }
-
-    if (conf[0] != '\0') {
-        r = qemu_rbd_set_conf(s->cluster, conf, errp);
-        if (r < 0) {
-            goto failed_shutdown;
-        }
-    }
-
     r = rados_connect(s->cluster);
     if (r < 0) {
         error_setg(errp, "error connecting");
-- 
1.9.1

Re: [Qemu-devel] [PATCH 3/4] rbd: make qemu's cache setting override any ceph setting

[Jeff Cody]

On Wed, Jun 10, 2015 at 08:28:45PM -0700, Josh Durgin wrote:
> To be safe, when cache=none is used ceph settings should not be able
> to override it to turn on caching. This was previously possible with
> rbd_cache=true in the rbd device configuration or a ceph configuration
> file. Similarly, rbd settings could have turned off caching when qemu
> requested it, although this would just be a performance problem.
> 
> Fix this by changing rbd's cache setting to match qemu after all other
> ceph settings have been applied.
> 
> Signed-off-by: Josh Durgin <jdurgin@redhat.com>
> ---
>  block/rbd.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 50b5f6b..00d027d 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -460,6 +460,18 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          s->snap = g_strdup(snap_buf);
>      }
>  
> +    if (strstr(conf, "conf=") == NULL) {
> +        /* try default location, but ignore failure */
> +        rados_conf_read_file(s->cluster, NULL);
> +    }
> +
> +    if (conf[0] != '\0') {
> +        r = qemu_rbd_set_conf(s->cluster, conf, errp);
> +        if (r < 0) {
> +            goto failed_shutdown;
> +        }
> +    }
> +
>      /*
>       * Fallback to more conservative semantics if setting cache
>       * options fails. Ignore errors from setting rbd_cache because the
> @@ -473,18 +485,6 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          rados_conf_set(s->cluster, "rbd_cache", "true");
>      }
>  
> -    if (strstr(conf, "conf=") == NULL) {
> -        /* try default location, but ignore failure */
> -        rados_conf_read_file(s->cluster, NULL);
> -    }
> -
> -    if (conf[0] != '\0') {
> -        r = qemu_rbd_set_conf(s->cluster, conf, errp);
> -        if (r < 0) {
> -            goto failed_shutdown;
> -        }
> -    }
> -
>      r = rados_connect(s->cluster);
>      if (r < 0) {
>          error_setg(errp, "error connecting");
> -- 
> 1.9.1
> 
Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH 4/4] rbd: fix ceph settings precedence

[Josh Durgin]

Apply the ceph settings from a config file before any ceph settings
from the command line. Since the ceph config file location may be
specified on the command line, parse it once to read the config file,
and do a second pass to apply the rest of the command line ceph
options.

Signed-off-by: Josh Durgin <jdurgin@redhat.com>
---
 block/rbd.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 00d027d..a60a19d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -228,7 +228,9 @@ static char *qemu_rbd_parse_clientname(const char *conf, char *clientname)
     return NULL;
 }
 
-static int qemu_rbd_set_conf(rados_t cluster, const char *conf, Error **errp)
+static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
+                             bool only_read_conf_file,
+                             Error **errp)
 {
     char *p, *buf;
     char name[RBD_MAX_CONF_NAME_SIZE];
@@ -260,14 +262,18 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf, Error **errp)
         qemu_rbd_unescape(value);
 
         if (strcmp(name, "conf") == 0) {
-            ret = rados_conf_read_file(cluster, value);
-            if (ret < 0) {
-                error_setg(errp, "error reading conf file %s", value);
-                break;
+            /* read the conf file alone, so it doesn't override more
+               specific settings for a particular device */
+            if (only_read_conf_file) {
+                ret = rados_conf_read_file(cluster, value);
+                if (ret < 0) {
+                    error_setg(errp, "error reading conf file %s", value);
+                    break;
+                }
             }
         } else if (strcmp(name, "id") == 0) {
             /* ignore, this is parsed by qemu_rbd_parse_clientname() */
-        } else {
+        } else if (!only_read_conf_file) {
             ret = rados_conf_set(cluster, name, value);
             if (ret < 0) {
                 error_setg(errp, "invalid conf option %s", name);
@@ -330,10 +336,15 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     if (strstr(conf, "conf=") == NULL) {
         /* try default location, but ignore failure */
         rados_conf_read_file(cluster, NULL);
+    } else if (conf[0] != '\0' &&
+               qemu_rbd_set_conf(cluster, conf, true, &local_err) < 0) {
+        rados_shutdown(cluster);
+        error_propagate(errp, local_err);
+        return -EIO;
     }
 
     if (conf[0] != '\0' &&
-        qemu_rbd_set_conf(cluster, conf, &local_err) < 0) {
+        qemu_rbd_set_conf(cluster, conf, false, &local_err) < 0) {
         rados_shutdown(cluster);
         error_propagate(errp, local_err);
         return -EIO;
@@ -463,10 +474,15 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     if (strstr(conf, "conf=") == NULL) {
         /* try default location, but ignore failure */
         rados_conf_read_file(s->cluster, NULL);
+    } else if (conf[0] != '\0') {
+        r = qemu_rbd_set_conf(s->cluster, conf, true, errp);
+        if (r < 0) {
+            goto failed_shutdown;
+        }
     }
 
     if (conf[0] != '\0') {
-        r = qemu_rbd_set_conf(s->cluster, conf, errp);
+        r = qemu_rbd_set_conf(s->cluster, conf, false, errp);
         if (r < 0) {
             goto failed_shutdown;
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/4] rbd cleanup and settings precedence fixes

[Josh Durgin]

Ping

On 06/10/2015 08:28 PM, Josh Durgin wrote:
> Patches 1 and 2 are simple cleanups. 3 and 4 fix the precedence of
> cache options and ceph settings. The cache option precedence in
> particular is important due to the potential for misconfigurations
> (ceph.conf setting rbd_cache=true, qemu setting cache=none) to
> accidentally put data at risk.
>
> Josh Durgin (4):
>    rbd: remove unused constants and fields
>    MAINTAINERS: update email address
>    rbd: make qemu's cache setting override any ceph setting
>    rbd: fix ceph settings precedence
>
>   MAINTAINERS |  2 +-
>   block/rbd.c | 64 +++++++++++++++++++++++++++++++++----------------------------
>   2 files changed, 36 insertions(+), 30 deletions(-)
>

Re: [Qemu-devel] [Qemu-block] [PATCH 0/4] rbd cleanup and settings precedence fixes

[Stefan Hajnoczi]

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

On Tue, Jun 30, 2015 at 11:28:18AM -0700, Josh Durgin wrote:
> Ping
> 
> On 06/10/2015 08:28 PM, Josh Durgin wrote:
> >Patches 1 and 2 are simple cleanups. 3 and 4 fix the precedence of
> >cache options and ceph settings. The cache option precedence in
> >particular is important due to the potential for misconfigurations
> >(ceph.conf setting rbd_cache=true, qemu setting cache=none) to
> >accidentally put data at risk.
> >
> >Josh Durgin (4):
> >   rbd: remove unused constants and fields
> >   MAINTAINERS: update email address
> >   rbd: make qemu's cache setting override any ceph setting
> >   rbd: fix ceph settings precedence
> >
> >  MAINTAINERS |  2 +-
> >  block/rbd.c | 64 +++++++++++++++++++++++++++++++++----------------------------
> >  2 files changed, 36 insertions(+), 30 deletions(-)

This should go via Jeff:

  $ scripts/get_maintainer.pl -f block/rbd.c
  Josh Durgin <josh.durgin@inktank.com> (supporter:RBD)
  Jeff Cody <jcody@redhat.com> (supporter:RBD)
  Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
  qemu-block@nongnu.org (open list:RBD)

Jeff is currently on vacation but will be back before the QEMU 2.4-rc1
tag is made.

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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/4] rbd cleanup and settings precedence fixes

[Kevin Wolf]

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

Am 09.07.2015 um 11:59 hat Stefan Hajnoczi geschrieben:
> On Tue, Jun 30, 2015 at 11:28:18AM -0700, Josh Durgin wrote:
> > Ping
> > 
> > On 06/10/2015 08:28 PM, Josh Durgin wrote:
> > >Patches 1 and 2 are simple cleanups. 3 and 4 fix the precedence of
> > >cache options and ceph settings. The cache option precedence in
> > >particular is important due to the potential for misconfigurations
> > >(ceph.conf setting rbd_cache=true, qemu setting cache=none) to
> > >accidentally put data at risk.
> > >
> > >Josh Durgin (4):
> > >   rbd: remove unused constants and fields
> > >   MAINTAINERS: update email address
> > >   rbd: make qemu's cache setting override any ceph setting
> > >   rbd: fix ceph settings precedence
> > >
> > >  MAINTAINERS |  2 +-
> > >  block/rbd.c | 64 +++++++++++++++++++++++++++++++++----------------------------
> > >  2 files changed, 36 insertions(+), 30 deletions(-)
> 
> This should go via Jeff:
> 
>   $ scripts/get_maintainer.pl -f block/rbd.c
>   Josh Durgin <josh.durgin@inktank.com> (supporter:RBD)
>   Jeff Cody <jcody@redhat.com> (supporter:RBD)
>   Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
>   qemu-block@nongnu.org (open list:RBD)
> 
> Jeff is currently on vacation but will be back before the QEMU 2.4-rc1
> tag is made.

Considering that Josh is the primary maintainer for rbd and Jeff is just
merging patches for him, and that Jeff has already given R-b for patches
1 to 3, and that Jeff doesn't seem to be back yet, and that the patches
look good to me, I'm taking this through my tree for -rc1.

(I guess this is my longest "thanks, applied" sentence so far...)

Kevin

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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/4] rbd cleanup and settings precedence fixes

[Josh Durgin]

On 07/14/2015 07:20 AM, Kevin Wolf wrote:
> Am 09.07.2015 um 11:59 hat Stefan Hajnoczi geschrieben:
>> On Tue, Jun 30, 2015 at 11:28:18AM -0700, Josh Durgin wrote:
>>> Ping
>>>
>>> On 06/10/2015 08:28 PM, Josh Durgin wrote:
>>>> Patches 1 and 2 are simple cleanups. 3 and 4 fix the precedence of
>>>> cache options and ceph settings. The cache option precedence in
>>>> particular is important due to the potential for misconfigurations
>>>> (ceph.conf setting rbd_cache=true, qemu setting cache=none) to
>>>> accidentally put data at risk.
>>>>
>>>> Josh Durgin (4):
>>>>    rbd: remove unused constants and fields
>>>>    MAINTAINERS: update email address
>>>>    rbd: make qemu's cache setting override any ceph setting
>>>>    rbd: fix ceph settings precedence
>>>>
>>>>   MAINTAINERS |  2 +-
>>>>   block/rbd.c | 64 +++++++++++++++++++++++++++++++++----------------------------
>>>>   2 files changed, 36 insertions(+), 30 deletions(-)
>>
>> This should go via Jeff:
>>
>>    $ scripts/get_maintainer.pl -f block/rbd.c
>>    Josh Durgin <josh.durgin@inktank.com> (supporter:RBD)
>>    Jeff Cody <jcody@redhat.com> (supporter:RBD)
>>    Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
>>    qemu-block@nongnu.org (open list:RBD)
>>
>> Jeff is currently on vacation but will be back before the QEMU 2.4-rc1
>> tag is made.
>
> Considering that Josh is the primary maintainer for rbd and Jeff is just
> merging patches for him, and that Jeff has already given R-b for patches
> 1 to 3, and that Jeff doesn't seem to be back yet, and that the patches
> look good to me, I'm taking this through my tree for -rc1.
>
> (I guess this is my longest "thanks, applied" sentence so far...)

Thanks!
Josh