Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
> On Fri, 19 Jan 2024 at 12:25, Gabriel Krisman Bertazi <krisman@xxxxxxx> 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
