Re: [PATCH] Re: Possible FS race condition between iterate_dir and d_alloc_parallel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Sep 14, 2019 at 5:51 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> d_subdirs/d_child become an hlist_head/hlist_node list; no cursors
> in there at any time.

Hmm. I like this.

I wonder if we could do that change independently first, and actually
shrink the dentry (or, more likely, just make the inline name longer).

I don't think that dcache_readdir() is actually stopping us from doing
that right now. Yes, we do that

    list_add_tail(&cursor->d_child, &parent->d_subdirs);

for the end case, but as mentioned, we could replace that with an EOF
flag, couldn't we?

Btw, if you do this change, we should take the opportunity to rename
those confusingly named things. "d_subdirs" are our children - which
aren't necessarily directories, and "d_child" are the nodes in there.

Your change would make that clearer wrt typing (good), but we could
make the naming clearer too (better).

So maybe rename "d_subdirs -> d_children" and "d_child -> d_sibling"
or something at the same time?

Wouldn't that clarify usage, particularly together with the
hlist_head/hlist_node typing?

Most of the users would have to change due to the type change anyway,
so changing the names shouldn't make the diff any worse, and might
make the diff easier to generate (simply because you can *grep* for
the places that need changing).

I wonder why we have that naming to begin with, but it's so old that I
can't remember the reason for that confusing naming. If there ever was
any, outside of "bad thinking".

> d_cursors is a new hlist_head, anchoring the set of cursor that
> point to this sucker.  The list is protected by ->d_lock of
> that dentry.
>
> d_cursors being non-empty contributes 1 to d_count.

My first reaction is that this sound clever, but in a good way (ie not
"too subtle" clever, but simply a clever way to avoid bloating the
dentry).

But I'd like to see the patch (and hear that it works) before I'd say
that it's the right thing to do. Maybe there's some reason why the
above would be problematic.

            Linus



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux