lkml.org 
[lkml]   [2010]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2 V3] io-controller: Add a new interface "weight_device" for IO-Controller
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
>
>
>




\
 
 \ /
  Last update: 2010-03-10 01:35    [W:0.133 / U:0.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site