Re: [PATCH v2 06/20] generic_ci_d_compare(): use shortname_storage

[Al Viro <viro@zeniv.linux.org.uk>]

On Thu, Jan 16, 2025 at 10:38:53AM -0500, Gabriel Krisman Bertazi wrote:
> >  	 * If the dentry name is stored in-line, then it may be concurrently
> >  	 * modified by a rename.  If this happens, the VFS will eventually retry
> >  	 * the lookup, so it doesn't matter what ->d_compare() returns.
> >  	 * However, it's unsafe to call utf8_strncasecmp() with an unstable
> >  	 * string.  Therefore, we have to copy the name into a temporary buffer.
> 
> This part of the comment needs updating since there is no more copying.
> 
> > +	 * As above, len is guaranteed to match str, so the shortname case
> > +	 * is exactly when str points to ->d_shortname.
> >  	 */
> > -	if (len <= DNAME_INLINE_LEN - 1) {
> > -		memcpy(strbuf, str, len);
> > -		strbuf[len] = 0;
> > -		str = strbuf;
> > +	if (qstr.name == dentry->d_shortname.string) {
> > +		strbuf = dentry->d_shortname; // NUL is guaranteed to be in there
> > +		qstr.name = strbuf.string;
> >  		/* prevent compiler from optimizing out the temporary buffer */
> >  		barrier();
> 
> If I read the code correctly, I admit I don't understand how this
> guarantees the stability.  Aren't you just assigning qstr.name back the
> same value it had in case of an inlined name through a bounce pointer?
> The previous implementation made sense to me, since the memcpy only
> accessed each character once, and we guaranteed the terminating
> character explicitly, but I'm having a hard time with this version.

This
		strbuf = dentry->d_shortname; // NUL is guaranteed to be in there
copies the entire array.  No bounce pointers of any sort; we copy
the array contents, all 40 bytes of it.  And yes, struct (or union,
in this case) assignment generates better code than manual memcpy()
here.