Re: [PATCH 08/18] io-controller: idle for sometime on sync queuebefore expiring it

From: Gui Jianfeng
Date: Tue Jun 09 2009 - 21:32:31 EST

Vivek Goyal wrote:
> On Tue, Jun 09, 2009 at 03:56:38PM +0800, Gui Jianfeng wrote:
>> Vivek Goyal wrote:
>> ...
>>> +ssize_t elv_fairness_store(struct request_queue *q, const char *name,
>>> + size_t count)
>>> +{
>>> + struct elv_fq_data *efqd;
>>> + unsigned int data;
>>> + unsigned long flags;
>>> +
>>> + char *p = (char *)name;
>>> +
>>> + data = simple_strtoul(p, &p, 10);
>>> +
>>> + if (data < 0)
>>> + data = 0;
>>> + else if (data > INT_MAX)
>>> + data = INT_MAX;
>> Hi Vivek,
>>
>> data might overflow on 64 bit systems. In addition, since "fairness" is nothing
>> more than a switch, just let it be.
>>
>> Signed-off-by: Gui Jianfeng <guijianfeng@xxxxxxxxxxxxxx>
>> ---
>
> Hi Gui,
>
> How about following patch? Currently this should apply at the end of the
> patch series. If it looks good, I will merge the changes in higher level
> patches.

This patch seems good to me. Some trivial issues comment below.

>
> Thanks
> Vivek
>
> o Previously common layer elevator parameters were appearing as request
> queue parameters in sysfs. But actually these are io scheduler parameters
> in hiearchical mode. Fix it.
>
> o Use macros to define multiple sysfs C functions doing the same thing. Code
> borrowed from CFQ. Helps reduce the number of lines of by 140.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
... \
> +}
> +SHOW_FUNCTION(elv_fairness_show, efqd->fairness, 0);
> +EXPORT_SYMBOL(elv_fairness_show);
> +SHOW_FUNCTION(elv_slice_idle_show, efqd->elv_slice_idle, 1);
> +EXPORT_SYMBOL(elv_slice_idle_show);
> +SHOW_FUNCTION(elv_async_slice_idle_show, efqd->elv_async_slice_idle, 1);
> +EXPORT_SYMBOL(elv_async_slice_idle_show);
> +SHOW_FUNCTION(elv_slice_sync_show, efqd->elv_slice[1], 1);
> +EXPORT_SYMBOL(elv_slice_sync_show);
> +SHOW_FUNCTION(elv_slice_async_show, efqd->elv_slice[0], 1);
> +EXPORT_SYMBOL(elv_slice_async_show);
> +#undef SHOW_FUNCTION
> +
> +#define STORE_FUNCTION(__FUNC, __PTR, MIN, MAX, __CONV) \
> +ssize_t __FUNC(struct elevator_queue *e, const char *page, size_t count) \
> +{ \
> + struct elv_fq_data *efqd = &e->efqd; \
> + unsigned int __data; \
> + int ret = elv_var_store(&__data, (page), count); \

Since simple_strtoul returns unsigned long, it's better to make __data
be that type.

> + if (__data < (MIN)) \
> + __data = (MIN); \
> + else if (__data > (MAX)) \
> + __data = (MAX); \
> + if (__CONV) \
> + *(__PTR) = msecs_to_jiffies(__data); \
> + else \
> + *(__PTR) = __data; \
> + return ret; \
> +}
> +STORE_FUNCTION(elv_fairness_store, &efqd->fairness, 0, 1, 0);
> +EXPORT_SYMBOL(elv_fairness_store);
> +STORE_FUNCTION(elv_slice_idle_store, &efqd->elv_slice_idle, 0, UINT_MAX, 1);

Do we need to set an actual max limitation rather than UINT_MAX for these entries?

