Re: [PATCH] block: make nr_requests tunable for loop
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: make nr_requests tunable for loop
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: make nr_requests tunable for loop
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: make nr_requests tunable for loop
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] block: make nr_requests tunable for loop
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 524288 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 524288 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s Signed-off-by: Junxiao Bi --- 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(, 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 = >root_rl; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] block: make nr_requests tunable for loop
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 524288 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 524288 bytes (5.2 GB) copied, 464.481 s, 11.3 MB/s Signed-off-by: Junxiao Bi junxiao...@oracle.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: make nr_requests tunable for loop
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: make nr_requests tunable for loop
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: make nr_requests tunable for loop
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: make nr_requests tunable for loop
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/