[PATCH 0/4] rbd: miscellaneous cleanups

[PATCH 0/4] rbd: miscellaneous cleanups

[Alex Elder]

This series makes a few small unrelated changes to the rbd code.
The first is a set of simple cleanups.  The second makes
ceph_parse_options() return a pointer to make the interface
a little more obvious.  The third gets rid of a duplicate
copy of the pointer to a ceph_client held in an rbd_device,
and the last one makes it so RBD_SNAP_HEAD_NAME is the only
way used for identifying the head snapshot.

					-Alex

[PATCH 1/4] rbd: a few small cleanups

[Alex Elder]

Some minor cleanups in "drivers/block/rbd.c:
     - Use the more meaningful "RBD_MAX_OBJ_NAME_LEN" in place if "96"
       in the definition of RBD_MAX_MD_NAME_LEN.
     - Use DEFINE_SPINLOCK() to define and initialize node_lock.
     - Drop a needless (char *) cast in parse_rbd_opts_token().
     - Make a few minor formatting changes.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   21 +++++++++------------
  1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 7f40cb4..7c8c07a 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -46,7 +46,7 @@

  #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */

-#define RBD_MAX_MD_NAME_LEN	(96 + sizeof(RBD_SUFFIX))
+#define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
  #define RBD_MAX_POOL_NAME_LEN	64
  #define RBD_MAX_SNAP_NAME_LEN	32
  #define RBD_MAX_OPT_LEN		1024
@@ -175,7 +175,7 @@ static struct bus_type rbd_bus_type = {
  	.name		= "rbd",
  };

-static spinlock_t node_lock;      /* protects client get/put */
+static DEFINE_SPINLOCK(node_lock);      /* protects client get/put */

  static DEFINE_MUTEX(ctl_mutex);	  /* Serialize 
open/close/setup/teardown */
  static LIST_HEAD(rbd_dev_list);    /* devices */
@@ -324,7 +324,7 @@ static int parse_rbd_opts_token(char *c, void *private)
  	substring_t argstr[MAX_OPT_ARGS];
  	int token, intval, ret;

-	token = match_token((char *)c, rbdopt_tokens, argstr);
+	token = match_token(c, rbdopt_tokens, argstr);
  	if (token < 0)
  		return -EINVAL;

@@ -372,7 +372,8 @@ static int rbd_get_client(struct rbd_device 
*rbd_dev, const char *mon_addr,
  	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;

  	ret = ceph_parse_options(&opt, options, mon_addr,
-				 mon_addr + strlen(mon_addr), parse_rbd_opts_token, rbd_opts);
+				mon_addr + strlen(mon_addr),
+				parse_rbd_opts_token, rbd_opts);
  	if (ret < 0)
  		goto done_err;

@@ -460,15 +461,13 @@ static int rbd_header_from_disk(struct 
rbd_image_header *header,
  	u32 snap_count = le32_to_cpu(ondisk->snap_count);
  	int ret = -ENOMEM;

-	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT))) {
+	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
  		return -ENXIO;
-	}

  	init_rwsem(&header->snap_rwsem);
  	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
  	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
-				snap_count *
-				 sizeof(struct rbd_image_snap_ondisk),
+				snap_count * sizeof *ondisk,
  				gfp_flags);
  	if (!header->snapc)
  		return -ENOMEM;
