Messages in this thread | | | Date | Thu, 04 Mar 2010 11:23:41 +0800 | From | Gui Jianfeng <> | Subject | Re: [PATCH 2/1] io-controller: Add a new interface "policy" for IO Controller |
| |
Chad Talbott wrote: > On Wed, Mar 3, 2010 at 3:19 PM, Nauman Rafique <nauman@google.com> wrote: >> On Wed, Mar 03, 2010 at 09:33:11AM -0500, Vivek Goyal wrote: >>> On Wed, Mar 03, 2010 at 02:21:11PM +0800, Gui Jianfeng wrote: >>>> You can use the following format to assigned specific weight for a given >>>> device: >>>> #echo "major:minor weight" > blkio.policy > >>> Can we use a different name for per device weight file name than "blkio.policy". >>> May be "blkio.weight_device" or "blkio.weight_per_device" etc. > > I agree with Vivek here, and his reasoning below. This becomes more > important as more attributes are added. > >>> The reason being that "blkio.policy" name might be more suitable to switch >>> between differnt kind of BW control policies (proportional, max bandwidth etc), >>> once we implement some kind of max BW controlling policies also. So it >>> might be a good idea to not use that file name for specifying per device >>> weights. >> Well, thinking more about it, what kind of policy you implement on a block >> device will not be a per cgroup property. It will not be the case that on >> a device you are implementing max-bw for one cgroup and proportional >> weight for other cgroup. It probably will be a per device attribute and >> if need be should be controlled by /sys/block/<dev> interface. >> >> Still, being very clear what a particular cgroup file does makes sense. So >> that in future for max-bw control, we can bring in more cgroup files like >> blkio.max_bw or blkio.max_iops etc which can co-exist with blkio.weight_device >> etc. > > Agreed. I'd like to add that since we are already thinking about > expanding the policy with more attributes, perhaps > blkio_update_group_weight_fn in blkio_policy_ops should be renamed to > blkio_policy_updated_fn. Then it could be called if the user changed > any part of the policy. Further, instead of storing "weight" in > blkio_cgroup, we could store a blkio_policy_node there instead. Then > the handling of whole-cgroup and per-block-device policy items could > be more regular. > > Some quick code comments:
Hi Chad,
Thanks for the comments.
> > policy_parse_and_set() could be simplified with scanf, like: > > if (sscanf(buf, "%d:%d %d", &major, &minor, &temp) != 3) > return -EINVAL;
This can't handle the invalid format like following echo "8:16 500 500 500 ...." > blkio.policy
> > blkcg_get_weight() might better be blkcg_get_policy() and it could > return a per-disk policy node, or fall back to the cgroup policy if > none existed for this dev. This would be across policy attributes, > rather than just weight.
For the time being, i still choose blkcg_get_weight(). For there's only one attributes here, when more attributs is added, then we might change the name.
Thanks Gui
> > Thanks, > Chad > >
| |