[PATCH 0/3] rbd: queue_depth map option

[PATCH 0/3] rbd: queue_depth map option

[Ilya Dryomov]

Hi,

See 3/3.  I'll patch rbd cli tool once this is in.

Thanks,

                Ilya


Ilya Dryomov (3):
  rbd: terminate rbd_opts_tokens with Opt_err
  rbd: store rbd_options in rbd_device
  rbd: queue_depth map option

 drivers/block/rbd.c | 59 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

-- 
1.9.3

[PATCH 1/3] rbd: terminate rbd_opts_tokens with Opt_err

[Ilya Dryomov]

Also nuke useless Opt_last_bool and don't break lines unnecessarily.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 drivers/block/rbd.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index bc88fbcb9715..4de8c9167c4b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -724,7 +724,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts)
 }
 
 /*
- * mount options
+ * (Per device) rbd map options
  */
 enum {
 	Opt_last_int,
@@ -733,8 +733,7 @@ enum {
 	/* string args above */
 	Opt_read_only,
 	Opt_read_write,
-	/* Boolean args above */
-	Opt_last_bool,
+	Opt_err
 };
 
 static match_table_t rbd_opts_tokens = {
@@ -744,8 +743,7 @@ static match_table_t rbd_opts_tokens = {
 	{Opt_read_only, "ro"},		/* Alternate spelling */
 	{Opt_read_write, "read_write"},
 	{Opt_read_write, "rw"},		/* Alternate spelling */
-	/* Boolean args above */
-	{-1, NULL}
+	{Opt_err, NULL}
 };
 
 struct rbd_options {
@@ -761,22 +759,15 @@ static int parse_rbd_opts_token(char *c, void *private)
 	int token, intval, ret;
 
 	token = match_token(c, rbd_opts_tokens, argstr);
-	if (token < 0)
-		return -EINVAL;
-
 	if (token < Opt_last_int) {
 		ret = match_int(&argstr[0], &intval);
 		if (ret < 0) {
-			pr_err("bad mount option arg (not int) "
-			       "at '%s'\n", c);
+			pr_err("bad mount option arg (not int) at '%s'\n", c);
 			return ret;
 		}
 		dout("got int token %d val %d\n", token, intval);
 	} else if (token > Opt_last_int && token < Opt_last_string) {
-		dout("got string token %d val %s\n", token,
-		     argstr[0].from);
-	} else if (token > Opt_last_string && token < Opt_last_bool) {
-		dout("got Boolean token %d\n", token);
+		dout("got string token %d val %s\n", token, argstr[0].from);
 	} else {
 		dout("got token %d\n", token);
 	}
@@ -789,9 +780,10 @@ static int parse_rbd_opts_token(char *c, void *private)
 		rbd_opts->read_only = false;
 		break;
 	default:
-		rbd_assert(false);
-		break;
+		/* libceph prints "bad option" msg */
+		return -EINVAL;
 	}
+
 	return 0;
 }
 
-- 
1.9.3

Re: [PATCH 1/3] rbd: terminate rbd_opts_tokens with Opt_err

[Alex Elder]

On 06/25/2015 04:11 AM, Ilya Dryomov wrote:
> Also nuke useless Opt_last_bool and don't break lines unnecessarily.
>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Good idea.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>   drivers/block/rbd.c | 24 ++++++++----------------
>   1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index bc88fbcb9715..4de8c9167c4b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -724,7 +724,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts)
>   }
>
>   /*
> - * mount options
> + * (Per device) rbd map options
>    */
>   enum {
>   	Opt_last_int,
> @@ -733,8 +733,7 @@ enum {
>   	/* string args above */
>   	Opt_read_only,
>   	Opt_read_write,
> -	/* Boolean args above */
> -	Opt_last_bool,
> +	Opt_err
>   };
>
>   static match_table_t rbd_opts_tokens = {
> @@ -744,8 +743,7 @@ static match_table_t rbd_opts_tokens = {
>   	{Opt_read_only, "ro"},		/* Alternate spelling */
>   	{Opt_read_write, "read_write"},
>   	{Opt_read_write, "rw"},		/* Alternate spelling */
> -	/* Boolean args above */
> -	{-1, NULL}
> +	{Opt_err, NULL}
>   };
>
>   struct rbd_options {
> @@ -761,22 +759,15 @@ static int parse_rbd_opts_token(char *c, void *private)
>   	int token, intval, ret;
>
>   	token = match_token(c, rbd_opts_tokens, argstr);
> -	if (token < 0)
> -		return -EINVAL;
> -
>   	if (token < Opt_last_int) {
>   		ret = match_int(&argstr[0], &intval);
>   		if (ret < 0) {
> -			pr_err("bad mount option arg (not int) "
> -			       "at '%s'\n", c);
> +			pr_err("bad mount option arg (not int) at '%s'\n", c);
>   			return ret;
>   		}
>   		dout("got int token %d val %d\n", token, intval);
>   	} else if (token > Opt_last_int && token < Opt_last_string) {
> -		dout("got string token %d val %s\n", token,
> -		     argstr[0].from);
> -	} else if (token > Opt_last_string && token < Opt_last_bool) {
> -		dout("got Boolean token %d\n", token);
> +		dout("got string token %d val %s\n", token, argstr[0].from);
>   	} else {
>   		dout("got token %d\n", token);
>   	}
> @@ -789,9 +780,10 @@ static int parse_rbd_opts_token(char *c, void *private)
>   		rbd_opts->read_only = false;
>   		break;
>   	default:
> -		rbd_assert(false);
> -		break;
> +		/* libceph prints "bad option" msg */
> +		return -EINVAL;
>   	}
> +
>   	return 0;
>   }
>
>

[PATCH 2/3] rbd: store rbd_options in rbd_device

[Ilya Dryomov]

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 drivers/block/rbd.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4de8c9167c4b..e502bce02d2c 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -346,6 +346,7 @@ struct rbd_device {
 	struct rbd_image_header	header;
 	unsigned long		flags;		/* possibly lock protected */
 	struct rbd_spec		*spec;
+	struct rbd_options	*opts;
 
 	char			*header_name;
 
@@ -4055,7 +4056,8 @@ static void rbd_spec_free(struct kref *kref)
 }
 
 static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
-				struct rbd_spec *spec)
+					 struct rbd_spec *spec,
+					 struct rbd_options *opts)
 {
 	struct rbd_device *rbd_dev;
 
@@ -4069,8 +4071,9 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
 	INIT_LIST_HEAD(&rbd_dev->node);
 	init_rwsem(&rbd_dev->header_rwsem);
 
-	rbd_dev->spec = spec;
 	rbd_dev->rbd_client = rbdc;
+	rbd_dev->spec = spec;
+	rbd_dev->opts = opts;
 
 	/* Initialize the layout used for all rbd requests */
 
@@ -4086,6 +4089,7 @@ static void rbd_dev_destroy(struct rbd_device *rbd_dev)
 {
 	rbd_put_client(rbd_dev->rbd_client);
 	rbd_spec_put(rbd_dev->spec);
+	kfree(rbd_dev->opts);
 	kfree(rbd_dev);
 }
 
@@ -5160,7 +5164,7 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev)
 	rbdc = __rbd_get_client(rbd_dev->rbd_client);
 
 	ret = -ENOMEM;
