[PATCH 0/6] rbd: updates from Josh Durgin

[PATCH 0/6] rbd: updates from Josh Durgin

[Alex Elder]

I am posting this series on behalf of Josh Durgin, who originally
created these at the end of last year.  I've updated them to apply
to the current rbd code.

The first two fix bugs, the last is a documentation fix, and the
others are cleanups.

					-Alex

[PATCH 1/6] rbd: protect read of snapshot sequence number

[Alex Elder]

This is updated whenever a snapshot is added or deleted, and the
snapc pointer is changed with every refresh of the header.

Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
Reviewed-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |    4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1685,7 +1685,9 @@
  	if (ret < 0)
  		return ret;

-	dev->header.snapc->seq =  new_snapid;
+	down_write(&dev->header_rwsem);
+	dev->header.snapc->seq = new_snapid;
+	up_write(&dev->header_rwsem);

  	return 0;
  bad:

Re: [PATCH 1/6] rbd: protect read of snapshot sequence number

[Yehuda Sadeh Weinraub]

On Wed, Apr 18, 2012 at 7:40 AM, Alex Elder <elder@dreamhost.com> wrote:
> This is updated whenever a snapshot is added or deleted, and the
> snapc pointer is changed with every refresh of the header.
>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> Reviewed-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1685,7 +1685,9 @@
>        if (ret < 0)
>                return ret;
>
> -       dev->header.snapc->seq =  new_snapid;
> +       down_write(&dev->header_rwsem);
> +       dev->header.snapc->seq = new_snapid;
> +       up_write(&dev->header_rwsem);
>
>        return 0;
>  bad:

Reviewed-by: Yehuda Sadeh <yehuda@hq.newdream.net>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] rbd: store snapshot id instead of index

[Alex Elder]

When a device was open at a snapshot, and snapshots were deleted or
added, data from the wrong snapshot could be read. Instead of
assuming the snap context is constant, store the actual snap id when
the device is initialized, and rely on the OSDs to signal an error
if we try reading from a snapshot that was deleted.

Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
Reviewed-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   27 +++++----------------------
  1 file changed, 5 insertions(+), 22 deletions(-)

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -175,8 +175,7 @@
  	/* protects updating the header */
  	struct rw_semaphore     header_rwsem;
  	char                    snap_name[RBD_MAX_SNAP_NAME_LEN];
-	u32 cur_snap;	/* index+1 of current snapshot within snap context
-			   0 - for the head */
+	u64                     snap_id;	/* current snapshot id */
  	int read_only;

  	struct list_head	node;
@@ -554,21 +553,6 @@
  	return -ENOMEM;
  }

-static int snap_index(struct rbd_image_header *header, int snap_num)
-{
-	return header->total_snaps - snap_num;
-}
-
-static u64 cur_snap_id(struct rbd_device *rbd_dev)
-{
-	struct rbd_image_header *header = &rbd_dev->header;
-
-	if (!rbd_dev->cur_snap)
-		return 0;
-
-	return header->snapc->snaps[snap_index(header, rbd_dev->cur_snap)];
-}
-
  static int snap_by_name(struct rbd_image_header *header, const char 
*snap_name,
  			u64 *seq, u64 *size)
  {
@@ -607,7 +591,7 @@
  			snapc->seq = header->snap_seq;
  		else
  			snapc->seq = 0;
-		dev->cur_snap = 0;
+		dev->snap_id = CEPH_NOSNAP;
  		dev->read_only = 0;
  		if (size)
  			*size = header->image_size;
@@ -615,8 +599,7 @@
  		ret = snap_by_name(header, dev->snap_name, &snapc->seq, size);
  		if (ret < 0)
  			goto done;
-
-		dev->cur_snap = header->total_snaps - ret;
+		dev->snap_id = snapc->seq;
  		dev->read_only = 1;
  	}

@@ -1523,7 +1506,7 @@
  					      coll, cur_seg);
  			else
  				rbd_req_read(rq, rbd_dev,
-					     cur_snap_id(rbd_dev),
+					     rbd_dev->snap_id,
  					     ofs,
  					     op_size, bio,
  					     coll, cur_seg);
@@ -1658,7 +1641,7 @@
  	struct ceph_mon_client *monc;

  	/* we should create a snapshot only if we're pointing at the head */
-	if (dev->cur_snap)
+	if (dev->snap_id != CEPH_NOSNAP)
  		return -EINVAL;

  	monc = &dev->rbd_client->client->monc;

Re: [PATCH 2/6] rbd: store snapshot id instead of index

[Yehuda Sadeh Weinraub]

