Re: [PATCH 1/2 V3] io-controller: Add a new interface "weight_device" for IO-Controller

[Gui Jianfeng <guijianfeng@cn.fujitsu.com>]

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.

Thanks
Gui

> 
> Jens, do you have any thoughts on this?
> 
> Thanks
> Vivek
> 
> 
>