[PATCH] loop: inherit the ioprio in loop woker thread

[PATCH] loop: inherit the ioprio in loop woker thread

[Yunlong Xing]

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(-)

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) {
-- 
2.25.1

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[Bart Van Assche]

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.

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[Jens Axboe]

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.

-- 
Jens Axboe

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[Bart Van Assche]

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.

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[Jens Axboe]

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

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[yunlong xing]

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?

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[yunlong xing]

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.
>
>

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[Jens Axboe]

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

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[yunlong xing]

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;

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[Jens Axboe]

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

Re: [PATCH] loop: inherit the ioprio in loop woker thread

[yunlong xing]

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
>