rbd: more miscellaneous refactoring

It's not obvious whether the snapshot pointer whose address is provided to __rbd_add_snap_dev() will be assigned by that function. Change it to return the snapshot, or a pointer-coded errno in the event of a failure.

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

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

drivers/block/rbd.c +11 -5

index fd5f3038..fd91964 100644
@@ -2025,15 +2025,21 @@ static int rbd_register_snap_dev(struct rbd_snap
 *snap,
  	return ret;
  }
 
-static int __rbd_add_snap_dev(struct rbd_device *rbd_dev,
-			      int i, const char *name,
-			      struct rbd_snap **snapp)
+static struct rbd_snap *__rbd_add_snap_dev(struct rbd_device *rbd_dev,
+					      int i, const char *name)
  {
+	struct rbd_snap *snap;
  	int ret;
-	struct rbd_snap *snap = kzalloc(sizeof(*snap), GFP_KERNEL);
+
+	snap = kzalloc(sizeof (*snap), GFP_KERNEL);
  	if (!snap)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
+
+	ret = -ENOMEM;
  	snap->name = kstrdup(name, GFP_KERNEL);
+	if (!snap->name)
+		goto err;
+
  	snap->size = rbd_dev->header.snap_sizes[i];
  	snap->id = rbd_dev->header.snapc->snaps[i];

  	if (device_is_registered(&rbd_dev->dev)) {

drivers/block/rbd.c +5 -3

@@ -2041,12 +2047,14 @@ static int __rbd_add_snap_dev(struct rbd_device
 *rbd_dev,
  		if (ret < 0)
  			goto err;
  	}
-	*snapp = snap;
-	return 0;
+
+	return snap;
+
  err:
  	kfree(snap->name);
  	kfree(snap);
-	return ret;
+
+	return ERR_PTR(ret);
  }
 

/*

drivers/block/rbd.c +0 -1

@@ -2079,7 +2087,6 @@ static int __rbd_init_snaps_header(struct
 rbd_device *rbd_dev)
  	const char *name, *first_name;
  	int i = rbd_dev->header.total_snaps;
  	struct rbd_snap *snap, *old_snap = NULL;
-	int ret;
  	struct list_head *p, *n;
 

  	first_name = rbd_dev->header.snap_names;

drivers/block/rbd.c +3 -3

@@ -2122,9 +2129,9 @@ static int __rbd_init_snaps_header(struct
 rbd_device *rbd_dev)
  			if (cur_id >= old_snap->id)
  				break;
  			/* a new snapshot */
-			ret = __rbd_add_snap_dev(rbd_dev, i - 1, name, &snap);
-			if (ret < 0)
-				return ret;
+			snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
+			if (IS_ERR(snap))
+				return PTR_ERR(snap);
 
  			/* note that we add it backward so using n and not p */

  			list_add(&snap->node, n);

drivers/block/rbd.c +3 -3

@@ -2138,9 +2145,9 @@ static int __rbd_init_snaps_header(struct
 rbd_device *rbd_dev)
  			WARN_ON(1);
  			return -EINVAL;
  		}
-		ret = __rbd_add_snap_dev(rbd_dev, i - 1, name, &snap);
-		if (ret < 0)
-			return ret;
+		snap = __rbd_add_snap_dev(rbd_dev, i - 1, name);
+		if (IS_ERR(snap))
+			return PTR_ERR(snap);
  		list_add(&snap->node, &rbd_dev->snaps);
  	}

Either rbd_create_rw_ops() will succeed, or it will fail because a memory allocation failed. Have it just return a valid pointer or null rather than stuffing a pointer into a provided address and returning an errno.

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

---
  drivers/block/rbd.c |   70
++++++++++++++++++++++++++++-----------------------
  1 file changed, 39 insertions(+), 31 deletions(-)

drivers/block/rbd.c +23 -19

index fd91964..4d8b52c 100644
@@ -789,22 +789,24 @@ err_out:
  /*
   * helpers for osd request op vectors.
   */
