rbd: miscellaneous cleanups

PATCH In review Alex Elder 3 participants 10 messages Feb 29, 2012 – Mar 2, 2012

Subsystems (4)

Block layer Ceph client Ceph common code (libceph) RADOS block device (RBD)

lore

Share & tools

lore

Alex Elder

author

2012-02-29 03:31 UTC #0 lore raw

cover rbd: miscellaneous cleanups

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

Alex Elder

author

2012-02-29 03:35 UTC #1 lore raw

PATCH 1/4 rbd: a few small cleanups

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 <[email protected]>

---
 drivers/block/rbd.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

drivers/block/rbd.c +2 -2

index 7f40cb4..7c8c07a 100644
@@ -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;
}

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 03:35 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

drivers/block/rbd.c +4 -2

index 7c8c07a..0430642 100644
@@ -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);

fs/ceph/super.c +4 -2

index e6325f0..1d325bc 100644
@@ -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;

include/linux/ceph/libceph.h +1 -1

index 95bd850..92eef7c 100644
@@ -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);

net/ceph/ceph_common.c +8 -8

index 761ad9d..621c322 100644
@@ -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);

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 03:35 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/block/rbd.c |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

drivers/block/rbd.c +22 ~~-22~~

index 0430642..3d0f8cf 100644
@@ -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);

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 03:35 UTC #6 lore raw

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

In reply to Alex Elder

From Josh Durgin <[email protected]>

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 <[email protected]>

---
 drivers/block/rbd.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

drivers/block/rbd.c +3 -5

index 3d0f8cf..25ed3c0 100644
@@ -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",

Signature · 1 line

1.7.5.4

Yehuda Sadeh Weinraub

2012-02-29 04:53 UTC #7 lore raw

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

Quoted 16 lines · Alex Elder

From Josh Durgin <[email protected]>
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 <[email protected]>
---
drivers/block/rbd.c | 8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)
drivers/block/rbd.c +0 -0
index 3d0f8cf..25ed3c0 100644

drivers/block/rbd.c static int rbd_header_set_snap(struct rbd_device *dev, · line 566

Quoted 8 lines

drivers/block/rbd.c +1 -4

@@ -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.

Quoted 17 lines

drivers/block/rbd.c +0 -0

 RBD_SNAP_HEAD_NAME)) {
                if (header->total_snaps)
                        snapc->seq = header->snap_seq;

else

drivers/block/rbd.c +2 -2

@@ -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

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-02-29 05:35 UTC #8 lore raw

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

Quoted 20 lines · Yehuda Sadeh Weinraub

Quoted 19 lines · Alex Elder
From Josh Durgin<[email protected]>
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<[email protected]>
---
  drivers/block/rbd.c |    8 +++-----
  1 files changed, 3 insertions(+), 5 deletions(-)
drivers/block/rbd.c +0 -0
index 3d0f8cf..25ed3c0 100644
@@ -563,10 +563,7 @@ static int rbd_header_set_snap(struct rbd_device *dev,
 
        down_write(&header->snap_rwsem);
 

Quoted 10 lines

drivers/block/rbd.c static int rbd_header_set_snap(struct rbd_device *dev,
Quoted 5 lines
drivers/block/rbd.c +1 -4
-       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

Quoted 22 lines

drivers/block/rbd.c +0 -0

 RBD_SNAP_HEAD_NAME)) {
                if (header->total_snaps)
                        snapc->seq = header->snap_seq;

else

drivers/block/rbd.c +2 -2

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sage Weil

2012-03-02 19:44 UTC #2 lore raw

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

In reply to Alex Elder

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

Quoted 112 lines · Alex Elder

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

---
 drivers/block/rbd.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

drivers/block/rbd.c +2 -2

index 7f40cb4..7c8c07a 100644
@@ -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 */

drivers/block/rbd.c +3 -2

@@ -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)

drivers/block/rbd.c +2 -4

 		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)

drivers/block/rbd.c +1 -2

 		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] =

drivers/block/rbd.c +1 -2

 				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;
 }

Signature · 6 lines

1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sage Weil

Reviewed-by on the v1 series

2012-03-02 19:46 UTC #9 lore raw

In reply to Alex Elder

This series looks good!

Reviewed-by Sage Weil <[email protected]>

Quoted 15 lines · 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
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-03-02 23:22 UTC #3 lore raw

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

In reply to Sage Weil

Quoted 3 lines · 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.

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

Quoted 116 lines

sage

Quoted 112 lines · Alex Elder

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

---
  drivers/block/rbd.c |   21 +++++++++------------
  1 files changed, 9 insertions(+), 12 deletions(-)

drivers/block/rbd.c +2 -2

index 7f40cb4..7c8c07a 100644
@@ -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 */

drivers/block/rbd.c +3 -2

@@ -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;

drivers/block/rbd.c +2 -4

@@ -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;

drivers/block/rbd.c +1 -2

@@ -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);

drivers/block/rbd.c +1 -3

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html