linux-kernel - Re: [PATCH v3] blk-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]

Date:	Tue, 26 Jul 2011 22:59:40 +0800
From:	Wanlong Gao <wanlong.gao@...il.com>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	Wanlong Gao <gaowanlong@...fujitsu.com>, menage@...gle.com,
	lizf@...fujitsu.com, axboe@...nel.dk, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] blk-cgroup:be able to remove the record of
 unplugged device

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@...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

--
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/