Re: [for-next][PATCH 2/3] eventfs: Stop using dcache_readdir() for getdents()

[Linus Torvalds]

On Thu, 4 Jan 2024 at 08:46, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
>         list_for_each_entry_srcu(ei_child, &ei->children, list,
>                                  srcu_read_lock_held(&eventfs_srcu)) {
> +
> +               if (c > 0) {
> +                       c--;
> +                       continue;
>                 }

Thanks for putting that at the top, I really do think it's not just
more efficient, but "more correct" too - ie if some entry that *used*
to exist and was previously counted by 'pos' went away, it's actually
*better* to count it again if we still see it, in order to not skip
subsequent entries that haven't been seen..

And that very fact actually makes me wonder:

>         for (i = 0; i < ei->nr_entries; i++) {
> +               void *cdata = ei->data;
> +
> +               if (c > 0) {
> +                       c--;
> +                       continue;
> +               }

The 'ei->nr_entries' things are in a stable array, so the indexing for
them cannot change (ie even if "is_freed" were to be set the array is
still stable).

So I wonder if - just from a 'pos' iterator stability standpoint - you
should change the tracefs directory iterator to always start with the
non-directory entries in ei->entries[]?

That way, even if concurrent dynamic add/remove events might change
the 'ei->children' list, it could never cause an 'ei->entry[]' to
disappear (or be returned twice).

This is very nitpicky and I doubt it matters, because I doubt the
whole "ls on a tracefs directory while changing it" case matters, but
I thought I'd mention it.

              Linus