rbd: four minor changes

This adds a verification that an rbd image's object order is within the upper and lower bounds supported by this implementation.

It must be at least 9 (SECTOR_SHIFT), because the Linux bio system assumes that minimum granularity.

It also must be less than 32 (at the moment anyway) because there exist spots in the code that store the size of a "segment" (object backing an rbd image) in a signed int variable, which can be 32 bits including the sign. We should be able to relax this limit once we've verified the code uses 64-bit types where needed.

Note that the CLI tool already limits the order to the range 12-25.

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

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

drivers/block/rbd.c +10 -0

index d032883..4734446 100644
@@ -533,6 +533,16 @@ static bool rbd_dev_ondisk_valid(struct
 rbd_image_header_ondisk *ondisk)
  	if (memcmp(&ondisk->text, RBD_HEADER_TEXT, sizeof (RBD_HEADER_TEXT)))
  		return false;
 
+	/* The bio layer requires at least sector-sized I/O */
+
+	if (ondisk->options.order < SECTOR_SHIFT)
+		return false;
+
+	/* If we use u64 in a few spots we may be able to loosen this */
+
+	if (ondisk->options.order > 8 * sizeof (int) - 1)
+		return false;
+
  	/*
  	 * The size of a snapshot header has to fit in a size_t, and

* that limits the number of snapshots.

This adds a verification that an rbd image's object order is within the upper and lower bounds supported by this implementation.

It must be at least 9 (SECTOR_SHIFT), because the Linux bio system assumes that minimum granularity.

It also must be less than 32 (at the moment anyway) because there exist spots in the code that store the size of a "segment" (object backing an rbd image) in a signed int variable, which can be 32 bits including the sign. We should be able to relax this limit once we've verified the code uses 64-bit types where needed.

Note that the CLI tool already limits the order to the range 12-25.

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

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

drivers/block/rbd.c +10 -0

index d032883..4734446 100644
@@ -533,6 +533,16 @@ static bool rbd_dev_ondisk_valid(struct
 rbd_image_header_ondisk *ondisk)
  	if (memcmp(&ondisk->text, RBD_HEADER_TEXT, sizeof (RBD_HEADER_TEXT)))
  		return false;
 
+	/* The bio layer requires at least sector-sized I/O */
+
+	if (ondisk->options.order < SECTOR_SHIFT)
+		return false;
+
+	/* If we use u64 in a few spots we may be able to loosen this */
+
+	if (ondisk->options.order > 8 * sizeof (int) - 1)
+		return false;
+
  	/*
  	 * The size of a snapshot header has to fit in a size_t, and

* that limits the number of snapshots.

Might want to add some output so we can tell which error is being hit.

Reviewed-by Josh Durgin <[email protected]>

Quoted 43 lines · Alex Elder

This adds a verification that an rbd image's object order is within the upper and lower bounds supported by this implementation.

It must be at least 9 (SECTOR_SHIFT), because the Linux bio system assumes that minimum granularity.

It also must be less than 32 (at the moment anyway) because there exist spots in the code that store the size of a "segment" (object backing an rbd image) in a signed int variable, which can be 32 bits including the sign. We should be able to relax this limit once we've verified the code uses 64-bit types where needed.

Note that the CLI tool already limits the order to the range 12-25.

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

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

drivers/block/rbd.c +10 -0

index d032883..4734446 100644
@@ -533,6 +533,16 @@ static bool rbd_dev_ondisk_valid(struct
 rbd_image_header_ondisk *ondisk)
      if (memcmp(&ondisk->text, RBD_HEADER_TEXT, sizeof
 (RBD_HEADER_TEXT)))
          return false;
 
+    /* The bio layer requires at least sector-sized I/O */
+
+    if (ondisk->options.order < SECTOR_SHIFT)
+        return false;
+
+    /* If we use u64 in a few spots we may be able to loosen this */
+
+    if (ondisk->options.order > 8 * sizeof (int) - 1)
+        return false;
+
      /*

* The size of a snapshot header has to fit in a size_t, and
* that limits the number of snapshots.

Change RBD_MAX_SNAP_NAME_LEN to be based on NAME_MAX. That is a practical limit for the length of a snapshot name (based on the presence of a directory using the name under /sys/bus/rbd to represent the snapshot).

The /sys entry is created by prefixing it with "snap_"; define that prefix symbolically, and take its length into account in defining the snapshot name length limit.

Enforce the limit in rbd_add_parse_args(). Also delete a dout() call in that function that was not meant to be committed.

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

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

drivers/block/rbd.c +5 -2

index 4734446..4858d92 100644
@@ -61,7 +61,10 @@
 
  #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */
 
