In blk-sysfs.c, queue_var_store uses unsigned long to store data,
but queue_var_show uses unsigned int to show data. This causes,
# echo 70000000000 > /sys/block/<dev>/queue/read_ahead_kb
# cat /sys/block/<dev>/queue/read_ahead_kb => get wrong value
Signed-off-by: Xiaotian Feng <[email protected]>
---
block/blk-sysfs.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index b1cd040..e177eb3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -16,9 +16,9 @@ struct queue_sysfs_entry {
};
static ssize_t
-queue_var_show(unsigned int var, char *page)
+queue_var_show(unsigned long var, char *page)
{
- return sprintf(page, "%d\n", var);
+ return sprintf(page, "lu\n", var);
}
static ssize_t
@@ -77,7 +77,8 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
static ssize_t queue_ra_show(struct request_queue *q, char *page)
{
- int ra_kb = q->backing_dev_info.ra_pages << (PAGE_CACHE_SHIFT - 10);
+ unsigned long ra_kb = q->backing_dev_info.ra_pages <<
+ (PAGE_CACHE_SHIFT - 10);
return queue_var_show(ra_kb, (page));
}
@@ -95,7 +96,7 @@ queue_ra_store(struct request_queue *q, const char *page, size_t count)
static ssize_t queue_max_sectors_show(struct request_queue *q, char *page)
{
- int max_sectors_kb = queue_max_sectors(q) >> 1;
+ unsigned long max_sectors_kb = queue_max_sectors(q) >> 1;
return queue_var_show(max_sectors_kb, (page));
}
@@ -140,7 +141,7 @@ queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page)
{
- int max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1;
+ unsigned long max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1;
return queue_var_show(max_hw_sectors_kb, (page));
}
@@ -189,7 +190,7 @@ static ssize_t queue_nomerges_store(struct request_queue *q, const char *page,
static ssize_t queue_rq_affinity_show(struct request_queue *q, char *page)
{
- unsigned int set = test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags);
+ unsigned long set = test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags);
return queue_var_show(set != 0, page);
}
--
1.6.2.5
On Tue, Jul 14, 2009 at 7:56 PM, Xiaotian Feng<[email protected]> wrote:
> In blk-sysfs.c, queue_var_store uses unsigned long to store data,
> but queue_var_show uses unsigned int to show data. This causes,
>
> # echo 70000000000 > /sys/block/<dev>/queue/read_ahead_kb
> # cat /sys/block/<dev>/queue/read_ahead_kb => get wrong value
>
> Signed-off-by: Xiaotian Feng <[email protected]>
> ---
> block/blk-sysfs.c | 13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index b1cd040..e177eb3 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -16,9 +16,9 @@ struct queue_sysfs_entry {
> };
>
> static ssize_t
> -queue_var_show(unsigned int var, char *page)
> +queue_var_show(unsigned long var, char *page)
> {
> - return sprintf(page, "%d\n", var);
> + return sprintf(page, "lu\n", var);
%lu
> }
>
> static ssize_t
> @@ -77,7 +77,8 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
>
> static ssize_t queue_ra_show(struct request_queue *q, char *page)
> {
> - int ra_kb = q->backing_dev_info.ra_pages << (PAGE_CACHE_SHIFT - 10);
> + unsigned long ra_kb = q->backing_dev_info.ra_pages <<
> + (PAGE_CACHE_SHIFT - 10);
It looks like that your email client replaces tab's with spaces.
On Tue, Jul 14, 2009 at 07:56:58PM +0800, Xiaotian Feng wrote:
> In blk-sysfs.c, queue_var_store uses unsigned long to store data,
> but queue_var_show uses unsigned int to show data. This causes,
>
> # echo 70000000000 > /sys/block/<dev>/queue/read_ahead_kb
> # cat /sys/block/<dev>/queue/read_ahead_kb => get wrong value
>
> Signed-off-by: Xiaotian Feng <[email protected]>
> ---
> block/blk-sysfs.c | 13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index b1cd040..e177eb3 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -16,9 +16,9 @@ struct queue_sysfs_entry {
> };
>
> static ssize_t
> -queue_var_show(unsigned int var, char *page)
> +queue_var_show(unsigned long var, char *page)
> {
> - return sprintf(page, "%d\n", var);
> + return sprintf(page, "lu\n", var);
%lu, I guess?
In blk-sysfs.c, queue_var_store uses unsigned long to store data,
but queue_var_show uses unsigned int to show data. This causes,
# echo 70000000000 > /sys/block/<dev>/queue/read_ahead_kb
# cat /sys/block/<dev>/queue/read_ahead_kb => get wrong value
Signed-off-by: Xiaotian Feng <[email protected]>
---
block/blk-sysfs.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index b1cd040..e177eb3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -16,9 +16,9 @@ struct queue_sysfs_entry {
};
static ssize_t
-queue_var_show(unsigned int var, char *page)
+queue_var_show(unsigned long var, char *page)
{
- return sprintf(page, "%d\n", var);
+ return sprintf(page, "%lu\n", var);
}
static ssize_t
@@ -77,7 +77,8 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
static ssize_t queue_ra_show(struct request_queue *q, char *page)
{
- int ra_kb = q->backing_dev_info.ra_pages << (PAGE_CACHE_SHIFT - 10);
+ unsigned long ra_kb = q->backing_dev_info.ra_pages <<
+ (PAGE_CACHE_SHIFT - 10);
return queue_var_show(ra_kb, (page));
}
@@ -95,7 +96,7 @@ queue_ra_store(struct request_queue *q, const char *page, size_t count)
static ssize_t queue_max_sectors_show(struct request_queue *q, char *page)
{
- int max_sectors_kb = queue_max_sectors(q) >> 1;
+ unsigned long max_sectors_kb = queue_max_sectors(q) >> 1;
return queue_var_show(max_sectors_kb, (page));
}
@@ -140,7 +141,7 @@ queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page)
{
- int max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1;
+ unsigned long max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1;
return queue_var_show(max_hw_sectors_kb, (page));
}
@@ -189,7 +190,7 @@ static ssize_t queue_nomerges_store(struct request_queue *q, const char *page,
static ssize_t queue_rq_affinity_show(struct request_queue *q, char *page)
{
- unsigned int set = test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags);
+ unsigned long set = test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags);
return queue_var_show(set != 0, page);
}
--
1.6.2.5
On 07/14/2009 09:53 PM, Américo Wang wrote:
> On Tue, Jul 14, 2009 at 7:56 PM, Xiaotian Feng<[email protected]> wrote:
>> In blk-sysfs.c, queue_var_store uses unsigned long to store data,
>> but queue_var_show uses unsigned int to show data. This causes,
>>
>> # echo 70000000000 > /sys/block/<dev>/queue/read_ahead_kb
>> # cat /sys/block/<dev>/queue/read_ahead_kb => get wrong value
>>
>> Signed-off-by: Xiaotian Feng <[email protected]>
>> ---
>> block/blk-sysfs.c | 13 +++++++------
>> 1 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index b1cd040..e177eb3 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -16,9 +16,9 @@ struct queue_sysfs_entry {
>> };
>>
>> static ssize_t
>> -queue_var_show(unsigned int var, char *page)
>> +queue_var_show(unsigned long var, char *page)
>> {
>> - return sprintf(page, "%d\n", var);
>> + return sprintf(page, "lu\n", var);
>
>
> %lu
>
My mistake, correct in V2. Thank you.
>> }
>>
>> static ssize_t
>> @@ -77,7 +77,8 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
>>
>> static ssize_t queue_ra_show(struct request_queue *q, char *page)
>> {
>> - int ra_kb = q->backing_dev_info.ra_pages<< (PAGE_CACHE_SHIFT - 10);
>> + unsigned long ra_kb = q->backing_dev_info.ra_pages<<
>> + (PAGE_CACHE_SHIFT - 10);
>
>
> It looks like that your email client replaces tab's with spaces.
I use git send-email, and in my thunderbird, they're all tabs...
Hello, Xiaotian.
Xiaotian Feng wrote:
> static ssize_t queue_ra_show(struct request_queue *q, char *page)
> {
> - int ra_kb = q->backing_dev_info.ra_pages << (PAGE_CACHE_SHIFT - 10);
> + unsigned long ra_kb = q->backing_dev_info.ra_pages <<
> + (PAGE_CACHE_SHIFT - 10);
>
> return queue_var_show(ra_kb, (page));
> }
Nice.
> @@ -95,7 +96,7 @@ queue_ra_store(struct request_queue *q, const char *page, size_t count)
>
> static ssize_t queue_max_sectors_show(struct request_queue *q, char *page)
> {
> - int max_sectors_kb = queue_max_sectors(q) >> 1;
> + unsigned long max_sectors_kb = queue_max_sectors(q) >> 1;
>
> return queue_var_show(max_sectors_kb, (page));
> }
> @@ -140,7 +141,7 @@ queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
>
> static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page)
> {
> - int max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1;
> + unsigned long max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1;
>
> return queue_var_show(max_hw_sectors_kb, (page));
> }
The above two aren't necessary but well why not.
> @@ -189,7 +190,7 @@ static ssize_t queue_nomerges_store(struct request_queue *q, const char *page,
>
> static ssize_t queue_rq_affinity_show(struct request_queue *q, char *page)
> {
> - unsigned int set = test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags);
> + unsigned long set = test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags);
>
> return queue_var_show(set != 0, page);
> }
Wouldn't it be better to make it "bool set = " and then remove the
"!= 0"?
Thanks.
--
tejun
Hello, Tejun:
Thank you very much for the review, I've attached v3 patch follow
your advices, please help me review.
Another thing on block sysfs is that we can echo some invalid value
(e.g 123abc), should sysfs return -EINVAL for those?
Regards
On 07/17/2009 02:48 PM, Tejun Heo wrote:
> Hello, Xiaotian.
>
> Xiaotian Feng wrote:
>> static ssize_t queue_ra_show(struct request_queue *q, char *page)
>> {
>> - int ra_kb = q->backing_dev_info.ra_pages<< (PAGE_CACHE_SHIFT - 10);
>> + unsigned long ra_kb = q->backing_dev_info.ra_pages<<
>> + (PAGE_CACHE_SHIFT - 10);
>>
>> return queue_var_show(ra_kb, (page));
>> }
>
> Nice.
>
>> @@ -95,7 +96,7 @@ queue_ra_store(struct request_queue *q, const char *page, size_t count)
>>
>> static ssize_t queue_max_sectors_show(struct request_queue *q, char *page)
>> {
>> - int max_sectors_kb = queue_max_sectors(q)>> 1;
>> + unsigned long max_sectors_kb = queue_max_sectors(q)>> 1;
>>
>> return queue_var_show(max_sectors_kb, (page));
>> }
>> @@ -140,7 +141,7 @@ queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
>>
>> static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page)
>> {
>> - int max_hw_sectors_kb = queue_max_hw_sectors(q)>> 1;
>> + unsigned long max_hw_sectors_kb = queue_max_hw_sectors(q)>> 1;
>>
>> return queue_var_show(max_hw_sectors_kb, (page));
>> }
>
> The above two aren't necessary but well why not.
>
>> @@ -189,7 +190,7 @@ static ssize_t queue_nomerges_store(struct request_queue *q, const char *page,
>>
>> static ssize_t queue_rq_affinity_show(struct request_queue *q, char *page)
>> {
>> - unsigned int set = test_bit(QUEUE_FLAG_SAME_COMP,&q->queue_flags);
>> + unsigned long set = test_bit(QUEUE_FLAG_SAME_COMP,&q->queue_flags);
>>
>> return queue_var_show(set != 0, page);
>> }
>
> Wouldn't it be better to make it "bool set = " and then remove the
> "!= 0"?
>
> Thanks.
>
Hello,
Danny Feng wrote:
> Thank you very much for the review, I've attached v3 patch follow
> your advices, please help me review.
Thanks. I queued the patch on the following temp block tree.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git tj-block-for-linus
> Another thing on block sysfs is that we can echo some invalid value
> (e.g 123abc), should sysfs return -EINVAL for those?
sysfs nodes are only accessible from root and it's customary to not
invest too much into safe guarding the input. So, leaving as it is
should be fine, I think.
--
tejun