On 10/7/24 19:51, Andreas Hindborg wrote: > Hi Damien, > > Damien Le Moal <dlemoal@xxxxxxxxxx> 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 ? -- Damien Le Moal Western Digital Research