On Sun, 8 Dec 2024 at 19:52, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
Editing the patch down so that you just see the end result (ie all the
"-" lines are gone):
> {
> + unsigned seq;
> +
> + rcu_read_lock();
> +retry:
> + seq = read_seqcount_begin(&dentry->d_seq);
> name->name = dentry->d_name;
> + if (read_seqcount_retry(&dentry->d_seq, seq))
> + goto retry;
Ugh. This early retry is cheap on x86, but on a lot of architectures
it will be a fairly expensive read barrier.
I see why you do it, sinc you want 'name.len' and 'name.name' to be
consistent, but I do hate it.
The name consistency issue is really annoying. Do we really need it
here? Because honestly, what you actually *really* care about here is
whether it's inline or not, and you do that test right afterwards:
> + // ->name and ->len are at least consistent with each other, so if
> + // ->name points to dentry->d_iname, ->len is below DNAME_INLINE_LEN
> + if (likely(name->name.name == dentry->d_iname)) {
> + memcpy(name->inline_name, dentry->d_iname, name->name.len + 1);
and here it would actually be more efficient to just use a
constant-sized memcpy with DNAME_INLINE_LEN, and never care about
'len' at all.
And if the length in name.len isn't right (we'll return it to the
user), the *final* seqcount check will catch it.
And in the other case, all you care about is the ref-count.
End result: I think your early retry is pointless and should just be
avoided. Instead, you should make sure to just read
dentry->d_name.name with a READ_ONCE() (and read the len any way you
want).
Hmm?
Linus
