[PATCH 0/2] rbd: more miscellaneous cleanups

[PATCH 0/2] rbd: more miscellaneous cleanups

[Alex Elder]

The patch messages explain the changes in detail.	-Alex

[PATCH 1/2] rbd: do some refactoring

[Alex Elder]

A few blocks of code are rearranged a bit here:
     - In rbd_header_from_disk():
	- Don't bother computing snap_count until we're sure the
	  on-disk header starts with a good signature.
	- Move a few independent lines of code so they are *after* a
	  check for a failed memory allocation.
	- Get rid of unnecessary local variable "ret".
     - Make a few other changes in rbd_read_header(), similar to the
       above--just moving things around a bit while preserving the
       functionality.
     - In rbd_rq_fn(), just assign rq in the while loop's controlling
       expression rather than duplicating it before and at the end of
       the loop body.  This allows the use of "continue" rather than
       "goto next" in a number of spots.
     - Rearrange the logic in snap_by_name().  End result is the same.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b7c9af5..a04322c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -486,19 +486,20 @@ static int rbd_header_from_disk(struct 
rbd_image_header *header,
  				 gfp_t gfp_flags)
  {
  	int i;
-	u32 snap_count = le32_to_cpu(ondisk->snap_count);
-	int ret = -ENOMEM;
+	u32 snap_count;

  	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);
+	snap_count = le32_to_cpu(ondisk->snap_count);
  	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
  				snap_count * sizeof *ondisk,
  				gfp_flags);
  	if (!header->snapc)
  		return -ENOMEM;
+
+	init_rwsem(&header->snap_rwsem);
+	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
  	if (snap_count) {
  		header->snap_names = kmalloc(header->snap_names_len,
  					     GFP_KERNEL);
@@ -544,7 +545,7 @@ err_names:
  	kfree(header->snap_names);
  err_snapc:
  	kfree(header->snapc);
-	return ret;
+	return -ENOMEM;
  }

  static int snap_index(struct rbd_image_header *header, int snap_num)
@@ -568,19 +569,20 @@ static int snap_by_name(struct rbd_image_header 
*header, const char *snap_name,
  	int i;
  	char *p = header->snap_names;

-	for (i = 0; i < header->total_snaps; i++, p += strlen(p) + 1) {
-		if (strcmp(snap_name, p) == 0)
-			break;
-	}
-	if (i == header->total_snaps)
-		return -ENOENT;
-	if (seq)
-		*seq = header->snapc->snaps[i];
+	for (i = 0; i < header->total_snaps; i++) {
+		if (!strcmp(snap_name, p)) {

-	if (size)
-		*size = header->snap_sizes[i];
+			/* Found it.  Pass back its id and/or size */

-	return i;
+			if (seq)
+				*seq = header->snapc->snaps[i];
+			if (size)
+				*size = header->snap_sizes[i];
+			return i;
+		}
+		p += strlen(p) + 1;	/* Skip ahead to the next name */
+	}
+	return -ENOENT;
  }

  static int rbd_header_set_snap(struct rbd_device *dev,
@@ -1443,9 +1445,7 @@ static void rbd_rq_fn(struct request_queue *q)
  	struct request *rq;
  	struct bio_pair *bp = NULL;

-	rq = blk_fetch_request(q);
-
-	while (1) {
+	while ((rq = blk_fetch_request(q))) {
  		struct bio *bio;
  		struct bio *rq_bio, *next_bio = NULL;
  		bool do_write;
@@ -1463,7 +1463,7 @@ static void rbd_rq_fn(struct request_queue *q)
  		/* filter out block requests we don't understand */
  		if ((rq->cmd_type != REQ_TYPE_FS)) {
  			__blk_end_request_all(rq, 0);
-			goto next;
+			continue;
  		}

  		/* deduce our operation (read, write) */
@@ -1474,7 +1474,7 @@ static void rbd_rq_fn(struct request_queue *q)
  		rq_bio = rq->bio;
  		if (do_write && rbd_dev->read_only) {
  			__blk_end_request_all(rq, -EROFS);
-			goto next;
+			continue;
  		}

  		spin_unlock_irq(q->queue_lock);
@@ -1488,7 +1488,7 @@ static void rbd_rq_fn(struct request_queue *q)
  		if (!coll) {
  			spin_lock_irq(q->queue_lock);
  			__blk_end_request_all(rq, -ENOMEM);
-			goto next;
+			continue;
  		}

  		do {
@@ -1534,8 +1534,6 @@ next_seg:
  		if (bp)
  			bio_pair_release(bp);
  		spin_lock_irq(q->queue_lock);
-next:
-		rq = blk_fetch_request(q);
  	}
  }

@@ -1587,15 +1585,16 @@ static int rbd_read_header(struct rbd_device 
*rbd_dev,
  	ssize_t rc;
  	struct rbd_image_header_ondisk *dh;
  	int snap_count = 0;
-	u64 snap_names_len = 0;
  	u64 ver;
+	size_t len;

+	/*
+	 * First reads the fixed-size header to determine the number
+	 * of snapshots, then re-reads it, along with all snapshot
+	 * records as well as their stored names.
+	 */
+	len = sizeof *dh;
  	while (1) {
-		int len = sizeof(*dh) +
-			  snap_count * sizeof(struct rbd_image_snap_ondisk) +
-			  snap_names_len;
-
-		rc = -ENOMEM;
  		dh = kmalloc(len, GFP_KERNEL);
  		if (!dh)
  			return -ENOMEM;
@@ -1610,21 +1609,22 @@ static int rbd_read_header(struct rbd_device 
*rbd_dev,

  		rc = rbd_header_from_disk(header, dh, snap_count, GFP_KERNEL);
  		if (rc < 0) {
-			if (rc == -ENXIO) {
+			if (rc == -ENXIO)
  				pr_warning("unrecognized header format"
  					   " for image %s", rbd_dev->obj);
-			}
  			goto out_dh;
  		}

-		if (snap_count != header->total_snaps) {
-			snap_count = header->total_snaps;
-			snap_names_len = header->snap_names_len;
-			rbd_header_free(header);
-			kfree(dh);
-			continue;
-		}
-		break;
+		if (snap_count == header->total_snaps)
+			break;
+
+		snap_count = header->total_snaps;
+		len = sizeof *dh +
+			snap_count * sizeof(struct rbd_image_snap_ondisk) +
+			header->snap_names_len;
+
+		rbd_header_free(header);
+		kfree(dh);
  	}
  	header->obj_version = ver;

-- 
1.7.5.4

[PATCH 2/2] rbd: small changes

[Alex Elder]

Here is another set of small code tidy-ups:
     - Define SECTOR_SHIFT and SECTOR_SIZE, and use these symbolic
       names throughout.  Tell the blk_queue system our physical
       block size, in the (unlikely) event we want to use something
       other than the default.
     - Delete the definition of struct rbd_info, which is never used.
     - Move the definition of dev_to_rbd() down in its source file,
       just above where it gets first used, and change its name to
       dev_to_rbd_dev().
     - Replace an open-coded operation in rbd_dev_release() to use
       dev_to_rbd_dev() instead.
     - Calculate the segment size for a given rbd_device just once in
       rbd_init_disk().
     - Use the '%zd' conversion specifier in rbd_snap_size_show(),
       since the value formatted is a size_t.
     - Switch to the '%llu' conversion specifier in rbd_snap_id_show().
       since the value formatted is unsigned.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c       |   85 
+++++++++++++++++++++++++++-----------------
  drivers/block/rbd_types.h |    4 --
  2 files changed, 52 insertions(+), 37 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a04322c..229f974 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -41,6 +41,15 @@

  #include "rbd_types.h"

+/*
+ * The basic unit of block I/O is a sector.  It is interpreted in a
+ * number of contexts in Linux (blk, bio, genhd), but the default is
+ * universally 512 bytes.  These symbols are just slightly more
+ * meaningful than the bare numbers they represent.
+ */
+#define	SECTOR_SHIFT	9
+#define	SECTOR_SIZE	(1ULL << SECTOR_SHIFT)
+
  #define RBD_DRV_NAME "rbd"
  #define RBD_DRV_NAME_LONG "rbd (rados block device)"

@@ -221,11 +230,6 @@ static struct device rbd_root_dev = {
  };


-static struct rbd_device *dev_to_rbd(struct device *dev)
-{
-	return container_of(dev, struct rbd_device, dev);
-}
-
  static struct device *rbd_get_dev(struct rbd_device *rbd_dev)
  {
  	return get_device(&rbd_dev->dev);
@@ -742,7 +746,7 @@ static struct bio *bio_chain_clone(struct bio **old, 
struct bio **next,

  			/* split the bio. We'll release it either in the next
  			   call, or it will have to be released outside */
-			bp = bio_split(old_chain, (len - total) / 512ULL);
+			bp = bio_split(old_chain, (len - total) / SECTOR_SIZE);
  			if (!bp)
  				goto err_out;

@@ -1470,7 +1474,7 @@ static void rbd_rq_fn(struct request_queue *q)
  		do_write = (rq_data_dir(rq) == WRITE);

  		size = blk_rq_bytes(rq);
-		ofs = blk_rq_pos(rq) * 512ULL;
+		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
  		rq_bio = rq->bio;
  		if (do_write && rbd_dev->read_only) {
  			__blk_end_request_all(rq, -EROFS);
@@ -1481,7 +1485,7 @@ static void rbd_rq_fn(struct request_queue *q)

  		dout("%s 0x%x bytes at 0x%llx\n",
  		     do_write ? "write" : "read",
-		     size, blk_rq_pos(rq) * 512ULL);
+		     size, blk_rq_pos(rq) * SECTOR_SIZE);

  		num_segs = rbd_get_num_segments(&rbd_dev->header, ofs, size);
  		coll = rbd_alloc_coll(num_segs);
@@ -1546,13 +1550,17 @@ static int rbd_merge_bvec(struct request_queue 
*q, struct bvec_merge_data *bmd,
  			  struct bio_vec *bvec)
  {
  	struct rbd_device *rbd_dev = q->queuedata;
-	unsigned int chunk_sectors = 1 << (rbd_dev->header.obj_order - 9);
-	sector_t sector = bmd->bi_sector + get_start_sect(bmd->bi_bdev);
-	unsigned int bio_sectors = bmd->bi_size >> 9;
+	unsigned int chunk_sectors;
+	sector_t sector;
+	unsigned int bio_sectors;
  	int max;

+	chunk_sectors = 1 << (rbd_dev->header.obj_order - SECTOR_SHIFT);
+	sector = bmd->bi_sector + get_start_sect(bmd->bi_bdev);
+	bio_sectors = bmd->bi_size >> SECTOR_SHIFT;
+
  	max =  (chunk_sectors - ((sector & (chunk_sectors - 1))
-				 + bio_sectors)) << 9;
+				 + bio_sectors)) << SECTOR_SHIFT;
  	if (max < 0)
  		max = 0; /* bio_add cannot handle a negative return */
  	if (max <= bvec->bv_len && bio_sectors == 0)
@@ -1707,7 +1715,7 @@ static int __rbd_update_snaps(struct rbd_device 
*rbd_dev)
  		return ret;

  	/* resized? */
-	set_capacity(rbd_dev->disk, h.image_size / 512ULL);
+	set_capacity(rbd_dev->disk, h.image_size / SECTOR_SIZE);

  	down_write(&rbd_dev->header.snap_rwsem);

@@ -1744,6 +1752,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	struct gendisk *disk;
  	struct request_queue *q;
  	int rc;
+	u64 segment_size;
  	u64 total_size = 0;

  	/* contact OSD, request size info about the object being mapped */
@@ -1779,11 +1788,15 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	if (!q)
  		goto out_disk;

+	/* We use the default size, but let's be explicit about it. */
+	blk_queue_physical_block_size(q, SECTOR_SIZE);
+
  	/* set io sizes to object size */
-	blk_queue_max_hw_sectors(q, rbd_obj_bytes(&rbd_dev->header) / 512ULL);
-	blk_queue_max_segment_size(q, rbd_obj_bytes(&rbd_dev->header));
-	blk_queue_io_min(q, rbd_obj_bytes(&rbd_dev->header));
-	blk_queue_io_opt(q, rbd_obj_bytes(&rbd_dev->header));
+	segment_size = rbd_obj_bytes(&rbd_dev->header);
+	blk_queue_max_hw_sectors(q, segment_size / SECTOR_SIZE);
+	blk_queue_max_segment_size(q, segment_size);
+	blk_queue_io_min(q, segment_size);
+	blk_queue_io_opt(q, segment_size);

  	blk_queue_merge_bvec(q, rbd_merge_bvec);
  	disk->queue = q;
@@ -1794,7 +1807,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
  	rbd_dev->q = q;

  	/* finally, announce the disk to the world */
-	set_capacity(disk, total_size / 512ULL);
+	set_capacity(disk, total_size / SECTOR_SIZE);
  	add_disk(disk);

  	pr_info("%s: added with size 0x%llx\n",
@@ -1811,10 +1824,15 @@ out:
    sysfs
  */

+static struct rbd_device *dev_to_rbd_dev(struct device *dev)
+{
+	return container_of(dev, struct rbd_device, dev);
+}
+
  static ssize_t rbd_size_show(struct device *dev,
  			     struct device_attribute *attr, char *buf)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

  	return sprintf(buf, "%llu\n", (unsigned long 
long)rbd_dev->header.image_size);
  }
@@ -1822,7 +1840,7 @@ static ssize_t rbd_size_show(struct device *dev,
  static ssize_t rbd_major_show(struct device *dev,
  			      struct device_attribute *attr, char *buf)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

  	return sprintf(buf, "%d\n", rbd_dev->major);
  }
@@ -1830,7 +1848,7 @@ static ssize_t rbd_major_show(struct device *dev,
  static ssize_t rbd_client_id_show(struct device *dev,
  				  struct device_attribute *attr, char *buf)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

  	return sprintf(buf, "client%lld\n",
  			ceph_client_id(rbd_dev->rbd_client->client));
@@ -1839,7 +1857,7 @@ static ssize_t rbd_client_id_show(struct device *dev,
  static ssize_t rbd_pool_show(struct device *dev,
  			     struct device_attribute *attr, char *buf)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

  	return sprintf(buf, "%s\n", rbd_dev->pool_name);
  }
@@ -1847,7 +1865,7 @@ static ssize_t rbd_pool_show(struct device *dev,
  static ssize_t rbd_name_show(struct device *dev,
  			     struct device_attribute *attr, char *buf)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

  	return sprintf(buf, "%s\n", rbd_dev->obj);
  }
@@ -1856,7 +1874,7 @@ static ssize_t rbd_snap_show(struct device *dev,
  			     struct device_attribute *attr,
  			     char *buf)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

  	return sprintf(buf, "%s\n", rbd_dev->snap_name);
  }
@@ -1866,7 +1884,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
  				 const char *buf,
  				 size_t size)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
  	int rc;
  	int ret = size;

@@ -1931,7 +1949,7 @@ static ssize_t rbd_snap_size_show(struct device *dev,
  {
  	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);

-	return sprintf(buf, "%lld\n", (long long)snap->size);
+	return sprintf(buf, "%zd\n", snap->size);
  }

  static ssize_t rbd_snap_id_show(struct device *dev,
@@ -1940,7 +1958,7 @@ static ssize_t rbd_snap_id_show(struct device *dev,
  {
  	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);

-	return sprintf(buf, "%lld\n", (long long)snap->id);
+	return sprintf(buf, "%llu\n", (unsigned long long) snap->id);
  }

  static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL);
@@ -2231,7 +2249,8 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
  /*
   * Skips over white space at *buf, and updates *buf to point to the
   * first found non-space character (if any). Returns the length of
- * the token (string of non-white space characters) found.
+ * the token (string of non-white space characters) found.  Note
+ * that *buf must be terminated with '\0'.
   */
  static inline size_t next_token(const char **buf)
  {
@@ -2249,13 +2268,14 @@ static inline size_t next_token(const char **buf)
  /*
   * Finds the next token in *buf, and if the provided token buffer is
   * big enough, copies the found token into it.  The result, if
- * copied, is guaranteed to be terminated with '\0'.
+ * copied, is guaranteed to be terminated with '\0'.  Note that *buf
+ * must be terminated with '\0' on entry.
   *
   * Returns the length of the token found (not including the '\0').
   * Return value will be 0 if no token is found, and it will be >=
   * token_size if the token would not fit.
   *
- * The *buf pointer will be updated point beyond the end of the
+ * The *buf pointer will be updated to point beyond the end of the
   * found token.  Note that this occurs even if the token buffer is
   * too small to hold it.
   */
@@ -2452,8 +2472,7 @@ static struct rbd_device *__rbd_get_dev(unsigned 
long id)

  static void rbd_dev_release(struct device *dev)
  {
-	struct rbd_device *rbd_dev =
-			container_of(dev, struct rbd_device, dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

  	if (rbd_dev->watch_request) {
  		struct ceph_client *client = rbd_dev->rbd_client->client;
@@ -2516,7 +2535,7 @@ static ssize_t rbd_snap_add(struct device *dev,
  			    const char *buf,
  			    size_t count)
  {
-	struct rbd_device *rbd_dev = dev_to_rbd(dev);
+	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
  	int ret;
  	char *name = kmalloc(count + 1, GFP_KERNEL);
  	if (!name)
diff --git a/drivers/block/rbd_types.h b/drivers/block/rbd_types.h
index fc6c678..9507086 100644
--- a/drivers/block/rbd_types.h
+++ b/drivers/block/rbd_types.h
@@ -41,10 +41,6 @@
  #define RBD_HEADER_SIGNATURE	"RBD"
  #define RBD_HEADER_VERSION	"001.005"

-struct rbd_info {
-	__le64 max_id;
-} __attribute__ ((packed));
-
  struct rbd_image_snap_ondisk {
  	__le64 id;
  	__le64 image_size;
-- 
1.7.5.4

Re: [PATCH 2/2] rbd: small changes

[Sage Weil]

On Tue, 28 Feb 2012, Alex Elder wrote:

> Here is another set of small code tidy-ups:
>     - Define SECTOR_SHIFT and SECTOR_SIZE, and use these symbolic
>       names throughout.  Tell the blk_queue system our physical
>       block size, in the (unlikely) event we want to use something
>       other than the default.
>     - Delete the definition of struct rbd_info, which is never used.
>     - Move the definition of dev_to_rbd() down in its source file,
>       just above where it gets first used, and change its name to
>       dev_to_rbd_dev().
>     - Replace an open-coded operation in rbd_dev_release() to use
>       dev_to_rbd_dev() instead.
>     - Calculate the segment size for a given rbd_device just once in
>       rbd_init_disk().
>     - Use the '%zd' conversion specifier in rbd_snap_size_show(),
>       since the value formatted is a size_t.
>     - Switch to the '%llu' conversion specifier in rbd_snap_id_show().
>       since the value formatted is unsigned.
> 
> Signed-off-by: Alex Elder <elder@dreamhost.com>
> ---
>  drivers/block/rbd.c       |   85 +++++++++++++++++++++++++++-----------------
>  drivers/block/rbd_types.h |    4 --
>  2 files changed, 52 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index a04322c..229f974 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -41,6 +41,15 @@
> 
>  #include "rbd_types.h"
> 
> +/*
> + * The basic unit of block I/O is a sector.  It is interpreted in a
> + * number of contexts in Linux (blk, bio, genhd), but the default is
> + * universally 512 bytes.  These symbols are just slightly more
> + * meaningful than the bare numbers they represent.
> + */
> +#define	SECTOR_SHIFT	9
> +#define	SECTOR_SIZE	(1ULL << SECTOR_SHIFT)
> +

I would expect the block layer already has #defines for these?



>  #define RBD_DRV_NAME "rbd"
>  #define RBD_DRV_NAME_LONG "rbd (rados block device)"
> 
> @@ -221,11 +230,6 @@ static struct device rbd_root_dev = {
>  };
> 
> 
> -static struct rbd_device *dev_to_rbd(struct device *dev)
> -{
> -	return container_of(dev, struct rbd_device, dev);
> -}
> -
>  static struct device *rbd_get_dev(struct rbd_device *rbd_dev)
>  {
>  	return get_device(&rbd_dev->dev);
> @@ -742,7 +746,7 @@ static struct bio *bio_chain_clone(struct bio **old,
> struct bio **next,
> 
>  			/* split the bio. We'll release it either in the next
>  			   call, or it will have to be released outside */
> -			bp = bio_split(old_chain, (len - total) / 512ULL);
> +			bp = bio_split(old_chain, (len - total) /
> SECTOR_SIZE);
>  			if (!bp)
>  				goto err_out;
> 
> @@ -1470,7 +1474,7 @@ static void rbd_rq_fn(struct request_queue *q)
>  		do_write = (rq_data_dir(rq) == WRITE);
> 
>  		size = blk_rq_bytes(rq);
> -		ofs = blk_rq_pos(rq) * 512ULL;
> +		ofs = blk_rq_pos(rq) * SECTOR_SIZE;
>  		rq_bio = rq->bio;
>  		if (do_write && rbd_dev->read_only) {
>  			__blk_end_request_all(rq, -EROFS);
> @@ -1481,7 +1485,7 @@ static void rbd_rq_fn(struct request_queue *q)
> 
>  		dout("%s 0x%x bytes at 0x%llx\n",
>  		     do_write ? "write" : "read",
> -		     size, blk_rq_pos(rq) * 512ULL);
> +		     size, blk_rq_pos(rq) * SECTOR_SIZE);
> 
>  		num_segs = rbd_get_num_segments(&rbd_dev->header, ofs, size);
>  		coll = rbd_alloc_coll(num_segs);
> @@ -1546,13 +1550,17 @@ static int rbd_merge_bvec(struct request_queue *q,
> struct bvec_merge_data *bmd,
>  			  struct bio_vec *bvec)
>  {
>  	struct rbd_device *rbd_dev = q->queuedata;
> -	unsigned int chunk_sectors = 1 << (rbd_dev->header.obj_order - 9);
> -	sector_t sector = bmd->bi_sector + get_start_sect(bmd->bi_bdev);
> -	unsigned int bio_sectors = bmd->bi_size >> 9;
> +	unsigned int chunk_sectors;
> +	sector_t sector;
> +	unsigned int bio_sectors;
>  	int max;
> 
> +	chunk_sectors = 1 << (rbd_dev->header.obj_order - SECTOR_SHIFT);
> +	sector = bmd->bi_sector + get_start_sect(bmd->bi_bdev);
> +	bio_sectors = bmd->bi_size >> SECTOR_SHIFT;
> +
>  	max =  (chunk_sectors - ((sector & (chunk_sectors - 1))
> -				 + bio_sectors)) << 9;
> +				 + bio_sectors)) << SECTOR_SHIFT;
>  	if (max < 0)
>  		max = 0; /* bio_add cannot handle a negative return */
>  	if (max <= bvec->bv_len && bio_sectors == 0)
> @@ -1707,7 +1715,7 @@ static int __rbd_update_snaps(struct rbd_device
> *rbd_dev)
>  		return ret;
> 
>  	/* resized? */
> -	set_capacity(rbd_dev->disk, h.image_size / 512ULL);
> +	set_capacity(rbd_dev->disk, h.image_size / SECTOR_SIZE);
> 
>  	down_write(&rbd_dev->header.snap_rwsem);
> 
> @@ -1744,6 +1752,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>  	struct gendisk *disk;
>  	struct request_queue *q;
>  	int rc;
> +	u64 segment_size;
>  	u64 total_size = 0;
> 
>  	/* contact OSD, request size info about the object being mapped */
> @@ -1779,11 +1788,15 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>  	if (!q)
>  		goto out_disk;
> 
> +	/* We use the default size, but let's be explicit about it. */
> +	blk_queue_physical_block_size(q, SECTOR_SIZE);
> +
>  	/* set io sizes to object size */
> -	blk_queue_max_hw_sectors(q, rbd_obj_bytes(&rbd_dev->header) / 512ULL);
> -	blk_queue_max_segment_size(q, rbd_obj_bytes(&rbd_dev->header));
> -	blk_queue_io_min(q, rbd_obj_bytes(&rbd_dev->header));
> -	blk_queue_io_opt(q, rbd_obj_bytes(&rbd_dev->header));
> +	segment_size = rbd_obj_bytes(&rbd_dev->header);
> +	blk_queue_max_hw_sectors(q, segment_size / SECTOR_SIZE);
> +	blk_queue_max_segment_size(q, segment_size);
> +	blk_queue_io_min(q, segment_size);
> +	blk_queue_io_opt(q, segment_size);
> 
>  	blk_queue_merge_bvec(q, rbd_merge_bvec);
>  	disk->queue = q;
> @@ -1794,7 +1807,7 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>  	rbd_dev->q = q;
> 
>  	/* finally, announce the disk to the world */
> -	set_capacity(disk, total_size / 512ULL);
> +	set_capacity(disk, total_size / SECTOR_SIZE);
>  	add_disk(disk);
> 
>  	pr_info("%s: added with size 0x%llx\n",
> @@ -1811,10 +1824,15 @@ out:
>    sysfs
>  */
> 
> +static struct rbd_device *dev_to_rbd_dev(struct device *dev)
> +{
> +	return container_of(dev, struct rbd_device, dev);
> +}
> +
>  static ssize_t rbd_size_show(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> 
>  	return sprintf(buf, "%llu\n", (unsigned long
> long)rbd_dev->header.image_size);
>  }
> @@ -1822,7 +1840,7 @@ static ssize_t rbd_size_show(struct device *dev,
>  static ssize_t rbd_major_show(struct device *dev,
>  			      struct device_attribute *attr, char *buf)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> 
>  	return sprintf(buf, "%d\n", rbd_dev->major);
>  }
> @@ -1830,7 +1848,7 @@ static ssize_t rbd_major_show(struct device *dev,
>  static ssize_t rbd_client_id_show(struct device *dev,
>  				  struct device_attribute *attr, char *buf)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> 
>  	return sprintf(buf, "client%lld\n",
>  			ceph_client_id(rbd_dev->rbd_client->client));
> @@ -1839,7 +1857,7 @@ static ssize_t rbd_client_id_show(struct device *dev,
>  static ssize_t rbd_pool_show(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> 
>  	return sprintf(buf, "%s\n", rbd_dev->pool_name);
>  }
> @@ -1847,7 +1865,7 @@ static ssize_t rbd_pool_show(struct device *dev,
>  static ssize_t rbd_name_show(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> 
>  	return sprintf(buf, "%s\n", rbd_dev->obj);
>  }
> @@ -1856,7 +1874,7 @@ static ssize_t rbd_snap_show(struct device *dev,
>  			     struct device_attribute *attr,
>  			     char *buf)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> 
>  	return sprintf(buf, "%s\n", rbd_dev->snap_name);
>  }
> @@ -1866,7 +1884,7 @@ static ssize_t rbd_image_refresh(struct device *dev,
>  				 const char *buf,
>  				 size_t size)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>  	int rc;
>  	int ret = size;
> 
> @@ -1931,7 +1949,7 @@ static ssize_t rbd_snap_size_show(struct device *dev,
>  {
>  	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);
> 
> -	return sprintf(buf, "%lld\n", (long long)snap->size);
> +	return sprintf(buf, "%zd\n", snap->size);
>  }
> 
>  static ssize_t rbd_snap_id_show(struct device *dev,
> @@ -1940,7 +1958,7 @@ static ssize_t rbd_snap_id_show(struct device *dev,
>  {
>  	struct rbd_snap *snap = container_of(dev, struct rbd_snap, dev);
> 
> -	return sprintf(buf, "%lld\n", (long long)snap->id);
> +	return sprintf(buf, "%llu\n", (unsigned long long) snap->id);
>  }
> 
>  static DEVICE_ATTR(snap_size, S_IRUGO, rbd_snap_size_show, NULL);
> @@ -2231,7 +2249,8 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
>  /*
>   * Skips over white space at *buf, and updates *buf to point to the
>   * first found non-space character (if any). Returns the length of
> - * the token (string of non-white space characters) found.
> + * the token (string of non-white space characters) found.  Note
> + * that *buf must be terminated with '\0'.
>   */
>  static inline size_t next_token(const char **buf)
>  {
> @@ -2249,13 +2268,14 @@ static inline size_t next_token(const char **buf)
>  /*
>   * Finds the next token in *buf, and if the provided token buffer is
>   * big enough, copies the found token into it.  The result, if
> - * copied, is guaranteed to be terminated with '\0'.
> + * copied, is guaranteed to be terminated with '\0'.  Note that *buf
> + * must be terminated with '\0' on entry.
>   *
>   * Returns the length of the token found (not including the '\0').
>   * Return value will be 0 if no token is found, and it will be >=
>   * token_size if the token would not fit.
>   *
> - * The *buf pointer will be updated point beyond the end of the
> + * The *buf pointer will be updated to point beyond the end of the
>   * found token.  Note that this occurs even if the token buffer is
>   * too small to hold it.
>   */
> @@ -2452,8 +2472,7 @@ static struct rbd_device *__rbd_get_dev(unsigned long
> id)
> 
>  static void rbd_dev_release(struct device *dev)
>  {
> -	struct rbd_device *rbd_dev =
> -			container_of(dev, struct rbd_device, dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
> 
>  	if (rbd_dev->watch_request) {
>  		struct ceph_client *client = rbd_dev->rbd_client->client;
> @@ -2516,7 +2535,7 @@ static ssize_t rbd_snap_add(struct device *dev,
>  			    const char *buf,
>  			    size_t count)
>  {
> -	struct rbd_device *rbd_dev = dev_to_rbd(dev);
> +	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);
>  	int ret;
>  	char *name = kmalloc(count + 1, GFP_KERNEL);
>  	if (!name)
> diff --git a/drivers/block/rbd_types.h b/drivers/block/rbd_types.h
> index fc6c678..9507086 100644
> --- a/drivers/block/rbd_types.h
> +++ b/drivers/block/rbd_types.h
> @@ -41,10 +41,6 @@
>  #define RBD_HEADER_SIGNATURE	"RBD"
>  #define RBD_HEADER_VERSION	"001.005"
> 
> -struct rbd_info {
> -	__le64 max_id;
> -} __attribute__ ((packed));
> -
>  struct rbd_image_snap_ondisk {
>  	__le64 id;
>  	__le64 image_size;
> -- 
> 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 2/2] rbd: small changes

[Alex Elder]

On 03/02/2012 01:31 PM, Sage Weil wrote:
>> >  +/*
>> >  + * The basic unit of block I/O is a sector.  It is interpreted in a
>> >  + * number of contexts in Linux (blk, bio, genhd), but the default is
>> >  + * universally 512 bytes.  These symbols are just slightly more
>> >  + * meaningful than the bare numbers they represent.
>> >  + */
>> >  +#define	SECTOR_SHIFT	9
>> >  +#define	SECTOR_SIZE	(1ULL<<  SECTOR_SHIFT)
>> >  +
> I would expect the block layer already has #defines for these?

I would too, but I don't see it.  I think "9" and "512" are
considered obvious enough.

I still prefer this though; if nothing else it makes it easy
to search for places we are using things with this meaning.

					-Alex