> +EXPORT_SYMBOL(elv_slice_idle_store);
> +STORE_FUNCTION(elv_async_slice_idle_store, &efqd->elv_async_slice_idle, 0, UINT_MAX, 1);
> +EXPORT_SYMBOL(elv_async_slice_idle_store);
> +STORE_FUNCTION(elv_slice_sync_store, &efqd->elv_slice[1], 1, UINT_MAX, 1);
> +EXPORT_SYMBOL(elv_slice_sync_store);
> +STORE_FUNCTION(elv_slice_async_store, &efqd->elv_slice[0], 1, UINT_MAX, 1);
> +EXPORT_SYMBOL(elv_slice_async_store);
> +#undef STORE_FUNCTION
>
> void elv_schedule_dispatch(struct request_queue *q)
> {
> Index: linux18/block/blk-sysfs.c
> ===================================================================
> --- linux18.orig/block/blk-sysfs.c 2009-06-09 10:34:59.000000000 -0400
> +++ linux18/block/blk-sysfs.c 2009-06-09 13:24:42.000000000 -0400
> @@ -307,38 +307,6 @@ static struct queue_sysfs_entry queue_io
> .store = queue_iostats_store,
> };
>
> -#ifdef CONFIG_ELV_FAIR_QUEUING
> -static struct queue_sysfs_entry queue_slice_idle_entry = {
> - .attr = {.name = "slice_idle", .mode = S_IRUGO | S_IWUSR },
> - .show = elv_slice_idle_show,
> - .store = elv_slice_idle_store,
> -};
> -
> -static struct queue_sysfs_entry queue_async_slice_idle_entry = {
> - .attr = {.name = "async_slice_idle", .mode = S_IRUGO | S_IWUSR },
> - .show = elv_async_slice_idle_show,
> - .store = elv_async_slice_idle_store,
> -};
> -
> -static struct queue_sysfs_entry queue_slice_sync_entry = {
> - .attr = {.name = "slice_sync", .mode = S_IRUGO | S_IWUSR },
> - .show = elv_slice_sync_show,
> - .store = elv_slice_sync_store,
> -};
> -
> -static struct queue_sysfs_entry queue_slice_async_entry = {
> - .attr = {.name = "slice_async", .mode = S_IRUGO | S_IWUSR },
> - .show = elv_slice_async_show,
> - .store = elv_slice_async_store,
> -};
> -
> -static struct queue_sysfs_entry queue_fairness_entry = {
> - .attr = {.name = "fairness", .mode = S_IRUGO | S_IWUSR },
> - .show = elv_fairness_show,
> - .store = elv_fairness_store,
> -};
> -#endif
> -
> static struct attribute *default_attrs[] = {
> &queue_requests_entry.attr,
> #ifdef CONFIG_GROUP_IOSCHED
> @@ -353,13 +321,6 @@ static struct attribute *default_attrs[]
> &queue_nomerges_entry.attr,
> &queue_rq_affinity_entry.attr,
> &queue_iostats_entry.attr,
> -#ifdef CONFIG_ELV_FAIR_QUEUING
> - &queue_slice_idle_entry.attr,
> - &queue_async_slice_idle_entry.attr,
> - &queue_slice_sync_entry.attr,
> - &queue_slice_async_entry.attr,
> - &queue_fairness_entry.attr,
> -#endif
> NULL,
> };
>
> Index: linux18/block/cfq-iosched.c
> ===================================================================
> --- linux18.orig/block/cfq-iosched.c 2009-06-09 10:34:55.000000000 -0400
> +++ linux18/block/cfq-iosched.c 2009-06-09 13:25:42.000000000 -0400
> @@ -2095,6 +2095,11 @@ static struct elv_fs_entry cfq_attrs[] =
> CFQ_ATTR(back_seek_max),
> CFQ_ATTR(back_seek_penalty),
> CFQ_ATTR(slice_async_rq),
> + ELV_ATTR(fairness),
> + ELV_ATTR(slice_idle),
> + ELV_ATTR(async_slice_idle),
> + ELV_ATTR(slice_sync),
> + ELV_ATTR(slice_async),
> __ATTR_NULL
> };
>
> Index: linux18/block/as-iosched.c
> ===================================================================
> --- linux18.orig/block/as-iosched.c 2009-06-09 10:34:58.000000000 -0400
> +++ linux18/block/as-iosched.c 2009-06-09 13:27:38.000000000 -0400
> @@ -1766,6 +1766,11 @@ static struct elv_fs_entry as_attrs[] =
> AS_ATTR(antic_expire),
> AS_ATTR(read_batch_expire),
> AS_ATTR(write_batch_expire),
> +#ifdef CONFIG_IOSCHED_AS_HIER
> + ELV_ATTR(fairness),
> + ELV_ATTR(slice_idle),
> + ELV_ATTR(slice_sync),
> +#endif
> __ATTR_NULL
> };
>
> Index: linux18/block/deadline-iosched.c
> ===================================================================
> --- linux18.orig/block/deadline-iosched.c 2009-06-09 10:34:55.000000000 -0400
> +++ linux18/block/deadline-iosched.c 2009-06-09 13:28:51.000000000 -0400
> @@ -460,6 +460,11 @@ static struct elv_fs_entry deadline_attr
> DD_ATTR(writes_starved),
> DD_ATTR(front_merges),
> DD_ATTR(fifo_batch),
> +#ifdef CONFIG_IOSCHED_DEADLINE_HIER
> + ELV_ATTR(fairness),
> + ELV_ATTR(slice_idle),
> + ELV_ATTR(slice_sync),
> +#endif
> __ATTR_NULL
> };
>
> Index: linux18/block/noop-iosched.c
> ===================================================================
> --- linux18.orig/block/noop-iosched.c 2009-06-09 10:34:52.000000000 -0400
> +++ linux18/block/noop-iosched.c 2009-06-09 13:31:48.000000000 -0400
> @@ -82,6 +82,15 @@ static void noop_free_noop_queue(struct
> kfree(nq);
> }
>
> +#ifdef CONFIG_IOSCHED_NOOP_HIER
> +static struct elv_fs_entry noop_attrs[] = {
> + ELV_ATTR(fairness),
> + ELV_ATTR(slice_idle),
> + ELV_ATTR(slice_sync),
> + __ATTR_NULL
> +};
> +#endif
> +
> static struct elevator_type elevator_noop = {
> .ops = {
> .elevator_merge_req_fn = noop_merged_requests,
> @@ -94,6 +103,7 @@ static struct elevator_type elevator_noo
> },
> #ifdef CONFIG_IOSCHED_NOOP_HIER
> .elevator_features = ELV_IOSCHED_NEED_FQ | ELV_IOSCHED_SINGLE_IOQ,
> + .elevator_attrs = noop_attrs,
> #endif
> .elevator_name = "noop",
> .elevator_owner = THIS_MODULE,
>
>
>

--
Regards
Gui Jianfeng

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/