Amir,
comments inline.
> 1 dec 2016 г., в 11:15, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Thu, Dec 1, 2016 at 6:18 AM, Alexey Lyashkov
> <alexey.lyashkov@xxxxxxxxx> wrote:
>> rehash process protected with d_seq and d_lock locks, but VFS have access to the
>> d_hashed field without any locks sometimes. It produce errors with get cwd
>> operations or fsnotify may report an unlink event sometimes. d_seq lock isn’t
>> used to protect due possibility to sleep with holding locks, and d_lock is too
>> hot to check a simple state, lets return a DCACHE_DENTRY_UNHASHED flag to
>> ability to check a unhashed state without locks held.
>>
>>
>
> If I am counting correctly, you modified all call sites of __d_drop() except
> for those in d_move() and d_delete(). Right?
correct.
> I suggest that you keep all those call sites in tact and only modify the
> 2 call sites with changed behavior.
> Also, best give the new helper a name that is a bit more meaningful to
> understand the changed behavior (maybe __d_unhash(), do_d_drop()).
i may agree with rename a __d_drop to something else, but export symbol rename will help to find any places need to be changed.
v3 patch was created from kbuild robot messages, which hides without it, a specially on platform other than x86 used for development.
> Then you may add a comment to explain why you need to use the
> modified behavior when you use it. Why do you need it in d_delete()?
d_delete remove a dentry from a tree completely, but d_move just change a hash point. So if we want to know a file unlinked
>
> Incidentally, I just wrote a helper I needed to find out if a dentry I got from
> exportfs_decode_fh() is "lookable" as opposed to a "connected zombie".
> So your patch is certainly usable for my use case.
NFS a area with big problems in dcache. it use d_drop to avoid an assertion in d_rehash, but it open a window when dentry out of tree, so several operations may fail.
I think we need something better in that case like an take a d_lock for whole or allow to rehash dentry without unlinking from tree (i think it better).
> Can anyone say if my helper is correct and whether is usable for anyone
> else?
may you explain how it should be called?
as i see comments
/*
* Inform d_walk() and shrink_dentry_list() that we are no longer
* attached to the dentry tree
*/
dentry->d_flags |= DCACHE_DENTRY_KILLED;
it’s better than d_unhashed...
>
> /* Check if p1 is connected with a chain of hashed dentries to p2 */
> static bool ovl_is_lookable(struct dentry *p1, struct dentry *p2)
> {
> struct dentry *p;
>
> for (p = p2; !IS_ROOT(p); p = p->d_parent) {
> /*
> * FIXME: there is a small window inside d_move()
> * where moved entry is unhashed and rehashed and
> * this can fail.
> */
> if (d_unhashed(p))
> return false;
> if (p->d_parent == p1)
> return true;
> }
> return false;
> }
>
>
> [...]
>
>> @@ -326,11 +327,12 @@ extern struct dentry *dget_parent(struct dentry *dentry);
>> * @dentry: entry to check
>> *
>> * Returns true if the dentry passed is not currently hashed.
>> + * hlist_bl_unhashed can't be used as race with d_move().
>> */
>>
>
> remove this newline while at it.
>
>> static inline int d_unhashed(const struct dentry *dentry)
>> {
>> - return hlist_bl_unhashed(&dentry->d_hash);
>> + return dentry->d_flags & DCACHE_DENTRY_UNHASHED;
>> }
>>
>
>
> Thanks,
> Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
