rbd: finish up basic format 2 support

Encapsulate the code that handles the case where an image's size has been found to have changed. This is done in anticipation of the next patch, which will make this common code for format 1 and 2 images.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   22 +++++++++++++---------
  1 file changed, 13 insertions(+), 9 deletions(-)

drivers/block/rbd.c +0 -0

index bb3d9be..d36e6d7 100644

drivers/block/rbd.c +5 -0

+    dout("setting size to %llu sectors", (unsigned long long) size);
+    rbd_dev->mapping.size = (u64) size;
+    set_capacity(rbd_dev->disk, size);
+}
+
  /*
   * only read the first part of the ondisk header, without the snaps info

   */

drivers/block/rbd.c +3 -9

@@ -1731,15 +1741,9 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev, u64 *hver)
      down_write(&rbd_dev->header_rwsem);
 
      /* resized? */
-    if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
-        sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
-
-        if (size != (sector_t) rbd_dev->mapping.size) {
-            dout("setting size to %llu sectors",
-                (unsigned long long) size);
-            rbd_dev->mapping.size = (u64) size;
-            set_capacity(rbd_dev->disk, size);
-        }
+    if (rbd_dev->header.image_size != h.image_size) {
+        rbd_dev->header.image_size = h.image_size;
+        rbd_update_size(rbd_dev);
      }
 

      /* rbd_dev->header.object_prefix shouldn't change */

Quoted 15 lines · Alex Elder

Encapsulate the code that handles the case where an image's size has been found to have changed. This is done in anticipation of the next patch, which will make this common code for format 1 and 2 images.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   22 +++++++++++++---------
  1 file changed, 13 insertions(+), 9 deletions(-)

drivers/block/rbd.c +0 -0

index bb3d9be..d36e6d7 100644

drivers/block/rbd.c static void __rbd_remove_all_snaps(struct · line 1724

Quoted 9 lines

drivers/block/rbd.c +5 -0

@@ -1716,6 +1716,16 @@ static void __rbd_remove_all_snaps(struct
 rbd_device *rbd_dev)
          __rbd_remove_snap_dev(snap);
  }
 
+static void rbd_update_size(struct rbd_device *rbd_dev)
+{
+    sector_t size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
+
+    rbd_assert(rbd_dev->mapping.snap_id == CEPH_NOSNAP);

I might be confused since it's been a while since I looked at these refactorings, but I think this should still be a conditional block instead of an assert.

Even if we've only mapped a snapshot, the non-snapshot version could still be resized.

Quoted 29 lines

drivers/block/rbd.c +5 -0

+    dout("setting size to %llu sectors", (unsigned long long) size);
+    rbd_dev->mapping.size = (u64) size;
+    set_capacity(rbd_dev->disk, size);
+}
+
  /*
   * only read the first part of the ondisk header, without the snaps

info
*/

drivers/block/rbd.c +3 -9

@@ -1731,15 +1741,9 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev, u64 *hver)
      down_write(&rbd_dev->header_rwsem);
 
      /* resized? */
-    if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
-        sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
-
-        if (size != (sector_t) rbd_dev->mapping.size) {
-            dout("setting size to %llu sectors",
-                (unsigned long long) size);
-            rbd_dev->mapping.size = (u64) size;
-            set_capacity(rbd_dev->disk, size);
-        }
+    if (rbd_dev->header.image_size != h.image_size) {
+        rbd_dev->header.image_size = h.image_size;
+        rbd_update_size(rbd_dev);
      }
 

      /* rbd_dev->header.object_prefix shouldn't change */

Encapsulate the code that handles updating the size of a mapping after an rbd image has been refreshed. This is done in anticipation of the next patch, which will make this common code for format 1 and 2 images.

Signed-off-by Alex Elder <[email protected]>

---
Changed since v1:
- renamed renamed the function to include "mapping"
- now call the function unconditionally--whether or not the size has changed, and only update the mapping if the mapping is to the base image.

  drivers/block/rbd.c |   27 ++++++++++++++++-----------
  1 file changed, 16 insertions(+), 11 deletions(-)

drivers/block/rbd.c +13 -0

index bb3d9be..b64125d 100644
@@ -1716,6 +1716,19 @@ static void __rbd_remove_all_snaps(struct
 rbd_device *rbd_dev)
          __rbd_remove_snap_dev(snap);
  }
 
+static void rbd_update_mapping_size(struct rbd_device *rbd_dev)
+{
+    sector_t size;
+
+    if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
+        return;
+
+    size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE;
+    dout("setting size to %llu sectors", (unsigned long long) size);
+    rbd_dev->mapping.size = (u64) size;
+    set_capacity(rbd_dev->disk, size);
+}
+
  /*
   * only read the first part of the ondisk header, without the snaps info

   */

drivers/block/rbd.c +3 -11

@@ -1730,17 +1743,9 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev, u64 *hver)
 
      down_write(&rbd_dev->header_rwsem);
 
-    /* resized? */
-    if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
-        sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
-
-        if (size != (sector_t) rbd_dev->mapping.size) {
-            dout("setting size to %llu sectors",
-                (unsigned long long) size);
-            rbd_dev->mapping.size = (u64) size;
-            set_capacity(rbd_dev->disk, size);
-        }
-    }
+    /* Update image size, and check for resize of mapped image */
+    rbd_dev->header.image_size = h.image_size;
+    rbd_update_mapping_size(rbd_dev);
 
      /* rbd_dev->header.object_prefix shouldn't change */

      kfree(rbd_dev->header.snap_sizes);

Define a new function rbd_dev_v2_refresh() to update/refresh the snapshot context for a format version 2 rbd image. This function will update anything that is not fixed for the life of an rbd image--at the moment this is mainly the snapshot context and (for a base mapping) the size.

Update rbd_refresh_header() so it selects which function to use based on the image format.

Rename __rbd_refresh_header() to be rbd_dev_v1_refresh() to be consistent with the naming of its version 2 counterpart. Similarly rename rbd_refresh_header() to be rbd_dev_refresh().

Unrelated--we use rbd_image_format_valid() here. Delete the other use of it, which was primarily put in place to ensure that function was referenced at the time it was defined.

Signed-off-by Alex Elder <[email protected]>

---
Changes in v2:
- also fetch size when refereshing image
- drop "snapc" from refresh function names

  drivers/block/rbd.c |   55
+++++++++++++++++++++++++++++++++++++++++++--------
  1 file changed, 47 insertions(+), 8 deletions(-)

drivers/block/rbd.c +3 -2

index d36e6d7..23f3beb 100644
@@ -268,7 +268,8 @@ static void rbd_put_dev(struct rbd_device *rbd_dev)
      put_device(&rbd_dev->dev);
  }
 
-static int rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver);
+static int rbd_dev_refresh(struct rbd_device *rbd_dev, u64 *hver);
+static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev, u64 *hver);
 
  static int rbd_open(struct block_device *bdev, fmode_t mode)
  {

@@ -1304,7 +1305,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
 u8 opcode, void *data)
      dout("rbd_watch_cb %s notify_id=%llu opcode=%u\n",
          rbd_dev->header_name, (unsigned long long) notify_id,
          (unsigned int) opcode);
-    rc = rbd_refresh_header(rbd_dev, &hver);
+    rc = rbd_dev_refresh(rbd_dev, &hver);
      if (rc)
          pr_warning(RBD_DRV_NAME "%d got notification but failed to "

                 " update snaps: %d\n", rbd_dev->major, rc);

drivers/block/rbd.c +1 -1

@@ -1729,7 +1730,7 @@ static void rbd_update_size(struct rbd_device
 *rbd_dev)
  /*
   * only read the first part of the ondisk header, without the snaps info
   */
-static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver)
+static int rbd_dev_v1_refresh(struct rbd_device *rbd_dev, u64 *hver)
  {
      int ret;

      struct rbd_image_header h;

drivers/block/rbd.c +6 -2

@@ -1772,12 +1773,16 @@ static int __rbd_refresh_header(struct
 rbd_device *rbd_dev, u64 *hver)
      return ret;
  }
 
