On Sun, Jul 3, 2022 at 7:53 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> FWIW, trying to write a coherent documentation had its usual effect...
> The thing is, we don't really need to fetch the inode that early.
Hmm. I like the patch, but as I was reading through it, I had a question...
In particular, I'd like it even more if each step when the sequence
number is updated also had a comment about what then protects the
previous sequence number up to and over that new sequence point.
For example, in __follow_mount_rcu(), when we jump to a new mount
point, and that sequence has
*seqp = read_seqcount_begin(&dentry->d_seq);
to reset the sequence number to the new path we jumped into.
But I don't actually see what checks the previous sequence number in
that path. We just reset it to the new one.
In contrast, in lookup_fast(), we get the new sequence number from
__d_lookup_rcu(), and then after getting the new one and before
"instantiating" it, we will revalidate the parent sequence number.
So lookup_fast() has that "chain of sequence numbers".
For __follow_mount_rcu it looks like validating the previous sequence
number is left to the caller, which then does try_to_unlazy_next().
So when reading this code, my reaction was that it really would have
been much nicer to have that kind of clear "handoff" of one sequence
number domain to the next that lookup_fast() has.
IOW, I think it would be lovely to clarify the sequence number handoff.
I only quickly scanned your second patch for this, it does seem to at
least collect it all into try_to_unlazy_next().
So maybe you already looked at exactly this, but it would be good to
be quite explicit about the sequence number logic because it's "a bit
opaque" to say the least.
Linus
