rbd: kill create_snap sysfs entry

In order to allow both read requests and write requests to be initiated using rbd_req_sync_exec(), add an OSD flags value which can be passed down to rbd_req_sync_op(). Rename the "data" and "len" parameters to be more clear that they represent data that is outbound.

At this point, this function is still only used (and only works) for write requests.

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

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

drivers/block/rbd.c +18 -9

index daa428e3..4e84f99 100644
@@ -1437,23 +1437,33 @@ fail:
  }
 
  /*
- * Request sync osd read
+ * Synchronous osd object method call
   */
  static int rbd_req_sync_exec(struct rbd_device *rbd_dev,
  			     const char *object_name,
  			     const char *class_name,
  			     const char *method_name,
-			     const char *data,
-			     int len,
+			     const char *outbound,
+			     size_t outbound_size,
+			     int flags,
  			     u64 *ver)
  {
  	struct ceph_osd_req_op *ops;
  	int class_name_len = strlen(class_name);
  	int method_name_len = strlen(method_name);
+	int payload_size;
  	int ret;
 
-	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_CALL,
-				    class_name_len + method_name_len + len);
+	/*
+	 * Any input parameters required by the method we're calling
+	 * will be sent along with the class and method names as
+	 * part of the message payload.  That data and its size are
+	 * supplied via the indata and indata_len fields (named from
+	 * the perspective of the server side) in the OSD request
+	 * operation.
+	 */
+	payload_size = class_name_len + method_name_len + outbound_size;
+	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_CALL, payload_size);
  	if (!ops)
  		return -ENOMEM;
 

@@ -1462,13 +1472,12 @@ static int rbd_req_sync_exec(struct rbd_device
 *rbd_dev,
  	ops[0].cls.method_name = method_name;
  	ops[0].cls.method_len = (__u8) method_name_len;
  	ops[0].cls.argc = 0;
-	ops[0].cls.indata = data;
-	ops[0].cls.indata_len = len;
+	ops[0].cls.indata = outbound;
+	ops[0].cls.indata_len = outbound_size;
 
  	ret = rbd_req_sync_op(rbd_dev, NULL,
  			       CEPH_NOSNAP,
-			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
-			       ops,
+			       flags, ops,
  			       object_name, 0, 0, NULL, NULL, ver);
 

  	rbd_destroy_ops(ops);

drivers/block/rbd.c +3 -1

@@ -1780,7 +1789,9 @@ 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, NULL);
+				data, (size_t) (p - data),
+				CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
+				NULL);
 
  	kfree(data);

An OSD object method call can be made using rbd_req_sync_exec(). Until now this has only been used for creating a new RBD snapshot, and that has only required sending data out, not receiving anything back from the OSD.

We will now need to get data back from an OSD on a method call, so add parameters to rbd_req_sync_exec() that allow a buffer into which returned data should be placed to be specified, along with its size.

Previously, rbd_req_sync_exec() passed a null pointer and zero size to rbd_req_sync_op(); change this so the new inbound buffer information is provided instead.

Rename the "buf" and "len" parameters in rbd_req_sync_op() to make it more obvious they are describing inbound data.

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

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

drivers/block/rbd.c +4 -4

index 4e84f99..ba68566 100644
@@ -1098,8 +1098,8 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
  			   int flags,
  			   struct ceph_osd_req_op *ops,
  			   const char *object_name,
