On Mar 8, 2016, at 8:26 PM, Al Viro wrote:
> On Wed, Mar 09, 2016 at 12:53:20AM +0000, Drokin, Oleg wrote:
>
>> I have not tried this exact one yet (will try in a moment).
>> But it appears we do need ll_d_init() call if we got a hit
>> in d_exact_alias().
>> A particular test case fails due to lack of the d_fsdata being null.
>
> Very interesting. I'd suggest checking if we hit that d_instantiate()
> in your namei.c with NULL ->d_fsdata. That really shouldn't happen, AFAICS…
Well, it appears there's another way of getting this in addition to
the already identified "return NULL" at the start of the ll_lookup_nd()
that was uncovered when reexporting via NFS.
ll_iget_for_nfs does d_obtain_alias() that might produce a dentry also
in need of initialization if it's a disconnected one.
This is a path that you did not list in your possible ways to find a
dentry in ll_find_alias() before.
Now, do you think I should just call ll_d_init() on it at all times,
since that one might be uninitialized.
BTW, I also did a quick look through the code and ext4 "encryption"
code potentially got it wrong too? ext4_lookup checks if the parent
dir have encryption and sets d_fsdata to 1 and also rewrites
dentry ops with ext4_encrypted_d_ops, but it then does
d_splice_alias that might find such a disconnected dentry and it would
lose all of that work in the end?
ocfs2 reimplements d_splice_alias as ocfs2_find_local_alias where they
sidestep the initialization of d_fsdata issue (even though they
refer to it as d_splice_alias elsewhere in the comments).
This would cause them to always reject disconnected dentries from
being found at the very least which is probably somewhat suboptimal
and leads to some wasted RAM for those extra dentries.
With this (and the other one) fixes in place I see no other problems
with your patch.
So if you think the patch below is ok, I can submit it to Greg
(with addition of removing the extra init after ll_find_alias call),
or to you if you think it's better for you to carry it in your tree
(the one below probably has whitespace all wrong, so it won't really
apply as is and needs a proper resend).
diff --git a/drivers/staging/lustre/lustre/llite/llite_nfs.c b/drivers/staging/lustre/lustre/llite/llite_nfs.c
index 193aab8..8a0bf73 100644
--- a/drivers/staging/lustre/lustre/llite/llite_nfs.c
+++ b/drivers/staging/lustre/lustre/llite/llite_nfs.c
@@ -173,6 +173,16 @@ ll_iget_for_nfs(struct super_block *sb, struct lu_fid *fid, struct lu_fid *paren
/* N.B. d_obtain_alias() drops inode ref on error */
result = d_obtain_alias(inode);
+ if (!IS_ERR(result)) {
+ int rc;
+
+ rc = ll_d_init(result);
+ if (rc < 0) {
+ dput(result);
+ result = ERR_PTR(rc);
+ }
+ }
+
return result;
}
diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index f8f98e4..e18f3a5 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -302,81 +302,44 @@ void ll_i2gids(__u32 *suppgids, struct inode *i1, struct inode *i2)
@@ -558,8 +522,13 @@ static struct dentry *ll_lookup_nd(struct inode *parent, struct dentry *dentry,
parent->i_generation, parent, flags);
/* Optimize away (CREATE && !OPEN). Let .create handle the race. */
- if ((flags & LOOKUP_CREATE) && !(flags & LOOKUP_OPEN))
+ if ((flags & LOOKUP_CREATE) && !(flags & LOOKUP_OPEN)) {
+ int rc;
+ rc = ll_d_init(dentry);
+ if (rc < 0)
+ return ERR_PTR(rc);
return NULL;
+ }
if (flags & (LOOKUP_PARENT|LOOKUP_OPEN|LOOKUP_CREATE))
itp = NULL;
--
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
