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

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Mar 08, 2010 at 11:39:54AM -0800, Nauman Rafique 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.

Hi Nauman,

Do we really store a device name in "struct gendisk"? IIUC, a disk is
identified by its major and minor number and then there can be multiple
device files pointing to same disk. 

So I have a disk /dev/sdc in my system and I created another alias to
same disk using mknod and mounted the disk using the alias.

mknod /dev/sdc-alias b 8 32
mount /dev/sdc-alias /mnt

If that's the case, there is no way gendisk can store the pathname.
Instead, device file has inode associated with it, and there we store
major, minor number of disk, and using that we operate on disk/partition.

If that's the case, then major/minor number based interface for blkio
makes sense. Because we also need to export stats regarding the disk
time consumed by cgroup on a particular device, the only unique identifier
of the disk seems to be {major,minor} pair and multiple block device
files can be pointing to same disk. Because it is many to one mapping, it
will not be possible to reverse map it.

So I guess we need to continue to handle rules and stats using major/minor
numbers. One improvement probably we can do and that is allow setting
rules both by major/minor number and device file path. But internally
cgroup code will map device file path to major minor numbers and rules
will be displayed against major/minor number and not original device path.

Thanks
Vivek