linux-kernel - Re: [PATCH] block/mq-deadline: Fix WARN when set async

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 2 Apr 2024 16:20:22 -0700
From: Bart Van Assche <bvanassche@....org>
To: Zhiguo Niu <niuzhiguo84@...il.com>
Cc: Zhiguo Niu <zhiguo.niu@...soc.com>, axboe@...nel.dk,
 linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
 ke.wang@...soc.com, hongyu.jin@...soc.com,
 Damien Le Moal <dlemoal@...nel.org>
Subject: Re: [PATCH] block/mq-deadline: Fix WARN when set async_depth by sysfs

On 4/1/24 10:44 PM, Zhiguo Niu wrote:
> On Tue, Apr 2, 2024 at 5:23 AM Bart Van Assche <bvanassche@....org> wrote:
>> Oops, I misread your patch. After having taken another look, my
>> conclusions are as follows:
>> * sbitmap_queue_min_shallow_depth() is called. This causes
>>     sbq->wake_batch to be modified but I don't think that it is a proper
>>     fix for dd_limit_depth().
> yes, it will affect sbq->wake_batch,  But judging from the following code:
> [ ... ]

If we want to allow small values of dd->async_depth, min_shallow_depth
must be 1. The BFQ I/O scheduler also follows this approach.

>> * dd_limit_depth() still assigns a number in the range 1..nr_requests to
>>     data->shallow_depth while a number in the range 1..(1<<bt->sb.shift)
>>     should be assigned.
> yes, In order to avoid the performance regression problem that Harshit
> Mogalapalli reported, this patch will not directly modify
> dd->async_depth,
> but user can modify dd->async_depth from sysfs according to user's
> request. which will modify data->shallow_depth after user modify it by
> sysfs.

It seems like there is no other option than keeping the current default
depth limit for async requests ...

> My personal opinion is to keep the current dd->aync_depth unchanged to
> avoid causing performance regression,
> but it should  allow users to set it by sysfs, and the WARN mentioned
> best to be solved.
> and just only change this part?
>   -       sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd->async_depth);
>   +       sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, 1);
> thanks!

The above change will suppress the kernel warning. I think the other
changes from patch 2/2 are also necessary. Otherwise the unit of
"async_depth" will depend on sbitmap word shift parameter. I don't think
that users should have to worry about which shift value has been chosen
by the sbitmap implementation.

Thanks,

Bart.