[PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Damien Le Moal]

Commit 734e1a860312 ("block: Prevent deadlocks when switching
elevators") introduced the function elv_iosched_load_module() to allow
loading an elevator module outside of elv_iosched_store() with the
target device queue not frozen, to avoid deadlocks. However, the "none"
scheduler does not have a module and as a result,
elv_iosched_load_module() always returns an error when trying to switch
to this valid scheduler.

Fix this by checking that the requested scheduler is "none" and doing
nothing in that case.

Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Fixes: 734e1a860312 ("block: Prevent deadlocks when switching elevators")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/elevator.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/elevator.c b/block/elevator.c
index c355b55d0107..d0ee9c0aaed2 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -714,6 +714,8 @@ int elv_iosched_load_module(struct gendisk *disk, const char *buf,
 		return -EOPNOTSUPP;
 
 	strscpy(elevator_name, buf, sizeof(elevator_name));
+	if (!strncmp(elevator_name, "none", 4))
+		return 0;
 
 	return request_module("%s-iosched", strstrip(elevator_name));
 }
-- 
2.46.0

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Christoph Hellwig]

On Tue, Sep 17, 2024 at 02:32:58PM +0900, Damien Le Moal wrote:
> Commit 734e1a860312 ("block: Prevent deadlocks when switching
> elevators") introduced the function elv_iosched_load_module() to allow
> loading an elevator module outside of elv_iosched_store() with the
> target device queue not frozen, to avoid deadlocks. However, the "none"
> scheduler does not have a module and as a result,
> elv_iosched_load_module() always returns an error when trying to switch
> to this valid scheduler.
> 
> Fix this by checking that the requested scheduler is "none" and doing
> nothing in that case.

The old code before this commit simply ignored the request_module,
just as most callers of it do.  I think that's the right approach
here as well.

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Shinichiro Kawasaki]

On Sep 17, 2024 / 14:32, Damien Le Moal wrote:
> Commit 734e1a860312 ("block: Prevent deadlocks when switching
> elevators") introduced the function elv_iosched_load_module() to allow
> loading an elevator module outside of elv_iosched_store() with the
> target device queue not frozen, to avoid deadlocks. However, the "none"
> scheduler does not have a module and as a result,
> elv_iosched_load_module() always returns an error when trying to switch
> to this valid scheduler.

This unexpected behavior (setting none scheduler has no effect) was found
by the blktests test case scsi/008 failure. This test case invokes the fio
command with the --ioscheduler=none option.

> 
> Fix this by checking that the requested scheduler is "none" and doing
> nothing in that case.

I confirmed this fix patch avoids the unexpected behavior, and makes scsi/008
pass.

> 
> Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Fixes: 734e1a860312 ("block: Prevent deadlocks when switching elevators")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

I also run whole blktests applying this patch on top of the v6.11 kernel, and
observed no regression. Looks good from testing point of view.

Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Ming Lei]

On Tue, Sep 17, 2024 at 1:53 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Sep 17, 2024 at 02:32:58PM +0900, Damien Le Moal wrote:
> > Commit 734e1a860312 ("block: Prevent deadlocks when switching
> > elevators") introduced the function elv_iosched_load_module() to allow
> > loading an elevator module outside of elv_iosched_store() with the
> > target device queue not frozen, to avoid deadlocks. However, the "none"
> > scheduler does not have a module and as a result,
> > elv_iosched_load_module() always returns an error when trying to switch
> > to this valid scheduler.
> >
> > Fix this by checking that the requested scheduler is "none" and doing
> > nothing in that case.
>
> The old code before this commit simply ignored the request_module,
> just as most callers of it do.  I think that's the right approach
> here as well.

freeze queue is actually easy to cause deadlock, and old code is to not
do it everywhere.

Probably it may be more reliable to replace 'load_module' with 'no_freeze',
and not to freeze queue in case that 'no_freeze' is set in queue_attr_store().

queue_wb_lat_store() need no_freeze too since there is GFP_KERNEL
allocation involved.

Thanks,

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Damien Le Moal]