-	parent = rbd_dev_create(rbdc, parent_spec);
+	parent = rbd_dev_create(rbdc, parent_spec, NULL);
 	if (!parent)
 		goto out_err;
 
@@ -5406,9 +5410,6 @@ static ssize_t do_rbd_add(struct bus_type *bus,
 	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
 	if (rc < 0)
 		goto err_out_module;
-	read_only = rbd_opts->read_only;
-	kfree(rbd_opts);
-	rbd_opts = NULL;	/* done with this */
 
 	rbdc = rbd_get_client(ceph_opts);
 	if (IS_ERR(rbdc)) {
@@ -5434,11 +5435,12 @@ static ssize_t do_rbd_add(struct bus_type *bus,
 		goto err_out_client;
 	}
 
-	rbd_dev = rbd_dev_create(rbdc, spec);
+	rbd_dev = rbd_dev_create(rbdc, spec, rbd_opts);
 	if (!rbd_dev)
 		goto err_out_client;
 	rbdc = NULL;		/* rbd_dev now owns this */
 	spec = NULL;		/* rbd_dev now owns this */
+	rbd_opts = NULL;	/* rbd_dev now owns this */
 
 	rc = rbd_dev_image_probe(rbd_dev, true);
 	if (rc < 0)
@@ -5446,6 +5448,7 @@ static ssize_t do_rbd_add(struct bus_type *bus,
 
 	/* If we are mapping a snapshot it must be marked read-only */
 
+	read_only = rbd_dev->opts->read_only;
 	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
 		read_only = true;
 	rbd_dev->mapping.read_only = read_only;
@@ -5470,6 +5473,7 @@ err_out_client:
 	rbd_put_client(rbdc);
 err_out_args:
 	rbd_spec_put(spec);
+	kfree(rbd_opts);
 err_out_module:
 	module_put(THIS_MODULE);
 
-- 
1.9.3

Re: [PATCH 2/3] rbd: store rbd_options in rbd_device

[Alex Elder]

On 06/25/2015 04:11 AM, Ilya Dryomov wrote:
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Now that you need it when initializing the disk, this
makes sense.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>   drivers/block/rbd.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 4de8c9167c4b..e502bce02d2c 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -346,6 +346,7 @@ struct rbd_device {
>   	struct rbd_image_header	header;
>   	unsigned long		flags;		/* possibly lock protected */
>   	struct rbd_spec		*spec;
> +	struct rbd_options	*opts;
>
>   	char			*header_name;
>
> @@ -4055,7 +4056,8 @@ static void rbd_spec_free(struct kref *kref)
>   }
>
>   static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
> -				struct rbd_spec *spec)
> +					 struct rbd_spec *spec,
> +					 struct rbd_options *opts)
>   {
>   	struct rbd_device *rbd_dev;
>
> @@ -4069,8 +4071,9 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
>   	INIT_LIST_HEAD(&rbd_dev->node);
>   	init_rwsem(&rbd_dev->header_rwsem);
>
> -	rbd_dev->spec = spec;
>   	rbd_dev->rbd_client = rbdc;
> +	rbd_dev->spec = spec;
> +	rbd_dev->opts = opts;
>
>   	/* Initialize the layout used for all rbd requests */
>
> @@ -4086,6 +4089,7 @@ static void rbd_dev_destroy(struct rbd_device *rbd_dev)
>   {
>   	rbd_put_client(rbd_dev->rbd_client);
>   	rbd_spec_put(rbd_dev->spec);
> +	kfree(rbd_dev->opts);
>   	kfree(rbd_dev);
>   }
>
> @@ -5160,7 +5164,7 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev)
>   	rbdc = __rbd_get_client(rbd_dev->rbd_client);
>
>   	ret = -ENOMEM;
> -	parent = rbd_dev_create(rbdc, parent_spec);
> +	parent = rbd_dev_create(rbdc, parent_spec, NULL);
>   	if (!parent)
>   		goto out_err;
>
> @@ -5406,9 +5410,6 @@ static ssize_t do_rbd_add(struct bus_type *bus,
>   	rc = rbd_add_parse_args(buf, &ceph_opts, &rbd_opts, &spec);
>   	if (rc < 0)
>   		goto err_out_module;
> -	read_only = rbd_opts->read_only;
> -	kfree(rbd_opts);
> -	rbd_opts = NULL;	/* done with this */
>
>   	rbdc = rbd_get_client(ceph_opts);
>   	if (IS_ERR(rbdc)) {
> @@ -5434,11 +5435,12 @@ static ssize_t do_rbd_add(struct bus_type *bus,
>   		goto err_out_client;
>   	}
>
> -	rbd_dev = rbd_dev_create(rbdc, spec);
> +	rbd_dev = rbd_dev_create(rbdc, spec, rbd_opts);
>   	if (!rbd_dev)
>   		goto err_out_client;
>   	rbdc = NULL;		/* rbd_dev now owns this */
>   	spec = NULL;		/* rbd_dev now owns this */
> +	rbd_opts = NULL;	/* rbd_dev now owns this */
>
>   	rc = rbd_dev_image_probe(rbd_dev, true);
>   	if (rc < 0)
> @@ -5446,6 +5448,7 @@ static ssize_t do_rbd_add(struct bus_type *bus,
>
>   	/* If we are mapping a snapshot it must be marked read-only */
>
> +	read_only = rbd_dev->opts->read_only;
>   	if (rbd_dev->spec->snap_id != CEPH_NOSNAP)
>   		read_only = true;
>   	rbd_dev->mapping.read_only = read_only;
> @@ -5470,6 +5473,7 @@ err_out_client:
>   	rbd_put_client(rbdc);
>   err_out_args:
>   	rbd_spec_put(spec);
> +	kfree(rbd_opts);
>   err_out_module:
>   	module_put(THIS_MODULE);
>
>

[PATCH 3/3] rbd: queue_depth map option

[Ilya Dryomov]

nr_requests (/sys/block/rbd<id>/queue/nr_requests) is pretty much
irrelevant in blk-mq case because each driver sets its own max depth
that it can handle and that's the number of tags that gets preallocated
on setup.  Users can't increase queue depth beyond that value via
writing to nr_requests.

For rbd we are happy with the default BLKDEV_MAX_RQ (128) for most
cases but we want to give users the opportunity to increase it.
Introduce a new per-device queue_depth option to do just that:

    $ sudo rbd map -o queue_depth=1024 ...

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 drivers/block/rbd.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index e502bce02d2c..b316ee48a30b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -728,6 +728,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts)
  * (Per device) rbd map options
  */
 enum {
+	Opt_queue_depth,
 	Opt_last_int,
 	/* int args above */
 	Opt_last_string,
@@ -738,6 +739,7 @@ enum {
 };
 
 static match_table_t rbd_opts_tokens = {
+	{Opt_queue_depth, "queue_depth=%d"},
 	/* int args above */
 	/* string args above */
 	{Opt_read_only, "read_only"},
@@ -748,9 +750,11 @@ static match_table_t rbd_opts_tokens = {
 };
 
 struct rbd_options {
+	int	queue_depth;
 	bool	read_only;
 };
 
+#define RBD_QUEUE_DEPTH_DEFAULT	BLKDEV_MAX_RQ
 #define RBD_READ_ONLY_DEFAULT	false
 
 static int parse_rbd_opts_token(char *c, void *private)
@@ -774,6 +778,13 @@ static int parse_rbd_opts_token(char *c, void *private)
 	}
 
 	switch (token) {
+	case Opt_queue_depth:
+		if (intval < 1) {
+			pr_err("queue_depth out of range\n");
+			return -EINVAL;
+		}
+		rbd_opts->queue_depth = intval;
+		break;
 	case Opt_read_only:
 		rbd_opts->read_only = true;
 		break;
@@ -3761,10 +3772,9 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
 
 	memset(&rbd_dev->tag_set, 0, sizeof(rbd_dev->tag_set));
 	rbd_dev->tag_set.ops = &rbd_mq_ops;
-	rbd_dev->tag_set.queue_depth = BLKDEV_MAX_RQ;
+	rbd_dev->tag_set.queue_depth = rbd_dev->opts->queue_depth;
 	rbd_dev->tag_set.numa_node = NUMA_NO_NODE;
-	rbd_dev->tag_set.flags =
-		BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_SG_MERGE;
+	rbd_dev->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_SG_MERGE;
 	rbd_dev->tag_set.nr_hw_queues = 1;
 	rbd_dev->tag_set.cmd_size = sizeof(struct work_struct);
 
@@ -4948,6 +4958,7 @@ static int rbd_add_parse_args(const char *buf,
 		goto out_mem;
 
 	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;
+	rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT;
 
 	copts = ceph_parse_options(options, mon_addrs,
 					mon_addrs + mon_addrs_size - 1,
-- 
1.9.3

Re: [PATCH 3/3] rbd: queue_depth map option

[Alex Elder]

On 06/25/2015 04:11 AM, Ilya Dryomov wrote:
> nr_requests (/sys/block/rbd<id>/queue/nr_requests) is pretty much
> irrelevant in blk-mq case because each driver sets its own max depth
> that it can handle and that's the number of tags that gets preallocated
> on setup.  Users can't increase queue depth beyond that value via
> writing to nr_requests.
>
> For rbd we are happy with the default BLKDEV_MAX_RQ (128) for most
> cases but we want to give users the opportunity to increase it.
> Introduce a new per-device queue_depth option to do just that:
>
>      $ sudo rbd map -o queue_depth=1024 ...
>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

I haven't gone to follow through what happens with this
but I assume a value that's too large will be caught when
it's attempted to be used or something.

In any case this looks good to me.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>   drivers/block/rbd.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index e502bce02d2c..b316ee48a30b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -728,6 +728,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts)
>    * (Per device) rbd map options
>    */
>   enum {
> +	Opt_queue_depth,
>   	Opt_last_int,
>   	/* int args above */
>   	Opt_last_string,
> @@ -738,6 +739,7 @@ enum {
>   };
>
>   static match_table_t rbd_opts_tokens = {
> +	{Opt_queue_depth, "queue_depth=%d"},
>   	/* int args above */
>   	/* string args above */
>   	{Opt_read_only, "read_only"},
> @@ -748,9 +750,11 @@ static match_table_t rbd_opts_tokens = {
>   };
>
>   struct rbd_options {
> +	int	queue_depth;
>   	bool	read_only;
>   };
>
> +#define RBD_QUEUE_DEPTH_DEFAULT	BLKDEV_MAX_RQ
>   #define RBD_READ_ONLY_DEFAULT	false
>
>   static int parse_rbd_opts_token(char *c, void *private)
> @@ -774,6 +778,13 @@ static int parse_rbd_opts_token(char *c, void *private)
>   	}
>
>   	switch (token) {
> +	case Opt_queue_depth:
> +		if (intval < 1) {
> +			pr_err("queue_depth out of range\n");
> +			return -EINVAL;
> +		}
> +		rbd_opts->queue_depth = intval;
> +		break;
>   	case Opt_read_only:
>   		rbd_opts->read_only = true;
>   		break;
> @@ -3761,10 +3772,9 @@ static int rbd_init_disk(struct rbd_device *rbd_dev)
>
>   	memset(&rbd_dev->tag_set, 0, sizeof(rbd_dev->tag_set));
>   	rbd_dev->tag_set.ops = &rbd_mq_ops;
> -	rbd_dev->tag_set.queue_depth = BLKDEV_MAX_RQ;
> +	rbd_dev->tag_set.queue_depth = rbd_dev->opts->queue_depth;
>   	rbd_dev->tag_set.numa_node = NUMA_NO_NODE;
> -	rbd_dev->tag_set.flags =
> -		BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_SG_MERGE;
> +	rbd_dev->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_SG_MERGE;
>   	rbd_dev->tag_set.nr_hw_queues = 1;
>   	rbd_dev->tag_set.cmd_size = sizeof(struct work_struct);
>
> @@ -4948,6 +4958,7 @@ static int rbd_add_parse_args(const char *buf,
>   		goto out_mem;
>
>   	rbd_opts->read_only = RBD_READ_ONLY_DEFAULT;
> +	rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT;
>
>   	copts = ceph_parse_options(options, mon_addrs,
>   					mon_addrs + mon_addrs_size - 1,
>

Re: [PATCH 3/3] rbd: queue_depth map option

[Ilya Dryomov]

On Thu, Jun 25, 2015 at 5:24 PM, Alex Elder <elder@ieee.org> wrote:
> On 06/25/2015 04:11 AM, Ilya Dryomov wrote:
>>
>> nr_requests (/sys/block/rbd<id>/queue/nr_requests) is pretty much
>> irrelevant in blk-mq case because each driver sets its own max depth
>> that it can handle and that's the number of tags that gets preallocated
>> on setup.  Users can't increase queue depth beyond that value via
>> writing to nr_requests.
>>
>> For rbd we are happy with the default BLKDEV_MAX_RQ (128) for most
>> cases but we want to give users the opportunity to increase it.
>> Introduce a new per-device queue_depth option to do just that:
>>
>>      $ sudo rbd map -o queue_depth=1024 ...
>>
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>
>
> I haven't gone to follow through what happens with this
> but I assume a value that's too large will be caught when
> it's attempted to be used or something.
>
> In any case this looks good to me.
>
> Reviewed-by: Alex Elder <elder@linaro.org>

Yeah, blk-mq core will cap it at BLK_MQ_MAX_DEPTH (10240).

Thanks for the review,

                Ilya