Re: [patch] mm, thp: fix defrag setting if newline is not used

[Vlastimil Babka <vbabka@suse.cz>]

On 1/17/20 10:43 AM, David Rientjes wrote:
> On Fri, 17 Jan 2020, Vlastimil Babka wrote:
> 
>>>> If thp defrag setting "defer" is used and a newline is *not* used when
>>>> writing to the sysfs file, this is interpreted as the "defer+madvise"
>>>> option.
>>>>
>>>> This is because we do prefix matching and if five characters are written
>>>> without a newline, the current code ends up comparing to the first five
>>>> bytes of the "defer+madvise" option and using that instead.
>>>>
>>>> Find the length of what the user is writing and use that to guide our
>>>> decision on which string comparison to do.
>>>
>>> Gee, why is this code so complicated?  Can't we just do
>>>
>>> 	if (sysfs_streq(buf, "always")) {
>>> 		...
>>> 	} else if sysfs_streq(buf, "defer+madvise")) {
>>> 		...
>>> 	}
>>> 	...
>>
>> Yeah, if we knew this existed :)
>>
>> We would lose the prefix matching but hopefully nobody will complain.
>>
> 
> I tested Vlastimil's patch and it works as intended so I was about to 
> modify the changelog and send his patch and ask for a sign-off line 
> because I think I agree the *partial* prefix matching has ~0.1% chance of 
> breaking userspace and that 0.1% chance outweighs my desire to make the 
> code consistent for all options.

If prefix matching worked with "echo alw > /sys..." then I would expect
some script out there relies on it, but since it only works with "echo
-n alw > /..." then perhaps there's no such script :)

> But if userspace were broken by this, then at least it was already broken 
> for "defer" depending on newline vs no newline.  (What we do know is that 
> nobody has used "defer" for the past couple years without a newline :).
> 
> If nobody objects, I'll test and send Andrew's version with the changelog 
> because I think we all agree the risk of breakage here is very minimal and 
> actually fixes the case for defer.  

Agreed.