-static int rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver)
+static int rbd_dev_refresh(struct rbd_device *rbd_dev, u64 *hver)
  {
      int ret;
 
+    rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
      mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-    ret = __rbd_refresh_header(rbd_dev, hver);
+    if (rbd_dev->image_format == 1)
+        ret = rbd_dev_v1_refresh(rbd_dev, hver);
+    else
+        ret = rbd_dev_v2_refresh(rbd_dev, hver);
      mutex_unlock(&ctl_mutex);
 

      return ret;

drivers/block/rbd.c +36 -1

@@ -1937,7 +1942,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
      struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
      int ret;
 
-    ret = rbd_refresh_header(rbd_dev, NULL);
+    ret = rbd_dev_refresh(rbd_dev, NULL);
 
      return ret < 0 ? ret : size;
  }

@@ -2401,6 +2406,41 @@ static char *rbd_dev_snap_info(struct rbd_device
 *rbd_dev, u32 which,
      return ERR_PTR(-EINVAL);
  }
 
+static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev, u64 *hver)
+{
+    int ret;
+    __u8 obj_order;
+    u64 image_size;
+
+    down_write(&rbd_dev->header_rwsem);
+
+    /* Grab old values first, to see if they change */
+
+    obj_order = rbd_dev->header.obj_order,
+    image_size = rbd_dev->header.image_size;
+    ret = rbd_dev_v2_image_size(rbd_dev);
+    if (ret)
+        goto out;
+    rbd_assert(rbd_dev->header.obj_order == obj_order);
+    if (image_size != rbd_dev->header.image_size)
+        rbd_update_size(rbd_dev);
+
+    ret = rbd_dev_v2_snap_context(rbd_dev, hver);
+    dout("rbd_dev_v2_snap_context returned %d\n", ret);
+    if (ret)
+        goto out;
+    ret = rbd_dev_snaps_update(rbd_dev);
+    dout("rbd_dev_snaps_update returned %d\n", ret);
+    if (ret)
+        goto out;
+    ret = rbd_dev_snaps_register(rbd_dev);
+    dout("rbd_dev_snaps_register returned %d\n", ret);
+out:
+    up_write(&rbd_dev->header_rwsem);
+
+    return 0;
+}
+
  /*
   * Scan the rbd device's current snapshot list and compare it to the

* newly-received snapshot context. Remove any existing snapshots

drivers/block/rbd.c +1 -1

@@ -2563,7 +2603,7 @@ static int rbd_init_watch_dev(struct rbd_device
 *rbd_dev)
      do {
          ret = rbd_req_sync_watch(rbd_dev);
          if (ret == -ERANGE) {
-            rc = rbd_refresh_header(rbd_dev, NULL);
+            rc = rbd_dev_refresh(rbd_dev, NULL);
              if (rc < 0)
                  return rc;

          }

drivers/block/rbd.c +0 -1

@@ -3044,7 +3084,6 @@ static ssize_t rbd_add(struct bus_type *bus,
      rc = rbd_dev_probe(rbd_dev);
      if (rc < 0)
          goto err_out_client;
-    rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
 
      /* no need to lock here, as rbd_dev is not registered yet */
      rc = rbd_dev_snaps_update(rbd_dev);

Version 2 images have two sets of feature bit fields. The first indicates features possibly used by the image. The second indicates features that the client *must* support in order to use the image.

When an image (or snapshot) is first examined, we need to make sure that the local implementation supports the image's required features. If not, fail the probe for the image.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   16 +++++++++++++++-
  1 file changed, 15 insertions(+), 1 deletion(-)

drivers/block/rbd.c +9 -0

index 23f3beb..86ab032 100644
@@ -70,6 +70,14 @@
  #define RBD_IMAGE_ID_LEN_MAX    64
  #define RBD_OBJ_PREFIX_LEN_MAX    64
 