-			   u64 ofs, u64 len,
-			   char *buf,
+			   u64 ofs, u64 inbound_size,
+			   char *inbound,
  			   struct ceph_osd_request **linger_req,
  			   u64 *ver)
  {

@@ -1109,13 +1109,13 @@ static int rbd_req_sync_op(struct rbd_device
 *rbd_dev,
 
  	rbd_assert(ops != NULL);
 
-	num_pages = calc_pages_for(ofs , len);
+	num_pages = calc_pages_for(ofs, inbound_size);
  	pages = ceph_alloc_page_vector(num_pages, GFP_KERNEL);
  	if (IS_ERR(pages))
  		return PTR_ERR(pages);
 
  	ret = rbd_do_request(NULL, rbd_dev, snapc, snapid,
-			  object_name, ofs, len, NULL,
+			  object_name, ofs, inbound_size, NULL,
  			  pages, num_pages,
  			  flags,

ops,

drivers/block/rbd.c +4 -2

@@ -1125,8 +1125,8 @@ static int rbd_req_sync_op(struct rbd_device *rbd_dev,
  	if (ret < 0)
  		goto done;
 
-	if ((flags & CEPH_OSD_FLAG_READ) && buf)
-		ret = ceph_copy_from_page_vector(pages, buf, ofs, ret);
+	if ((flags & CEPH_OSD_FLAG_READ) && inbound)
+		ret = ceph_copy_from_page_vector(pages, inbound, ofs, ret);
 
  done:
  	ceph_release_page_vector(pages, num_pages);

@@ -1445,6 +1445,8 @@ static int rbd_req_sync_exec(struct rbd_device
 *rbd_dev,
  			     const char *method_name,
  			     const char *outbound,
  			     size_t outbound_size,
+			     char *inbound,
+			     size_t inbound_size,
  			     int flags,
  			     u64 *ver)

  {

drivers/block/rbd.c +3 -2

@@ -1478,7 +1480,8 @@ static int rbd_req_sync_exec(struct rbd_device
 *rbd_dev,
  	ret = rbd_req_sync_op(rbd_dev, NULL,
  			       CEPH_NOSNAP,
  			       flags, ops,
-			       object_name, 0, 0, NULL, NULL, ver);
+			       object_name, 0, inbound_size, inbound,
+			       NULL, ver);
 
  	rbd_destroy_ops(ops);

@@ -1789,7 +1792,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, (size_t) (p - data),
+				data, (size_t) (p - data), NULL, 0,
  				CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
  				NULL);

Define constant symbols related to the rbd format 2 object names. This begins to bring this version of the "rbd_types.h" header more in line with the current user-space version of that file. Complete reconciliation of differences will be done at some point later, as a separate task.

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

---
  drivers/block/rbd_types.h |   25 ++++++++++++++++++++-----
  1 file changed, 20 insertions(+), 5 deletions(-)

drivers/block/rbd_types.h +20 -5

index d9d8a77..cbe77fa 100644
@@ -15,15 +15,30 @@
 
  #include <linux/types.h>
 
+/* For format version 2, rbd image 'foo' consists of objects
+ *   rbd_id.foo		- id of image
+ *   rbd_header.<id>	- image metadata
+ *   rbd_data.<id>.0000000000000000
+ *   rbd_data.<id>.0000000000000001
+ *   ...		- data
+ * Clients do not access header data directly in rbd format 2.
+ */
+
+#define RBD_HEADER_PREFIX      "rbd_header."
+#define RBD_DATA_PREFIX        "rbd_data."
+#define RBD_ID_PREFIX          "rbd_id."
+
  /*
- * rbd image 'foo' consists of objects
- *   foo.rbd      - image metadata
- *   foo.00000000
- *   foo.00000001
- *   ...          - data
+ * For format version 1, rbd image 'foo' consists of objects
+ *   foo.rbd		- image metadata
+ *   rb.<idhi>.<idlo>.00000000
+ *   rb.<idhi>.<idlo>.00000001
+ *   ...		- data
+ * There is no notion of a persistent image id in rbd format 1.
   */
 
  #define RBD_SUFFIX		".rbd"
+
  #define RBD_DIRECTORY           "rbd_directory"
  #define RBD_INFO                "rbd_info"
 

New format 2 rbd images are permanently identified by a unique image id. Each rbd image also has a name, but the name can be changed. A format 2 rbd image will have an object--whose name is based on the image name--which maps an image's name to its image id.

Create a new function rbd_dev_image_id() that checks for the existence of the image id object, and if it's found, records the image id in the rbd_device structure.

Create a new rbd device attribute (/sys/bus/rbd/<num>/image_id) that makes this information available.

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

---
  Documentation/ABI/testing/sysfs-bus-rbd |    5 ++
  drivers/block/rbd.c                     |   99
+++++++++++++++++++++++++++++++
  2 files changed, 104 insertions(+)

Documentation/ABI/testing/sysfs-bus-rbd +5 -0

index 3c17b62..7cbbe34 100644
@@ -33,6 +33,11 @@ name
 
  	The name of the rbd image.
 
+image_id
+
+	The unique id for the rbd image.  (For rbd image format 1
+	this is empty.)
+
  pool
 
  	The name of the storage pool where this rbd image resides.

drivers/block/rbd.c +13 -0

index ba68566..5a3132e 100644
@@ -66,6 +66,8 @@
 
  #define RBD_SNAP_HEAD_NAME	"-"
 
+#define	RBD_IMAGE_ID_LEN_MAX	64
+
  /*
   * An RBD device name will be "rbd#", where the "rbd" comes from
   * RBD_DRV_NAME above, and # is a unique integer identifier.

@@ -173,6 +175,8 @@ struct rbd_device {
  	spinlock_t		lock;		/* queue lock */
 
  	struct rbd_image_header	header;
+	char			*image_id;
+	size_t			image_id_len;
  	char			*image_name;
  	size_t			image_name_len;
  	char			*header_name;

@@ -1987,6 +1991,14 @@ static ssize_t rbd_name_show(struct device *dev,
  	return sprintf(buf, "%s\n", rbd_dev->image_name);
  }
 
+static ssize_t rbd_image_id_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
+
+	return sprintf(buf, "%s\n", rbd_dev->image_id);
+}
+
  static ssize_t rbd_snap_show(struct device *dev,
  			     struct device_attribute *attr,
  			     char *buf)

@@ -2015,6 +2027,7 @@ static DEVICE_ATTR(client_id, S_IRUGO,
 rbd_client_id_show, NULL);
  static DEVICE_ATTR(pool, S_IRUGO, rbd_pool_show, NULL);
  static DEVICE_ATTR(pool_id, S_IRUGO, rbd_pool_id_show, NULL);
  static DEVICE_ATTR(name, S_IRUGO, rbd_name_show, NULL);
+static DEVICE_ATTR(image_id, S_IRUGO, rbd_image_id_show, NULL);
  static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
  static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);

  static DEVICE_ATTR(create_snap, S_IWUSR, NULL, rbd_snap_add);

drivers/block/rbd.c +42 -0

@@ -2026,6 +2039,7 @@ static struct attribute *rbd_attrs[] = {
  	&dev_attr_pool.attr,
  	&dev_attr_pool_id.attr,
  	&dev_attr_name.attr,
+	&dev_attr_image_id.attr,
  	&dev_attr_current_snap.attr,
  	&dev_attr_refresh.attr,
  	&dev_attr_create_snap.attr,

@@ -2548,6 +2562,75 @@ out_err:
  	return err_ptr;
  }
 
+/*
+ * An rbd format 2 image has a unique identifier, distinct from the
+ * name given to it by the user.  Internally, that identifier is
+ * what's used to specify the names of objects related to the image.
+ *
+ * A special "rbd id" object is used to map an rbd image name to its
+ * id.  If that object doesn't exist, then there is no v2 rbd image
+ * with the supplied name.
+ *
+ * This function will record the given rbd_dev's image_id field if
+ * it can be determined, and in that case will return 0.  If any
+ * errors occur a negative errno will be returned and the rbd_dev's
+ * image_id field will be unchanged (and should be NULL).
+ */
+static int rbd_dev_image_id(struct rbd_device *rbd_dev)
+{
+	int ret;
+	size_t size;
+	char *object_name;
+	void *response;
+	void *p;
+
+	/*
+	 * First, see if the format 2 image id file exists, and if
+	 * so, get the image's persistent id from it.
+	 */
+	size = sizeof (RBD_ID_PREFIX) + rbd_dev->image_name_len;
+	object_name = kmalloc(size, GFP_NOIO);
+	if (!object_name)
+		return -ENOMEM;
+	sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->image_name);
+	dout("rbd id object name is %s\n", object_name);
+
+	/* Response will be an encoded string, which includes a length */
+
+	size = sizeof (__le32) + RBD_IMAGE_ID_LEN_MAX;
+	response = kzalloc(size, GFP_NOIO);
+	if (!response) {
+		ret = -ENOMEM;
+		goto out;
+	}

drivers/block/rbd.c +7 -0

+out:
+	kfree(response);
+	kfree(object_name);
+
+	return ret;
+}
+
  static ssize_t rbd_add(struct bus_type *bus,
  		       const char *buf,
  		       size_t count)

drivers/block/rbd.c +14 -0

+		/* Version 1 images have no id; empty string is used */
+		rbd_dev->image_id = kstrdup("", GFP_KERNEL);
+		if (!rbd_dev->image_id) {
+			rc = -ENOMEM;
+			goto err_out_client;
+		}
+		rbd_dev->image_id_len = 0;
+	} else {
+	        /* Not actually supporting format 2 yet */
+		goto err_out_client;
+	}
+
  	/* Create the name of the header object */
 
  	rbd_dev->header_name = kmalloc(rbd_dev->image_name_len

@@ -2688,6 +2785,7 @@ err_out_header:
  err_out_client:
  	kfree(rbd_dev->header_name);
  	rbd_put_client(rbd_dev);
+	kfree(rbd_dev->image_id);
  err_out_args:
  	kfree(rbd_dev->mapping.snap_name);
  	kfree(rbd_dev->image_name);

@@ -2744,6 +2842,7 @@ static void rbd_dev_release(struct device *dev)
 
  	/* done with the id, and with the rbd_dev */
  	kfree(rbd_dev->mapping.snap_name);
+	kfree(rbd_dev->image_id);
  	kfree(rbd_dev->header_name);
  	kfree(rbd_dev->pool_name);
  	kfree(rbd_dev->image_name);

Josh proposed the following change, and I don't think I could explain it any better than he did:

From: Josh Durgin <[email protected]>
Date: Tue, 24 Jul 2012 14:22:11 -0700
To: ceph-devel <[email protected]>
Message-ID: <[email protected]>

Right now the kernel still has one piece of rbd management duplicated from the rbd command line tool: snapshot creation. There's nothing special about snapshot creation that makes it advantageous to do from the kernel, so I'd like to remove the create_snap sysfs interface. That is,
/sys/bus/rbd/devices/<id>/create_snap
would be removed.

Does anyone rely on the sysfs interface for creating rbd snapshots? If so, how hard would it be to replace with:

rbd snap create pool/image@snap

Is there any benefit to the sysfs interface that I'm missing?

Josh

This patch implements this proposal, removing the code that implements the "snap_create" sysfs interface for rbd images. As a result, quite a lot of other supporting code goes away.

Suggested-by Josh Durgin <[email protected]>

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

---
  Documentation/ABI/testing/sysfs-bus-rbd |    6 --
  drivers/block/rbd.c                     |  158
-------------------------------
  2 files changed, 164 deletions(-)

Documentation/ABI/testing/sysfs-bus-rbd +0 -6

index 7cbbe34..6fe4224 100644
@@ -62,12 +62,6 @@ current_snap
 
  	The current snapshot for which the device is mapped.
 
-create_snap
-
-	Create a snapshot:
-
-	 $ echo <snap-name> > /sys/bus/rbd/devices/<dev-id>/snap_create
-
  snap_*
 
  	A directory per each snapshot

drivers/block/rbd.c +0 -4

index 5a3132e..d73edb1 100644
@@ -212,10 +212,6 @@ static int rbd_dev_snaps_update(struct rbd_device
 *rbd_dev);
  static int rbd_dev_snaps_register(struct rbd_device *rbd_dev);
 
  static void rbd_dev_release(struct device *dev);
-static ssize_t rbd_snap_add(struct device *dev,
-			    struct device_attribute *attr,
-			    const char *buf,
-			    size_t count);
  static void __rbd_remove_snap_dev(struct rbd_snap *snap);
 

static ssize_t rbd_add(struct bus_type *bus, const char *buf,

drivers/block/rbd.c +0 -65

@@ -1375,71 +1371,6 @@ static int rbd_req_sync_unwatch(struct rbd_device
 *rbd_dev)
  	return ret;
  }
 
-struct rbd_notify_info {
-	struct rbd_device *rbd_dev;
-};
-
-static void rbd_notify_cb(u64 ver, u64 notify_id, u8 opcode, void *data)
-{
-	struct rbd_device *rbd_dev = (struct rbd_device *)data;
-	if (!rbd_dev)
-		return;
-
-	dout("rbd_notify_cb %s notify_id=%llu opcode=%u\n",
-			rbd_dev->header_name, (unsigned long long) notify_id,
-			(unsigned int) opcode);
-}
-
-/*
- * Request sync osd notify
- */
-static int rbd_req_sync_notify(struct rbd_device *rbd_dev)
-{
-	struct ceph_osd_req_op *ops;
-	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
-	struct ceph_osd_event *event;
-	struct rbd_notify_info info;
-	int payload_len = sizeof(u32) + sizeof(u32);
-	int ret;
-
-	ops = rbd_create_rw_ops(1, CEPH_OSD_OP_NOTIFY, payload_len);
-	if (!ops)
-		return -ENOMEM;
-
-	info.rbd_dev = rbd_dev;
-
-	ret = ceph_osdc_create_event(osdc, rbd_notify_cb, 1,
-				     (void *)&info, &event);
-	if (ret < 0)
-		goto fail;
-
-	ops[0].watch.ver = 1;
-	ops[0].watch.flag = 1;
-	ops[0].watch.cookie = event->cookie;
-	ops[0].watch.prot_ver = RADOS_NOTIFY_VER;
-	ops[0].watch.timeout = 12;
-
-	ret = rbd_req_sync_op(rbd_dev, NULL,
-			       CEPH_NOSNAP,
-			       CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
-			       ops,
-			       rbd_dev->header_name,
-			       0, 0, NULL, NULL, NULL);
-	if (ret < 0)
-		goto fail_event;
-
-	ret = ceph_osdc_wait_event(event, CEPH_OSD_TIMEOUT_DEFAULT);
-	dout("ceph_osdc_wait_event returned %d\n", ret);
-	rbd_destroy_ops(ops);
-	return 0;
-
-fail_event:
-	ceph_osdc_cancel_event(event);
-fail:
-	rbd_destroy_ops(ops);
-	return ret;
-}
-
  /*
   * Synchronous osd object method call

   */

drivers/block/rbd.c +0 -46

@@ -1761,52 +1692,6 @@ static int rbd_read_header(struct rbd_device
 *rbd_dev,
  	return ret;
  }
 
-/*
- * create a snapshot
- */
-static int rbd_header_add_snap(struct rbd_device *rbd_dev,
-			       const char *snap_name,
-			       gfp_t gfp_flags)
-{
-	int name_len = strlen(snap_name);
-	u64 new_snapid;
-	int ret;
-	void *data, *p, *e;
-	struct ceph_mon_client *monc;
-
-	/* we should create a snapshot only if we're pointing at the head */
-	if (rbd_dev->mapping.snap_id != CEPH_NOSNAP)
-		return -EINVAL;
-
-	monc = &rbd_dev->rbd_client->client->monc;
-	ret = ceph_monc_create_snapid(monc, rbd_dev->pool_id, &new_snapid);
-	dout("created snapid=%llu\n", (unsigned long long) new_snapid);
-	if (ret < 0)
-		return ret;
-
-	data = kmalloc(name_len + 16, gfp_flags);
-	if (!data)
-		return -ENOMEM;
-
-	p = data;
-	e = data + name_len + 16;
-
-	ceph_encode_string_safe(&p, e, snap_name, name_len, bad);
-	ceph_encode_64_safe(&p, e, new_snapid, bad);
-
-	ret = rbd_req_sync_exec(rbd_dev, rbd_dev->header_name,
-				"rbd", "snap_add",
-				data, (size_t) (p - data), NULL, 0,
-				CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK,
-				NULL);
-
-	kfree(data);
-
-	return ret < 0 ? ret : 0;
-bad:
-	return -ERANGE;
-}
-
  static void __rbd_remove_all_snaps(struct rbd_device *rbd_dev)
  {

struct rbd_snap *snap;

drivers/block/rbd.c +0 -1

@@ -2030,7 +1915,6 @@ static DEVICE_ATTR(name, S_IRUGO, rbd_name_show,
 NULL);
  static DEVICE_ATTR(image_id, S_IRUGO, rbd_image_id_show, NULL);
  static DEVICE_ATTR(refresh, S_IWUSR, NULL, rbd_image_refresh);
  static DEVICE_ATTR(current_snap, S_IRUGO, rbd_snap_show, NULL);
-static DEVICE_ATTR(create_snap, S_IWUSR, NULL, rbd_snap_add);
 
  static struct attribute *rbd_attrs[] = {

&dev_attr_size.attr,

drivers/block/rbd.c +0 -42

@@ -2042,7 +1926,6 @@ static struct attribute *rbd_attrs[] = {
  	&dev_attr_image_id.attr,
  	&dev_attr_current_snap.attr,
  	&dev_attr_refresh.attr,
-	&dev_attr_create_snap.attr,
  	NULL
  };
 

@@ -2888,47 +2771,6 @@ done:
  	return ret;
  }
 
-static ssize_t rbd_snap_add(struct device *dev,
-			    struct device_attribute *attr,
-			    const char *buf,
-			    size_t count)
-{
-	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
-	int ret;
-	char *name = kmalloc(count + 1, GFP_KERNEL);
-	if (!name)
-		return -ENOMEM;
-
-	snprintf(name, count, "%s", buf);
-
-	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-
-	ret = rbd_header_add_snap(rbd_dev,
-				  name, GFP_KERNEL);
-	if (ret < 0)
-		goto err_unlock;
-
-	ret = __rbd_refresh_header(rbd_dev, NULL);
-	if (ret < 0)
-		goto err_unlock;
-
-	/* shouldn't hold ctl_mutex when notifying.. notify might
-	   trigger a watch callback that would need to get that mutex */
-	mutex_unlock(&ctl_mutex);
-
-	/* make a best effort, don't error if failed */
-	rbd_req_sync_notify(rbd_dev);
-
-	ret = count;
-	kfree(name);
-	return ret;
-
-err_unlock:
-	mutex_unlock(&ctl_mutex);
-	kfree(name);
-	return ret;
-}
-
  /*
   * create control files in sysfs
   * /sys/bus/rbd/...