On Thu, Dec 1, 2016 at 12:03 PM, Alexey Lyashkov
<alexey.lyashkov@xxxxxxxxx> wrote:
...
>
>
>> 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.
>
You have not convinced me. The way I see it you created a variant of __d_drop()
that is specific for using in rename for temporary unhash before rehash.
But it's up to me to decide if the practice on replacing all call site
is valid or not.
>> 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
I don't follow. Why wouldn't we want to set the UNHASHED flag on d_delete()?
>>
>> 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?
For my use case I am trying to figure out if a file handle is stale
in a stronger sense then what exportfs_decode_fh() already provides.
exportfs_decode_fh() returns ESTALE is the inode in question is no longer
in the file system or no longer listed in the parent directory, but if
the handle
represents an open and unlinked file/dir, it is not considered stale.
I am using the helper to filter out the open and unlinked file/dir case.
>
> 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...
>
Sorry. I did not properly define the terms "lookable" and "connected zombie".
"lookable" := there exists a path for which lookup(path) will return this dentry
"connected zombie" := dentry is "connected" with parent chain to root, but
one or more of the dentries in the chain have been unlinked, but they still
have non zero refcount and therefore not yet killed.
>>
>> /* 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;
>> }
>>
>>
--
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
