On Tue, Mar 08, 2016 at 11:18:09PM +0000, Drokin, Oleg wrote:
> Rename on server cannot get us to see the same directory in two places, or what's
> the scenario?
> In Lustre:
> thread1: lookup a directory on the server.
> get a lock back
> instantiate a dentry (guarded by the lock)
> make a lock revocable (ll_lookup_finish_locks() in ll_lookup_it())
> thread2: rename the directory moving it somewhere else
> server attempts to revoke the lock from thread1
> node that runs thread1 drops the lock and marks all dentries for that
> inode invalid
> server completes rename and releases the lock it holds
> thread3: lookup the directory under new name on the server
> this can only return from server when the rename has completed and the
> dentry from thread1 marked invalid.
thread2 might run on another client; might or might not make any difference,
but in any case server going nuts shouldn't corrupt data structures on client...
> Ok, let me try my hand at that. Hopefully whatever complications are there would
> show themselves right away too.
>
> > would always either inserted inode reference into a new dentry or dropped it.
> > I'm still trying to trace what does iput() in case of error in your current
> > code; I understand the one in do_statahead_enter(), but what does it in
> > ll_lookup_it_finish()?
>
> You mean when ll_d_init() fails in ll_splice_alias()?
> Hm… It appears that we are indeed leaking the inode in that case, thanks.
> I'll try to address that too.
> Hm, in fact this was almost noticed, but I guess nobody retested it after
> fixing the initial crash we had with 7486bc06ab2c46d6957f0211d09bc549aaf9cc87
If that's the case, I'd try this (on top of the patch from upthread):
diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index da5f443..bcc9841 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -320,81 +320,37 @@ 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.
- */
-static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
-{
- struct dentry *alias, *discon_alias, *invalid_alias;
-
- if (hlist_empty(&inode->i_dentry))
- return NULL;
-
- discon_alias = invalid_alias = NULL;
-
- ll_lock_dcache(inode);
- 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)
- /* 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)
- 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);
- }
- ll_unlock_dcache(inode);
-
- return alias;
-}
-
-/*
* Similar to d_splice_alias(), but lustre treats invalid alias
* similar to DCACHE_DISCONNECTED, and tries to use it anyway.
*/
struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de)
{
- struct dentry *new;
+ struct dentry *alias = NULL;
int rc;
if (inode) {
- new = ll_find_alias(inode, de);
- if (new) {
- rc = ll_d_init(new);
- if (rc < 0) {
- dput(new);
- return ERR_PTR(rc);
- }
- d_move(new, de);
+ alias = d_exact_alias(de, inode);
+ if (alias)
+ iput(inode);
+ }
+
+ if (!alias) {
+ rc = ll_d_init(de);
+ if (rc < 0) {
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 ERR_PTR(rc);
}
+ alias = d_splice_alias(inode, de);
+ if (IS_ERR(alias))
+ return alias;
+ }
+
+ if (alias) {
+ CDEBUG(D_DENTRY,
+ "Reuse dentry %p inode %p refc %d flags %#x\n",
+ alias, d_inode(alias), d_count(alias), alias->d_flags);
+ return alias;
}
- rc = ll_d_init(de);
- if (rc < 0)
- return ERR_PTR(rc);
- 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);
return de;
diff --git a/drivers/staging/lustre/lustre/llite/statahead.c b/drivers/staging/lustre/lustre/llite/statahead.c
index 88ffd8e..6c64de0 100644
--- a/drivers/staging/lustre/lustre/llite/statahead.c
+++ b/drivers/staging/lustre/lustre/llite/statahead.c
@@ -1576,6 +1576,7 @@ int do_statahead_enter(struct inode *dir, struct dentry **dentryp,
alias = ll_splice_alias(inode,
*dentryp);
if (IS_ERR(alias)) {
+ entry->se_inode = NULL;
ll_sai_unplug(sai, entry);
return PTR_ERR(alias);
}
--
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
