Re: [PATCH v3 1/2] dcache: Expose dentry_string_cmp outside of dcache

[Gabriel Krisman Bertazi <krisman@suse.de>]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Fri, 19 Jan 2024 at 12:25, Gabriel Krisman Bertazi <krisman@suse.de> wrote:
>>
>> In preparation to call these from libfs, expose dentry_string_cmp in the
>> header file.
>
> Let's not make these header files bigger and more complex.
>
> Particularly not for generic_ci_d_compare() to inline it, which makes
> no sense: generic_ci_d_compare() is so heavy with a big stack frame
> anyway, that the inlining of this would seem to be just in the noise.
>
> And when I look closer, it turns out that __d_lookup_rcu_op_compare()
> that does this all also does the proper sequence number magic to make
> the name pointer and the length consistent.

Ok. I see that we retry the read before calling d_compare here:

    /* we want a consistent (name,len) pair */
    if (read_seqcount_retry(&dentry->d_seq, seq)) {
	cpu_relax();
	goto seqretry;
    }

for RCU and for d_same_name we are holding the d_lock.

So, I guess I was right for the wrong reason in the earlier
versions. which doesn't really do me any good.

> So I don't think we need the careful name compare after all, because
> the caller has fixed the consistency issue.
>
> I do also wonder if we should just move the "identical always compares
> equal" case into __d_lookup_rcu_op_compare(), and not have
> ->d_compare() have to worry about it at all?

I considered that, and I can do it as a follow up.  I'd like to audit
d_compare to ensure we can change its semantics by only calling it if
the exact-match d_compare failed.  Is there a filesystem where that
really matters?  If so, we can add a new ->d_weak_compare hook.
Considering the ones I looked already will do fine with this change,
it's likely possible.

Are you ok with the earlier v2 of this patchset as-is, and I'll send a
new series proposing this change?

Thank you,

-- 
Gabriel Krisman Bertazi