On 2024/09/17 14:33, Ming Lei wrote:
> On Tue, Sep 17, 2024 at 1:53 PM Christoph Hellwig <hch@lst.de> wrote:
>>
>> On Tue, Sep 17, 2024 at 02:32:58PM +0900, Damien Le Moal wrote:
>>> Commit 734e1a860312 ("block: Prevent deadlocks when switching
>>> elevators") introduced the function elv_iosched_load_module() to allow
>>> loading an elevator module outside of elv_iosched_store() with the
>>> target device queue not frozen, to avoid deadlocks. However, the "none"
>>> scheduler does not have a module and as a result,
>>> elv_iosched_load_module() always returns an error when trying to switch
>>> to this valid scheduler.
>>>
>>> Fix this by checking that the requested scheduler is "none" and doing
>>> nothing in that case.
>>
>> The old code before this commit simply ignored the request_module,
>> just as most callers of it do.  I think that's the right approach
>> here as well.
> 
> freeze queue is actually easy to cause deadlock, and old code is to not
> do it everywhere.
> 
> Probably it may be more reliable to replace 'load_module' with 'no_freeze',
> and not to freeze queue in case that 'no_freeze' is set in queue_attr_store().

load_module or whatever the name you prefer, should NOT imply that freezing the
queue is not necessary. Switching the IO scheduler is really one such case.
Switching the scheduler really needs to be done with the queue frozen, but the
scheduler module loading needs to be done with the queue live.

> queue_wb_lat_store() need no_freeze too since there is GFP_KERNEL
> allocation involved.

No, because again the attribute may need to have the queue frozen to correctly
be changed. To avoid hangs, what is needed is to force a GFP_NOIO context before
calling the attribute ->store() operation. Doing so, any memory allocation that
the attribute change may need will not cause re-entry into a frozen queue (which
would result in a hang).

This is easy to do with memalloc_noio_save()/memalloc_noio_restore().

> 
> Thanks,
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Ming Lei]

On Tue, Sep 17, 2024 at 02:48:06PM +0200, Damien Le Moal wrote:
> On 2024/09/17 14:33, Ming Lei wrote:
> > On Tue, Sep 17, 2024 at 1:53 PM Christoph Hellwig <hch@lst.de> wrote:
> >>
> >> On Tue, Sep 17, 2024 at 02:32:58PM +0900, Damien Le Moal wrote:
> >>> Commit 734e1a860312 ("block: Prevent deadlocks when switching
> >>> elevators") introduced the function elv_iosched_load_module() to allow
> >>> loading an elevator module outside of elv_iosched_store() with the
> >>> target device queue not frozen, to avoid deadlocks. However, the "none"
> >>> scheduler does not have a module and as a result,
> >>> elv_iosched_load_module() always returns an error when trying to switch
> >>> to this valid scheduler.
> >>>
> >>> Fix this by checking that the requested scheduler is "none" and doing
> >>> nothing in that case.
> >>
> >> The old code before this commit simply ignored the request_module,
> >> just as most callers of it do.  I think that's the right approach
> >> here as well.
> > 
> > freeze queue is actually easy to cause deadlock, and old code is to not
> > do it everywhere.
> > 
> > Probably it may be more reliable to replace 'load_module' with 'no_freeze',
> > and not to freeze queue in case that 'no_freeze' is set in queue_attr_store().
> 
> load_module or whatever the name you prefer, should NOT imply that freezing the
> queue is not necessary. Switching the IO scheduler is really one such case.
> Switching the scheduler really needs to be done with the queue frozen, but the
> scheduler module loading needs to be done with the queue live.

Here 'no_freeze' means that automatic 'freeze queue' isn't needed, or
it can be named as 'no_auto_freeze'.

Again, 'load_module' is one bad name from interface viewpoint, which is just
needed by 'scheduler' only.

> 
> > queue_wb_lat_store() need no_freeze too since there is GFP_KERNEL
> > allocation involved.
> 
> No, because again the attribute may need to have the queue frozen to correctly
> be changed. To avoid hangs, what is needed is to force a GFP_NOIO context before
> calling the attribute ->store() operation. Doing so, any memory allocation that
> the attribute change may need will not cause re-entry into a frozen queue (which
> would result in a hang).
> 
> This is easy to do with memalloc_noio_save()/memalloc_noio_restore().

But why do we need that? Just for paper over the problem caused by the
unnecessary freeze queue?


Thanks,
Ming

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Christoph Hellwig]

