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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJniumVKTYcJc6K=X1gGufQLX7dqNvaS-hgkzgz2st37Fyd8iw@mail.gmail.com>
Date:	Mon, 25 Jul 2011 18:29:22 -0700
From:	Paul Menage <menage@...gle.com>
To:	Wanlong Gao <gaowanlong@...fujitsu.com>
Cc:	lizf@...fujitsu.com, axboe@...nel.dk, linux-kernel@...r.kernel.org,
	wanlong.gao@...il.com
Subject: Re: [PATCH v2] cgroup:be able to remove the record of unplugged device

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@...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@...t ~]# mount -t cgroup -o blkio nodev /cgroup/
> [root@...t ~]# cd /cgroup/
> [root@...t 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@...t cgroup]# cat blkio.throttle.read_bps_device
> [root@...t cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@...t cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> [root@...t cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@...t cgroup]# ll /dev/sd*
> ls: cannot access /dev/sd*: No such file or directory
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@...t cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
>
> 2.Test with this patch:
>
> [root@...t ~]# mount -t cgroup -o blkio nodev /cgroup/
> [root@...t ~]# 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@...t ~]# cd /cgroup/
> [root@...t cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> 8:0     1000
> [root@...t 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@...t cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> [root@...t 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@...t cgroup]# echo 8:0 2000 > blkio.throttle.read_bps_device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> 8:0     2000
>
> [root@...t cgroup]# ll /dev/sd*
> ls: cannot access /dev/sd*: No such file or directory
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> 8:0     2000
> [root@...t cgroup]# echo 8:0 0 > blkio.throttle.read_bps_device
> [root@...t cgroup]# cat blkio.throttle.read_bps_device
> [root@...t cgroup]# echo 8:0 1000 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@...t cgroup]# echo 8:0 01100 > blkio.throttle.read_bps_device
> -bash: echo: write error: No such device
> [root@...t cgroup]#
>
> Signed-off-by: Wanlong Gao <gaowanlong@...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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/