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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux