| Message ID | 20240917053258.128827-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix elv_iosched_local_module handling of "none" scheduler | expand |
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.
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>
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,
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, >
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
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.
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.
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.
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.
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.
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.
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)); }
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(+)