[PATCH] cgroup:be able to remove the record of unplugged device

[PATCH] cgroup:be able to remove the record of unplugged device

[Wanlong Gao]

If doing like below:
Write a record like:
"echo 8:0 1000 > blkio.throttle.read_bps_device" to control the bps of the device.
And then unplug this device without doing
"8:0 0 > blkio.throttle.read_bps_device" to remove the record,
it will not be removed until reboot, because if the device is removed,
write some record will return "-ENODEV".

With this patch, when the device is removed, then if we want to remove
the record for this removed device, just write "0" then it'll not check
if it is a present device, just remove the present record.

BTW:If it can support hot-plug, will it be more better?

Thanks

Below is my test:
1. Test native:
[root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
[root@test ~]# cd /cgroup/
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:27 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:27 /dev/sda1
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# ll /dev/sd*
ls: cannot access /dev/sd*: No such file or directory
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000

2.Test with this patch:

[root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
[root@test ~]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test ~]# cd /cgroup/
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test cgroup]# echo 8:0 2000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	2000

[root@test cgroup]# ll /dev/sd*
ls: cannot access /dev/sd*: No such file or directory
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	2000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]# echo 8:0 01100 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]#

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 block/blk-cgroup.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index bcaf16e..ea32b01 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -826,15 +826,15 @@ static int blkio_policy_parse_and_set(char *buf,
 
 	dev = MKDEV(major, minor);
 
+	if (s[1] == NULL)
+		return -EINVAL;
+
 	ret = blkio_check_dev_num(dev);
-	if (ret)
+	if (ret && (strcmp(s[1], "0\0")))
 		return ret;
 
 	newpn->dev = dev;
 
-	if (s[1] == NULL)
-		return -EINVAL;
-
 	switch (plid) {
 	case BLKIO_POLICY_PROP:
 		ret = strict_strtoul(s[1], 10, &temp);
-- 
1.7.6

Re: [PATCH] cgroup:be able to remove the record of unplugged device

[Paul Menage]

On Sun, Jul 24, 2011 at 11:03 PM, Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> @@ -826,15 +826,15 @@ static int blkio_policy_parse_and_set(char *buf,
>
>        dev = MKDEV(major, minor);
>
> +       if (s[1] == NULL)
> +               return -EINVAL;
> +

How can s[1] be NULL? If there aren't two parameters in the input we'd
have returned earlier since i!=2, and each element in s[] is
initialized from a pointer that has been dereference immediately
beforehand, and hence can't be NULL.

>        ret = blkio_check_dev_num(dev);
> -       if (ret)
> +       if (ret && (strcmp(s[1], "0\0")))
>                return ret;

Better to move the parsing of s[1] ahead of this, and do a numerical
comparison? Otherwise this will miss the case where the input string
is e.g. "8:0 00"

Paul

[PATCH v2] cgroup:be able to remove the record of unplugged device

[Wanlong Gao]

Hi Paul:
How about this version?
Thanks

Write a record like:
echo 8:0 1000 > blkio.throttle.read_bps_device
to control the bps of the device.
And then unplug this device without doing
"8:0 0 > blkio.throttle.read_bps_device" to remove the record,
it will not be removed until reboot, because if the device is removed,
write some record will return "-ENODEV".

With this patch, when the device is removed, then if we want to remove
the record for this removed device, just write "0" then it'll not check
if it is a present device, just remove the present record.

Below is my test:
1. Test native.
[root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
[root@test ~]# cd /cgroup/
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:27 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:27 /dev/sda1
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# ll /dev/sd*
ls: cannot access /dev/sd*: No such file or directory
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000

2.Test with this patch:

[root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
[root@test ~]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test ~]# cd /cgroup/
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test cgroup]# echo 8:0 2000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	2000

[root@test cgroup]# ll /dev/sd*
ls: cannot access /dev/sd*: No such file or directory
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	2000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]# echo 8:0 01100 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]#

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 block/blk-cgroup.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index bcaf16e..4e81170 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -826,19 +826,19 @@ static int blkio_policy_parse_and_set(char *buf,
 
 	dev = MKDEV(major, minor);
 
-	ret = blkio_check_dev_num(dev);
+	ret = strict_strtoul(s[1], 10, &temp);
 	if (ret)
+		return -EINVAL;
+
+	ret = blkio_check_dev_num(dev);
+	if (ret && (temp != 0))
 		return ret;
 
 	newpn->dev = dev;
 
-	if (s[1] == NULL)
-		return -EINVAL;
-
 	switch (plid) {
 	case BLKIO_POLICY_PROP:
-		ret = strict_strtoul(s[1], 10, &temp);
-		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
+		if ((temp < BLKIO_WEIGHT_MIN && temp > 0) ||
 			temp > BLKIO_WEIGHT_MAX)
 			return -EINVAL;
 
-- 
1.7.6

Re: [PATCH v2] cgroup:be able to remove the record of unplugged device

[Paul Menage]

I'd be inclined to call that variable something like 'weight' rather
than 'temp', for clarity.

The change seems to fit the intent, AFAICS. But someone who has a
really good understanding of this file should review it.
blkiocg_file_write() in particular needs a bunch of comments to make
the logic more immediately clear, and to explain why it's OK to do
e.g. blkio_update_policy_rule() outside the lock.

Paul

On Mon, Jul 25, 2011 at 5:37 PM, Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> Hi Paul:
> How about this version?
> Thanks
>
> Write a record like:
> echo 8:0 1000 > blkio.throttle.read_bps_device
> to control the bps of the device.
> And then unplug this device without doing
> "8:0 0 > blkio.throttle.read_bps_device" to remove the record,
> it will not be removed until reboot, because if the device is removed,
> write some record will return "-ENODEV".
>
> With this patch, when the device is removed, then if we want to remove
> the record for this removed device, just write "0" then it'll not check
> if it is a present device, just remove the present record.
>
> Below is my test:
> 1. Test native.
> [root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
> [root@test ~]# cd /cgroup/
> [root@test cgroup]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:27 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:27 /dev/sda1
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@test cgroup]# ll /dev/sd*
> ls: cannot access /dev/sd*: No such file or directory
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
>
> 2.Test with this patch:
>
> [root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
> [root@test ~]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
> [root@test ~]# cd /cgroup/
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@test cgroup]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
> [root@test cgroup]# echo 8:0 2000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0     2000
>
> [root@test cgroup]# ll /dev/sd*
> ls: cannot access /dev/sd*: No such file or directory
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0     2000
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@test cgroup]# echo 8:0 01100 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@test cgroup]#
>
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  block/blk-cgroup.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index bcaf16e..4e81170 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -826,19 +826,19 @@ static int blkio_policy_parse_and_set(char *buf,
>
>        dev = MKDEV(major, minor);
>
> -       ret = blkio_check_dev_num(dev);
> +       ret = strict_strtoul(s[1], 10, &temp);
>        if (ret)
> +               return -EINVAL;
> +
> +       ret = blkio_check_dev_num(dev);
> +       if (ret && (temp != 0))
>                return ret;
>
>        newpn->dev = dev;
>
> -       if (s[1] == NULL)
> -               return -EINVAL;
> -
>        switch (plid) {
>        case BLKIO_POLICY_PROP:
> -               ret = strict_strtoul(s[1], 10, &temp);
> -               if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> +               if ((temp < BLKIO_WEIGHT_MIN && temp > 0) ||
>                        temp > BLKIO_WEIGHT_MAX)
>                        return -EINVAL;
>
> --
> 1.7.6
>
>

[PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Wanlong Gao]

Write a record like:
echo 8:0 1000 > blkio.throttle.read_bps_device
to control the bps of the device.
And then unplug this device without doing
"8:0 0 > blkio.throttle.read_bps_device" to remove the record,
it will not be removed until reboot, because if the device is removed,
write some record will return "-ENODEV".

With this patch, when the device is removed, then if we want to remove
the record for this removed device, just write "0" then it'll not check
if it is a present device, just remove the present record.

Below is my test:
1. Test native.
[root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
[root@test ~]# cd /cgroup/
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:27 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:27 /dev/sda1
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# ll /dev/sd*
ls: cannot access /dev/sd*: No such file or directory
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000

2.Test with this patch:

[root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
[root@test ~]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test ~]# cd /cgroup/
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	1000
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# ll /dev/sd*
brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
[root@test cgroup]# echo 8:0 2000 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	2000

[root@test cgroup]# ll /dev/sd*
ls: cannot access /dev/sd*: No such file or directory
[root@test cgroup]# cat blkio.throttle.read_bps_device
8:0	2000
[root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
[root@test cgroup]# cat blkio.throttle.read_bps_device
[root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]# echo 8:0 01100 > blkio.throttle.read_bps_device
-bash: echo: write error: No such device
[root@test cgroup]#

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 block/blk-cgroup.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index bcaf16e..246645c 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -785,7 +785,7 @@ 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;
+	unsigned long major, minor, weight;
 	int i = 0;
 	dev_t dev;
 	u64 bps, iops;
@@ -826,25 +826,25 @@ static int blkio_policy_parse_and_set(char *buf,
 
 	dev = MKDEV(major, minor);
 
-	ret = blkio_check_dev_num(dev);
+	ret = strict_strtoul(s[1], 10, &weight);
 	if (ret)
+		return -EINVAL;
+
+	ret = blkio_check_dev_num(dev);
+	if (ret && (weight != 0))
 		return ret;
 
 	newpn->dev = dev;
 
-	if (s[1] == NULL)
-		return -EINVAL;
-
 	switch (plid) {
 	case BLKIO_POLICY_PROP:
-		ret = strict_strtoul(s[1], 10, &temp);
-		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
-			temp > BLKIO_WEIGHT_MAX)
+		if ((weight < BLKIO_WEIGHT_MIN && weight > 0) ||
+			weight > BLKIO_WEIGHT_MAX)
 			return -EINVAL;
 
 		newpn->plid = plid;
 		newpn->fileid = fileid;
-		newpn->val.weight = temp;
+		newpn->val.weight = weight;
 		break;
 	case BLKIO_POLICY_THROTL:
 		switch(fileid) {
-- 
1.7.6

Re: [PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Li Zefan]

patch looks good to me, just some minor nits below..

09:56, Wanlong Gao wrote:
> Write a record like:
> echo 8:0 1000 > blkio.throttle.read_bps_device
> to control the bps of the device.
> And then unplug this device without doing
> "8:0 0 > blkio.throttle.read_bps_device" to remove the record,
> it will not be removed until reboot, because if the device is removed,
> write some record will return "-ENODEV".
> 
> With this patch, when the device is removed, then if we want to remove
> the record for this removed device, just write "0" then it'll not check
> if it is a present device, just remove the present record.
> 
> Below is my test:
> 1. Test native.
> [root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
> [root@test ~]# cd /cgroup/
> [root@test cgroup]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:27 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:27 /dev/sda1
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0	1000
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0	1000
> [root@test cgroup]# ll /dev/sd*
> ls: cannot access /dev/sd*: No such file or directory
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0	1000
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0	1000
> 
> 2.Test with this patch:
> 
> [root@test ~]# mount -t cgroup -o blkio nodev /cgroup/
> [root@test ~]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
> [root@test ~]# cd /cgroup/
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0	1000
> [root@test cgroup]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# ll /dev/sd*
> brw-rw---- 1 root disk 8, 0 Jul 25 13:11 /dev/sda
> brw-rw---- 1 root disk 8, 1 Jul 25 13:11 /dev/sda1
> [root@test cgroup]# echo 8:0 2000 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0	2000
> 
> [root@test cgroup]# ll /dev/sd*
> ls: cannot access /dev/sd*: No such file or directory
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> 8:0	2000
> [root@test cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@test cgroup]# cat blkio.throttle.read_bps_device
> [root@test cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@test cgroup]# echo 8:0 01100 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@test cgroup]#
> 

The changelog is too verbose. How about:

The bug is we're not able to remove the device from blkio cgroup's
per-device control files if it gets unplugged.

To reproduce the bug:

  # mount -t cgroup -o blkio xxx /cgroup
  # cd /cgroup
  # echo "8:0 1000" > blkio.throttle.read_bps_device
  # unplug the device
  # cat blkio.throttle.read_bps_device
  8:0	1000
  # echo "8:0 0" > blkio.throttle.read_bps_device
  -bash: echo: write error: No such device

After patching, the device removal will succeed.

> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  block/blk-cgroup.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index bcaf16e..246645c 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -785,7 +785,7 @@ 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;
> +	unsigned long major, minor, weight;
>  	int i = 0;
>  	dev_t dev;
>  	u64 bps, iops;
> @@ -826,25 +826,25 @@ static int blkio_policy_parse_and_set(char *buf,
>  
>  	dev = MKDEV(major, minor);
>  
> -	ret = blkio_check_dev_num(dev);
> +	ret = strict_strtoul(s[1], 10, &weight);
>  	if (ret)
> +		return -EINVAL;
> +
> +	ret = blkio_check_dev_num(dev);
> +	if (ret && (weight != 0))

unnecessary parentheses.

if (ret && weight != 0)

or

if (ret && !weight)

>  		return ret;
>  
>  	newpn->dev = dev;
>  
> -	if (s[1] == NULL)
> -		return -EINVAL;
> -
>  	switch (plid) {
>  	case BLKIO_POLICY_PROP:
> -		ret = strict_strtoul(s[1], 10, &temp);
> -		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> -			temp > BLKIO_WEIGHT_MAX)
> +		if ((weight < BLKIO_WEIGHT_MIN && weight > 0) ||
> +			weight > BLKIO_WEIGHT_MAX)
>  			return -EINVAL;

Normally we align the above lines this way:

if ((weight < BLKIO_WEIGHT_MIN && weight > 0) ||
    weight > BLKIO_WEIGHT_MAX)
	return -EINVAL;

>  
>  		newpn->plid = plid;
>  		newpn->fileid = fileid;
> -		newpn->val.weight = temp;
> +		newpn->val.weight = weight;
>  		break;
>  	case BLKIO_POLICY_THROTL:
>  		switch(fileid) {

[PATCH v4] blk-cgroup:be able to remove the record of unplugged device

[Wanlong Gao]

Thanks for the comments of Paul and Li Zefan.

The bug is we're not able to remove the device from blkio cgroup's
per-device control files if it gets unplugged.

To reproduce the bug:

  # mount -t cgroup -o blkio xxx /cgroup
  # cd /cgroup
  # echo "8:0 1000" > blkio.throttle.read_bps_device
  # unplug the device
  # cat blkio.throttle.read_bps_device
  8:0	1000
  # echo "8:0 0" > blkio.throttle.read_bps_device
  -bash: echo: write error: No such device

After patching, the device removal will succeed.

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 block/blk-cgroup.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index bcaf16e..9825094 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -785,7 +785,7 @@ 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;
+	unsigned long major, minor, weight;
 	int i = 0;
 	dev_t dev;
 	u64 bps, iops;
@@ -826,25 +826,25 @@ static int blkio_policy_parse_and_set(char *buf,
 
 	dev = MKDEV(major, minor);
 
-	ret = blkio_check_dev_num(dev);
+	ret = strict_strtoul(s[1], 10, &weight);
 	if (ret)
+		return -EINVAL;
+
+	ret = blkio_check_dev_num(dev);
+	if (ret && !weight)
 		return ret;
 
 	newpn->dev = dev;
 
-	if (s[1] == NULL)
-		return -EINVAL;
-
 	switch (plid) {
 	case BLKIO_POLICY_PROP:
-		ret = strict_strtoul(s[1], 10, &temp);
-		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
-			temp > BLKIO_WEIGHT_MAX)
+		if ((weight < BLKIO_WEIGHT_MIN && weight > 0) ||
+			weight > BLKIO_WEIGHT_MAX)
 			return -EINVAL;
 
 		newpn->plid = plid;
 		newpn->fileid = fileid;
-		newpn->val.weight = temp;
+		newpn->val.weight = weight;
 		break;
 	case BLKIO_POLICY_THROTL:
 		switch(fileid) {
-- 
1.7.6

Re: [PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Vivek Goyal]

On Tue, Jul 26, 2011 at 09:56:40AM +0800, Wanlong Gao wrote:
> 
> Write a record like:
> echo 8:0 1000 > blkio.throttle.read_bps_device
> to control the bps of the device.
> And then unplug this device without doing
> "8:0 0 > blkio.throttle.read_bps_device" to remove the record,
> it will not be removed until reboot, because if the device is removed,
> write some record will return "-ENODEV".
> 
> With this patch, when the device is removed, then if we want to remove
> the record for this removed device, just write "0" then it'll not check
> if it is a present device, just remove the present record.

Ok, not checking for device presence while deleting a rule makes sense.
Some comments inline.

[..]
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  block/blk-cgroup.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index bcaf16e..246645c 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -785,7 +785,7 @@ 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;
> +	unsigned long major, minor, weight;

Do not replace temp with name weight. The place you are parsing it,
it could be weight, bps or iops. So lets keep the generic name in
place.

>  	int i = 0;
>  	dev_t dev;
>  	u64 bps, iops;
> @@ -826,25 +826,25 @@ static int blkio_policy_parse_and_set(char *buf,
>  
>  	dev = MKDEV(major, minor);
>  
> -	ret = blkio_check_dev_num(dev);
> +	ret = strict_strtoul(s[1], 10, &weight);
>  	if (ret)
> +		return -EINVAL;
> +
> +	ret = blkio_check_dev_num(dev);
> +	if (ret && (weight != 0))
>  		return ret;

Can you simplify above and just put a one comment also. Don't even call
blkio_check_dev_num() if we are removing the rule. 

/* For rule removal, do not check for device presence. */

if (temp != 0)
	ret = blkio_check_dev_num(dev);

Thanks
Vivek

Re: [PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Wanlong Gao]

On Tue, 2011-07-26 at 10:44 -0400, Vivek Goyal wrote:
> On Tue, Jul 26, 2011 at 09:56:40AM +0800, Wanlong Gao wrote:
> > 
> > Write a record like:
> > echo 8:0 1000 > blkio.throttle.read_bps_device
> > to control the bps of the device.
> > And then unplug this device without doing
> > "8:0 0 > blkio.throttle.read_bps_device" to remove the record,
> > it will not be removed until reboot, because if the device is removed,
> > write some record will return "-ENODEV".
> > 
> > With this patch, when the device is removed, then if we want to remove
> > the record for this removed device, just write "0" then it'll not check
> > if it is a present device, just remove the present record.
> 
> Ok, not checking for device presence while deleting a rule makes sense.
> Some comments inline.
> 
> [..]
> > 
> > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> > ---
> >  block/blk-cgroup.c |   18 +++++++++---------
> >  1 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> > index bcaf16e..246645c 100644
> > --- a/block/blk-cgroup.c
> > +++ b/block/blk-cgroup.c
> > @@ -785,7 +785,7 @@ 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;
> > +	unsigned long major, minor, weight;
> 
> Do not replace temp with name weight. The place you are parsing it,
> it could be weight, bps or iops. So lets keep the generic name in
> place.
OK, will keep it.
> 
> >  	int i = 0;
> >  	dev_t dev;
> >  	u64 bps, iops;
> > @@ -826,25 +826,25 @@ static int blkio_policy_parse_and_set(char *buf,
> >  
> >  	dev = MKDEV(major, minor);
> >  
> > -	ret = blkio_check_dev_num(dev);
> > +	ret = strict_strtoul(s[1], 10, &weight);
> >  	if (ret)
> > +		return -EINVAL;
> > +
> > +	ret = blkio_check_dev_num(dev);
> > +	if (ret && (weight != 0))
> >  		return ret;
> 
> Can you simplify above and just put a one comment also. Don't even call
> blkio_check_dev_num() if we are removing the rule. 
> 
> /* For rule removal, do not check for device presence. */
> 
> if (temp != 0)
> 	ret = blkio_check_dev_num(dev);
Yeah, it's better. I'll make another version of v5 and send tomorrow.

Meanwhile, Vivek, maybe you missed the v4 patch, which simplify the
comments of this patch suggested by Zefan.
The v4 patch is here:   https://lkml.org/lkml/2011/7/25/532

Thanks a lot
Best regards
Wanlong Gao

Re: [PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Paul Menage]

On Tue, Jul 26, 2011 at 7:44 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>       char *s[4], *p, *major_s = NULL, *minor_s = NULL;
>>       int ret;
>> -     unsigned long major, minor, temp;
>> +     unsigned long major, minor, weight;
>
> Do not replace temp with name weight. The place you are parsing it,
> it could be weight, bps or iops. So lets keep the generic name in
> place.

The variable currently called "temp" is only ever stored in
newpn->val.weight. iops and bps already have their own local
variables.

Paul

Re: [PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Vivek Goyal]

On Tue, Jul 26, 2011 at 10:17:16AM -0700, Paul Menage wrote:
> On Tue, Jul 26, 2011 at 7:44 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >>       char *s[4], *p, *major_s = NULL, *minor_s = NULL;
> >>       int ret;
> >> -     unsigned long major, minor, temp;
> >> +     unsigned long major, minor, weight;
> >
> > Do not replace temp with name weight. The place you are parsing it,
> > it could be weight, bps or iops. So lets keep the generic name in
> > place.
> 
> The variable currently called "temp" is only ever stored in
> newpn->val.weight. iops and bps already have their own local
> variables.

They have but the context where temp is being used to figure out if it
is a deletion rule is generic. So if a rule is being deleted (be it
weight, bps or iops), we are going to parse it early in the function.
So in that context not calling it "weight" makes sense to me.

We use bps, iops names only after knowing the value type.

Thanks
Vivek

Re: [PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Paul Menage]

On Tue, Jul 26, 2011 at 10:41 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> They have but the context where temp is being used to figure out if it
> is a deletion rule is generic. So if a rule is being deleted (be it
> weight, bps or iops), we are going to parse it early in the function.
> So in that context not calling it "weight" makes sense to me.
>
> We use bps, iops names only after knowing the value type.

Fair enough - but in that case, wouldn't it make sense to make "temp"
a u64 and just parse it once, rather than reparsing the iops/bps later
in their switch cases?

Paul

Re: [PATCH v3] blk-cgroup:be able to remove the record of unplugged device

[Vivek Goyal]

On Tue, Jul 26, 2011 at 11:40:26AM -0700, Paul Menage wrote:
> On Tue, Jul 26, 2011 at 10:41 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > They have but the context where temp is being used to figure out if it
> > is a deletion rule is generic. So if a rule is being deleted (be it
> > weight, bps or iops), we are going to parse it early in the function.
> > So in that context not calling it "weight" makes sense to me.
> >
> > We use bps, iops names only after knowing the value type.
> 
> Fair enough - but in that case, wouldn't it make sense to make "temp"
> a u64 and just parse it once, rather than reparsing the iops/bps later
> in their switch cases?

Agreed. That makes sense. Make temp u64 and use strict_strtoull() and
once parsed use that value for weight, bps and iops and don't do
parsing again.

Wanlong, could you please take care of above also in your next posting?

Thanks
Vivek

[PATCH v5] blk-cgroup:be able to remove the record of unplugged device

[Wanlong Gao]

The bug is we're not able to remove the device from blkio cgroup's
per-device control files if it gets unplugged.

To reproduce the bug:

  # mount -t cgroup -o blkio xxx /cgroup
  # cd /cgroup
  # echo "8:0 1000" > blkio.throttle.read_bps_device
  # unplug the device
  # cat blkio.throttle.read_bps_device
  8:0	1000
  # echo "8:0 0" > blkio.throttle.read_bps_device
  -bash: echo: write error: No such device

After patching, the device removal will succeed.

Thanks for the comments of Paul, Zefan, and Vivek.

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 block/blk-cgroup.c |   37 ++++++++++++++++---------------------
 1 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index bcaf16e..b596e54 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -785,10 +785,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;
+	unsigned long major, minor;
 	int i = 0;
 	dev_t dev;
-	u64 bps, iops;
+	u64 temp;
 
 	memset(s, 0, sizeof(s));
 
@@ -826,20 +826,23 @@ static int blkio_policy_parse_and_set(char *buf,
 
 	dev = MKDEV(major, minor);
 
-	ret = blkio_check_dev_num(dev);
+	ret = strict_strtoull(s[1], 10, &temp);
 	if (ret)
-		return ret;
+		return -EINVAL;
 
-	newpn->dev = dev;
+	/* For rule removal, do not check for device presence. */
+	if (temp) {
+		ret = blkio_check_dev_num(dev);
+		if (ret)
+			return ret;
+	}
 
-	if (s[1] == NULL)
-		return -EINVAL;
+	newpn->dev = dev;
 
 	switch (plid) {
 	case BLKIO_POLICY_PROP:
-		ret = strict_strtoul(s[1], 10, &temp);
-		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
-			temp > BLKIO_WEIGHT_MAX)
+		if ((temp < BLKIO_WEIGHT_MIN && temp > 0) ||
+		     temp > BLKIO_WEIGHT_MAX)
 			return -EINVAL;
 
 		newpn->plid = plid;
@@ -850,26 +853,18 @@ static int blkio_policy_parse_and_set(char *buf,
 		switch(fileid) {
 		case BLKIO_THROTL_read_bps_device:
 		case BLKIO_THROTL_write_bps_device:
-			ret = strict_strtoull(s[1], 10, &bps);
-			if (ret)
-				return -EINVAL;
-
 			newpn->plid = plid;
 			newpn->fileid = fileid;
-			newpn->val.bps = bps;
+			newpn->val.bps = temp;
 			break;
 		case BLKIO_THROTL_read_iops_device:
 		case BLKIO_THROTL_write_iops_device:
-			ret = strict_strtoull(s[1], 10, &iops);
-			if (ret)
-				return -EINVAL;
-
-			if (iops > THROTL_IOPS_MAX)
+			if (temp > THROTL_IOPS_MAX)
 				return -EINVAL;
 
 			newpn->plid = plid;
 			newpn->fileid = fileid;
-			newpn->val.iops = (unsigned int)iops;
+			newpn->val.iops = (unsigned int)temp;
 			break;
 		}
 		break;
-- 
1.7.6

Re: [PATCH v5] blk-cgroup:be able to remove the record of unplugged device

[Vivek Goyal]

On Wed, Jul 27, 2011 at 08:11:48AM +0800, Wanlong Gao wrote:
> The bug is we're not able to remove the device from blkio cgroup's
> per-device control files if it gets unplugged.
> 
> To reproduce the bug:
> 
>   # mount -t cgroup -o blkio xxx /cgroup
>   # cd /cgroup
>   # echo "8:0 1000" > blkio.throttle.read_bps_device
>   # unplug the device
>   # cat blkio.throttle.read_bps_device
>   8:0	1000
>   # echo "8:0 0" > blkio.throttle.read_bps_device
>   -bash: echo: write error: No such device
> 
> After patching, the device removal will succeed.
> 
> Thanks for the comments of Paul, Zefan, and Vivek.
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>

Thanks. This looks good to me.

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

> ---
>  block/blk-cgroup.c |   37 ++++++++++++++++---------------------
>  1 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index bcaf16e..b596e54 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -785,10 +785,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;
> +	unsigned long major, minor;
>  	int i = 0;
>  	dev_t dev;
> -	u64 bps, iops;
> +	u64 temp;
>  
>  	memset(s, 0, sizeof(s));
>  
> @@ -826,20 +826,23 @@ static int blkio_policy_parse_and_set(char *buf,
>  
>  	dev = MKDEV(major, minor);
>  
> -	ret = blkio_check_dev_num(dev);
> +	ret = strict_strtoull(s[1], 10, &temp);
>  	if (ret)
> -		return ret;
> +		return -EINVAL;
>  
> -	newpn->dev = dev;
> +	/* For rule removal, do not check for device presence. */
> +	if (temp) {
> +		ret = blkio_check_dev_num(dev);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	if (s[1] == NULL)
> -		return -EINVAL;
> +	newpn->dev = dev;
>  
>  	switch (plid) {
>  	case BLKIO_POLICY_PROP:
> -		ret = strict_strtoul(s[1], 10, &temp);
> -		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> -			temp > BLKIO_WEIGHT_MAX)
> +		if ((temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> +		     temp > BLKIO_WEIGHT_MAX)
>  			return -EINVAL;
>  
>  		newpn->plid = plid;
> @@ -850,26 +853,18 @@ static int blkio_policy_parse_and_set(char *buf,
>  		switch(fileid) {
>  		case BLKIO_THROTL_read_bps_device:
>  		case BLKIO_THROTL_write_bps_device:
> -			ret = strict_strtoull(s[1], 10, &bps);
> -			if (ret)
> -				return -EINVAL;
> -
>  			newpn->plid = plid;
>  			newpn->fileid = fileid;
> -			newpn->val.bps = bps;
> +			newpn->val.bps = temp;
>  			break;
>  		case BLKIO_THROTL_read_iops_device:
>  		case BLKIO_THROTL_write_iops_device:
> -			ret = strict_strtoull(s[1], 10, &iops);
> -			if (ret)
> -				return -EINVAL;
> -
> -			if (iops > THROTL_IOPS_MAX)
> +			if (temp > THROTL_IOPS_MAX)
>  				return -EINVAL;
>  
>  			newpn->plid = plid;
>  			newpn->fileid = fileid;
> -			newpn->val.iops = (unsigned int)iops;
> +			newpn->val.iops = (unsigned int)temp;
>  			break;
>  		}
>  		break;
> -- 
> 1.7.6

Re: [PATCH v5] blk-cgroup:be able to remove the record of unplugged device

[Wanlong Gao]

On Wed, 2011-07-27 at 10:11 -0400, Vivek Goyal wrote:
> On Wed, Jul 27, 2011 at 08:11:48AM +0800, Wanlong Gao wrote:
> > The bug is we're not able to remove the device from blkio cgroup's
> > per-device control files if it gets unplugged.
> > 
> > To reproduce the bug:
> > 
> >   # mount -t cgroup -o blkio xxx /cgroup
> >   # cd /cgroup
> >   # echo "8:0 1000" > blkio.throttle.read_bps_device
> >   # unplug the device
> >   # cat blkio.throttle.read_bps_device
> >   8:0	1000
> >   # echo "8:0 0" > blkio.throttle.read_bps_device
> >   -bash: echo: write error: No such device
> > 
> > After patching, the device removal will succeed.
> > 
> > Thanks for the comments of Paul, Zefan, and Vivek.
> > 
> > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> 
> Thanks. This looks good to me.
> 
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Vivek
> 
> > ---
> >  block/blk-cgroup.c |   37 ++++++++++++++++---------------------
> >  1 files changed, 16 insertions(+), 21 deletions(-)
> > 
> > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> > index bcaf16e..b596e54 100644
> > --- a/block/blk-cgroup.c
> > +++ b/block/blk-cgroup.c
> > @@ -785,10 +785,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;
> > +	unsigned long major, minor;
> >  	int i = 0;
> >  	dev_t dev;
> > -	u64 bps, iops;
> > +	u64 temp;
> >  
> >  	memset(s, 0, sizeof(s));
> >  
> > @@ -826,20 +826,23 @@ static int blkio_policy_parse_and_set(char *buf,
> >  
> >  	dev = MKDEV(major, minor);
> >  
> > -	ret = blkio_check_dev_num(dev);
> > +	ret = strict_strtoull(s[1], 10, &temp);
> >  	if (ret)
> > -		return ret;
> > +		return -EINVAL;
> >  
> > -	newpn->dev = dev;
> > +	/* For rule removal, do not check for device presence. */
> > +	if (temp) {
> > +		ret = blkio_check_dev_num(dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> > -	if (s[1] == NULL)
> > -		return -EINVAL;
> > +	newpn->dev = dev;
> >  
> >  	switch (plid) {
> >  	case BLKIO_POLICY_PROP:
> > -		ret = strict_strtoul(s[1], 10, &temp);
> > -		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> > -			temp > BLKIO_WEIGHT_MAX)
> > +		if ((temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> > +		     temp > BLKIO_WEIGHT_MAX)
> >  			return -EINVAL;
> >  
> >  		newpn->plid = plid;
> > @@ -850,26 +853,18 @@ static int blkio_policy_parse_and_set(char *buf,
> >  		switch(fileid) {
> >  		case BLKIO_THROTL_read_bps_device:
> >  		case BLKIO_THROTL_write_bps_device:
> > -			ret = strict_strtoull(s[1], 10, &bps);
> > -			if (ret)
> > -				return -EINVAL;
> > -
> >  			newpn->plid = plid;
> >  			newpn->fileid = fileid;
> > -			newpn->val.bps = bps;
> > +			newpn->val.bps = temp;
> >  			break;
> >  		case BLKIO_THROTL_read_iops_device:
> >  		case BLKIO_THROTL_write_iops_device:
> > -			ret = strict_strtoull(s[1], 10, &iops);
> > -			if (ret)
> > -				return -EINVAL;
> > -
> > -			if (iops > THROTL_IOPS_MAX)
> > +			if (temp > THROTL_IOPS_MAX)
> >  				return -EINVAL;
> >  
> >  			newpn->plid = plid;
> >  			newpn->fileid = fileid;
> > -			newpn->val.iops = (unsigned int)iops;
> > +			newpn->val.iops = (unsigned int)temp;
> >  			break;
> >  		}
> >  		break;
> > -- 
> > 1.7.6

How about this? For a long time.

Thanks
Wanlong Gao

Re: [PATCH v5] blk-cgroup:be able to remove the record of unplugged device

[Vivek Goyal]

On Wed, Aug 17, 2011 at 08:57:45PM +0800, Wanlong Gao wrote:
> On Wed, 2011-07-27 at 10:11 -0400, Vivek Goyal wrote:
> > On Wed, Jul 27, 2011 at 08:11:48AM +0800, Wanlong Gao wrote:
> > > The bug is we're not able to remove the device from blkio cgroup's
> > > per-device control files if it gets unplugged.
> > > 
> > > To reproduce the bug:
> > > 
> > >   # mount -t cgroup -o blkio xxx /cgroup
> > >   # cd /cgroup
> > >   # echo "8:0 1000" > blkio.throttle.read_bps_device
> > >   # unplug the device
> > >   # cat blkio.throttle.read_bps_device
> > >   8:0	1000
> > >   # echo "8:0 0" > blkio.throttle.read_bps_device
> > >   -bash: echo: write error: No such device
> > > 
> > > After patching, the device removal will succeed.
> > > 
> > > Thanks for the comments of Paul, Zefan, and Vivek.
> > > 
> > > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> > 
> > Thanks. This looks good to me.
> > 
> > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > Vivek
> > 
> > > ---
> > >  block/blk-cgroup.c |   37 ++++++++++++++++---------------------
> > >  1 files changed, 16 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> > > index bcaf16e..b596e54 100644
> > > --- a/block/blk-cgroup.c
> > > +++ b/block/blk-cgroup.c
> > > @@ -785,10 +785,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;
> > > +	unsigned long major, minor;
> > >  	int i = 0;
> > >  	dev_t dev;
> > > -	u64 bps, iops;
> > > +	u64 temp;
> > >  
> > >  	memset(s, 0, sizeof(s));
> > >  
> > > @@ -826,20 +826,23 @@ static int blkio_policy_parse_and_set(char *buf,
> > >  
> > >  	dev = MKDEV(major, minor);
> > >  
> > > -	ret = blkio_check_dev_num(dev);
> > > +	ret = strict_strtoull(s[1], 10, &temp);
> > >  	if (ret)
> > > -		return ret;
> > > +		return -EINVAL;
> > >  
> > > -	newpn->dev = dev;
> > > +	/* For rule removal, do not check for device presence. */
> > > +	if (temp) {
> > > +		ret = blkio_check_dev_num(dev);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > >  
> > > -	if (s[1] == NULL)
> > > -		return -EINVAL;
> > > +	newpn->dev = dev;
> > >  
> > >  	switch (plid) {
> > >  	case BLKIO_POLICY_PROP:
> > > -		ret = strict_strtoul(s[1], 10, &temp);
> > > -		if (ret || (temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> > > -			temp > BLKIO_WEIGHT_MAX)
> > > +		if ((temp < BLKIO_WEIGHT_MIN && temp > 0) ||
> > > +		     temp > BLKIO_WEIGHT_MAX)
> > >  			return -EINVAL;
> > >  
> > >  		newpn->plid = plid;
> > > @@ -850,26 +853,18 @@ static int blkio_policy_parse_and_set(char *buf,
> > >  		switch(fileid) {
> > >  		case BLKIO_THROTL_read_bps_device:
> > >  		case BLKIO_THROTL_write_bps_device:
> > > -			ret = strict_strtoull(s[1], 10, &bps);
> > > -			if (ret)
> > > -				return -EINVAL;
> > > -
> > >  			newpn->plid = plid;
> > >  			newpn->fileid = fileid;
> > > -			newpn->val.bps = bps;
> > > +			newpn->val.bps = temp;
> > >  			break;
> > >  		case BLKIO_THROTL_read_iops_device:
> > >  		case BLKIO_THROTL_write_iops_device:
> > > -			ret = strict_strtoull(s[1], 10, &iops);
> > > -			if (ret)
> > > -				return -EINVAL;
> > > -
> > > -			if (iops > THROTL_IOPS_MAX)
> > > +			if (temp > THROTL_IOPS_MAX)
> > >  				return -EINVAL;
> > >  
> > >  			newpn->plid = plid;
> > >  			newpn->fileid = fileid;
> > > -			newpn->val.iops = (unsigned int)iops;
> > > +			newpn->val.iops = (unsigned int)temp;
> > >  			break;
> > >  		}
> > >  		break;
> > > -- 
> > > 1.7.6
> 
> How about this? For a long time.

I have already acked this patch. Jens, what do you think about it.

Thanks
Vivek