@@ -498,8 +497,7 @@ static int rbd_header_from_disk(struct 
rbd_image_header *header,
  	header->snapc->num_snaps = snap_count;
  	header->total_snaps = snap_count;

-	if (snap_count &&
-	    allocated_snaps == snap_count) {
+	if (snap_count && allocated_snaps == snap_count) {
  		for (i = 0; i < snap_count; i++) {
  			header->snapc->snaps[i] =
  				le64_to_cpu(ondisk->snaps[i].id);
@@ -2421,7 +2419,7 @@ static int rbd_sysfs_init(void)
  	rbd_bus_type.bus_attrs = rbd_bus_attrs;

  	ret = bus_register(&rbd_bus_type);
-	 if (ret < 0)
+	if (ret < 0)
  		return ret;

  	ret = device_register(&rbd_root_dev);
@@ -2442,7 +2440,6 @@ int __init rbd_init(void)
  	rc = rbd_sysfs_init();
  	if (rc)
  		return rc;
-	spin_lock_init(&node_lock);
  	pr_info("loaded " DRV_NAME_LONG "\n");
  	return 0;
  }
-- 
1.7.5.4

[PATCH 2/4] rbd: make ceph_parse_options() return a pointer

[Alex Elder]

ceph_parse_options() takes the address of a pointer as an argument
and uses it to return the address of an allocated structure if
successful.  With this interface is not evident at call sites that
the pointer is always initialized.  Change the interface to return
the address instead (or a pointer-coded error code) to make the
validity of the returned pointer obvious.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c          |    6 ++++--
  fs/ceph/super.c              |    6 ++++--
  include/linux/ceph/libceph.h |    2 +-
  net/ceph/ceph_common.c       |   16 ++++++++--------
  4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 7c8c07a..0430642 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -371,11 +371,13 @@ static int rbd_get_client(struct rbd_device 
*rbd_dev, const char *mon_addr,

  	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;

-	ret = ceph_parse_options(&opt, options, mon_addr,
+	opt = ceph_parse_options(options, mon_addr,
  				mon_addr + strlen(mon_addr),
  				parse_rbd_opts_token, rbd_opts);
-	if (ret < 0)
+	if (IS_ERR(opt)) {
+		ret = PTR_ERR(opt);
  		goto done_err;
+	}

  	spin_lock(&node_lock);
  	rbdc = __rbd_client_find(opt);
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index e6325f0..1d325bc 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -324,10 +324,12 @@ static int parse_mount_options(struct 
ceph_mount_options **pfsopt,
  	*path += 2;
  	dout("server path '%s'\n", *path);

-	err = ceph_parse_options(popt, options, dev_name, dev_name_end,
+	*popt = ceph_parse_options(options, dev_name, dev_name_end,
  				 parse_fsopt_token, (void *)fsopt);
-	if (err)
+	if (IS_ERR(*popt)) {
+		err = PTR_ERR(*popt);
  		goto out;
+	}

  	/* success */
  	*pfsopt = fsopt;
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 95bd850..92eef7c 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -207,7 +207,7 @@ extern struct kmem_cache *ceph_cap_cachep;
  extern struct kmem_cache *ceph_dentry_cachep;
  extern struct kmem_cache *ceph_file_cachep;

-extern int ceph_parse_options(struct ceph_options **popt, char *options,
+extern struct ceph_options *ceph_parse_options(char *options,
  			      const char *dev_name, const char *dev_name_end,
  			      int (*parse_extra_token)(char *c, void *private),
  			      void *private);
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index 761ad9d..621c322 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -277,10 +277,11 @@ out:
  	return err;
  }

-int ceph_parse_options(struct ceph_options **popt, char *options,
-		       const char *dev_name, const char *dev_name_end,
-		       int (*parse_extra_token)(char *c, void *private),
-		       void *private)
+struct ceph_options *
+ceph_parse_options(char *options, const char *dev_name,
+			const char *dev_name_end,
+			int (*parse_extra_token)(char *c, void *private),
+			void *private)
  {
  	struct ceph_options *opt;
  	const char *c;
@@ -289,7 +290,7 @@ int ceph_parse_options(struct ceph_options **popt, 
char *options,

  	opt = kzalloc(sizeof(*opt), GFP_KERNEL);
  	if (!opt)
-		return err;
+		return ERR_PTR(-ENOMEM);
  	opt->mon_addr = kcalloc(CEPH_MAX_MON, sizeof(*opt->mon_addr),
  				GFP_KERNEL);
  	if (!opt->mon_addr)
@@ -412,12 +413,11 @@ int ceph_parse_options(struct ceph_options **popt, 
char *options,
  	}

  	/* success */
-	*popt = opt;
-	return 0;
+	return opt;

  out:
  	ceph_destroy_options(opt);
-	return err;
+	return ERR_PTR(err);
  }
  EXPORT_SYMBOL(ceph_parse_options);

-- 
1.7.5.4

[PATCH 3/4] rbd: do not duplicate ceph_client pointer in rbd_device

[Alex Elder]

The rbd_device structure maintains a duplicate copy of the
ceph_client pointer maintained in its rbd_client structure.  There
appears to be no good reason for this, and its presence presents a
risk of them getting out of synch or otherwise misused.  So kill it
off, and use the rbd_client copy only.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   44 ++++++++++++++++++++++----------------------
  1 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0430642..3d0f8cf 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -140,7 +140,6 @@ struct rbd_device {
  	struct gendisk		*disk;		/* blkdev's gendisk and rq */
  	struct request_queue	*q;

-	struct ceph_client	*client;
  	struct rbd_client	*rbd_client;

  	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */
@@ -388,7 +387,6 @@ static int rbd_get_client(struct rbd_device 
*rbd_dev, const char *mon_addr,
  		/* using an existing client */
  		kref_get(&rbdc->kref);
  		rbd_dev->rbd_client = rbdc;
-		rbd_dev->client = rbdc->client;
  		spin_unlock(&node_lock);
  		return 0;
  	}
@@ -401,7 +399,6 @@ static int rbd_get_client(struct rbd_device 
*rbd_dev, const char *mon_addr,
  	}

  	rbd_dev->rbd_client = rbdc;
-	rbd_dev->client = rbdc->client;
  	return 0;
  done_err:
  	kfree(rbd_opts);
@@ -435,7 +432,6 @@ static void rbd_put_client(struct rbd_device *rbd_dev)
  	kref_put(&rbd_dev->rbd_client->kref, rbd_client_release);
  	spin_unlock(&node_lock);
  	rbd_dev->rbd_client = NULL;
-	rbd_dev->client = NULL;
  }

  /*
@@ -858,6 +854,7 @@ static int rbd_do_request(struct request *rq,
  	struct rbd_request *req_data;
  	struct ceph_osd_request_head *reqhead;
  	struct rbd_image_header *header = &dev->header;
+	struct ceph_osd_client *osdc;

  	req_data = kzalloc(sizeof(*req_data), GFP_NOIO);
  	if (!req_data) {
@@ -876,11 +873,9 @@ static int rbd_do_request(struct request *rq,

  	down_read(&header->snap_rwsem);

-	req = ceph_osdc_alloc_request(&dev->client->osdc, flags,
-				      snapc,
-				      ops,
-				      false,
-				      GFP_NOIO, pages, bio);
+	osdc = &dev->rbd_client->client->osdc;
+	req = ceph_osdc_alloc_request(osdc, flags, snapc, ops,
+					false, GFP_NOIO, pages, bio);
  	if (!req) {
  		up_read(&header->snap_rwsem);
  		ret = -ENOMEM;
@@ -909,8 +904,8 @@ static int rbd_do_request(struct request *rq,
  	layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER);
  	layout->fl_pg_preferred = cpu_to_le32(-1);
  	layout->fl_pg_pool = cpu_to_le32(dev->poolid);
-	ceph_calc_raw_layout(&dev->client->osdc, layout, snapid,
-			     ofs, &len, &bno, req, ops);
+	ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno,
+				req, ops);

  	ceph_osdc_build_request(req, ofs, &len,
  				ops,
@@ -920,16 +915,16 @@ static int rbd_do_request(struct request *rq,
  	up_read(&header->snap_rwsem);

  	if (linger_req) {
-		ceph_osdc_set_request_linger(&dev->client->osdc, req);
+		ceph_osdc_set_request_linger(osdc, req);
  		*linger_req = req;
  	}

-	ret = ceph_osdc_start_request(&dev->client->osdc, req, false);
+	ret = ceph_osdc_start_request(osdc, req, false);
  	if (ret < 0)
  		goto done_err;

  	if (!rbd_cb) {
-		ret = ceph_osdc_wait_request(&dev->client->osdc, req);
+		ret = ceph_osdc_wait_request(osdc, req);
  		if (ver)
  			*ver = le64_to_cpu(req->r_reassert_version.version);
  		dout("reassert_ver=%lld\n",
@@ -1227,7 +1222,7 @@ static int rbd_req_sync_watch(struct rbd_device *dev,
  			      u64 ver)
  {
  	struct ceph_osd_req_op *ops;
-	struct ceph_osd_client *osdc = &dev->client->osdc;
+	struct ceph_osd_client *osdc = &dev->rbd_client->client->osdc;

  	int ret = rbd_create_rw_ops(&ops, 1, CEPH_OSD_OP_WATCH, 0);
  	if (ret < 0)
@@ -1314,7 +1309,7 @@ static int rbd_req_sync_notify(struct rbd_device *dev,
  		          const char *obj)
  {
  	struct ceph_osd_req_op *ops;
-	struct ceph_osd_client *osdc = &dev->client->osdc;
+	struct ceph_osd_client *osdc = &dev->rbd_client->client->osdc;
  	struct ceph_osd_event *event;
  	struct rbd_notify_info info;
  	int payload_len = sizeof(u32) + sizeof(u32);
@@ -1623,13 +1618,14 @@ static int rbd_header_add_snap(struct rbd_device 
*dev,
  	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 */
  	if (dev->cur_snap)
  		return -EINVAL;

-	ret = ceph_monc_create_snapid(&dev->client->monc, dev->poolid,
-				      &new_snapid);
+	monc = &dev->rbd_client->client->monc;
+	ret = ceph_monc_create_snapid(monc, dev->poolid, &new_snapid);
  	dout("created snapid=%lld\n", new_snapid);
  	if (ret < 0)
  		return ret;
@@ -1809,7 +1805,8 @@ static ssize_t rbd_client_id_show(struct device *dev,
  {
  	struct rbd_device *rbd_dev = dev_to_rbd(dev);

-	return sprintf(buf, "client%lld\n", ceph_client_id(rbd_dev->client));
+	return sprintf(buf, "client%lld\n",
+			ceph_client_id(rbd_dev->rbd_client->client));
  }

  static ssize_t rbd_pool_show(struct device *dev,
@@ -2231,7 +2228,7 @@ static ssize_t rbd_add(struct bus_type *bus,
  	mutex_unlock(&ctl_mutex);

  	/* pick the pool */
-	osdc = &rbd_dev->client->osdc;
+	osdc = &rbd_dev->rbd_client->client->osdc;
  	rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name);
  	if (rc < 0)
  		goto err_out_client;
@@ -2310,9 +2307,12 @@ static void rbd_dev_release(struct device *dev)
  	struct rbd_device *rbd_dev =
  			container_of(dev, struct rbd_device, dev);

-	if (rbd_dev->watch_request)
-		ceph_osdc_unregister_linger_request(&rbd_dev->client->osdc,
+	if (rbd_dev->watch_request) {
+		struct ceph_client *client = rbd_dev->rbd_client->client;
+
+		ceph_osdc_unregister_linger_request(&client->osdc,
  						    rbd_dev->watch_request);
+	}
  	if (rbd_dev->watch_event)
  		rbd_req_sync_unwatch(rbd_dev, rbd_dev->obj_md_name);

-- 
1.7.5.4

[PATCH 4/4] rbd: use a single value of snap_name to mean no snap

[Alex Elder]

 From Josh Durgin <josh.durgin@dreamhost.com>

There's already a constant for this anyway.

(I changed Josh's code to use memcmp() and memcpy() instead. -Alex)

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |    8 +++-----
  1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 3d0f8cf..25ed3c0 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -563,10 +563,7 @@ static int rbd_header_set_snap(struct rbd_device *dev,

  	down_write(&header->snap_rwsem);

-	if (!snap_name ||
-	    !*snap_name ||
-	    strcmp(snap_name, "-") == 0 ||
-	    strcmp(snap_name, RBD_SNAP_HEAD_NAME) == 0) {
+	if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME, sizeof RBD_SNAP_HEAD_NAME)) {
  		if (header->total_snaps)
  			snapc->seq = header->snap_seq;
  		else
@@ -2213,7 +2210,8 @@ static ssize_t rbd_add(struct bus_type *bus,
  	}

  	if (rbd_dev->snap_name[0] == 0)
-		rbd_dev->snap_name[0] = '-';
+		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+			sizeof RBD_SNAP_HEAD_NAME);

  	rbd_dev->obj_len = strlen(rbd_dev->obj);
  	snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
-- 
1.7.5.4

Re: [PATCH 4/4] rbd: use a single value of snap_name to mean no snap

[Yehuda Sadeh Weinraub]

On Tue, Feb 28, 2012 at 7:35 PM, Alex Elder <elder@dreamhost.com> wrote:
> From Josh Durgin <josh.durgin@dreamhost.com>
>
> There's already a constant for this anyway.
>
> (I changed Josh's code to use memcmp() and memcpy() instead. -Alex)
>
> Signed-off-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 3d0f8cf..25ed3c0 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -563,10 +563,7 @@ static int rbd_header_set_snap(struct rbd_device *dev,
>
>        down_write(&header->snap_rwsem);
>
> -       if (!snap_name ||
> -           !*snap_name ||
> -           strcmp(snap_name, "-") == 0 ||
> -           strcmp(snap_name, RBD_SNAP_HEAD_NAME) == 0) {
> +       if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME, sizeof

The original code checked for snap_name, we don't do it here. Also, do
we know that snap_name is pointing to at least
sizeof(RBD_SNAP_HEAD_NAME)? if not (or in any case) we should use
strcmp instead of memcmp. Also, sizeof(x) instead of sizeof x.

> RBD_SNAP_HEAD_NAME)) {
>                if (header->total_snaps)
>                        snapc->seq = header->snap_seq;
>                else
> @@ -2213,7 +2210,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>        }
>
>        if (rbd_dev->snap_name[0] == 0)
> -               rbd_dev->snap_name[0] = '-';
> +               memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
> +                       sizeof RBD_SNAP_HEAD_NAME);
>
>        rbd_dev->obj_len = strlen(rbd_dev->obj);
>        snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
> --
> 1.7.5.4
>
> --
> 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
--
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

Re: [PATCH 4/4] rbd: use a single value of snap_name to mean no snap

[Alex Elder]

On 02/28/2012 08:53 PM, Yehuda Sadeh Weinraub wrote:
> On Tue, Feb 28, 2012 at 7:35 PM, Alex Elder<elder@dreamhost.com>  wrote:
>>  From Josh Durgin<josh.durgin@dreamhost.com>
>>
>> There's already a constant for this anyway.
>>
>> (I changed Josh's code to use memcmp() and memcpy() instead. -Alex)
>>
>> Signed-off-by: Alex Elder<elder@dreamhost.com>
>> ---
>>   drivers/block/rbd.c |    8 +++-----
>>   1 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 3d0f8cf..25ed3c0 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -563,10 +563,7 @@ static int rbd_header_set_snap(struct rbd_device *dev,
>>
>>         down_write(&header->snap_rwsem);
>>
>> -       if (!snap_name ||
>> -           !*snap_name ||
>> -           strcmp(snap_name, "-") == 0 ||
>> -           strcmp(snap_name, RBD_SNAP_HEAD_NAME) == 0) {
>> +       if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME, sizeof
>
> The original code checked for snap_name, we don't do it here. Also, do
> we know that snap_name is pointing to at least
> sizeof(RBD_SNAP_HEAD_NAME)? if not (or in any case) we should use
> strcmp instead of memcmp. Also, sizeof(x) instead of sizeof x.

This function is only called in one place, within this file.
That place passes rbd_dev->snap_name as the "snap_name" argument.
rbd_dev->snap_name is an array.  So if rbd_dev is a valid structure,
rbd_dev->snap_name will not be a null pointer.

The size of that string is RBD_MAX_SNAP_NAME_LEN.  It would be
an error for sizeof(RBD_SNAP_HEAD_NAME) to exceed that length,
but I could add a check for that.  (Checking at build time would
be best but sizeof() is not available then.)

I'll put in a BUG_ON() or something, or will see if I can put in
some other kind of check.

					-Alex

>
>> RBD_SNAP_HEAD_NAME)) {
>>                 if (header->total_snaps)
>>                         snapc->seq = header->snap_seq;
>>                 else
>> @@ -2213,7 +2210,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>>         }
>>
>>         if (rbd_dev->snap_name[0] == 0)
>> -               rbd_dev->snap_name[0] = '-';
>> +               memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>> +                       sizeof RBD_SNAP_HEAD_NAME);
>>
>>         rbd_dev->obj_len = strlen(rbd_dev->obj);
>>         snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
>> --
>> 1.7.5.4
>>
>> --
>> 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

Re: [PATCH 1/4] rbd: a few small cleanups

[Sage Weil]

The "sizeof *foo" weirds me out too, but probably just because I've never 
seen it.  Visually it isn't as obvious that "foo * sizeof *bar" parses 
and associates correctly. 

sage

On Tue, 28 Feb 2012, Alex Elder wrote:

> Some minor cleanups in "drivers/block/rbd.c:
>     - Use the more meaningful "RBD_MAX_OBJ_NAME_LEN" in place if "96"
>       in the definition of RBD_MAX_MD_NAME_LEN.
>     - Use DEFINE_SPINLOCK() to define and initialize node_lock.
>     - Drop a needless (char *) cast in parse_rbd_opts_token().
>     - Make a few minor formatting changes.
> 
> Signed-off-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c |   21 +++++++++------------
>  1 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 7f40cb4..7c8c07a 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -46,7 +46,7 @@
> 
>  #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */
> 
> -#define RBD_MAX_MD_NAME_LEN	(96 + sizeof(RBD_SUFFIX))
> +#define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
>  #define RBD_MAX_POOL_NAME_LEN	64
>  #define RBD_MAX_SNAP_NAME_LEN	32
>  #define RBD_MAX_OPT_LEN		1024
> @@ -175,7 +175,7 @@ static struct bus_type rbd_bus_type = {
>  	.name		= "rbd",
>  };
> 
> -static spinlock_t node_lock;      /* protects client get/put */
> +static DEFINE_SPINLOCK(node_lock);      /* protects client get/put */
> 
>  static DEFINE_MUTEX(ctl_mutex);	  /* Serialize
> open/close/setup/teardown */
>  static LIST_HEAD(rbd_dev_list);    /* devices */
> @@ -324,7 +324,7 @@ static int parse_rbd_opts_token(char *c, void *private)
>  	substring_t argstr[MAX_OPT_ARGS];
>  	int token, intval, ret;
> 
> -	token = match_token((char *)c, rbdopt_tokens, argstr);
> +	token = match_token(c, rbdopt_tokens, argstr);
>  	if (token < 0)
>  		return -EINVAL;
> 
> @@ -372,7 +372,8 @@ static int rbd_get_client(struct rbd_device *rbd_dev,
> const char *mon_addr,
>  	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;
> 
>  	ret = ceph_parse_options(&opt, options, mon_addr,
> -				 mon_addr + strlen(mon_addr),
> parse_rbd_opts_token, rbd_opts);
> +				mon_addr + strlen(mon_addr),
> +				parse_rbd_opts_token, rbd_opts);
>  	if (ret < 0)
>  		goto done_err;
> 
> @@ -460,15 +461,13 @@ static int rbd_header_from_disk(struct rbd_image_header
> *header,
>  	u32 snap_count = le32_to_cpu(ondisk->snap_count);
>  	int ret = -ENOMEM;
> 
> -	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT))) {
> +	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
>  		return -ENXIO;
> -	}
> 
>  	init_rwsem(&header->snap_rwsem);
>  	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
>  	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
> -				snap_count *
> -				 sizeof(struct rbd_image_snap_ondisk),
> +				snap_count * sizeof *ondisk,
>  				gfp_flags);
>  	if (!header->snapc)
>  		return -ENOMEM;
> @@ -498,8 +497,7 @@ static int rbd_header_from_disk(struct rbd_image_header
> *header,
>  	header->snapc->num_snaps = snap_count;
>  	header->total_snaps = snap_count;
> 
> -	if (snap_count &&
> -	    allocated_snaps == snap_count) {
> +	if (snap_count && allocated_snaps == snap_count) {
>  		for (i = 0; i < snap_count; i++) {
>  			header->snapc->snaps[i] =
>  				le64_to_cpu(ondisk->snaps[i].id);
> @@ -2421,7 +2419,7 @@ static int rbd_sysfs_init(void)
>  	rbd_bus_type.bus_attrs = rbd_bus_attrs;
> 
>  	ret = bus_register(&rbd_bus_type);
> -	 if (ret < 0)
> +	if (ret < 0)
>  		return ret;
> 
>  	ret = device_register(&rbd_root_dev);
> @@ -2442,7 +2440,6 @@ int __init rbd_init(void)
>  	rc = rbd_sysfs_init();
>  	if (rc)
>  		return rc;
> -	spin_lock_init(&node_lock);
>  	pr_info("loaded " DRV_NAME_LONG "\n");
>  	return 0;
>  }
> -- 
> 1.7.5.4
> 
> --
> 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
> 
> 

Re: [PATCH 0/4] rbd: miscellaneous cleanups

[Sage Weil]

This series looks good!

Reviewed-by: Sage Weil <sage@newdream.net>

On Tue, 28 Feb 2012, Alex Elder wrote:

> This series makes a few small unrelated changes to the rbd code.
> The first is a set of simple cleanups.  The second makes
> ceph_parse_options() return a pointer to make the interface
> a little more obvious.  The third gets rid of a duplicate
> copy of the pointer to a ceph_client held in an rbd_device,
> and the last one makes it so RBD_SNAP_HEAD_NAME is the only
> way used for identifying the head snapshot.
> 
> 					-Alex
> --
> 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
> 
> 

Re: [PATCH 1/4] rbd: a few small cleanups

[Alex Elder]

On 03/02/2012 11:44 AM, Sage Weil wrote:
> The "sizeof *foo" weirds me out too, but probably just because I've never
> seen it.  Visually it isn't as obvious that "foo * sizeof *bar" parses
> and associates correctly.

I've already redone all of my patches to put the parentheses around
the thing to the right of the sizeof operator.  And I'll always put
them there from now on.

(I think of it more like a return operator--where I don't like
parentheses that aren't needed, and as I said elsewhere the
lack of them conveys a tiny bit of information.)

					-Alex

> sage
>
> On Tue, 28 Feb 2012, Alex Elder wrote:
>
>> Some minor cleanups in "drivers/block/rbd.c:
>>      - Use the more meaningful "RBD_MAX_OBJ_NAME_LEN" in place if "96"
>>        in the definition of RBD_MAX_MD_NAME_LEN.
>>      - Use DEFINE_SPINLOCK() to define and initialize node_lock.
>>      - Drop a needless (char *) cast in parse_rbd_opts_token().
>>      - Make a few minor formatting changes.
>>
>> Signed-off-by: Alex Elder<elder@dreamhost.com>
>> ---
>>   drivers/block/rbd.c |   21 +++++++++------------
>>   1 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 7f40cb4..7c8c07a 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -46,7 +46,7 @@
>>
>>   #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */
>>
>> -#define RBD_MAX_MD_NAME_LEN	(96 + sizeof(RBD_SUFFIX))
>> +#define RBD_MAX_MD_NAME_LEN	(RBD_MAX_OBJ_NAME_LEN + sizeof(RBD_SUFFIX))
>>   #define RBD_MAX_POOL_NAME_LEN	64
>>   #define RBD_MAX_SNAP_NAME_LEN	32
>>   #define RBD_MAX_OPT_LEN		1024
>> @@ -175,7 +175,7 @@ static struct bus_type rbd_bus_type = {
>>   	.name		= "rbd",
>>   };
>>
>> -static spinlock_t node_lock;      /* protects client get/put */
>> +static DEFINE_SPINLOCK(node_lock);      /* protects client get/put */
>>
>>   static DEFINE_MUTEX(ctl_mutex);	  /* Serialize
>> open/close/setup/teardown */
>>   static LIST_HEAD(rbd_dev_list);    /* devices */
>> @@ -324,7 +324,7 @@ static int parse_rbd_opts_token(char *c, void *private)
>>   	substring_t argstr[MAX_OPT_ARGS];
>>   	int token, intval, ret;
>>
>> -	token = match_token((char *)c, rbdopt_tokens, argstr);
>> +	token = match_token(c, rbdopt_tokens, argstr);
>>   	if (token<  0)
>>   		return -EINVAL;
>>
>> @@ -372,7 +372,8 @@ static int rbd_get_client(struct rbd_device *rbd_dev,
>> const char *mon_addr,
>>   	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;
>>
>>   	ret = ceph_parse_options(&opt, options, mon_addr,
>> -				 mon_addr + strlen(mon_addr),
>> parse_rbd_opts_token, rbd_opts);
>> +				mon_addr + strlen(mon_addr),
>> +				parse_rbd_opts_token, rbd_opts);
>>   	if (ret<  0)
>>   		goto done_err;
>>
>> @@ -460,15 +461,13 @@ static int rbd_header_from_disk(struct rbd_image_header
>> *header,
>>   	u32 snap_count = le32_to_cpu(ondisk->snap_count);
>>   	int ret = -ENOMEM;
>>
>> -	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT))) {
>> +	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
>>   		return -ENXIO;
>> -	}
>>
>>   	init_rwsem(&header->snap_rwsem);
>>   	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
>>   	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
>> -				snap_count *
>> -				 sizeof(struct rbd_image_snap_ondisk),
>> +				snap_count * sizeof *ondisk,
>>   				gfp_flags);
>>   	if (!header->snapc)
>>   		return -ENOMEM;
>> @@ -498,8 +497,7 @@ static int rbd_header_from_disk(struct rbd_image_header
>> *header,
>>   	header->snapc->num_snaps = snap_count;
>>   	header->total_snaps = snap_count;
>>
>> -	if (snap_count&&
>> -	    allocated_snaps == snap_count) {
>> +	if (snap_count&&  allocated_snaps == snap_count) {
>>   		for (i = 0; i<  snap_count; i++) {
>>   			header->snapc->snaps[i] =
>>   				le64_to_cpu(ondisk->snaps[i].id);
>> @@ -2421,7 +2419,7 @@ static int rbd_sysfs_init(void)
>>   	rbd_bus_type.bus_attrs = rbd_bus_attrs;
>>
>>   	ret = bus_register(&rbd_bus_type);
>> -	 if (ret<  0)
>> +	if (ret<  0)
>>   		return ret;
>>
>>   	ret = device_register(&rbd_root_dev);
>> @@ -2442,7 +2440,6 @@ int __init rbd_init(void)
>>   	rc = rbd_sysfs_init();
>>   	if (rc)
>>   		return rc;
>> -	spin_lock_init(&node_lock);
>>   	pr_info("loaded " DRV_NAME_LONG "\n");
>>   	return 0;
>>   }
>> --
>> 1.7.5.4
>>
>> --
>> 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
>>
>>