| Message ID | 20240522074829.1750204-1-yunlong.xing@unisoc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | loop: inherit the ioprio in loop woker thread | expand |
On 5/22/24 00:48, Yunlong Xing wrote: > @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) > set_active_memcg(old_memcg); > css_put(cmd_memcg_css); > } > + > + if (ori_ioprio != cmd_ioprio) > + set_task_ioprio(current, ori_ioprio); > + > failed: > /* complete non-aio request */ > if (!use_aio || ret) { Does adding this call in the hot path have a measurable performance impact? Thanks, Bart.
On 5/22/24 11:38 AM, Bart Van Assche wrote: > On 5/22/24 00:48, Yunlong Xing wrote: >> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) >> set_active_memcg(old_memcg); >> css_put(cmd_memcg_css); >> } >> + >> + if (ori_ioprio != cmd_ioprio) >> + set_task_ioprio(current, ori_ioprio); >> + >> failed: >> /* complete non-aio request */ >> if (!use_aio || ret) { > > Does adding this call in the hot path have a measurable performance impact? It's loop, I would not be concerned with overhead. But it does look pretty bogus to modify the task ioprio from here.
On 5/22/24 10:57, Jens Axboe wrote: > On 5/22/24 11:38 AM, Bart Van Assche wrote: >> On 5/22/24 00:48, Yunlong Xing wrote: >>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) >>> set_active_memcg(old_memcg); >>> css_put(cmd_memcg_css); >>> } >>> + >>> + if (ori_ioprio != cmd_ioprio) >>> + set_task_ioprio(current, ori_ioprio); >>> + >>> failed: >>> /* complete non-aio request */ >>> if (!use_aio || ret) { >> >> Does adding this call in the hot path have a measurable performance impact? > > It's loop, I would not be concerned with overhead. But it does look pretty > bogus to modify the task ioprio from here. Hi Jens, Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? I think that it is easy to pass the I/O priority to the kiocb submitted by lo_rw_aio() without calling set_task_ioprio(). lo_read_simple() and lo_write_simple() however call vfs_iter_read() / vfs_iter_write(). This results in a call of do_iter_readv_writev() and init_sync_kiocb(). The latter function calls get_current_ioprio(). This is probably why the set_task_ioprio() call has been added? Thanks, Bart.
On 5/22/24 12:12 PM, Bart Van Assche wrote: > On 5/22/24 10:57, Jens Axboe wrote: >> On 5/22/24 11:38 AM, Bart Van Assche wrote: >>> On 5/22/24 00:48, Yunlong Xing wrote: >>>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) >>>> set_active_memcg(old_memcg); >>>> css_put(cmd_memcg_css); >>>> } >>>> + >>>> + if (ori_ioprio != cmd_ioprio) >>>> + set_task_ioprio(current, ori_ioprio); >>>> + >>>> failed: >>>> /* complete non-aio request */ >>>> if (!use_aio || ret) { >>> >>> Does adding this call in the hot path have a measurable performance impact? >> >> It's loop, I would not be concerned with overhead. But it does look pretty >> bogus to modify the task ioprio from here. > > Hi Jens, > > Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? > > I think that it is easy to pass the I/O priority to the kiocb submitted by > lo_rw_aio() without calling set_task_ioprio(). Yeah that was my point, it's both the completely wrong way to do it, nor is it a sane way to do it. If the current stack off that doesn't allow priority to be passed, then that work would need to be done first.
On Thu, May 23, 2024 at 2:13 AM Jens Axboe <axboe@kernel.dk> wrote: > > On 5/22/24 12:12 PM, Bart Van Assche wrote: > > On 5/22/24 10:57, Jens Axboe wrote: > >> On 5/22/24 11:38 AM, Bart Van Assche wrote: > >>> On 5/22/24 00:48, Yunlong Xing wrote: > >>>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) > >>>> set_active_memcg(old_memcg); > >>>> css_put(cmd_memcg_css); > >>>> } > >>>> + > >>>> + if (ori_ioprio != cmd_ioprio) > >>>> + set_task_ioprio(current, ori_ioprio); > >>>> + > >>>> failed: > >>>> /* complete non-aio request */ > >>>> if (!use_aio || ret) { > >>> > >>> Does adding this call in the hot path have a measurable performance impact? > >> > >> It's loop, I would not be concerned with overhead. But it does look pretty > >> bogus to modify the task ioprio from here. > > > > Hi Jens, > > > > Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? > > > > I think that it is easy to pass the I/O priority to the kiocb submitted by > > lo_rw_aio() without calling set_task_ioprio(). > > Yeah that was my point, it's both the completely wrong way to do it, nor > is it a sane way to do it. If the current stack off that doesn't allow > priority to be passed, then that work would need to be done first. > > -- > Jens Axboe > Hi jens and bart， Please let me explain the problem I'm having. There are many system service processes in the Android system that access loop devices. The tasks of these system services themselves have high IO priorities. In the I/O stress tests such as filling disks, some disk devices like emmc will cause serious slowdowns. So now the disk speed is very slow, and there are still many requests that need to be processed. I/O requests from system service taskes with high I/O priority should be prioritized over filling disk task with low I/O priority. The purpose is that the I/O requests of the system service process are processed as quickly as possible, without causing some stability problems due to being in the ioschdule state for a long time. However, these system service taskes access the loop device. If the priority cannot be passed, the I/O requests submitted to the real disk device through the loop kwoker cannot be prioritized over the I/O requests of the filling disk task. Then the complete process of system process accessing the loop device will take a very long time. So the purpose of what I do is to let the I/O priority of the task accessing the loop device pass to the kworker process that submit i/o to the real disk device. Do you think the io priority of the task accessing the loop device should not be passed to the kwoker task? Do you have any suggestions for my above-mentioned questions?
Bart Van Assche <bvanassche@acm.org> 于2024年5月23日周四 02:12写道： > > On 5/22/24 10:57, Jens Axboe wrote: > > On 5/22/24 11:38 AM, Bart Van Assche wrote: > >> On 5/22/24 00:48, Yunlong Xing wrote: > >>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) > >>> set_active_memcg(old_memcg); > >>> css_put(cmd_memcg_css); > >>> } > >>> + > >>> + if (ori_ioprio != cmd_ioprio) > >>> + set_task_ioprio(current, ori_ioprio); > >>> + > >>> failed: > >>> /* complete non-aio request */ > >>> if (!use_aio || ret) { > >> > >> Does adding this call in the hot path have a measurable performance impact? > > > > It's loop, I would not be concerned with overhead. But it does look pretty > > bogus to modify the task ioprio from here. > > Hi Jens, > > Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? > > I think that it is easy to pass the I/O priority to the kiocb submitted by > lo_rw_aio() without calling set_task_ioprio(). > > lo_read_simple() and lo_write_simple() however call vfs_iter_read() / > vfs_iter_write(). This results in a call of do_iter_readv_writev() and > init_sync_kiocb(). The latter function calls get_current_ioprio(). This is > probably why the set_task_ioprio() call has been added? Yeah that's why I call set_task_ioprio. I want to the loop kwoker task，submit I/O to the real disk device，can pass the iopriority of the loop device request， both lo_rw_aio() and lo_read_simple()/lo_write_simple(). Thanks, yunlong. > > Thanks, > > Bart. > >
On 5/23/24 12:04 AM, yunlong xing wrote: > Bart Van Assche <bvanassche@acm.org> ?2024?5?23??? 02:12??? >> >> On 5/22/24 10:57, Jens Axboe wrote: >>> On 5/22/24 11:38 AM, Bart Van Assche wrote: >>>> On 5/22/24 00:48, Yunlong Xing wrote: >>>>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) >>>>> set_active_memcg(old_memcg); >>>>> css_put(cmd_memcg_css); >>>>> } >>>>> + >>>>> + if (ori_ioprio != cmd_ioprio) >>>>> + set_task_ioprio(current, ori_ioprio); >>>>> + >>>>> failed: >>>>> /* complete non-aio request */ >>>>> if (!use_aio || ret) { >>>> >>>> Does adding this call in the hot path have a measurable performance impact? >>> >>> It's loop, I would not be concerned with overhead. But it does look pretty >>> bogus to modify the task ioprio from here. >> >> Hi Jens, >> >> Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? >> >> I think that it is easy to pass the I/O priority to the kiocb submitted by >> lo_rw_aio() without calling set_task_ioprio(). >> >> lo_read_simple() and lo_write_simple() however call vfs_iter_read() / >> vfs_iter_write(). This results in a call of do_iter_readv_writev() and >> init_sync_kiocb(). The latter function calls get_current_ioprio(). This is >> probably why the set_task_ioprio() call has been added? > > Yeah that's why I call set_task_ioprio. I want to the loop kwoker > task?submit I/O to the real disk device?can pass the iopriority of the > loop device request? both lo_rw_aio() and > lo_read_simple()/lo_write_simple(). And that's a totally backwards and suboptimal way to do it. The task priority is only used as a last resort lower down, if the IO itself hasn't been appropriately marked. Like I said, it's back to the drawing board on this patch, there's no way it's acceptable in its current form.
Jens Axboe <axboe@kernel.dk> 于2024年5月23日周四 21:04写道： > > On 5/23/24 12:04 AM, yunlong xing wrote: > > Bart Van Assche <bvanassche@acm.org> ?2024?5?23??? 02:12??? > >> > >> On 5/22/24 10:57, Jens Axboe wrote: > >>> On 5/22/24 11:38 AM, Bart Van Assche wrote: > >>>> On 5/22/24 00:48, Yunlong Xing wrote: > >>>>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) > >>>>> set_active_memcg(old_memcg); > >>>>> css_put(cmd_memcg_css); > >>>>> } > >>>>> + > >>>>> + if (ori_ioprio != cmd_ioprio) > >>>>> + set_task_ioprio(current, ori_ioprio); > >>>>> + > >>>>> failed: > >>>>> /* complete non-aio request */ > >>>>> if (!use_aio || ret) { > >>>> > >>>> Does adding this call in the hot path have a measurable performance impact? > >>> > >>> It's loop, I would not be concerned with overhead. But it does look pretty > >>> bogus to modify the task ioprio from here. > >> > >> Hi Jens, > >> > >> Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? > >> > >> I think that it is easy to pass the I/O priority to the kiocb submitted by > >> lo_rw_aio() without calling set_task_ioprio(). > >> > >> lo_read_simple() and lo_write_simple() however call vfs_iter_read() / > >> vfs_iter_write(). This results in a call of do_iter_readv_writev() and > >> init_sync_kiocb(). The latter function calls get_current_ioprio(). This is > >> probably why the set_task_ioprio() call has been added? > > > > Yeah that's why I call set_task_ioprio. I want to the loop kwoker > > task?submit I/O to the real disk device?can pass the iopriority of the > > loop device request? both lo_rw_aio() and > > lo_read_simple()/lo_write_simple(). > > And that's a totally backwards and suboptimal way to do it. The task > priority is only used as a last resort lower down, if the IO itself > hasn't been appropriately marked. > > Like I said, it's back to the drawing board on this patch, there's no > way it's acceptable in its current form. > > -- > Jens Axboe > Thanks for your advice. So, you can't accept pass the ioprio by set_task_ioprio？ If only the method of lo_rw_aio() counld you accept? I don't want to submit this part of the modifications separately. I just want to know, this is ok to you or not？ @@ -442,7 +442,6 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, cmd->iocb.ki_filp = file; cmd->iocb.ki_complete = lo_rw_aio_complete; cmd->iocb.ki_flags = IOCB_DIRECT; - cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); if (rw == ITER_SOURCE) ret = call_write_iter(file, &cmd->iocb, &iter); @@ -1856,6 +1855,9 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx, break; } + /* get request's ioprio */ + cmd->iocb.ki_ioprio = rq->ioprio; + /* always use the first bio's css */ cmd->blkcg_css = NULL; cmd->memcg_css = NULL;
On 5/23/24 8:52 AM, yunlong xing wrote: > Jens Axboe <axboe@kernel.dk> ?2024?5?23??? 21:04??? >> >> On 5/23/24 12:04 AM, yunlong xing wrote: >>> Bart Van Assche <bvanassche@acm.org> ?2024?5?23??? 02:12??? >>>> >>>> On 5/22/24 10:57, Jens Axboe wrote: >>>>> On 5/22/24 11:38 AM, Bart Van Assche wrote: >>>>>> On 5/22/24 00:48, Yunlong Xing wrote: >>>>>>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) >>>>>>> set_active_memcg(old_memcg); >>>>>>> css_put(cmd_memcg_css); >>>>>>> } >>>>>>> + >>>>>>> + if (ori_ioprio != cmd_ioprio) >>>>>>> + set_task_ioprio(current, ori_ioprio); >>>>>>> + >>>>>>> failed: >>>>>>> /* complete non-aio request */ >>>>>>> if (!use_aio || ret) { >>>>>> >>>>>> Does adding this call in the hot path have a measurable performance impact? >>>>> >>>>> It's loop, I would not be concerned with overhead. But it does look pretty >>>>> bogus to modify the task ioprio from here. >>>> >>>> Hi Jens, >>>> >>>> Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? >>>> >>>> I think that it is easy to pass the I/O priority to the kiocb submitted by >>>> lo_rw_aio() without calling set_task_ioprio(). >>>> >>>> lo_read_simple() and lo_write_simple() however call vfs_iter_read() / >>>> vfs_iter_write(). This results in a call of do_iter_readv_writev() and >>>> init_sync_kiocb(). The latter function calls get_current_ioprio(). This is >>>> probably why the set_task_ioprio() call has been added? >>> >>> Yeah that's why I call set_task_ioprio. I want to the loop kwoker >>> task?submit I/O to the real disk device?can pass the iopriority of the >>> loop device request? both lo_rw_aio() and >>> lo_read_simple()/lo_write_simple(). >> >> And that's a totally backwards and suboptimal way to do it. The task >> priority is only used as a last resort lower down, if the IO itself >> hasn't been appropriately marked. >> >> Like I said, it's back to the drawing board on this patch, there's no >> way it's acceptable in its current form. >> >> -- >> Jens Axboe >> > Thanks for your advice. So, you can't accept pass the ioprio by > set_task_ioprio? Not sure how many times I'd have to state that, no. > If only the method of lo_rw_aio() counld you accept? I don't want to > submit this part of the modifications separately. I just want to know, > this is ok to you or not? Inheriting the kiocb ioprio from the request is the right approach, so yeah that part is fine.
Jens Axboe <axboe@kernel.dk> 于2024年5月23日周四 22:58写道： > > On 5/23/24 8:52 AM, yunlong xing wrote: > > Jens Axboe <axboe@kernel.dk> ?2024?5?23??? 21:04??? > >> > >> On 5/23/24 12:04 AM, yunlong xing wrote: > >>> Bart Van Assche <bvanassche@acm.org> ?2024?5?23??? 02:12??? > >>>> > >>>> On 5/22/24 10:57, Jens Axboe wrote: > >>>>> On 5/22/24 11:38 AM, Bart Van Assche wrote: > >>>>>> On 5/22/24 00:48, Yunlong Xing wrote: > >>>>>>> @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) > >>>>>>> set_active_memcg(old_memcg); > >>>>>>> css_put(cmd_memcg_css); > >>>>>>> } > >>>>>>> + > >>>>>>> + if (ori_ioprio != cmd_ioprio) > >>>>>>> + set_task_ioprio(current, ori_ioprio); > >>>>>>> + > >>>>>>> failed: > >>>>>>> /* complete non-aio request */ > >>>>>>> if (!use_aio || ret) { > >>>>>> > >>>>>> Does adding this call in the hot path have a measurable performance impact? > >>>>> > >>>>> It's loop, I would not be concerned with overhead. But it does look pretty > >>>>> bogus to modify the task ioprio from here. > >>>> > >>>> Hi Jens, > >>>> > >>>> Maybe Yunlong uses that call to pass the I/O priority to the I/O submitter? > >>>> > >>>> I think that it is easy to pass the I/O priority to the kiocb submitted by > >>>> lo_rw_aio() without calling set_task_ioprio(). > >>>> > >>>> lo_read_simple() and lo_write_simple() however call vfs_iter_read() / > >>>> vfs_iter_write(). This results in a call of do_iter_readv_writev() and > >>>> init_sync_kiocb(). The latter function calls get_current_ioprio(). This is > >>>> probably why the set_task_ioprio() call has been added? > >>> > >>> Yeah that's why I call set_task_ioprio. I want to the loop kwoker > >>> task?submit I/O to the real disk device?can pass the iopriority of the > >>> loop device request? both lo_rw_aio() and > >>> lo_read_simple()/lo_write_simple(). > >> > >> And that's a totally backwards and suboptimal way to do it. The task > >> priority is only used as a last resort lower down, if the IO itself > >> hasn't been appropriately marked. > >> > >> Like I said, it's back to the drawing board on this patch, there's no > >> way it's acceptable in its current form. > >> > >> -- > >> Jens Axboe > >> > > Thanks for your advice. So, you can't accept pass the ioprio by > > set_task_ioprio? > > Not sure how many times I'd have to state that, no. Of course, I understand what you mean. I would like to ask if you only disagree with this part. Sorry for missing a word "just". Back to the patch, I couldn't find a better way to pass the ioprio in the lo_read/write_simple(). Do you have some suggestions or ideas？ > > > If only the method of lo_rw_aio() counld you accept? I don't want to > > submit this part of the modifications separately. I just want to know, > > this is ok to you or not? > > Inheriting the kiocb ioprio from the request is the right approach, so > yeah that part is fine. > > -- > Jens Axboe >
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 28a95fd366fe..404ac113c71b 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -442,7 +442,6 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, cmd->iocb.ki_filp = file; cmd->iocb.ki_complete = lo_rw_aio_complete; cmd->iocb.ki_flags = IOCB_DIRECT; - cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); if (rw == ITER_SOURCE) ret = call_write_iter(file, &cmd->iocb, &iter); @@ -1856,6 +1855,9 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx, break; } + /* get request's ioprio */ + cmd->iocb.ki_ioprio = rq->ioprio; + /* always use the first bio's css */ cmd->blkcg_css = NULL; cmd->memcg_css = NULL; @@ -1886,12 +1888,18 @@ static void loop_handle_cmd(struct loop_cmd *cmd) int ret = 0; struct mem_cgroup *old_memcg = NULL; const bool use_aio = cmd->use_aio; + int ori_ioprio = 0; + int cmd_ioprio = cmd->iocb.ki_ioprio; if (write && (lo->lo_flags & LO_FLAGS_READ_ONLY)) { ret = -EIO; goto failed; } + ori_ioprio = get_current_ioprio(); + if (ori_ioprio != cmd_ioprio) + set_task_ioprio(current, cmd_ioprio); + if (cmd_blkcg_css) kthread_associate_blkcg(cmd_blkcg_css); if (cmd_memcg_css) @@ -1913,6 +1921,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd) set_active_memcg(old_memcg); css_put(cmd_memcg_css); } + + if (ori_ioprio != cmd_ioprio) + set_task_ioprio(current, ori_ioprio); + failed: /* complete non-aio request */ if (!use_aio || ret) {
The loop worker thread only inherit the blkcg of original request, but does not inherit it's ioprio. So, when a task with the high ioprio but in root blkcg accesses the loop device, the worker thread handle this cmd with a normal ioprio. This results in the request of high ioprio task doesn't be prioritized. Signed-off-by: Yunlong Xing <yunlong.xing@unisoc.com> --- drivers/block/loop.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)