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

[Ming Lei <ming.lei@redhat.com>]

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