+/* Feature bits */
+
+#define RBD_FEATURE_LAYERING      1
+
+/* Features supported by this (client software) implementation. */
+
+#define RBD_FEATURES_ALL          (0)
+
  /*
   * An RBD device name will be "rbd#", where the "rbd" comes from
   * RBD_DRV_NAME above, and # is a unique integer identifier.

@@ -2225,6 +2233,7 @@ static int _rbd_dev_v2_snap_features(struct
 rbd_device *rbd_dev, u64 snap_id,
          __le64 features;
          __le64 incompat;
      } features_buf = { 0 };
+    u64 incompat;
      int ret;
 

      ret = rbd_req_sync_exec(rbd_dev, rbd_dev->header_name,

drivers/block/rbd.c +5 -0

@@ -2235,6 +2244,11 @@ static int _rbd_dev_v2_snap_features(struct
 rbd_device *rbd_dev, u64 snap_id,
      dout("%s: rbd_req_sync_exec returned %d\n", __func__, ret);
      if (ret < 0)
          return ret;
+
+    incompat = le64_to_cpu(features_buf.incompat);
+    if (incompat & ~RBD_FEATURES_ALL)
+        return -ENOTSUPP;
+
      *snap_features = le64_to_cpu(features_buf.features);
 

      dout("  snap_id 0x%016llx features = 0x%016llx incompat =

0x%016llx\n",

drivers/block/rbd.c +1 -1

@@ -2976,7 +2990,7 @@ static int rbd_dev_v2_probe(struct rbd_device
 *rbd_dev)
      if (ret < 0)
          goto out_err;
 
-    /* Get the features for the image */
+    /* Get the and check features for the image */
 
      ret = rbd_dev_v2_features(rbd_dev);

      if (ret < 0)

Now that v2 images support is fully implemented, have rbd_dev_v2_probe() return 0 to indicate a successful probe.

(Note that an image that implements layering will fail the probe early because of the feature chekc.)

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/block/rbd.c +1 -1

index 86ab032..6052fff 100644
@@ -3013,7 +3013,7 @@ static int rbd_dev_v2_probe(struct rbd_device
 *rbd_dev)
      dout("discovered version 2 image, header name is %s\n",
          rbd_dev->header_name);
 
-    return -ENOTSUPP;
+    return 0;
  out_err:
      kfree(rbd_dev->header_name);

      rbd_dev->header_name = NULL;

This series includes updates for two patches posted previously.

-Alex

Quoted 3 lines · On Tue, 09 Oct 2012 13:57:09 -0700 Alex Elder <[email protected]> wrote:

This series includes updates for two patches posted previously.

-Alex

Greetings,

We're gearing up to test v0.52 (specifically the RBD stuff) on our
cluster. After reading this series of posts about rbd format 2 patches I began wondering if we should start testing these patches as well or not. To put it simply, what I'd like to know is:

Is it enough to use the 3.6 vanilla kernel client to take full
advantage of the rbd changes in v0.52 (i.e. new RBD cloning features)?

Do we have any benefits from applying any of these patches on top of
v3.6 and using "format 2", assuming that we stick to v0.52 on the server, or is this strictly v0.53 and beyond stuff?

I apologize if this is a dumb question, but by looking at the v0.52
changelog, at doc/rbd/* and the list, it doesn't seem clear how this fits with v0.52.

Thanks in advance

Best regards

Cláudio

This series includes updates for two patches posted previously.

-Alex

[PATCH 1/4] rbd: define rbd_update_size()
This is a simple addition so a bit of code used by both format 1 and 2 can be common rather than duplicated.
[PATCH 2/4, v2] rbd: define rbd_dev_v2_refresh()
Updated to fetch the image size in addition to the snapshot context when refreshing. The names of some functions no longer contain "snapc" to reflect this.
[PATCH 3/4] rbd: implement feature checks
This ensures that images requiring features not supported by the local rbd client software are rejected.
[PATCH 4/4] rbd: activate v2 image support
Same as before, and trivial.
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

When will 'format 2' support be available in rbd.ko ?