Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 17 Jan 2020 01:43:59 -0800 (PST)
From:   David Rientjes <rientjes@google.com>
To:     Vlastimil Babka <vbabka@suse.cz>
cc:     Andrew Morton <akpm@linux-foundation.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [patch] mm, thp: fix defrag setting if newline is not used
In-Reply-To: <025511aa-4721-2edb-d658-78d6368a9101@suse.cz>
Message-ID: <alpine.DEB.2.21.2001170136280.20618@chino.kir.corp.google.com>
References: <alpine.DEB.2.21.2001141757490.108121@chino.kir.corp.google.com> <20200116191609.3972fd5301cf364a27381923@linux-foundation.org> <025511aa-4721-2edb-d658-78d6368a9101@suse.cz>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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.