-#define RBD_MAX_SNAP_NAME_LEN	32
+#define RBD_SNAP_DEV_NAME_PREFIX	"snap_"
+#define RBD_MAX_SNAP_NAME_LEN	\
+			(NAME_MAX - (sizeof (RBD_SNAP_DEV_NAME_PREFIX) - 1))
+
  #define RBD_MAX_SNAP_COUNT	510	/* allows max snapc to fit in 4KB */
  #define RBD_MAX_OPT_LEN		1024
 

@@ -2063,7 +2066,7 @@ static int rbd_register_snap_dev(struct rbd_snap
 *snap,
  	dev->type = &rbd_snap_device_type;
  	dev->parent = parent;
  	dev->release = rbd_snap_dev_release;
-	dev_set_name(dev, "snap_%s", snap->name);
+	dev_set_name(dev, "%s%s", RBD_SNAP_DEV_NAME_PREFIX, snap->name);
  	dout("%s: registering device for snapshot %s\n", __func__, snap->name);
 

  	ret = device_register(dev);

drivers/block/rbd.c +6 -1

@@ -2797,8 +2800,13 @@ static char *rbd_add_parse_args(struct rbd_device
 *rbd_dev,
  	if (!rbd_dev->image_name)
  		goto out_err;
 
-	/* Snapshot name is optional */
+	/* Snapshot name is optional; default is to use "head" */
+
  	len = next_token(&buf);
+	if (len > RBD_MAX_SNAP_NAME_LEN) {
+		err_ptr = ERR_PTR(-ENAMETOOLONG);
+		goto out_err;
+	}
  	if (!len) {
  		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */

  		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;

drivers/block/rbd.c +0 -2

@@ -2809,8 +2817,6 @@ static char *rbd_add_parse_args(struct rbd_device
 *rbd_dev,
  	memcpy(snap_name, buf, len);
  	*(snap_name + len) = '\0';
 
-dout("    SNAP_NAME is <%s>, len is %zd\n", snap_name, len);
-
  	return snap_name;
 

out_err:

Change RBD_MAX_SNAP_NAME_LEN to be based on NAME_MAX. That is a practical limit for the length of a snapshot name (based on the presence of a directory using the name under /sys/bus/rbd to represent the snapshot).

The /sys entry is created by prefixing it with "snap_"; define that prefix symbolically, and take its length into account in defining the snapshot name length limit.

Enforce the limit in rbd_add_parse_args(). Also delete a dout() call in that function that was not meant to be committed.

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

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

drivers/block/rbd.c +5 -2

index 4734446..4858d92 100644
@@ -61,7 +61,10 @@
 
  #define RBD_MINORS_PER_MAJOR	256		/* max minors per blkdev */
 
-#define RBD_MAX_SNAP_NAME_LEN	32
+#define RBD_SNAP_DEV_NAME_PREFIX	"snap_"
+#define RBD_MAX_SNAP_NAME_LEN	\
+			(NAME_MAX - (sizeof (RBD_SNAP_DEV_NAME_PREFIX) - 1))
+
  #define RBD_MAX_SNAP_COUNT	510	/* allows max snapc to fit in 4KB */
  #define RBD_MAX_OPT_LEN		1024
 

@@ -2063,7 +2066,7 @@ static int rbd_register_snap_dev(struct rbd_snap
 *snap,
  	dev->type = &rbd_snap_device_type;
  	dev->parent = parent;
  	dev->release = rbd_snap_dev_release;
-	dev_set_name(dev, "snap_%s", snap->name);
+	dev_set_name(dev, "%s%s", RBD_SNAP_DEV_NAME_PREFIX, snap->name);
  	dout("%s: registering device for snapshot %s\n", __func__, snap->name);
 

  	ret = device_register(dev);

drivers/block/rbd.c +6 -1

@@ -2797,8 +2800,13 @@ static char *rbd_add_parse_args(struct rbd_device
 *rbd_dev,
  	if (!rbd_dev->image_name)
  		goto out_err;
 
-	/* Snapshot name is optional */
+	/* Snapshot name is optional; default is to use "head" */
+
  	len = next_token(&buf);
+	if (len > RBD_MAX_SNAP_NAME_LEN) {
+		err_ptr = ERR_PTR(-ENAMETOOLONG);
+		goto out_err;
+	}
  	if (!len) {
  		buf = RBD_SNAP_HEAD_NAME; /* No snapshot supplied */

  		len = sizeof (RBD_SNAP_HEAD_NAME) - 1;

drivers/block/rbd.c +0 -2

@@ -2809,8 +2817,6 @@ static char *rbd_add_parse_args(struct rbd_device
 *rbd_dev,
  	memcpy(snap_name, buf, len);
  	*(snap_name + len) = '\0';
 
-dout("    SNAP_NAME is <%s>, len is %zd\n", snap_name, len);
-
  	return snap_name;
 

out_err:

The aim of this patch is to make what's going on rbd_merge_bvec() a bit more obvious than it was before. This was an issue when a recent btrfs bug led us to question whether the merge function was working correctly.

Use "seg" rather than "chunk" to indicate the units whose boundaries we care about we call "segments."

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

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

drivers/block/rbd.c +35 -16

index 4858d92..4ccce4d 100644
@@ -1566,22 +1566,41 @@ 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;
-	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)) << SECTOR_SHIFT;
-	if (max < 0)
-		max = 0; /* bio_add cannot handle a negative return */
-	if (max <= bvec->bv_len && bio_sectors == 0)
-		return bvec->bv_len;
-	return max;
+	sector_t sector_offset;
+	sector_t sectors_per_seg;
+	sector_t seg_sector_offset;
+	int ret;
+
+	/*
+	 * Find how far into its rbd segment the partition-relative
+	 * bio start sector is to offset relative to the enclosing
+	 * device.
+	 */
+	sector_offset = get_start_sect(bmd->bi_bdev) + bmd->bi_sector;
+	sectors_per_seg = 1 << (rbd_dev->header.obj_order - SECTOR_SHIFT);
+	seg_sector_offset = sector_offset & (sectors_per_seg - 1);
+
+	/*
+	 * Compute the number of bytes from that offset to the end
+	 * of the segment.  Account for what's already used by the bio.
+	 */
+	ret = (int) (sectors_per_seg - seg_sector_offset) << SECTOR_SHIFT;
+	if (ret >= bmd->bi_size)
+		ret -= bmd->bi_size;
+	else
+		ret = 0;
+
+	/*
+	 * Don't send back more than was asked for.  And if the bio
+	 * was empty, let the whole thing through because:  "Note
+	 * that a block device *must* allow a single page to be
+	 * added to an empty bio."
+	 */
+	rbd_assert(bvec->bv_len <= PAGE_SIZE);
+	if (ret > (int) bvec->bv_len || !bmd->bi_size)
+		ret = (int) bvec->bv_len;
+
+	return ret;
  }
 

static void rbd_free_disk(struct rbd_device *rbd_dev)

'segment' is better than 'chunk', but as these are RADOS objects, I really prefer just "object" here to make that clear. Maybe we can add a small block comment explaining the terminology just to make it crystal-clear?

rbd_object would be fine with me too. The problem with "segment" is it tends to make me think I'm missing a subdivision of the RADOS objects that make up an rbd image that I didn't know about.

Otherwise,

Reviewed-by Dan Mick <[email protected]>

Quoted 78 lines · Alex Elder

The aim of this patch is to make what's going on rbd_merge_bvec() a bit more obvious than it was before. This was an issue when a recent btrfs bug led us to question whether the merge function was working correctly.

Use "seg" rather than "chunk" to indicate the units whose boundaries we care about we call "segments."

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

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

drivers/block/rbd.c +35 -16

index 4858d92..4ccce4d 100644
@@ -1566,22 +1566,41 @@ 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;
-    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)) << SECTOR_SHIFT;
-    if (max < 0)
-        max = 0; /* bio_add cannot handle a negative return */
-    if (max <= bvec->bv_len && bio_sectors == 0)
-        return bvec->bv_len;
-    return max;
+    sector_t sector_offset;
+    sector_t sectors_per_seg;
+    sector_t seg_sector_offset;
+    int ret;
+
+    /*
+     * Find how far into its rbd segment the partition-relative
+     * bio start sector is to offset relative to the enclosing
+     * device.
+     */
+    sector_offset = get_start_sect(bmd->bi_bdev) + bmd->bi_sector;
+    sectors_per_seg = 1 << (rbd_dev->header.obj_order - SECTOR_SHIFT);
+    seg_sector_offset = sector_offset & (sectors_per_seg - 1);
+
+    /*
+     * Compute the number of bytes from that offset to the end
+     * of the segment.  Account for what's already used by the bio.
+     */
+    ret = (int) (sectors_per_seg - seg_sector_offset) << SECTOR_SHIFT;
+    if (ret >= bmd->bi_size)
+        ret -= bmd->bi_size;
+    else
+        ret = 0;
+
+    /*
+     * Don't send back more than was asked for.  And if the bio
+     * was empty, let the whole thing through because:  "Note
+     * that a block device *must* allow a single page to be
+     * added to an empty bio."
+     */
+    rbd_assert(bvec->bv_len <= PAGE_SIZE);
+    if (ret > (int) bvec->bv_len || !bmd->bi_size)
+        ret = (int) bvec->bv_len;
+
+    return ret;
  }
 

static void rbd_free_disk(struct rbd_device *rbd_dev)

The rbd_device structure has an embedded rbd_options structure. Such a structure is needed to work with the generic ceph argument parsing code, but there's no need to keep it around once argument parsing is done.

Use a local variable to hold the rbd options used in parsing in rbd_get_client(), and just transfer its content (it's just a read_only flag) into the field in the rbd_mapping sub-structure that requires that information.

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

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

drivers/block/rbd.c +9 -4

index 4ccce4d..3c131ab 100644
@@ -184,7 +184,6 @@ struct rbd_device {
  	struct gendisk		*disk;		/* blkdev's gendisk and rq */
 
  	u32			image_format;	/* Either 1 or 2 */
-	struct rbd_options	rbd_opts;
  	struct rbd_client	*rbd_client;
 
  	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */

@@ -456,18 +455,24 @@ static int parse_rbd_opts_token(char *c, void
 *private)
  static int rbd_get_client(struct rbd_device *rbd_dev, const char *mon_addr,
  				size_t mon_addr_len, char *options)
  {
-	struct rbd_options *rbd_opts = &rbd_dev->rbd_opts;
+	struct rbd_options rbd_opts;
  	struct ceph_options *ceph_opts;
  	struct rbd_client *rbdc;
 
-	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;
+	/* Initialize all rbd options to the defaults */
+
+	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
 
  	ceph_opts = ceph_parse_options(options, mon_addr,
  					mon_addr + mon_addr_len,
-					parse_rbd_opts_token, rbd_opts);
+					parse_rbd_opts_token, &rbd_opts);
  	if (IS_ERR(ceph_opts))
  		return PTR_ERR(ceph_opts);
 
+	/* Record the parsed rbd options */
+
+	rbd_dev->mapping.read_only = rbd_opts.read_only;
+
  	rbdc = rbd_client_find(ceph_opts);
  	if (rbdc) {

/* using an existing client */

drivers/block/rbd.c +0 -1

@@ -685,7 +690,6 @@ static int rbd_dev_set_mapping(struct rbd_device
 *rbd_dev, char *snap_name)
  		rbd_dev->mapping.size = rbd_dev->header.image_size;
  		rbd_dev->mapping.features = rbd_dev->header.features;
  		rbd_dev->mapping.snap_exists = false;
-		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
  		ret = 0;
  	} else {

  		ret = snap_by_name(rbd_dev, snap_name);

The rbd_device structure has an embedded rbd_options structure. Such a structure is needed to work with the generic ceph argument parsing code, but there's no need to keep it around once argument parsing is done.

Use a local variable to hold the rbd options used in parsing in rbd_get_client(), and just transfer its content (it's just a read_only flag) into the field in the rbd_mapping sub-structure that requires that information.

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

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

drivers/block/rbd.c +9 -4

index 4ccce4d..3c131ab 100644
@@ -184,7 +184,6 @@ struct rbd_device {
  	struct gendisk		*disk;		/* blkdev's gendisk and rq */
 
  	u32			image_format;	/* Either 1 or 2 */
-	struct rbd_options	rbd_opts;
  	struct rbd_client	*rbd_client;
 
  	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */

@@ -456,18 +455,24 @@ static int parse_rbd_opts_token(char *c, void
 *private)
  static int rbd_get_client(struct rbd_device *rbd_dev, const char *mon_addr,
  				size_t mon_addr_len, char *options)
  {
-	struct rbd_options *rbd_opts = &rbd_dev->rbd_opts;
+	struct rbd_options rbd_opts;
  	struct ceph_options *ceph_opts;
  	struct rbd_client *rbdc;
 
-	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;
+	/* Initialize all rbd options to the defaults */
+
+	rbd_opts.read_only = RBD_READ_ONLY_DEFAULT;
 
  	ceph_opts = ceph_parse_options(options, mon_addr,
  					mon_addr + mon_addr_len,
-					parse_rbd_opts_token, rbd_opts);
+					parse_rbd_opts_token, &rbd_opts);
  	if (IS_ERR(ceph_opts))
  		return PTR_ERR(ceph_opts);
 
+	/* Record the parsed rbd options */
+
+	rbd_dev->mapping.read_only = rbd_opts.read_only;
+
  	rbdc = rbd_client_find(ceph_opts);
  	if (rbdc) {

/* using an existing client */

drivers/block/rbd.c +0 -1

@@ -685,7 +690,6 @@ static int rbd_dev_set_mapping(struct rbd_device
 *rbd_dev, char *snap_name)
  		rbd_dev->mapping.size = rbd_dev->header.image_size;
  		rbd_dev->mapping.features = rbd_dev->header.features;
  		rbd_dev->mapping.snap_exists = false;
-		rbd_dev->mapping.read_only = rbd_dev->rbd_opts.read_only;
  		ret = 0;
  	} else {

  		ret = snap_by_name(rbd_dev, snap_name);