lkml.org 
[lkml]   [2017]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias.
    1/ The testing of DCACHE_DISCONNECTED is wrong.
    see upstream commit da093a9b76ef ("dcache: d_splice_alias should
    ignore DCACHE_DISCONNECTED")

    As this is a notoriously difficult piece of code to get right,
    it makes sense to use d_splice_alias() directly and no try to
    create a local version of it.

    2/ ll_find_alias() currently:
    locks and alias
    checks that it is the one we want
    unlock it
    locks it again
    gets a reference
    unlocks it

    This isn't safe. Anything could happen to the dentry while we
    don't hold a reference. We need to dget the reference while
    still holding the lock.

    3/ The d_move() in ll_splice_alias() is pointless. We have
    already checked the hash, name, and parent are the same, and
    these are the only fields that d_move() will change.

    4/ The call to d_add() is outside of any locking. This makes it
    possible for two identical dentries to be added to the same
    inode, which would cause confusion.

    Prior to 4.7, i_mutex would have provided exclusion, but since
    the VFS supports parallel lookups, only a shared lock is held
    on i_mutex.

    Because ll_d_init() creates a dentry in a state where
    ll_dcompare will no recognize it, the VFS provides no guarantee
    that we won't have two concurrent calls to ll_lookup_dn() for
    the same parent/name.


    So: rename ll_find_alias() to ll_find_invalid_alias() and have it
    just focus on finding an invalid alias.

    For directories, we can just use d__splice_alias() directly.
    There must only be one alias for a directory, and
    ll_splice_alias() will find it where it is "invalid" or not.

    For non-directories, we call ll_find_invalid_alias(), and either
    use the result or call d_add(). We need a lock to protect from
    races with other threads calling ll_find_invalid_alias() and
    d_add() at the same time. lli_lock seems suitable for this
    purpose.

    Signed-off-by: NeilBrown <neilb@suse.com>
    ---
    drivers/staging/lustre/lustre/llite/namei.c | 69 +++++++++++++--------------
    1 file changed, 34 insertions(+), 35 deletions(-)

    diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
    index 293a3180ec70..6204c3e70d45 100644
    --- a/drivers/staging/lustre/lustre/llite/namei.c
    +++ b/drivers/staging/lustre/lustre/llite/namei.c
    @@ -378,75 +378,74 @@ void ll_i2gids(__u32 *suppgids, struct inode *i1, struct inode *i2)
    }

    /*
    - * try to reuse three types of dentry:
    - * 1. unhashed alias, this one is unhashed by d_invalidate (but it may be valid
    - * by concurrent .revalidate).
    - * 2. INVALID alias (common case for no valid ldlm lock held, but this flag may
    - * be cleared by others calling d_lustre_revalidate).
    - * 3. DISCONNECTED alias.
    + * Try to find an "invalid" alias. i.e. one that was unhashed by
    + * d_invalidate(), or that was instantiated with no valid ldlm lock.
    + * These can be rehased by d_lustre_revalidate(), which could race
    + * with this code.
    */
    -static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
    +static struct dentry *ll_find_invalid_alias(struct inode *inode,
    + struct dentry *dentry)
    {
    - struct dentry *alias, *discon_alias, *invalid_alias;
    + struct dentry *alias, *invalid_alias = NULL;

    if (hlist_empty(&inode->i_dentry))
    return NULL;

    - discon_alias = NULL;
    - invalid_alias = NULL;
    -
    spin_lock(&inode->i_lock);
    hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
    LASSERT(alias != dentry);

    spin_lock(&alias->d_lock);
    - if ((alias->d_flags & DCACHE_DISCONNECTED) &&
    - S_ISDIR(inode->i_mode))
    - /* LASSERT(last_discon == NULL); LU-405, bz 20055 */
    - discon_alias = alias;
    - else if (alias->d_parent == dentry->d_parent &&
    - alias->d_name.hash == dentry->d_name.hash &&
    - alias->d_name.len == dentry->d_name.len &&
    - memcmp(alias->d_name.name, dentry->d_name.name,
    - dentry->d_name.len) == 0)
    + if (alias->d_parent == dentry->d_parent &&
    + alias->d_name.hash == dentry->d_name.hash &&
    + alias->d_name.len == dentry->d_name.len &&
    + memcmp(alias->d_name.name, dentry->d_name.name,
    + dentry->d_name.len) == 0) {
    + dget_dlock(alias);
    invalid_alias = alias;
    + }
    spin_unlock(&alias->d_lock);

    if (invalid_alias)
    break;
    }
    - alias = invalid_alias ?: discon_alias ?: NULL;
    - if (alias) {
    - spin_lock(&alias->d_lock);
    - dget_dlock(alias);
    - spin_unlock(&alias->d_lock);
    - }
    spin_unlock(&inode->i_lock);

    - return alias;
    + return invalid_alias;
    }

    /*
    - * Similar to d_splice_alias(), but lustre treats invalid alias
    - * similar to DCACHE_DISCONNECTED, and tries to use it anyway.
    + * Similar to d_splice_alias(), but also look for an "invalid" alias,
    + * specific to lustre, and use that if found.
    */
    struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de)
    {
    - if (inode) {
    - struct dentry *new = ll_find_alias(inode, de);
    + if (inode && !S_ISDIR(inode->i_mode)) {
    + struct ll_inode_info *lli = ll_i2info(inode);
    + struct dentry *new;
    +
    + /* We need lli_lock here as another thread could
    + * be running this code, and i_lock cannot protect us.
    + */
    + spin_lock(&lli->lli_lock);
    + new = ll_find_invalid_alias(inode, de);
    + if (!new)
    + d_add(de, inode);
    + spin_lock(&lli->lli_lock);

    if (new) {
    - d_move(new, de);
    iput(inode);
    CDEBUG(D_DENTRY,
    "Reuse dentry %p inode %p refc %d flags %#x\n",
    new, d_inode(new), d_count(new), new->d_flags);
    return new;
    }
    + return de;
    }
    - d_add(de, inode);
    - CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
    - de, d_inode(de), d_count(de), de->d_flags);
    + de = d_splice_alias(inode, de);
    + if (!IS_ERR(de))
    + CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
    + de, d_inode(de), d_count(de), de->d_flags);
    return de;
    }


    \
     
     \ /
      Last update: 2017-07-19 01:30    [W:3.131 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site