[PATCH 0/3] blkio-throttle: Few more cleanup and fixes

[PATCH 0/3] blkio-throttle: Few more cleanup and fixes

[Vivek Goyal]

Hi Jens,

There are 3 more cleanups and fixes for blkio throttle. Nothing major, just
that while scanning the code and while doing some testing I found good to
have fixes. Please apply.

[PATCH 1/3] blkio-throttle: There is no need to convert jiffies to milli seconds
[PATCH 2/3] blkio-throttle: limit max iops value to UINT_MAX
[PATCH 3/3] blkio-throttle: Fix possible multiplication overflow in iops calculations

Thanks
Vivek

[PATCH 1/3] blkio-throttle: There is no need to convert jiffies to milli seconds

[Vivek Goyal]

o Do not convert jiffies to mili seconds as it is not required. Just work
  with jiffies and HZ.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-throttle.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index a467002..c1bc1b6 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -439,8 +439,7 @@ static bool tg_with_in_iops_limit(struct throtl_data *td, struct throtl_grp *tg,
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, throtl_slice);
 
-	io_allowed = (tg->iops[rw] * jiffies_to_msecs(jiffy_elapsed_rnd))
-				/ MSEC_PER_SEC;
+	io_allowed = (tg->iops[rw] * jiffy_elapsed_rnd) / HZ;
 
 	if (tg->io_disp[rw] + 1 <= io_allowed) {
 		if (wait)
@@ -476,8 +475,8 @@ static bool tg_with_in_bps_limit(struct throtl_data *td, struct throtl_grp *tg,
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, throtl_slice);
 
-	tmp = tg->bps[rw] * jiffies_to_msecs(jiffy_elapsed_rnd);
-	do_div(tmp, MSEC_PER_SEC);
+	tmp = tg->bps[rw] * jiffy_elapsed_rnd;
+	do_div(tmp, HZ);
 	bytes_allowed = tmp;
 
 	if (tg->bytes_disp[rw] + bio->bi_size <= bytes_allowed) {
-- 
1.7.2.3

[PATCH 2/3] blkio-throttle: limit max iops value to UINT_MAX

[Vivek Goyal]

- Limit max iops value to UINT_MAX and return error to user if value is more
  than that instead of accepting bigger values and truncating implicitly.
---
 block/blk-cgroup.c |   11 +++++++----
 block/blk-cgroup.h |    3 +++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index e863418..b1febd0 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -656,10 +656,10 @@ static int blkio_policy_parse_and_set(char *buf,
 {
 	char *s[4], *p, *major_s = NULL, *minor_s = NULL;
 	int ret;
-	unsigned long major, minor, temp, iops;
+	unsigned long major, minor, temp;
 	int i = 0;
 	dev_t dev;
-	u64 bps;
+	u64 bps, iops;
 
 	memset(s, 0, sizeof(s));
 
@@ -731,13 +731,16 @@ static int blkio_policy_parse_and_set(char *buf,
 			break;
 		case BLKIO_THROTL_read_iops_device:
 		case BLKIO_THROTL_write_iops_device:
-			ret = strict_strtoul(s[1], 10, &iops);
+			ret = strict_strtoull(s[1], 10, &iops);
 			if (ret)
 				return -EINVAL;
 
+			if (iops > THROTL_IOPS_MAX)
+				return -EINVAL;
+
 			newpn->plid = plid;
 			newpn->fileid = fileid;
-			newpn->val.iops = iops;
+			newpn->val.iops = (unsigned int)iops;
 			break;
 		}
 		break;
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 034c355..ea4861b 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -20,6 +20,9 @@ enum blkio_policy_id {
 	BLKIO_POLICY_THROTL,		/* Throttling */
 };
 
+/* Max limits for throttle policy */
+#define THROTL_IOPS_MAX		UINT_MAX
+
 #if defined(CONFIG_BLK_CGROUP) || defined(CONFIG_BLK_CGROUP_MODULE)
 
 #ifndef CONFIG_BLK_CGROUP
-- 
1.7.2.3

[PATCH 3/3] blkio-throttle: Fix possible multiplication overflow in iops calculations

[Vivek Goyal]

o User can specify max iops value of 32bit (UINT_MAX), through cgroup
  interface. If a user has specified say 4294967294 (UNIT_MAX  - 2), then
  on 32bit platform, following multiplication can overflow.

  io_allowed = (tg->iops[rw] * jiffy_elapsed_rnd)

o Explicitly cast the multiplication to 64bit and then perform division and
  then check whether result is still great then UNINT_MAX.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-throttle.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index c1bc1b6..56ad453 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -430,6 +430,7 @@ static bool tg_with_in_iops_limit(struct throtl_data *td, struct throtl_grp *tg,
 	bool rw = bio_data_dir(bio);
 	unsigned int io_allowed;
 	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
+	u64 tmp;
 
 	jiffy_elapsed = jiffy_elapsed_rnd = jiffies - tg->slice_start[rw];
 
@@ -439,7 +440,20 @@ static bool tg_with_in_iops_limit(struct throtl_data *td, struct throtl_grp *tg,
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, throtl_slice);
 
-	io_allowed = (tg->iops[rw] * jiffy_elapsed_rnd) / HZ;
+	/*
+	 * jiffy_elapsed_rnd should not be a big value as minimum iops can be
+	 * 1 then at max jiffy elapsed should be equivalent of 1 second as we
+	 * will allow dispatch after 1 second and after that slice should
+	 * have been trimmed.
+	 */
+
+	tmp = (u64)tg->iops[rw] * jiffy_elapsed_rnd;
+	do_div(tmp, HZ);
+
+	if (tmp > UINT_MAX)
+		io_allowed = UINT_MAX;
+	else
+		io_allowed = tmp;
 
 	if (tg->io_disp[rw] + 1 <= io_allowed) {
 		if (wait)
-- 
1.7.2.3

Re: [PATCH 0/3] blkio-throttle: Few more cleanup and fixes

[Jens Axboe]

On 2010-10-01 18:20, Vivek Goyal wrote:
> Hi Jens,
> 
> There are 3 more cleanups and fixes for blkio throttle. Nothing major, just
> that while scanning the code and while doing some testing I found good to
> have fixes. Please apply.
> 
> [PATCH 1/3] blkio-throttle: There is no need to convert jiffies to milli seconds
> [PATCH 2/3] blkio-throttle: limit max iops value to UINT_MAX
> [PATCH 3/3] blkio-throttle: Fix possible multiplication overflow in iops calculations

Added, thanks Vivek.

-- 
Jens Axboe