-static int rbd_create_rw_ops(struct ceph_osd_req_op **ops,
-			    int num_ops,
-			    int opcode,
-			    u32 payload_len)
-{
-	*ops = kzalloc(sizeof(struct ceph_osd_req_op) * (num_ops + 1),
-		       GFP_NOIO);
-	if (!*ops)
-		return -ENOMEM;
-	(*ops)[0].op = opcode;
+static struct ceph_osd_req_op *rbd_create_rw_ops(int num_ops,
+					int opcode, u32 payload_len)
+{
+	struct ceph_osd_req_op *ops;
+
+	ops = kzalloc(sizeof (*ops) * (num_ops + 1), GFP_NOIO);
+	if (!ops)
+		return NULL;
+
+	ops[0].op = opcode;
+
  	/*
  	 * op extent offset and length will be set later on
  	 * in calc_raw_layout()
  	 */
-	(*ops)[0].payload_len = payload_len;
-	return 0;
+	ops[0].payload_len = payload_len;
+
+	return ops;
  }
 
  static void rbd_destroy_ops(struct ceph_osd_req_op *ops)

@@ -1039,8 +1041,9 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
 
  	if (!orig_ops) {
  		payload_len = (flags & CEPH_OSD_FLAG_WRITE ? len : 0);
-		ret = rbd_create_rw_ops(&ops, 1, opcode, payload_len);
-		if (ret < 0)
+		ret = -ENOMEM;
+		ops = rbd_create_rw_ops(1, opcode, payload_len);
+		if (!ops)
  			goto done;
 
  		if ((flags & CEPH_OSD_FLAG_WRITE) && buf) {

@@ -1103,8 +1106,9 @@ static int rbd_do_op(struct request *rq,
 
  	payload_len = (flags & CEPH_OSD_FLAG_WRITE ? seg_len : 0);
 
-	ret = rbd_create_rw_ops(&ops, 1, opcode, payload_len);
-	if (ret < 0)
+	ret = -ENOMEM;
+	ops = rbd_create_rw_ops(1, opcode, payload_len);
+	if (!ops)
  		goto done;
 
  	/* we've taken care of segment sizes earlier when we

@@ -1190,9 +1194,9 @@ static int rbd_req_sync_notify_ack(struct
 rbd_device *rbd_dev,
  	struct ceph_osd_req_op *ops;
  	int ret;
 
-	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY_ACK, 0);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_NOTIFY_ACK, 0);
+	if (!ops)
+		return -ENOMEM;
 
  	ops[0].watch.ver = cpu_to_le64(ver);

  	ops[0].watch.cookie = notify_id;

drivers/block/rbd.c +4 -3

@@ -1239,10 +1243,11 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev)
  {
  	struct ceph_osd_req_op *ops;
  	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
+	int ret;
 
-	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_WATCH, 0);
+	if (!ops)
+		return -ENOMEM;
 
  	ret = ceph_osdc_create_event(osdc, rbd_watch_cb, 0,

  				     (void *)rbd_dev, &rbd_dev->watch_event);

drivers/block/rbd.c +12 -9

@@ -1282,10 +1287,11 @@ fail:
  static int rbd_req_sync_unwatch(struct rbd_device *rbd_dev)
  {
  	struct ceph_osd_req_op *ops;
+	int ret;
 
-	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_WATCH, 0);
+	if (!ops)
+		return -ENOMEM;
 
  	ops[0].watch.ver = 0;
  	ops[0].watch.cookie = cpu_to_le64(rbd_dev->watch_event->cookie);

@@ -1332,9 +1338,9 @@ static int rbd_req_sync_notify(struct rbd_device
 *rbd_dev)
  	int payload_len = sizeof(u32) + sizeof(u32);
  	int ret;
 
-	ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_NOTIFY, payload_len);
-	if (ret < 0)
-		return ret;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_NOTIFY, payload_len);
+	if (!ops)
+		return -ENOMEM;
 
  	info.rbd_dev = rbd_dev;

@@ -1385,10 +1391,12 @@ static int rbd_req_sync_exec(struct rbd_device
 *rbd_dev,
  	struct ceph_osd_req_op *ops;
  	int class_name_len = strlen(class_name);
  	int method_name_len = strlen(method_name);
-	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_CALL,
+	int ret;
+
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_CALL,
  				    class_name_len + method_name_len + len);
-	if (ret < 0)
-		return ret;
+	if (!ops)
+		return -ENOMEM;
 
  	ops[0].cls.class_name = class_name;

  	ops[0].cls.class_len = (__u8) class_name_len;

rbd_header_add_snap() passes the address of a version variable to rbd_req_sync_exec(), but it ignores the result. Just pass a null pointer instead.

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

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

drivers/block/rbd.c +0 -1

index 4d8b52c..eacf255 100644
@@ -1664,7 +1664,6 @@ static int rbd_header_add_snap(struct rbd_device
 *rbd_dev,
  	u64 new_snapid;
  	int ret;
  	void *data, *p, *e;
-	u64 ver;
  	struct ceph_mon_client *monc;
 

/* we should create a snapshot only if we're pointing at the head */

drivers/block/rbd.c +1 -1

@@ -1689,7 +1688,7 @@ static int rbd_header_add_snap(struct rbd_device
 *rbd_dev,
 
  	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->header_name,
  				"rbd", "snap_add",
-				data, p - data, &ver);
+				data, p - data, NULL);
 
  	kfree(data);

All of the callers of rbd_req_sync_op() except one pass a non-null "ops" pointer. The only one that does not is rbd_req_sync_read(), which passes CEPH_OSD_OP_READ as its "opcode" and, CEPH_OSD_FLAG_READ for "flags".

By allocating the ops array in rbd_req_sync_read() and moving the special case code for the null ops pointer into it, it becomes clear that much of that code is not even necessary.

In addition, the "opcode" argument to rbd_req_sync_op() is never actually used, so get rid of that.

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

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

drivers/block/rbd.c +0 -0

index eacf255..4584500 100644
@@ -1019,9 +1019,8 @@ static void rbd_simple_req_cb(struct

ceph_osd_request *req, struct ceph_msg *msg
static int rbd_req_sync_op(struct rbd_device *rbd_dev,
struct ceph_snap_context *snapc, u64 snapid,

drivers/block/rbd.c +3 -18

-			   int opcode,
  			   int flags,
-			   struct ceph_osd_req_op *orig_ops,
+			   struct ceph_osd_req_op *ops,
  			   const char *object_name,
  			   u64 ofs, u64 len,
  			   char *buf,

@@ -1031,28 +1030,14 @@ static int rbd_req_sync_op(struct rbd_device
 *rbd_dev,
  	int ret;
  	struct page **pages;
  	int num_pages;
-	struct ceph_osd_req_op *ops = orig_ops;
-	u32 payload_len;
+
+	BUG_ON(ops == NULL);
 
  	num_pages = calc_pages_for(ofs , len);
  	pages = ceph_alloc_page_vector(num_pages, GFP_KERNEL);
  	if (IS_ERR(pages))
  		return PTR_ERR(pages);
 
-	if (!orig_ops) {
-		payload_len = (flags & CEPH_OSD_FLAG_WRITE ? len : 0);
-		ret = -ENOMEM;
-		ops = rbd_create_rw_ops(1, opcode, payload_len);
-		if (!ops)
-			goto done;
-
-		if ((flags & CEPH_OSD_FLAG_WRITE) && buf) {
-			ret = ceph_copy_to_page_vector(pages, buf, ofs, len);
-			if (ret < 0)
-				goto done_ops;
-		}
-	}
-
  	ret = rbd_do_request(NULL, rbd_dev, snapc, snapid,
  			  object_name, ofs, len, NULL,

pages, num_pages,

drivers/block/rbd.c +1 -4

@@ -1062,14 +1047,11 @@ static int rbd_req_sync_op(struct rbd_device
 *rbd_dev,
  			  NULL,
  			  linger_req, ver);
  	if (ret < 0)
-		goto done_ops;
+		goto done;
 
  	if ((flags & CEPH_OSD_FLAG_READ) && buf)
  		ret = ceph_copy_from_page_vector(pages, buf, ofs, ret);
 
-done_ops:
-	if (!orig_ops)
-		rbd_destroy_ops(ops);
  done:
  	ceph_release_page_vector(pages, num_pages);

return ret;

drivers/block/rbd.c +12 -4

@@ -1176,12 +1158,20 @@ static int rbd_req_sync_read(struct rbd_device
 *rbd_dev,
  			  char *buf,
  			  u64 *ver)
  {
-	return rbd_req_sync_op(rbd_dev, NULL,
+	struct ceph_osd_req_op *ops;
+	int ret;
+
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_READ, 0);
+	if (!ops)
+		return -ENOMEM;
+
+	ret = rbd_req_sync_op(rbd_dev, NULL,
  			       snapid,
-			       CEPH_OSD_OP_READ,
  			       CEPH_OSD_FLAG_READ,
-			       NULL,
-			       object_name, ofs, len, buf, NULL, ver);
+			       ops, object_name, ofs, len, buf, NULL, ver);
+	rbd_destroy_ops(ops);
+
+	return ret;
  }
 

/*

drivers/block/rbd.c +0 -1

@@ -1260,7 +1250,6 @@ static int rbd_req_sync_watch(struct rbd_device
 *rbd_dev)
 
  	ret = rbd_req_sync_op(rbd_dev, NULL,
  			      CEPH_NOSNAP,
-			      0,
  			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
  			      ops,

rbd_dev->header_name,

drivers/block/rbd.c +0 -1

@@ -1299,7 +1288,6 @@ static int rbd_req_sync_unwatch(struct rbd_device
 *rbd_dev)
 
  	ret = rbd_req_sync_op(rbd_dev, NULL,
  			      CEPH_NOSNAP,
-			      0,
  			      CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
  			      ops,

rbd_dev->header_name,

drivers/block/rbd.c +0 -1

@@ -1357,7 +1345,6 @@ static int rbd_req_sync_notify(struct rbd_device
 *rbd_dev)
 
  	ret = rbd_req_sync_op(rbd_dev, NULL,
  			       CEPH_NOSNAP,
-			       0,
  			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
  			       ops,

rbd_dev->header_name,

drivers/block/rbd.c +0 -1

@@ -1408,7 +1395,6 @@ static int rbd_req_sync_exec(struct rbd_device
 *rbd_dev,
 
  	ret = rbd_req_sync_op(rbd_dev, NULL,
  			       CEPH_NOSNAP,
-			       0,
  			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
  			       ops,

object_name, 0, 0, NULL, NULL, ver);

This fixes a few issues in rbd_header_from_disk():
- There is a check intended to catch overflow, but it's wrong in two ways.
- First, the type we don't want to overflow is size_t, not unsigned int, and there is now a SIZE_MAX we can use for use with that type.
- Second, we're allocating the snapshot ids and snapshot image sizes separately (each has type u64; on disk they grouped together as a rbd_image_header_ondisk structure). So we can use the size of u64 in this overflow check.
- If there are no snapshots, then there should be no snapshot names. Enforce this, and issue a warning if we encounter a header with no snapshots but a non-zero snap_names_len.

- When saving the snapshot names into the header, be more direct in defining the offset in the on-disk structure from which they're being copied by using "snap_count" rather than "i" in the array index.
- If an error occurs, the "snapc" and "snap_names" fields are freed at the end of the function. Make those fields be null pointers after they're freed, to be explicit that they are no longer valid.

- Finally, move the definition of the local variable "i" to the innermost scope in which it's needed.

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

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

drivers/block/rbd.c +3 -3

index 4584500..3daf8fb 100644
@@ -494,14 +494,14 @@ static int rbd_header_from_disk(struct
 rbd_image_header *header,
  				 struct rbd_image_header_ondisk *ondisk,
  				 u32 allocated_snaps)
  {
-	u32 i, snap_count;
+	u32 snap_count;
 
  	if (!rbd_dev_ondisk_valid(ondisk))
  		return -ENXIO;
 
  	snap_count = le32_to_cpu(ondisk->snap_count);
-	if (snap_count > (UINT_MAX - sizeof(struct ceph_snap_context))
-			 / sizeof (*ondisk))
+	if (snap_count > (SIZE_MAX - sizeof(struct ceph_snap_context))
+				 / sizeof (u64))
  		return -EINVAL;
  	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +

snap_count * sizeof(u64),

drivers/block/rbd.c +1 -1

@@ -509,8 +509,8 @@ static int rbd_header_from_disk(struct
 rbd_image_header *header,
  	if (!header->snapc)
  		return -ENOMEM;
 
-	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
  	if (snap_count) {
+		header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
  		header->snap_names = kmalloc(header->snap_names_len,
  					     GFP_KERNEL);

if (!header->snap_names)

drivers/block/rbd.c +2 -0

@@ -520,6 +520,8 @@ static int rbd_header_from_disk(struct
 rbd_image_header *header,
  		if (!header->snap_sizes)
  			goto err_names;
  	} else {
+		WARN_ON(ondisk->snap_names_len);
+		header->snap_names_len = 0;
  		header->snap_names = NULL;
  		header->snap_sizes = NULL;

  	}

drivers/block/rbd.c +2 -0

@@ -544,6 +546,8 @@ static int rbd_header_from_disk(struct
 rbd_image_header *header,
  	header->total_snaps = snap_count;
 
  	if (snap_count && allocated_snaps == snap_count) {
+		int i;
+
  		for (i = 0; i < snap_count; i++) {
  			header->snapc->snaps[i] =

  				le64_to_cpu(ondisk->snaps[i].id);

drivers/block/rbd.c +4 -1

@@ -552,7 +556,7 @@ static int rbd_header_from_disk(struct
 rbd_image_header *header,
  		}
 
  		/* copy snapshot names */
-		memcpy(header->snap_names, &ondisk->snaps[i],
+		memcpy(header->snap_names, &ondisk->snaps[snap_count],
  			header->snap_names_len);
  	}

@@ -562,8 +566,11 @@ err_sizes:
  	kfree(header->snap_sizes);
  err_names:
  	kfree(header->snap_names);
+	header->snap_names = NULL;
  err_snapc:
  	kfree(header->snapc);
+	header->snapc = NULL;
+
  	return -ENOMEM;
  }
 

Why not do this for snap_sizes too?

Add a new parameter to __rbd_refresh_header() through which the version of the header object is passed back to the caller. In most cases this isn't needed. The main motivation is to normalize (almost) all calls to __rbd_refresh_header() so they are all wrapped immediately by mutex_lock()/mutex_unlock().

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

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

drivers/block/rbd.c +2 -3

index 3daf8fb..31be4ca 100644
@@ -240,7 +240,7 @@ 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);
+static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver);
 
  static int rbd_open(struct block_device *bdev, fmode_t mode)
  {

@@ -1223,8 +1223,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
 u8 opcode, void *data)
  	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
  		rbd_dev->header_name, notify_id, (int) opcode);
  	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	rc = __rbd_refresh_header(rbd_dev);
-	hver = rbd_dev->header.obj_version;
+	rc = __rbd_refresh_header(rbd_dev, &hver);
  	mutex_unlock(&ctl_mutex);
  	if (rc)

pr_warning(RBD_DRV_NAME "%d got notification but failed to "

drivers/block/rbd.c +1 -1

@@ -1702,7 +1701,7 @@ static void __rbd_remove_all_snaps(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)
+static int __rbd_refresh_header(struct rbd_device *rbd_dev, u64 *hver)
  {
  	int ret;

struct rbd_image_header h;

drivers/block/rbd.c +2 -0

@@ -1727,6 +1726,8 @@ static int __rbd_refresh_header(struct rbd_device
 *rbd_dev)
  	/* osd requests may still refer to snapc */
  	ceph_put_snap_context(rbd_dev->header.snapc);
 
+	if (hver)
+		*hver = h.obj_version;
  	rbd_dev->header.obj_version = h.obj_version;
  	rbd_dev->header.image_size = h.image_size;

  	rbd_dev->header.total_snaps = h.total_snaps;

drivers/block/rbd.c +5 -9

@@ -1896,17 +1897,13 @@ static ssize_t rbd_image_refresh(struct device *dev,
  				 size_t size)
  {
  	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
-	int rc;
-	int ret = size;
+	int ret;
 
  	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-
-	rc = __rbd_refresh_header(rbd_dev);
-	if (rc < 0)
-		ret = rc;
-
+	ret = __rbd_refresh_header(rbd_dev, NULL);
  	mutex_unlock(&ctl_mutex);
-	return ret;
+
+	return ret < 0 ? ret : size;
  }
 
  static DEVICE_ATTR(size, S_IRUGO, rbd_size_show, NULL);

@@ -2195,7 +2192,7 @@ static int rbd_init_watch_dev(struct rbd_device
 *rbd_dev)
  		ret = rbd_req_sync_watch(rbd_dev);
  		if (ret == -ERANGE) {
  			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-			rc = __rbd_refresh_header(rbd_dev);
+			rc = __rbd_refresh_header(rbd_dev, NULL);
  			mutex_unlock(&ctl_mutex);
  			if (rc < 0)

return rc;

drivers/block/rbd.c +1 -1

@@ -2645,7 +2642,7 @@ static ssize_t rbd_snap_add(struct device *dev,
  	if (ret < 0)
  		goto err_unlock;
 
-	ret = __rbd_refresh_header(rbd_dev);
+	ret = __rbd_refresh_header(rbd_dev, NULL);
  	if (ret < 0)
  		goto err_unlock;
 

Create a simple helper that handles the common case of calling __rbd_refresh_header() while holding the ctl_mutex.

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

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

drivers/block/rbd.c +2 -4

index 31be4ca..94d0745 100644
@@ -240,7 +240,7 @@ 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_refresh_header(struct rbd_device *rbd_dev, u64 *hver);
 
  static int rbd_open(struct block_device *bdev, fmode_t mode)
  {

@@ -1222,9 +1222,7 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
 u8 opcode, void *data)
 
  	dout("rbd_watch_cb %s notify_id=%lld opcode=%d\n",
  		rbd_dev->header_name, notify_id, (int) opcode);
-	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	rc = __rbd_refresh_header(rbd_dev, &hver);
-	mutex_unlock(&ctl_mutex);
+	rc = rbd_refresh_header(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 +11 -0

@@ -1746,6 +1744,17 @@ 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)
+{
+	int ret;
+
+	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
+	ret = __rbd_refresh_header(rbd_dev, hver);
+	mutex_unlock(&ctl_mutex);
+
+	return ret;
+}
+
  static int rbd_init_disk(struct rbd_device *rbd_dev)
  {

struct gendisk *disk;

drivers/block/rbd.c +2 -6

@@ -1899,9 +1908,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
  	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
  	int ret;
 
-	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	ret = __rbd_refresh_header(rbd_dev, NULL);
-	mutex_unlock(&ctl_mutex);
+	ret = rbd_refresh_header(rbd_dev, NULL);
 
  	return ret < 0 ? ret : size;
  }

@@ -2191,9 +2198,7 @@ static int rbd_init_watch_dev(struct rbd_device
 *rbd_dev)
  	do {
  		ret = rbd_req_sync_watch(rbd_dev);
  		if (ret == -ERANGE) {
-			mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-			rc = __rbd_refresh_header(rbd_dev, NULL);
-			mutex_unlock(&ctl_mutex);
+			rc = rbd_refresh_header(rbd_dev, NULL);
  			if (rc < 0)
  				return rc;

  		}