commit 7b5a3522 (loop: Limit the number of requests in the bio list) limit
the request number in loop queue to not over 128. Make the number tunable
from sysfs can improve performance.
The following test is done on a machine with 512M memory. The backend of
/dev/loop1 is a nfs file.
[root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
128
[root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
5000+0 records in
5000+0 records out
5242880000 bytes (5.2 GB) copied, 501.572 s, 10.5 MB/s
[root@bijx mnt]#
[root@bijx mnt]# echo 1024 > /sys/block/loop0/queue/nr_requests
[root@bijx mnt]# cat /sys/block/loop0/queue/nr_requests
1024
[root@bijx mnt]# dd if=/dev/zero of=/dev/loop0 bs=1M count=5000
5000+0 records in
5000+0 records out
5242880000 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s
Signed-off-by: Junxiao Bi <[email protected]>
---
block/blk-sysfs.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 7500f87..193ad8a 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -52,9 +52,6 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
unsigned long nr;
int ret;
- if (!q->request_fn)
- return -EINVAL;
-
ret = queue_var_store(&nr, page, count);
if (ret < 0)
return ret;
@@ -66,6 +63,11 @@ queue_requests_store(struct request_queue *q, const char *page, size_t count)
q->nr_requests = nr;
blk_queue_congestion_threshold(q);
+ if (!q->request_fn) {
+ spin_unlock_irq(q->queue_lock);
+ return ret;
+ }
+
/* congestion isn't cgroup aware and follows root blkcg for now */
rl = &q->root_rl;
--
1.7.9.5
Hi,
having had a look at current mainline sources,
frankly I've (well, initially...) got trouble understanding
what this patch is doing.
It's replacing an aggressive error-type bail-out (-EINVAL) for NULL request_fn
with an inoccuous-looking "return ret;", yet that ret content currently
*implicitly* is a >= 0 value (resulting from processing by earlier code
which may or may not get incomprehensibly rewritten in future).
I don't understand the reasons for this huge change in return value handling
(since it's now not assigning a specific return value
for this modified bail-out case).
OK, well... you could say that since all this function ever was
interested in is the result value of queue_var_store()
(except for error bail-out cases), doing an interim "return ret;"
(which is exactly what the function tail is also doing)
is exactly right.
But still simple textual appearance of the resulting patch hunks
seems strangely asymmetric
which may easily be a canary for structurally wrong layering of this function.
Not to mention the now required extra spin_unlock_irq()
in interim return handler...
Well, after further analysis I would come to the conclusion
that in general queue_requests_store() does a LOT more than it should -
since blk-sysfs.c's only (expected!) purpose is
to do parameterization of request_queue behaviour as gathered
from sysfs attribute space,
all that function should ever be concerned with is parsing that sysfs value
and then calling a blk helper for configuration of that very attribute value
which would *internally* do all the strange internal queue magic
that is currently being updated *open-coded*
at this supposedly *sysfs*-specific place. Ugh.
Main question here: what would one do if one decided to rip out sysfs
and use something entirely different for parameterization?
Yeah indeed - thought so...
So yeah, I'd definitely say that that function is lacking some cleanup
which would possibly then even lead (or: would have led ;)
to a much more nicely symmetric textual appearance
of the patch hunk of the small but quite likely useful change
that you currently intend to have here.
Thanks,
Andreas Mohr
On 2014-06-09 01:29, Andreas Mohr wrote:
> Hi,
>
> having had a look at current mainline sources,
> frankly I've (well, initially...) got trouble understanding
> what this patch is doing.
>
> It's replacing an aggressive error-type bail-out (-EINVAL) for NULL request_fn
> with an inoccuous-looking "return ret;", yet that ret content currently
> *implicitly* is a >= 0 value (resulting from processing by earlier code
> which may or may not get incomprehensibly rewritten in future).
> I don't understand the reasons for this huge change in return value handling
> (since it's now not assigning a specific return value
> for this modified bail-out case).
>
> OK, well... you could say that since all this function ever was
> interested in is the result value of queue_var_store()
> (except for error bail-out cases), doing an interim "return ret;"
> (which is exactly what the function tail is also doing)
> is exactly right.
>
> But still simple textual appearance of the resulting patch hunks
> seems strangely asymmetric
> which may easily be a canary for structurally wrong layering of this function.
> Not to mention the now required extra spin_unlock_irq()
> in interim return handler...
>
>
> Well, after further analysis I would come to the conclusion
> that in general queue_requests_store() does a LOT more than it should -
> since blk-sysfs.c's only (expected!) purpose is
> to do parameterization of request_queue behaviour as gathered
> from sysfs attribute space,
> all that function should ever be concerned with is parsing that sysfs value
> and then calling a blk helper for configuration of that very attribute value
> which would *internally* do all the strange internal queue magic
> that is currently being updated *open-coded*
> at this supposedly *sysfs*-specific place. Ugh.
> Main question here: what would one do if one decided to rip out sysfs
> and use something entirely different for parameterization?
> Yeah indeed - thought so...
>
>
> So yeah, I'd definitely say that that function is lacking some cleanup
> which would possibly then even lead (or: would have led ;)
> to a much more nicely symmetric textual appearance
> of the patch hunk of the small but quite likely useful change
> that you currently intend to have here.
If you are done ranting, look at the current tree where it has been
split out. There was no reason to have it split before, since the sysfs
entry point was the only place where we updated nr_requests. If that
code has been duplicated, there would have been a justified reason for
writing two pages about it.
--
Jens Axboe
On 06/09/2014 11:53 PM, Jens Axboe wrote:
> On 2014-06-09 01:29, Andreas Mohr wrote:
>> Hi,
>>
>> having had a look at current mainline sources,
>> frankly I've (well, initially...) got trouble understanding
>> what this patch is doing.
>>
>> It's replacing an aggressive error-type bail-out (-EINVAL) for NULL
>> request_fn
>> with an inoccuous-looking "return ret;", yet that ret content currently
>> *implicitly* is a >= 0 value (resulting from processing by earlier code
>> which may or may not get incomprehensibly rewritten in future).
>> I don't understand the reasons for this huge change in return value
>> handling
>> (since it's now not assigning a specific return value
>> for this modified bail-out case).
>>
>> OK, well... you could say that since all this function ever was
>> interested in is the result value of queue_var_store()
>> (except for error bail-out cases), doing an interim "return ret;"
>> (which is exactly what the function tail is also doing)
>> is exactly right.
>>
>> But still simple textual appearance of the resulting patch hunks
>> seems strangely asymmetric
>> which may easily be a canary for structurally wrong layering of this
>> function.
>> Not to mention the now required extra spin_unlock_irq()
>> in interim return handler...
>>
>>
>> Well, after further analysis I would come to the conclusion
>> that in general queue_requests_store() does a LOT more than it should -
>> since blk-sysfs.c's only (expected!) purpose is
>> to do parameterization of request_queue behaviour as gathered
>> from sysfs attribute space,
>> all that function should ever be concerned with is parsing that sysfs
>> value
>> and then calling a blk helper for configuration of that very
>> attribute value
>> which would *internally* do all the strange internal queue magic
>> that is currently being updated *open-coded*
>> at this supposedly *sysfs*-specific place. Ugh.
>> Main question here: what would one do if one decided to rip out sysfs
>> and use something entirely different for parameterization?
>> Yeah indeed - thought so...
>>
>>
>> So yeah, I'd definitely say that that function is lacking some cleanup
>> which would possibly then even lead (or: would have led ;)
>> to a much more nicely symmetric textual appearance
>> of the patch hunk of the small but quite likely useful change
>> that you currently intend to have here.
>
> If you are done ranting, look at the current tree where it has been
> split out. There was no reason to have it split before, since the
> sysfs entry point was the only place where we updated nr_requests. If
> that code has been duplicated, there would have been a justified
> reason for writing two pages about it.
Yes, agree, this is the only place updating nr_requests, we can split
it as a separated function if it needs updating at some other places in
future.
On 2014-06-09 19:35, Junxiao Bi wrote:
> On 06/09/2014 11:53 PM, Jens Axboe wrote:
>> On 2014-06-09 01:29, Andreas Mohr wrote:
>>> Hi,
>>>
>>> having had a look at current mainline sources,
>>> frankly I've (well, initially...) got trouble understanding
>>> what this patch is doing.
>>>
>>> It's replacing an aggressive error-type bail-out (-EINVAL) for NULL
>>> request_fn
>>> with an inoccuous-looking "return ret;", yet that ret content currently
>>> *implicitly* is a >= 0 value (resulting from processing by earlier code
>>> which may or may not get incomprehensibly rewritten in future).
>>> I don't understand the reasons for this huge change in return value
>>> handling
>>> (since it's now not assigning a specific return value
>>> for this modified bail-out case).
>>>
>>> OK, well... you could say that since all this function ever was
>>> interested in is the result value of queue_var_store()
>>> (except for error bail-out cases), doing an interim "return ret;"
>>> (which is exactly what the function tail is also doing)
>>> is exactly right.
>>>
>>> But still simple textual appearance of the resulting patch hunks
>>> seems strangely asymmetric
>>> which may easily be a canary for structurally wrong layering of this
>>> function.
>>> Not to mention the now required extra spin_unlock_irq()
>>> in interim return handler...
>>>
>>>
>>> Well, after further analysis I would come to the conclusion
>>> that in general queue_requests_store() does a LOT more than it should -
>>> since blk-sysfs.c's only (expected!) purpose is
>>> to do parameterization of request_queue behaviour as gathered
>>> from sysfs attribute space,
>>> all that function should ever be concerned with is parsing that sysfs
>>> value
>>> and then calling a blk helper for configuration of that very
>>> attribute value
>>> which would *internally* do all the strange internal queue magic
>>> that is currently being updated *open-coded*
>>> at this supposedly *sysfs*-specific place. Ugh.
>>> Main question here: what would one do if one decided to rip out sysfs
>>> and use something entirely different for parameterization?
>>> Yeah indeed - thought so...
>>>
>>>
>>> So yeah, I'd definitely say that that function is lacking some cleanup
>>> which would possibly then even lead (or: would have led ;)
>>> to a much more nicely symmetric textual appearance
>>> of the patch hunk of the small but quite likely useful change
>>> that you currently intend to have here.
>>
>> If you are done ranting, look at the current tree where it has been
>> split out. There was no reason to have it split before, since the
>> sysfs entry point was the only place where we updated nr_requests. If
>> that code has been duplicated, there would have been a justified
>> reason for writing two pages about it.
> Yes, agree, this is the only place updating nr_requests, we can split
> it as a separated function if it needs updating at some other places in
> future.
Please look at the current tree... It is already split up.
--
Jens Axboe