Messages in this thread |  | | From | Linus Torvalds <> | | Date | Thu, 4 Jan 2024 10:46:04 -0800 | | Subject | Re: [for-next][PATCH 2/3] eventfs: Stop using dcache_readdir() for getdents() |
| |
On Thu, 4 Jan 2024 at 08:46, Steven Rostedt <rostedt@goodmis.org> 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
|  |