On Wed, Apr 18, 2012 at 7:41 AM, Alex Elder <elder@dreamhost.com> wrote:
> When a device was open at a snapshot, and snapshots were deleted or
> added, data from the wrong snapshot could be read. Instead of
> assuming the snap context is constant, store the actual snap id when
> the device is initialized, and rely on the OSDs to signal an error
> if we try reading from a snapshot that was deleted.
>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> Reviewed-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c |   27 +++++----------------------
>  1 file changed, 5 insertions(+), 22 deletions(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -175,8 +175,7 @@
>        /* protects updating the header */
>        struct rw_semaphore     header_rwsem;
>        char                    snap_name[RBD_MAX_SNAP_NAME_LEN];
> -       u32 cur_snap;   /* index+1 of current snapshot within snap context
> -                          0 - for the head */
> +       u64                     snap_id;        /* current snapshot id */
>        int read_only;
>
>        struct list_head        node;
> @@ -554,21 +553,6 @@
>        return -ENOMEM;
>  }
>
> -static int snap_index(struct rbd_image_header *header, int snap_num)
> -{
> -       return header->total_snaps - snap_num;
> -}
> -
> -static u64 cur_snap_id(struct rbd_device *rbd_dev)
> -{
> -       struct rbd_image_header *header = &rbd_dev->header;
> -
> -       if (!rbd_dev->cur_snap)
> -               return 0;
> -
> -       return header->snapc->snaps[snap_index(header, rbd_dev->cur_snap)];
> -}
> -
>  static int snap_by_name(struct rbd_image_header *header, const char
> *snap_name,
>                        u64 *seq, u64 *size)
>  {
> @@ -607,7 +591,7 @@
>                        snapc->seq = header->snap_seq;
>                else
>                        snapc->seq = 0;
> -               dev->cur_snap = 0;
> +               dev->snap_id = CEPH_NOSNAP;
>                dev->read_only = 0;
>                if (size)
>                        *size = header->image_size;
> @@ -615,8 +599,7 @@
>                ret = snap_by_name(header, dev->snap_name, &snapc->seq,
> size);
>                if (ret < 0)
>                        goto done;
> -
> -               dev->cur_snap = header->total_snaps - ret;
> +               dev->snap_id = snapc->seq;
>                dev->read_only = 1;
>        }
>
> @@ -1523,7 +1506,7 @@
>                                              coll, cur_seg);
>                        else
>                                rbd_req_read(rq, rbd_dev,
> -                                            cur_snap_id(rbd_dev),
> +                                            rbd_dev->snap_id,
>                                             ofs,
>                                             op_size, bio,
>                                             coll, cur_seg);
> @@ -1658,7 +1641,7 @@
>        struct ceph_mon_client *monc;
>
>        /* we should create a snapshot only if we're pointing at the head */
> -       if (dev->cur_snap)
> +       if (dev->snap_id != CEPH_NOSNAP)
>                return -EINVAL;
>
>        monc = &dev->rbd_client->client->monc;

Reviewed-by: Yehuda Sadeh <yehuda@hq.newdream.net>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] rbd: remove conditional snapid parameters

[Alex Elder]

The snapid parameters passed to rbd_do_op() and rbd_req_sync_op()
are now always either a valid snapid or an explicit CEPH_NOSNAP.

