On Fri, Apr 26, 2019 at 12:25:03PM -0400, Jeff Layton wrote:
> It turns out though that using name_snapshot from ceph is a bit more
> tricky. In some cases, we have to call ceph_mdsc_build_path to build up
> a full path string. We can't easily populate a name_snapshot from there
> because struct external_name is only defined in fs/dcache.c.
Explain, please. For ceph_mdsc_build_path() you don't need name
snapshots at all and existing code is, AFAICS, just fine, except
for pointless pr_err() there.
I _probably_ would take allocation out of the loop (e.g. make it
__getname(), called unconditionally) and turned it into the
d_path.c-style read_seqbegin_or_lock()/need_seqretry()/done_seqretry()
loop, so that the first pass would go under rcu_read_lock(), while
the second (if needed) would just hold rename_lock exclusive (without
bumping the refcount). But that's a matter of (theoretical) livelock
avoidance, not the locking correctness for ->d_name accesses.
Oh, and
*base = ceph_ino(d_inode(temp));
*plen = len;
probably belongs in critical section - _that_ might be a correctness
issue, since temp is not held by anything once you are out of there.
> I could add some routines to do this, but it feels a lot like I'm
> abusing internal dcache interfaces. I'll keep thinking about it though.
>
> While we're on the subject though:
>
> struct external_name {
> union {
> atomic_t count;
> struct rcu_head head;
> } u;
> unsigned char name[];
> };
>
> Is it really ok to union the count and rcu_head there?
>
> I haven't trawled through all of the code yet, but what prevents someone
> from trying to access the count inside an RCU critical section, after
> call_rcu has been called on it?
The fact that no lockless accesses to ->count are ever done?
