rbd: isolate mapping info, rearrange init

Several fields in a struct rbd_dev are related to what is mapped, as opposed to the actual base rbd image. If the base image is mapped these are almost unneeded, but if a snapshot is mapped they describe information about that snapshot.

In some contexts this can be a little bit confusing. So group these mapping-related field into a structure to make it clear what they are describing.

This also includes a minor change that rearranges the fields in the in-core image header structure so that invariant fields are at the top, followed by those that change.

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

---
  drivers/block/rbd.c |   83
++++++++++++++++++++++++++++-----------------------
  1 file changed, 45 insertions(+), 38 deletions(-)

drivers/block/rbd.c +15 -11

index eb6b772..dff6210 100644
@@ -81,13 +81,15 @@
   * block device image metadata (in-memory version)
   */
  struct rbd_image_header {
-	u64 image_size;
+	/* These four fields never change for a given rbd image */
  	char *object_prefix;
  	__u8 obj_order;
  	__u8 crypt_type;
  	__u8 comp_type;
-	struct ceph_snap_context *snapc;
 
+	/* The remaining fields need to be updated occasionally */
+	u64 image_size;
+	struct ceph_snap_context *snapc;
  	char *snap_names;
  	u64 *snap_sizes;
 

@@ -146,6 +148,13 @@ struct rbd_snap {
  	u64			id;
  };
 
+struct rbd_mapping {
+	char                    *snap_name;
+	u64                     snap_id;
+	bool                    snap_exists;
+	bool			read_only;
+};
+
  /*
   * a single device
   */

@@ -174,13 +183,8 @@ struct rbd_device {
 
  	/* protects updating the header */
  	struct rw_semaphore     header_rwsem;
-	/* name of the snapshot this device reads from */
-	char                    *snap_name;
-	/* id of the snapshot this device reads from */
-	u64                     snap_id;	/* current snapshot id */
-	/* whether the snap_id this device reads from still exists */
-	bool                    snap_exists;
-	bool			read_only;
+
+	struct rbd_mapping	mapping;
 
  	struct list_head	node;
 

@@ -261,11 +265,11 @@ static int rbd_open(struct block_device *bdev,
 fmode_t mode)
  {
  	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
 
-	if ((mode & FMODE_WRITE) && rbd_dev->read_only)
+	if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
  		return -EROFS;
 
  	rbd_get_dev(rbd_dev);
-	set_device_ro(bdev, rbd_dev->read_only);
+	set_device_ro(bdev, rbd_dev->mapping.read_only);
 
  	return 0;

  }

drivers/block/rbd.c +2 -2

@@ -375,7 +379,7 @@ enum {
  static match_table_t rbd_opts_tokens = {
  	/* int args above */
  	/* string args above */
-	{Opt_read_only, "read_only"},
+	{Opt_read_only, "mapping.read_only"},
  	{Opt_read_only, "ro"},		/* Alternate spelling */
  	{Opt_read_write, "read_write"},
  	{Opt_read_write, "rw"},		/* Alternate spelling */

@@ -583,13 +587,13 @@ static int rbd_header_from_disk(struct
 rbd_image_header *header,
  		header->snap_sizes = NULL;
  	}
 
-	header->image_size = le64_to_cpu(ondisk->image_size);
  	header->obj_order = ondisk->options.order;
  	header->crypt_type = ondisk->options.crypt_type;
  	header->comp_type = ondisk->options.comp_type;
 
  	/* Allocate and fill in the snapshot context */
 
+	header->image_size = le64_to_cpu(ondisk->image_size);
  	size = sizeof (struct ceph_snap_context);
  	size += snap_count * sizeof (header->snapc->snaps[0]);

  	header->snapc = kzalloc(size, GFP_KERNEL);

drivers/block/rbd.c +9 -8

@@ -645,23 +649,24 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, u64 *size)
 
  	down_write(&rbd_dev->header_rwsem);
 
-	if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
  		    sizeof (RBD_SNAP_HEAD_NAME))) {
-		rbd_dev->snap_id = CEPH_NOSNAP;
-		rbd_dev->snap_exists = false;
-		rbd_dev->read_only = rbd_dev->rbd_opts.read_only;
+		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
+		rbd_dev->mapping.snap_exists = false;
+		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
  		if (size)
  			*size = rbd_dev->header.image_size;
  	} else {
  		u64 snap_id = 0;
 
-		ret = snap_by_name(&rbd_dev->header, rbd_dev->snap_name,
+		ret = snap_by_name(&rbd_dev->header,
+					rbd_dev->mapping.snap_name,
  					&snap_id, size);
  		if (ret < 0)
  			goto done;
-		rbd_dev->snap_id = snap_id;
-		rbd_dev->snap_exists = true;
-		rbd_dev->read_only = true;	/* No choice for snapshots */
+		rbd_dev->mapping.snap_id = snap_id;
+		rbd_dev->mapping.snap_exists = true;
+		rbd_dev->mapping.read_only = true;
  	}
 

  	ret = 0;

drivers/block/rbd.c +5 -4

@@ -1532,7 +1537,7 @@ static void rbd_rq_fn(struct request_queue *q)
  		size = blk_rq_bytes(rq);
  		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
  		rq_bio = rq->bio;
-		if (do_write && rbd_dev->read_only) {
+		if (do_write && rbd_dev->mapping.read_only) {
  			__blk_end_request_all(rq, -EROFS);
  			continue;
  		}

@@ -1541,7 +1546,8 @@ static void rbd_rq_fn(struct request_queue *q)
 
  		down_read(&rbd_dev->header_rwsem);
 
-		if (rbd_dev->snap_id != CEPH_NOSNAP && !rbd_dev->snap_exists) {
+		if (rbd_dev->mapping.snap_id != CEPH_NOSNAP &&
+				!rbd_dev->mapping.snap_exists) {
  			up_read(&rbd_dev->header_rwsem);
  			dout("request for non-existent snapshot");
  			spin_lock_irq(q->queue_lock);

@@ -1595,7 +1601,7 @@ static void rbd_rq_fn(struct request_queue *q)
  					      coll, cur_seg);
  			else
  				rbd_req_read(rq, rbd_dev,
-					     rbd_dev->snap_id,
+					     rbd_dev->mapping.snap_id,
  					     ofs,
  					     op_size, bio,
  					     coll, cur_seg);

@@ -1767,7 +1773,7 @@ static int rbd_header_add_snap(struct rbd_device
 *rbd_dev,
  	struct ceph_mon_client *monc;
 
  	/* we should create a snapshot only if we're pointing at the head */
-	if (rbd_dev->snap_id != CEPH_NOSNAP)
+	if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
  		return -EINVAL;
 

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

drivers/block/rbd.c +1 -1

@@ -1821,7 +1827,7 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev, u64 *hver)
  	down_write(&rbd_dev->header_rwsem);
 
  	/* resized? */
-	if (rbd_dev->snap_id == CEPH_NOSNAP) {
+	if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
  		sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
 

dout("setting size to %llu sectors", (unsigned long long) size);

drivers/block/rbd.c +5 -4

@@ -2004,7 +2010,7 @@ static ssize_t rbd_snap_show(struct device *dev,
  {
  	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
 
-	return sprintf(buf, "%s\n", rbd_dev->snap_name);
+	return sprintf(buf, "%s\n", rbd_dev->mapping.snap_name);
  }
 
  static ssize_t rbd_image_refresh(struct device *dev,

@@ -2205,11 +2211,12 @@ static int rbd_dev_snap_devs_update(struct
 rbd_device *rbd_dev)
 
  			/* Existing snapshot not in the new snap context */
 
-			if (rbd_dev->snap_id == snap->id)
-				rbd_dev->snap_exists = false;
+			if (rbd_dev->mapping.snap_id == snap->id)
+				rbd_dev->mapping.snap_exists = false;
  			__rbd_remove_snap_dev(snap);
  			dout("%ssnap id %llu has been removed\n",
-				rbd_dev->snap_id == snap->id ? "mapped " : "",
+				rbd_dev->mapping.snap_id == snap->id ?
+								"mapped " : "",
  				(unsigned long long) snap->id);
 

/* Done with this list entry; advance */

drivers/block/rbd.c +8 -8

@@ -2522,18 +2529,18 @@ static int rbd_add_parse_args(struct rbd_device
 *rbd_dev,
  	 * The snapshot name is optional.  If none is is supplied,
  	 * we use the default value.
  	 */
-	rbd_dev->snap_name = dup_token(&buf, &len);
-	if (!rbd_dev->snap_name)
+	rbd_dev->mapping.snap_name = dup_token(&buf, &len);
+	if (!rbd_dev->mapping.snap_name)
  		goto out_err;
  	if (!len) {
  		/* Replace the empty name with the default */
-		kfree(rbd_dev->snap_name);
-		rbd_dev->snap_name
+		kfree(rbd_dev->mapping.snap_name);
+		rbd_dev->mapping.snap_name
  			= kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
-		if (!rbd_dev->snap_name)
+		if (!rbd_dev->mapping.snap_name)
  			goto out_err;
 
-		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+		memcpy(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
  			sizeof (RBD_SNAP_HEAD_NAME));
  	}

@@ -2642,7 +2649,7 @@ err_out_client:
  	rbd_put_client(rbd_dev);
  err_put_id:
  	if (rbd_dev->pool_name) {
-		kfree(rbd_dev->snap_name);
+		kfree(rbd_dev->mapping.snap_name);
  		kfree(rbd_dev->header_name);
  		kfree(rbd_dev->image_name);
  		kfree(rbd_dev->pool_name);

@@ -2695,7 +2702,7 @@ static void rbd_dev_release(struct device *dev)
  	unregister_blkdev(rbd_dev->major, rbd_dev->name);
 
  	/* done with the id, and with the rbd_dev */
-	kfree(rbd_dev->snap_name);
+	kfree(rbd_dev->mapping.snap_name);
  	kfree(rbd_dev->header_name);
  	kfree(rbd_dev->pool_name);
  	kfree(rbd_dev->image_name);

Might want to keep the comments about the fields moved to the mapping struct, but it looks good to me.

Reviewed-by Josh Durgin <[email protected]>

Quoted 266 lines

Several fields in a struct rbd_dev are related to what is mapped, as opposed to the actual base rbd image. If the base image is mapped these are almost unneeded, but if a snapshot is mapped they describe information about that snapshot.

In some contexts this can be a little bit confusing. So group these mapping-related field into a structure to make it clear what they are describing.

This also includes a minor change that rearranges the fields in the in-core image header structure so that invariant fields are at the top, followed by those that change.

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

---
  drivers/block/rbd.c |   83
++++++++++++++++++++++++++++-----------------------
  1 file changed, 45 insertions(+), 38 deletions(-)

drivers/block/rbd.c +15 -11

index eb6b772..dff6210 100644
@@ -81,13 +81,15 @@
   * block device image metadata (in-memory version)
   */
  struct rbd_image_header {
-    u64 image_size;
+    /* These four fields never change for a given rbd image */
      char *object_prefix;
      __u8 obj_order;
      __u8 crypt_type;
      __u8 comp_type;
-    struct ceph_snap_context *snapc;
 
+    /* The remaining fields need to be updated occasionally */
+    u64 image_size;
+    struct ceph_snap_context *snapc;
      char *snap_names;
      u64 *snap_sizes;
 

@@ -146,6 +148,13 @@ struct rbd_snap {
      u64            id;
  };
 
+struct rbd_mapping {
+    char                    *snap_name;
+    u64                     snap_id;
+    bool                    snap_exists;
+    bool            read_only;
+};
+
  /*
   * a single device
   */

@@ -174,13 +183,8 @@ struct rbd_device {
 
      /* protects updating the header */
      struct rw_semaphore     header_rwsem;
-    /* name of the snapshot this device reads from */
-    char                    *snap_name;
-    /* id of the snapshot this device reads from */
-    u64                     snap_id;    /* current snapshot id */
-    /* whether the snap_id this device reads from still exists */
-    bool                    snap_exists;
-    bool            read_only;
+
+    struct rbd_mapping    mapping;
 
      struct list_head    node;
 

@@ -261,11 +265,11 @@ static int rbd_open(struct block_device *bdev,
 fmode_t mode)
  {
      struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
 
-    if ((mode & FMODE_WRITE) && rbd_dev->read_only)
+    if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
          return -EROFS;
 
      rbd_get_dev(rbd_dev);
-    set_device_ro(bdev, rbd_dev->read_only);
+    set_device_ro(bdev, rbd_dev->mapping.read_only);
 
      return 0;

  }

drivers/block/rbd.c +2 -2

@@ -375,7 +379,7 @@ enum {
  static match_table_t rbd_opts_tokens = {
      /* int args above */
      /* string args above */
-    {Opt_read_only, "read_only"},
+    {Opt_read_only, "mapping.read_only"},
      {Opt_read_only, "ro"},        /* Alternate spelling */
      {Opt_read_write, "read_write"},
      {Opt_read_write, "rw"},        /* Alternate spelling */

@@ -583,13 +587,13 @@ static int rbd_header_from_disk(struct
 rbd_image_header *header,
          header->snap_sizes = NULL;
      }
 
-    header->image_size = le64_to_cpu(ondisk->image_size);
      header->obj_order = ondisk->options.order;
      header->crypt_type = ondisk->options.crypt_type;
      header->comp_type = ondisk->options.comp_type;
 
      /* Allocate and fill in the snapshot context */
 
+    header->image_size = le64_to_cpu(ondisk->image_size);
      size = sizeof (struct ceph_snap_context);
      size += snap_count * sizeof (header->snapc->snaps[0]);

      header->snapc = kzalloc(size, GFP_KERNEL);

drivers/block/rbd.c +9 -8

@@ -645,23 +649,24 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, u64 *size)
 
      down_write(&rbd_dev->header_rwsem);
 
-    if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+    if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
              sizeof (RBD_SNAP_HEAD_NAME))) {
-        rbd_dev->snap_id = CEPH_NOSNAP;
-        rbd_dev->snap_exists = false;
-        rbd_dev->read_only = rbd_dev->rbd_opts.read_only;
+        rbd_dev->mapping.snap_id = CEPH_NOSNAP;
+        rbd_dev->mapping.snap_exists = false;
+        rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
          if (size)
              *size = rbd_dev->header.image_size;
      } else {
          u64 snap_id = 0;
 
-        ret = snap_by_name(&rbd_dev->header, rbd_dev->snap_name,
+        ret = snap_by_name(&rbd_dev->header,
+                    rbd_dev->mapping.snap_name,
                      &snap_id, size);
          if (ret < 0)
              goto done;
-        rbd_dev->snap_id = snap_id;
-        rbd_dev->snap_exists = true;
-        rbd_dev->read_only = true;    /* No choice for snapshots */
+        rbd_dev->mapping.snap_id = snap_id;
+        rbd_dev->mapping.snap_exists = true;
+        rbd_dev->mapping.read_only = true;
      }
 

      ret = 0;

drivers/block/rbd.c +4 -3

@@ -1532,7 +1537,7 @@ static void rbd_rq_fn(struct request_queue *q)
          size = blk_rq_bytes(rq);
          ofs = blk_rq_pos(rq) * SECTOR_SIZE;
          rq_bio = rq->bio;
-        if (do_write && rbd_dev->read_only) {
+        if (do_write && rbd_dev->mapping.read_only) {
              __blk_end_request_all(rq, -EROFS);
              continue;
          }

@@ -1541,7 +1546,8 @@ static void rbd_rq_fn(struct request_queue *q)
 
          down_read(&rbd_dev->header_rwsem);
 
-        if (rbd_dev->snap_id != CEPH_NOSNAP && !rbd_dev->snap_exists) {
+        if (rbd_dev->mapping.snap_id != CEPH_NOSNAP &&
+                !rbd_dev->mapping.snap_exists) {
              up_read(&rbd_dev->header_rwsem);
              dout("request for non-existent snapshot");
              spin_lock_irq(q->queue_lock);

@@ -1595,7 +1601,7 @@ static void rbd_rq_fn(struct request_queue *q)
                            coll, cur_seg);
              else
                  rbd_req_read(rq, rbd_dev,
-                         rbd_dev->snap_id,
+                         rbd_dev->mapping.snap_id,
                           ofs,
                           op_size, bio,
                           coll, cur_seg);

@@ -1767,7 +1773,7 @@ static int rbd_header_add_snap(struct rbd_device
 *rbd_dev,
      struct ceph_mon_client *monc;
 
      /* we should create a snapshot only if we're pointing at the

head */

drivers/block/rbd.c +2 -2

-    if (rbd_dev->snap_id != CEPH_NOSNAP)
+    if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
          return -EINVAL;
 
      monc = &rbd_dev->rbd_client->client->monc;

@@ -1821,7 +1827,7 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev, u64 *hver)
      down_write(&rbd_dev->header_rwsem);
 
      /* resized? */
-    if (rbd_dev->snap_id == CEPH_NOSNAP) {
+    if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
          sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
 

dout("setting size to %llu sectors", (unsigned long long)
size);

drivers/block/rbd.c +5 -4

@@ -2004,7 +2010,7 @@ static ssize_t rbd_snap_show(struct device *dev,
  {
      struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
 
-    return sprintf(buf, "%s\n", rbd_dev->snap_name);
+    return sprintf(buf, "%s\n", rbd_dev->mapping.snap_name);
  }
 
  static ssize_t rbd_image_refresh(struct device *dev,

@@ -2205,11 +2211,12 @@ static int rbd_dev_snap_devs_update(struct
 rbd_device *rbd_dev)
 
              /* Existing snapshot not in the new snap context */
 
-            if (rbd_dev->snap_id == snap->id)
-                rbd_dev->snap_exists = false;
+            if (rbd_dev->mapping.snap_id == snap->id)
+                rbd_dev->mapping.snap_exists = false;
              __rbd_remove_snap_dev(snap);
              dout("%ssnap id %llu has been removed\n",
-                rbd_dev->snap_id == snap->id ? "mapped " : "",
+                rbd_dev->mapping.snap_id == snap->id ?
+                                "mapped " : "",
                  (unsigned long long) snap->id);
 

              /* Done with this list entry; advance */

drivers/block/rbd.c +8 -8

@@ -2522,18 +2529,18 @@ static int rbd_add_parse_args(struct rbd_device
 *rbd_dev,
       * The snapshot name is optional.  If none is is supplied,
       * we use the default value.
       */
-    rbd_dev->snap_name = dup_token(&buf, &len);
-    if (!rbd_dev->snap_name)
+    rbd_dev->mapping.snap_name = dup_token(&buf, &len);
+    if (!rbd_dev->mapping.snap_name)
          goto out_err;
      if (!len) {
          /* Replace the empty name with the default */
-        kfree(rbd_dev->snap_name);
-        rbd_dev->snap_name
+        kfree(rbd_dev->mapping.snap_name);
+        rbd_dev->mapping.snap_name
              = kmalloc(sizeof (RBD_SNAP_HEAD_NAME), GFP_KERNEL);
-        if (!rbd_dev->snap_name)
+        if (!rbd_dev->mapping.snap_name)
              goto out_err;
 
-        memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+        memcpy(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
              sizeof (RBD_SNAP_HEAD_NAME));
      }

@@ -2642,7 +2649,7 @@ err_out_client:
      rbd_put_client(rbd_dev);
  err_put_id:
      if (rbd_dev->pool_name) {
-        kfree(rbd_dev->snap_name);
+        kfree(rbd_dev->mapping.snap_name);
          kfree(rbd_dev->header_name);
          kfree(rbd_dev->image_name);
          kfree(rbd_dev->pool_name);

@@ -2695,7 +2702,7 @@ static void rbd_dev_release(struct device *dev)
      unregister_blkdev(rbd_dev->major, rbd_dev->name);
 
      /* done with the id, and with the rbd_dev */
-    kfree(rbd_dev->snap_name);
+    kfree(rbd_dev->mapping.snap_name);
      kfree(rbd_dev->header_name);
      kfree(rbd_dev->pool_name);
      kfree(rbd_dev->image_name);

Add the size of the mapped image to the set of mapping-specific fields in an rbd_device, and use it when setting the capacity of the disk.

Rename the "seq" argument to snap_by_name() to be "snap_id" to be consistent with other usage.

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

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

drivers/block/rbd.c +5 -7

index dff6210..4377a83 100644
@@ -151,6 +151,7 @@ struct rbd_snap {
  struct rbd_mapping {
  	char                    *snap_name;
  	u64                     snap_id;
+	u64                     size;
  	bool                    snap_exists;
  	bool			read_only;
  };

@@ -643,7 +644,7 @@ static int snap_by_name(struct rbd_image_header
 *header, const char *snap_name,
  	return -ENOENT;
  }
 
-static int rbd_header_set_snap(struct rbd_device *rbd_dev, u64 *size)
+static int rbd_header_set_snap(struct rbd_device *rbd_dev)
  {
  	int ret;

@@ -652,19 +653,16 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, u64 *size)
  	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
  		    sizeof (RBD_SNAP_HEAD_NAME))) {
  		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
+		rbd_dev->mapping.size = rbd_dev->header.image_size;
  		rbd_dev->mapping.snap_exists = false;
  		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
-		if (size)
-			*size = rbd_dev->header.image_size;
  	} else {
-		u64 snap_id = 0;
-
  		ret = snap_by_name(&rbd_dev->header,
  					rbd_dev->mapping.snap_name,
-					&snap_id, size);
+					&rbd_dev->mapping.snap_id,
+					&rbd_dev->mapping.size);
  		if (ret < 0)
  			goto done;
-		rbd_dev->mapping.snap_id = snap_id;
  		rbd_dev->mapping.snap_exists = true;
  		rbd_dev->mapping.read_only = true;

  	}

drivers/block/rbd.c +6 -2

@@ -1830,8 +1828,12 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev, u64 *hver)
  	if (rbd_dev->mapping.snap_id == CEPH_NOSNAP) {
  		sector_t size = (sector_t) h.image_size / SECTOR_SIZE;
 
-		dout("setting size to %llu sectors", (unsigned long long) size);
-		set_capacity(rbd_dev->disk, 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);
+		}
  	}
 

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

drivers/block/rbd.c +3 -4

@@ -1875,7 +1877,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	struct request_queue *q;
  	int rc;
  	u64 segment_size;
-	u64 total_size = 0;
 
  	/* contact OSD, request size info about the object being mapped */
  	rc = rbd_read_header(rbd_dev, &rbd_dev->header);

@@ -1887,7 +1888,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	if (rc)
  		return rc;
 
-	rc = rbd_header_set_snap(rbd_dev, &total_size);
+	rc = rbd_header_set_snap(rbd_dev);
  	if (rc)
  		return rc;
 

@@ -1928,11 +1929,11 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	rbd_dev->disk = disk;
 
  	/* finally, announce the disk to the world */
-	set_capacity(disk, total_size / SECTOR_SIZE);
+	set_capacity(disk, (sector_t) rbd_dev->mapping.size / SECTOR_SIZE);
  	add_disk(disk);
 
  	pr_info("%s: added with size 0x%llx\n",
-		disk->disk_name, (unsigned long long)total_size);
+		disk->disk_name, (unsigned long long) rbd_dev->mapping.size);
  	return 0;
 
  out_disk:

Quoted 6 lines · Alex Elder

Add the size of the mapped image to the set of mapping-specific fields in an rbd_device, and use it when setting the capacity of the disk.

Rename the "seq" argument to snap_by_name() to be "snap_id" to be consistent with other usage.

This doesn't seem to be part of this commit anymore. Assuming the commit message is fixed,

Reviewed-by Josh Durgin <[email protected]>

This is the first of two patches aimed at isolating the code that sets the mapping information into a single spot.

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

---
  drivers/block/rbd.c |   72
++++++++++++++++++++++++++-------------------------
  1 file changed, 37 insertions(+), 35 deletions(-)

drivers/block/rbd.c +17 -14

index 4377a83..1a64ba2 100644
@@ -2477,28 +2477,31 @@ static inline char *dup_token(const char **buf,
 size_t *lenp)
  }
 
  /*
- * This fills in the pool_name, image_name, image_name_len, snap_name,
- * rbd_dev, rbd_md_name, and name fields of the given rbd_dev, based
- * on the list of monitor addresses and other options provided via
- * /sys/bus/rbd/add.
+ * This fills in the pool_name, image_name, image_name_len, rbd_dev,
+ * rbd_md_name, and name fields of the given rbd_dev, based on the
+ * list of monitor addresses and other options provided via
+ * /sys/bus/rbd/add.  Returns a pointer to a dynamically-allocated
+ * copy of the snapshot name to map if successful, or a
+ * pointer-coded error otherwise.
   *
   * Note: rbd_dev is assumed to have been initially zero-filled.
   */
-static int rbd_add_parse_args(struct rbd_device *rbd_dev,
-			      const char *buf,
-			      const char **mon_addrs,
-			      size_t *mon_addrs_size,
-			      char *options,
-			     size_t options_size)
+static char *rbd_add_parse_args(struct rbd_device *rbd_dev,
+				const char *buf,
+				const char **mon_addrs,
+				size_t *mon_addrs_size,
+				char *options,
+				size_t options_size)
  {
  	size_t len;
-	int ret;
+	char *err_ptr = ERR_PTR(-EINVAL);
+	char *snap_name;
 
  	/* The first four tokens are required */
 
  	len = next_token(&buf);
  	if (!len)
-		return -EINVAL;
+		return err_ptr;
  	*mon_addrs_size = len + 1;
  	*mon_addrs = buf;

@@ -2506,9 +2509,9 @@ static int rbd_add_parse_args(struct rbd_device
 *rbd_dev,
 
  	len = copy_token(&buf, options, options_size);
  	if (!len || len >= options_size)
-		return -EINVAL;
+		return err_ptr;
 
-	ret = -ENOMEM;
+	err_ptr = ERR_PTR(-ENOMEM);
  	rbd_dev->pool_name = dup_token(&buf, NULL);
  	if (!rbd_dev->pool_name)

goto out_err;

drivers/block/rbd.c +1 -7

@@ -2526,26 +2529,21 @@ static int rbd_add_parse_args(struct rbd_device
 *rbd_dev,
  		goto out_err;
  	sprintf(rbd_dev->header_name, "%s%s", rbd_dev->image_name, RBD_SUFFIX);
 
-	/*
-	 * The snapshot name is optional.  If none is is supplied,
-	 * we use the default value.
-	 */
-	rbd_dev->mapping.snap_name = dup_token(&buf, &len);
-	if (!rbd_dev->mapping.snap_name)
-		goto out_err;
+	/* Snapshot name is optional */

drivers/block/rbd.c +10 -2

-			sizeof (RBD_SNAP_HEAD_NAME));
+		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */
+		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;
  	}
+	snap_name = kmalloc(len + 1, GFP_KERNEL);
+	if (!snap_name)
+		goto out_err;
+	memcpy(snap_name, buf, len);
+	*(snap_name + len) = '\0';
 
-	return 0;
+dout("    SNAP_NAME is <%s>, len is %zd\n", snap_name, len);
+
+	return snap_name;
 
  out_err:

  	kfree(rbd_dev->header_name);

drivers/block/rbd.c +8 -4

@@ -2556,7 +2554,7 @@ out_err:
  	kfree(rbd_dev->pool_name);
  	rbd_dev->pool_name = NULL;
 
-	return ret;
+	return err_ptr;
  }
 
  static ssize_t rbd_add(struct bus_type *bus,

@@ -2569,6 +2567,7 @@ static ssize_t rbd_add(struct bus_type *bus,
  	size_t mon_addrs_size = 0;
  	struct ceph_osd_client *osdc;
  	int rc = -ENOMEM;
+	char *snap_name;
 
  	if (!try_module_get(THIS_MODULE))
  		return -ENODEV;

@@ -2595,10 +2594,13 @@ static ssize_t rbd_add(struct bus_type *bus,
  	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->dev_id);
 
  	/* parse add command */
-	rc = rbd_add_parse_args(rbd_dev, buf, &mon_addrs, &mon_addrs_size,
-				options, count);
-	if (rc)
+	snap_name = rbd_add_parse_args(rbd_dev, buf,
+				&mon_addrs, &mon_addrs_size, options, count);
+	if (IS_ERR(snap_name)) {
+		rc = PTR_ERR(snap_name);
  		goto err_put_id;
+	}
+	rbd_dev->mapping.snap_name = snap_name;
 
  	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
  	if (rc < 0)

With the exception of the snapshot name, all of the mapping-specific fields in an rbd device structure are set in rbd_header_set_snap().

Pass the snapshot name to be assigned into rbd_header_set_snap() to keep all of the mapping assignments together.

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

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

drivers/block/rbd.c +3 -4

index 1a64ba2..23fa962 100644
@@ -644,21 +644,20 @@ static int snap_by_name(struct rbd_image_header
 *header, const char *snap_name,
  	return -ENOENT;
  }
 
-static int rbd_header_set_snap(struct rbd_device *rbd_dev)
+static int rbd_header_set_snap(struct rbd_device *rbd_dev, char *snap_name)
  {
  	int ret;
 
  	down_write(&rbd_dev->header_rwsem);
 
-	if (!memcmp(rbd_dev->mapping.snap_name, RBD_SNAP_HEAD_NAME,
+	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME,
  		    sizeof (RBD_SNAP_HEAD_NAME))) {
  		rbd_dev->mapping.snap_id = CEPH_NOSNAP;
  		rbd_dev->mapping.size = rbd_dev->header.image_size;
  		rbd_dev->mapping.snap_exists = false;
  		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
  	} else {
-		ret = snap_by_name(&rbd_dev->header,
-					rbd_dev->mapping.snap_name,
+		ret = snap_by_name(&rbd_dev->header, snap_name,
  					&rbd_dev->mapping.snap_id,
  					&rbd_dev->mapping.size);

if (ret < 0)

drivers/block/rbd.c +1 -0

@@ -666,6 +665,7 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev)
  		rbd_dev->mapping.snap_exists = true;
  		rbd_dev->mapping.read_only = true;
  	}
+	rbd_dev->mapping.snap_name = snap_name;
 
  	ret = 0;

done:

drivers/block/rbd.c +1 -2

@@ -1888,7 +1888,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	if (rc)
  		return rc;
 
-	rc = rbd_header_set_snap(rbd_dev);
+	rc = rbd_header_set_snap(rbd_dev, snap_name);
  	if (rc)
  		return rc;
 

@@ -2600,7 +2600,6 @@ static ssize_t rbd_add(struct bus_type *bus,
  		rc = PTR_ERR(snap_name);
  		goto err_put_id;
  	}
-	rbd_dev->mapping.snap_name = snap_name;
 
  	rc = rbd_get_client(rbd_dev, mon_addrs, mon_addrs_size - 1, options);
  	if (rc < 0)

There is only one caller of snap_by_name(), and it passes two values to be assigned, both of which are found within an rbd device structure.

Change the interface so it just passes the address of the rbd_dev, and make the assignments to its fields directly.

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

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

drivers/block/rbd.c +2 -2

index 23fa962..46b8f8e 100644
@@ -621,10 +621,10 @@ out_err:
  	return -ENOMEM;
  }
 
-static int snap_by_name(struct rbd_image_header *header, const char
 *snap_name,
-			u64 *seq, u64 *size)
+static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name)
  {
  	int i;
+	struct rbd_image_header *header = &rbd_dev->header;
  	char *p = header->snap_names;
 

  	rbd_assert(header->snapc != NULL);

drivers/block/rbd.c +3 -4

@@ -633,10 +633,9 @@ static int snap_by_name(struct rbd_image_header
 *header, const char *snap_name,
 
  			/* Found it.  Pass back its id and/or size */
 
-			if (seq)
-				*seq = header->snapc->snaps[i];
-			if (size)
-				*size = header->snap_sizes[i];
+			rbd_dev->mapping.snap_id = header->snapc->snaps[i];
+			rbd_dev->mapping.size = header->snap_sizes[i];
+
  			return i;
  		}

p += strlen(p) + 1; /* Skip ahead to the next name */

drivers/block/rbd.c +1 -3

@@ -657,9 +656,7 @@ static int rbd_header_set_snap(struct rbd_device
 *rbd_dev, char *snap_name)
  		rbd_dev->mapping.snap_exists = false;
  		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
  	} else {
-		ret = snap_by_name(&rbd_dev->header, snap_name,
-					&rbd_dev->mapping.snap_id,
-					&rbd_dev->mapping.size);
+		ret = snap_by_name(rbd_dev, snap_name);
  		if (ret < 0)
  			goto done;

  		rbd_dev->mapping.snap_exists = true;

Move some of the code that initializes an rbd header out of rbd_init_disk() and into its caller.

Move the code at the end of rbd_init_disk() that sets the device capacity and activates the Linux device out of that function and into the caller, ensuring we still have the disk size available where we need it.

Update rbd_free_disk() so it still aligns well as an inverse of rbd_init_disk(), moving the rbd_header_free() call out to its caller.

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

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

drivers/block/rbd.c +27 -24

index 46b8f8e..6e735a7 100644
@@ -1652,8 +1652,6 @@ static void rbd_free_disk(struct rbd_device *rbd_dev)
  	if (!disk)
  		return;
 
-	rbd_header_free(&rbd_dev->header);
-
  	if (disk->flags & GENHD_FL_UP)
  		del_gendisk(disk);
  	if (disk->queue)

@@ -1875,20 +1873,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	int rc;
  	u64 segment_size;
 
-	/* contact OSD, request size info about the object being mapped */
-	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
-	if (rc)
-		return rc;
-
-	/* no need to lock here, as rbd_dev is not registered yet */
-	rc = rbd_dev_snap_devs_update(rbd_dev);
-	if (rc)
-		return rc;
-
-	rc = rbd_header_set_snap(rbd_dev, snap_name);
-	if (rc)
-		return rc;
-
  	/* create gendisk info */
  	rc = -ENOMEM;
  	disk = alloc_disk(RBD_MINORS_PER_MAJOR);

@@ -1925,12 +1909,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 
  	rbd_dev->disk = disk;
 
-	/* finally, announce the disk to the world */
-	set_capacity(disk, (sector_t) rbd_dev->mapping.size / SECTOR_SIZE);
-	add_disk(disk);
-
-	pr_info("%s: added with size 0x%llx\n",
-		disk->disk_name, (unsigned long long) rbd_dev->mapping.size);
  	return 0;
 
  out_disk:

@@ -2622,13 +2600,35 @@ static ssize_t rbd_add(struct bus_type *bus,
  	/*
  	 * At this point cleanup in the event of an error is the job
  	 * of the sysfs code (initiated by rbd_bus_del_dev()).
-	 *
-	 * Set up and announce blkdev mapping.
  	 */
+
+	/* contact OSD, request size info about the object being mapped */
+	rc = rbd_read_header(rbd_dev, &rbd_dev->header);
+	if (rc)
+		goto err_out_bus;
+
+	/* no need to lock here, as rbd_dev is not registered yet */
+	rc = rbd_dev_snap_devs_update(rbd_dev);
+	if (rc)
+		goto err_out_bus;
+
+	rc = rbd_header_set_snap(rbd_dev, snap_name);
+	if (rc)
+		goto err_out_bus;
+
+	/* Set up the blkdev mapping. */
+
  	rc = rbd_init_disk(rbd_dev);
  	if (rc)
  		goto err_out_bus;
 
+	/* Everything's ready.  Announce the disk to the world. */
+
+	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
+	add_disk(rbd_dev->disk);
+	pr_info("%s: added with size 0x%llx\n", rbd_dev->disk->disk_name,
+		(unsigned long long) rbd_dev->mapping.size);
+
  	rc = rbd_init_watch_dev(rbd_dev);
  	if (rc)
  		goto err_out_bus;

@@ -2700,6 +2700,9 @@ static void rbd_dev_release(struct device *dev)
  	rbd_free_disk(rbd_dev);
  	unregister_blkdev(rbd_dev->major, rbd_dev->name);
 
+	/* release allocated disk header fields */
+	rbd_header_free(&rbd_dev->header);
+
  	/* done with the id, and with the rbd_dev */
  	kfree(rbd_dev->mapping.snap_name);
  	kfree(rbd_dev->header_name);

This just simplifies a few things in rbd_init_disk(), now that the previous patch has moved a bunch of initialization code out if it. Done separately to facilitate review.

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

---
  drivers/block/rbd.c |   10 +++-------
  1 file changed, 3 insertions(+), 7 deletions(-)

drivers/block/rbd.c +3 -7

index 6e735a7..634a16c 100644
@@ -1870,14 +1870,12 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  {
  	struct gendisk *disk;
  	struct request_queue *q;
-	int rc;
  	u64 segment_size;
 
  	/* create gendisk info */
-	rc = -ENOMEM;
  	disk = alloc_disk(RBD_MINORS_PER_MAJOR);
  	if (!disk)
-		goto out;
+		return -ENOMEM;
 
  	snprintf(disk->disk_name, sizeof(disk->disk_name), RBD_DRV_NAME "%d",
  		 rbd_dev->dev_id);

@@ -1887,7 +1885,6 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	disk->private_data = rbd_dev;
 
  	/* init rq */
-	rc = -ENOMEM;
  	q = blk_init_queue(rbd_rq_fn, &rbd_dev->lock);
  	if (!q)
  		goto out_disk;

@@ -1910,11 +1907,10 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	rbd_dev->disk = disk;
 
  	return 0;
-
  out_disk:
  	put_disk(disk);
-out:
-	return rc;
+
+	return -ENOMEM;
  }
 
  /*