linux-kernel - Re: [PATCH 1/2 V3] io-controller: Add a new interface "weight

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: <20100310153032.GB10738@redhat.com>
Date:	Wed, 10 Mar 2010 10:30:32 -0500
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Gui Jianfeng <guijianfeng@...fujitsu.com>
Cc:	Nauman Rafique <nauman@...gle.com>, jens.axboe@...cle.com,
	Chad Talbott <ctalbott@...gle.com>,
	linux-kernel@...r.kernel.org, Li Zefan <lizf@...fujitsu.com>
Subject: Re: [PATCH 1/2 V3] io-controller: Add a new interface
	"weight_device" for IO-Controller

On Wed, Mar 10, 2010 at 08:33:16AM +0800, Gui Jianfeng wrote:
> Vivek Goyal wrote:
> > On Tue, Mar 09, 2010 at 09:52:06AM +0800, Gui Jianfeng wrote:
> > 
> > [..]
> >>>>>> +static int blkio_policy_parse_and_set(char *buf,
> >>>>>> +                                   struct blkio_policy_node *newpn)
> >>>>>> +{
> >>>>>> +     char *s[4], *p, *major_s = NULL, *minor_s = NULL;
> >>>>>> +     int ret;
> >>>>>> +     unsigned long major, minor, temp;
> >>>>>> +     int i = 0;
> >>>>>> +     dev_t dev;
> >>>>>> +
> >>>>>> +     memset(s, 0, sizeof(s));
> >>>>>> +
> >>>>>> +     while ((p = strsep(&buf, " ")) != NULL) {
> >>>>>> +             if (!*p)
> >>>>>> +                     continue;
> >>>>>> +
> >>>>>> +             s[i++] = p;
> >>>>>> +
> >>>>>> +             /* Prevent from inputing too many things */
> >>>>>> +             if (i == 3)
> >>>>>> +                     break;
> >>>>>> +     }
> >>>>>> +
> >>>>>> +     if (i != 2)
> >>>>>> +             return -EINVAL;
> >>>>>> +
> >>>>>> +     p = strsep(&s[0], ":");
> >>>>>> +     if (p != NULL)
> >>>>>> +             major_s = p;
> >>>>>> +     else
> >>>>>> +             return -EINVAL;
> >>>>>> +
> >>>>>> +     minor_s = s[0];
> >>>>>> +     if (!minor_s)
> >>>>>> +             return -EINVAL;
> >>>>>> +
> >>>>>> +     ret = strict_strtoul(major_s, 10, &major);
> >>>>>> +     if (ret)
> >>>>>> +             return -EINVAL;
> >>>>>> +
> >>>>>> +     ret = strict_strtoul(minor_s, 10, &minor);
> >>>>>> +     if (ret)
> >>>>>> +             return -EINVAL;
> >>>>>> +
> >>>>>> +     dev = MKDEV(major, minor);
> >>>> I am not quite sure if exposing a mojor,minor number is the best
> >>>> interface that can be exposed to user space. How about actual disk
> >>>> names like sda, sdb, .. etc? The only problem I see there is that it
> >>>> seems tricky to get to these disk names from within the block layer.
> >>>> "struct request_queue" has a pointer to backing_dev which has a device
> >>>> from which we can get major,minor. But in order to get to disk name,
> >>>> we would have to call get_gendisk which can hold a semaphore. Is this
> >>>> the reason for us going with major,minor as a user interface to
> >>>> specify a disk? I bet there are good reasons for us not keeping a
> >>>> pointer to "struct gendisk" from "struct request_queue". If we could
> >>>> keep that pointer, our user interface could be very easily modified to
> >>>> be the disk name like sda, sdb, etc.
> >>> That's a good question. Why not use device names instead of device
> >>> numbers? From user's perspective, device names will be more intutive
> >>> to use.
> >>>
> >>> At the same time, will it look odd to handle devices with their names as WWID.
> >>>
> >>> /dev/mapper/3600508b400105df70000e000026f0000
> >>>
> >>> Though I see that there is an alternate way to address the same device
> >>> like /dev/dm-2 etc.
> >>>
> >>> So from user's perspective I think it will be more intutive to handle
> >>> disk names instead of numbers.
> >>>
> >>> Gui, did you forsee issues in implementing disk names?
> >> Hi Vivek,
> >>
> >> >From the implementation of view, we need a device number as a key in blkio_policy_node, 
> >> if using device name as user interface, i can't figure out a way to retirve the 
> >> corresponding device number by means of device name (like sda, not "/dev/sda"). 
> > 
> > Hi Gui,
> > 
> > How about using full device path names (/dev/sda)? "blockdev" utility also
> > expects full device pathnames. Same seems to be the case with device mapper
> > targets.
> > 
> > "device" cgroup controller probably is using major and minor numbers because
> > it needs to control creation of device file (mknod).
> > 
> > May be we can use lookup_bdev() to get block_device pointer and then
> > get_gendisk() to check if it is a partition.
> > 
> > I am not very sure but device name/path interface might turn out to be
> > more intutive.
> 
> Hi Vivek,
> 
> I don't think using an inode path name as interface is a good idea. Because, one 
> can create new file to point to the same device. Also, pathname could be removed
> or renamed by user.
> So, i think device number is a better choice.
> 

Hi Gui,

Yes, full device file path name might not be the best option. But
disk_name (as seen in sysfs) still remains appealing.

For mapping a device name (sda) to its gendisk object, can't we iterate
through all the devices in block class in sysfs and then verify that device
name user has passed to set the rule is valid or not?

Look at block/genhd.c printk_all_partitions(). This functions iterates
through all gendisk objects.

This still leaves the issue of reaching a gendisk object from request
queue. Looking into it.

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