[elder@dreamhost.com: Rephrased the description]

Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
Reviewed-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1153,7 +1153,7 @@
  			 int coll_index)
  {
  	return rbd_do_op(rq, rbd_dev, NULL,
-			 (snapid ? snapid : CEPH_NOSNAP),
+			 snapid,
  			 CEPH_OSD_OP_READ,
  			 CEPH_OSD_FLAG_READ,
  			 2,
@@ -1172,7 +1172,7 @@
  			  u64 *ver)
  {
  	return rbd_req_sync_op(dev, NULL,
-			       (snapid ? snapid : CEPH_NOSNAP),
+			       snapid,
  			       CEPH_OSD_OP_READ,
  			       CEPH_OSD_FLAG_READ,
  			       NULL,

Re: [PATCH 3/6] rbd: remove conditional snapid parameters

[Yehuda Sadeh Weinraub]

On Wed, Apr 18, 2012 at 7:41 AM, Alex Elder <elder@dreamhost.com> wrote:
> The snapid parameters passed to rbd_do_op() and rbd_req_sync_op()
> are now always either a valid snapid or an explicit CEPH_NOSNAP.
>
> [elder@dreamhost.com: Rephrased the description]
>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> Reviewed-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1153,7 +1153,7 @@
>                         int coll_index)
>  {
>        return rbd_do_op(rq, rbd_dev, NULL,
> -                        (snapid ? snapid : CEPH_NOSNAP),
> +                        snapid,
>                         CEPH_OSD_OP_READ,
>                         CEPH_OSD_FLAG_READ,
>                         2,
> @@ -1172,7 +1172,7 @@
>                          u64 *ver)
>  {
>        return rbd_req_sync_op(dev, NULL,
> -                              (snapid ? snapid : CEPH_NOSNAP),
> +                              snapid,
>                               CEPH_OSD_OP_READ,
>                               CEPH_OSD_FLAG_READ,
>                               NULL,
Reviewed-by: Yehuda Sadeh <yehuda@hq.newdream.net>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] rbd: fix snapshot size type

[Alex Elder]

Snapshot sizes should be the same type as regular image sizes. This
only affects their displayed size in sysfs, not the reported size of
an actual block device sizes.

Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
Reviewed-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |    6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -141,7 +141,7 @@
  struct rbd_snap {
  	struct	device		dev;
  	const char		*name;
-	size_t			size;
+	u64			size;
  	struct list_head	node;
  	u64			id;
  };
@@ -1936,7 +1936,7 @@
  {
  	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);

-	return sprintf(buf, "%zd\n", snap->size);
+	return sprintf(buf, "%llu\n", (unsigned long long)snap->size);
  }

  static ssize_t rbd_snap_id_show(struct device *dev,
@@ -1945,7 +1945,7 @@
  {
  	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);

-	return sprintf(buf, "%llu\n", (unsigned long long) snap->id);
+	return sprintf(buf, "%llu\n", (unsigned long long)snap->id);
  }

  static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL);

Re: [PATCH 4/6] rbd: fix snapshot size type

[Yehuda Sadeh Weinraub]

On Wed, Apr 18, 2012 at 7:41 AM, Alex Elder <elder@dreamhost.com> wrote:
> Snapshot sizes should be the same type as regular image sizes. This
> only affects their displayed size in sysfs, not the reported size of
> an actual block device sizes.
>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> Reviewed-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -141,7 +141,7 @@
>  struct rbd_snap {
>        struct  device          dev;
>        const char              *name;
> -       size_t                  size;
> +       u64                     size;
>        struct list_head        node;
>        u64                     id;
>  };
> @@ -1936,7 +1936,7 @@
>  {
>        struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);
>
> -       return sprintf(buf, "%zd\n", snap->size);
> +       return sprintf(buf, "%llu\n", (unsigned long long)snap->size);
>  }
>
>  static ssize_t rbd_snap_id_show(struct device *dev,
> @@ -1945,7 +1945,7 @@
>  {
>        struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);
>
> -       return sprintf(buf, "%llu\n", (unsigned long long) snap->id);
> +       return sprintf(buf, "%llu\n", (unsigned long long)snap->id);
>  }
>
>  static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL);

Reviewed-by: Yehuda Sadeh <yehuda@hq.newdream.net>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] rbd: rename __rbd_update_snaps to __rbd_refresh_header

[Alex Elder]

This function rereads the entire header and handles any changes in
it, not just changes in snapshots.

Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
Reviewed-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

Index: b/drivers/block/rbd.c
===================================================================
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -240,7 +240,7 @@
  	put_device(&rbd_dev->dev);
  }

-static int __rbd_update_snaps(struct rbd_device *rbd_dev);
+static int __rbd_refresh_header(struct rbd_device *rbd_dev);

  static int rbd_open(struct block_device *bdev, fmode_t mode)
  {
@@ -1223,7 +1223,7 @@
  	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", dev->obj_md_name,
  		notify_id, (int)opcode);
  	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	rc = __rbd_update_snaps(dev);
+	rc = __rbd_refresh_header(dev);
  	mutex_unlock(&ctl_mutex);
  	if (rc)
  		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
@@ -1690,7 +1690,7 @@
  /*
   * only read the first part of the ondisk header, without the snaps info
   */
-static int __rbd_update_snaps(struct rbd_device *rbd_dev)
+static int __rbd_refresh_header(struct rbd_device *rbd_dev)
  {
  	int ret;
  	struct rbd_image_header h;
@@ -1877,7 +1877,7 @@

  	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);

-	rc = __rbd_update_snaps(rbd_dev);
+	rc = __rbd_refresh_header(rbd_dev);
  	if (rc < 0)
  		ret = rc;