On Tue, Sep 17, 2024 at 09:02:36PM +0800, Ming Lei wrote:
> 
> Here 'no_freeze' means that automatic 'freeze queue' isn't needed, or
> it can be named as 'no_auto_freeze'.
> 
> Again, 'load_module' is one bad name from interface viewpoint, which is just
> needed by 'scheduler' only.

If we want to reshuffle this we could have a ->store_unfrozen method
that does all the work.  But as long as the elevator loading is the
only thing that needs do to unfrozen work I'd just keep things as it
and not rock the boat.

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Jens Axboe]

On 9/17/24 7:05 AM, Christoph Hellwig wrote:
> On Tue, Sep 17, 2024 at 09:02:36PM +0800, Ming Lei wrote:
>>
>> Here 'no_freeze' means that automatic 'freeze queue' isn't needed, or
>> it can be named as 'no_auto_freeze'.
>>
>> Again, 'load_module' is one bad name from interface viewpoint, which is just
>> needed by 'scheduler' only.
> 
> If we want to reshuffle this we could have a ->store_unfrozen method
> that does all the work.  But as long as the elevator loading is the
> only thing that needs do to unfrozen work I'd just keep things as it
> and not rock the boat.

Whatever reshuffling people have in mind, that needs to happen AFTER
this bug is sorted out.

-- 
Jens Axboe

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Christoph Hellwig]

On Tue, Sep 17, 2024 at 07:11:22AM -0600, Jens Axboe wrote:
> Whatever reshuffling people have in mind, that needs to happen AFTER
> this bug is sorted out.

Yes.  The fix from Damien will work, but reverting to the old behavior
of ignoring the request_module return value feel much better.  I can
prepare a patch, but I didn't want to steal the credits from Damien.

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Jens Axboe]

On 9/17/24 7:14 AM, Christoph Hellwig wrote:
> On Tue, Sep 17, 2024 at 07:11:22AM -0600, Jens Axboe wrote:
>> Whatever reshuffling people have in mind, that needs to happen AFTER
>> this bug is sorted out.
> 
> Yes.  The fix from Damien will work, but reverting to the old behavior
> of ignoring the request_module return value feel much better.  I can
> prepare a patch, but I didn't want to steal the credits from Damien.

Oh I agree, I think we should just ignore that and that would be the
better patch. My point is just that larger reworking should be done
after the fix, not advocating for a particular solution.

That said, someone did just email me privately because they ran 
into this with 6.11. So we should do a clean simple patch to help
facilitate getting it to -stable sooner rather than later.

-- 
Jens Axboe

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Damien Le Moal]

On 2024/09/17 15:14, Christoph Hellwig wrote:
> On Tue, Sep 17, 2024 at 07:11:22AM -0600, Jens Axboe wrote:
>> Whatever reshuffling people have in mind, that needs to happen AFTER
>> this bug is sorted out.
> 
> Yes.  The fix from Damien will work, but reverting to the old behavior
> of ignoring the request_module return value feel much better.  I can
> prepare a patch, but I didn't want to steal the credits from Damien.
> 

OK. I can send a v2 ignoring the request_module() result, as it was before.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: Fix elv_iosched_local_module handling of "none" scheduler

[Jens Axboe]

On 9/17/24 7:18 AM, Damien Le Moal wrote:
> On 2024/09/17 15:14, Christoph Hellwig wrote:
>> On Tue, Sep 17, 2024 at 07:11:22AM -0600, Jens Axboe wrote:
>>> Whatever reshuffling people have in mind, that needs to happen AFTER
>>> this bug is sorted out.
>>
>> Yes.  The fix from Damien will work, but reverting to the old behavior
>> of ignoring the request_module return value feel much better.  I can
>> prepare a patch, but I didn't want to steal the credits from Damien.
>>
> 
> OK. I can send a v2 ignoring the request_module() result, as it was before.

Sounds good, let's do that.

-- 
Jens Axboe