| Message ID | 20240917133231.134806-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] block: Fix elv_iosched_local_module handling of "none" scheduler | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, 17 Sep 2024 22:32:31 +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. > > [...] Applied, thanks! [1/1] block: Fix elv_iosched_local_module handling of "none" scheduler commit: e3accac1a976e65491a9b9fba82ce8ddbd3d2389 Best regards,
Hi Damien, Damien Le Moal <dlemoal@kernel.org> writes: > 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. The commit message here is a bit misleading. The problem is not that `request_module` can fail, the problem is that some failure modes cause this function to return a positive integer. This is not caught by callers and it ends up causing all kinds of problems in user space. Perhaps it makes sense to check for a positive return value at the call site of the `load_module` pointer in `queue_attr_store`, so this does not repeat at some point in the future? Or maybe document that `load_module` implementations should not return a positive value unless it actually wants to send this to user space as the result of a write to the `scheduler` sysfs file? Best regards, Andreas
On 10/7/24 19:51, Andreas Hindborg wrote: > Hi Damien, > > Damien Le Moal <dlemoal@kernel.org> writes: > >> 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. > > The commit message here is a bit misleading. The problem is not that > `request_module` can fail, the problem is that some failure modes cause > this function to return a positive integer. This is not caught by > callers and it ends up causing all kinds of problems in user space. > > Perhaps it makes sense to check for a positive return value at the call > site of the `load_module` pointer in `queue_attr_store`, so this does > not repeat at some point in the future? Well, the patch version that went upstream totally ignores the return value of request_module(), as it did before. So I do not think there are any issues anymore... ? Might be good to add a comment about it above that request_module() call in elv_iosched_load_module(). > > Or maybe document that `load_module` implementations should not return a > positive value unless it actually wants to send this to user space as > the result of a write to the `scheduler` sysfs file? Yes, ->load_module() should return 0 on success and negative error code on error. Otherwise, a positive error may be interpreted by user space as a success write to the sysfs attribute. Adding a comment for that will be good too. Care to send a patch ?
On Tue, Oct 08, 2024 at 07:48:10AM +0900, Damien Le Moal wrote: > Yes, ->load_module() should return 0 on success and negative error code on > error. Otherwise, a positive error may be interpreted by user space as a success > write to the sysfs attribute. Adding a comment for that will be good too. > > Care to send a patch ? It should not return anything at all as alreaday said last round. Jens just asked for that to be sent layer, so I guess I'll do it now.
On 10/8/24 13:20, Christoph Hellwig wrote: > On Tue, Oct 08, 2024 at 07:48:10AM +0900, Damien Le Moal wrote: >> Yes, ->load_module() should return 0 on success and negative error code on >> error. Otherwise, a positive error may be interpreted by user space as a success >> write to the sysfs attribute. Adding a comment for that will be good too. >> >> Care to send a patch ? > > It should not return anything at all as alreaday said last round. > Jens just asked for that to be sent layer, so I guess I'll do it now. Ah, yes, that is even better.
diff --git a/block/elevator.c b/block/elevator.c index c355b55d0107..4122026b11f1 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -715,7 +715,9 @@ int elv_iosched_load_module(struct gendisk *disk, const char *buf, strscpy(elevator_name, buf, sizeof(elevator_name)); - return request_module("%s-iosched", strstrip(elevator_name)); + request_module("%s-iosched", strstrip(elevator_name)); + + return 0; } ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
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 ignoring the return value of the request_module() call done by elv_iosched_load_module(). This restores the behavior before commit 734e1a860312, which was to ignore the request_module() result and instead rely on elevator_change() to handle the "none" scheduler 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> --- Changes from v1: - Switch to ignoring the return value of request_module() instead of doing nothing if the scheduler name is "none". block/elevator.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)