@@ -2160,7 +2160,7 @@
  					 rbd_dev->header.obj_version);
  		if (ret == -ERANGE) {
  			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-			rc = __rbd_update_snaps(rbd_dev);
+			rc = __rbd_refresh_header(rbd_dev);
  			mutex_unlock(&ctl_mutex);
  			if (rc < 0)
  				return rc;
@@ -2545,7 +2545,7 @@
  	if (ret < 0)
  		goto err_unlock;

-	ret = __rbd_update_snaps(rbd_dev);
+	ret = __rbd_refresh_header(rbd_dev);
  	if (ret < 0)
  		goto err_unlock;

Re: [PATCH 5/6] rbd: rename __rbd_update_snaps to __rbd_refresh_header

[Yehuda Sadeh Weinraub]

On Wed, Apr 18, 2012 at 7:41 AM, Alex Elder <elder@dreamhost.com> wrote:
> This function rereads the entire header and handles any changes in
> it, not just changes in snapshots.
>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> Reviewed-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> Index: b/drivers/block/rbd.c
> ===================================================================
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -240,7 +240,7 @@
>        put_device(&rbd_dev->dev);
>  }
>
> -static int __rbd_update_snaps(struct rbd_device *rbd_dev);
> +static int __rbd_refresh_header(struct rbd_device *rbd_dev);
>
>  static int rbd_open(struct block_device *bdev, fmode_t mode)
>  {
> @@ -1223,7 +1223,7 @@
>        dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n", dev->obj_md_name,
>                notify_id, (int)opcode);
>        mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -       rc = __rbd_update_snaps(dev);
> +       rc = __rbd_refresh_header(dev);
>        mutex_unlock(&ctl_mutex);
>        if (rc)
>                pr_warning(RBD_DRV_NAME "%d got notification but failed to "
> @@ -1690,7 +1690,7 @@
>  /*
>  * only read the first part of the ondisk header, without the snaps info
>  */
> -static int __rbd_update_snaps(struct rbd_device *rbd_dev)
> +static int __rbd_refresh_header(struct rbd_device *rbd_dev)
>  {
>        int ret;
>        struct rbd_image_header h;
> @@ -1877,7 +1877,7 @@
>
>        mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
>
> -       rc = __rbd_update_snaps(rbd_dev);
> +       rc = __rbd_refresh_header(rbd_dev);
>        if (rc < 0)
>                ret = rc;
>
> @@ -2160,7 +2160,7 @@
>                                         rbd_dev->header.obj_version);
>                if (ret == -ERANGE) {
>                        mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -                       rc = __rbd_update_snaps(rbd_dev);
> +                       rc = __rbd_refresh_header(rbd_dev);
>                        mutex_unlock(&ctl_mutex);
>                        if (rc < 0)
>                                return rc;
> @@ -2545,7 +2545,7 @@
>        if (ret < 0)
>                goto err_unlock;
>
> -       ret = __rbd_update_snaps(rbd_dev);
> +       ret = __rbd_refresh_header(rbd_dev);
>        if (ret < 0)
>                goto err_unlock;
>

Reviewed-by: Yehuda Sadeh <yehuda@hq.newdream.net>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] rbd: correct sysfs snap attribute documentation

[Alex Elder]

Each attribute is prefixed with "snap_".

Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
Reviewed-by: Alex Elder <elder@dreamhost.com>
---
  Documentation/ABI/testing/sysfs-bus-rbd |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

Index: b/Documentation/ABI/testing/sysfs-bus-rbd
===================================================================
--- a/Documentation/ABI/testing/sysfs-bus-rbd
+++ b/Documentation/ABI/testing/sysfs-bus-rbd
@@ -65,11 +65,11 @@
  Entries under /sys/bus/rbd/devices/<dev-id>/snap_<snap-name>
  -------------------------------------------------------------

-id
+snap_id

  	The rados internal snapshot id assigned for this snapshot

-size
+snap_size

  	The size of the image when this snapshot was taken.

Re: [PATCH 6/6] rbd: correct sysfs snap attribute documentation

[Yehuda Sadeh Weinraub]

On Wed, Apr 18, 2012 at 7:41 AM, Alex Elder <elder@dreamhost.com> wrote:
> Each attribute is prefixed with "snap_".
>
> Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
> Reviewed-by: Alex Elder <elder@dreamhost.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-rbd |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: b/Documentation/ABI/testing/sysfs-bus-rbd
> ===================================================================
> --- a/Documentation/ABI/testing/sysfs-bus-rbd
> +++ b/Documentation/ABI/testing/sysfs-bus-rbd
> @@ -65,11 +65,11 @@
>  Entries under /sys/bus/rbd/devices/<dev-id>/snap_<snap-name>
>  -------------------------------------------------------------
>
> -id
> +snap_id
>
>        The rados internal snapshot id assigned for this snapshot
>
> -size
> +snap_size
>
>        The size of the image when this snapshot was taken.
>


Reviewed-by: Yehuda Sadeh <yehuda@hq.newdream.net>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html