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…
No, the d_instantiate for the hashed case comes through with filled in d_fsdata.
Ah! Found it. the lookup (ll_lookup_nd) checks if the operation is CREATE without
corresponding open (i.e. mknod) and returns NULL right away to sort it out in
ll_mknod() later. This adds the new dentry into the cache that is not otherwise
initialized. I guess we should call ll_d_init() right there.
This in particular works so far:
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -558,8 +518,13 @@ static struct dentry *ll_lookup_nd(struct inode *parent, st
ruct 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;
>> @@ -374,9 +368,9 @@ struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de)
>> }
>> }
>> rc = ll_d_init(de);
>> + d_add(de, inode);
>> if (rc < 0)
>> return ERR_PTR(rc);
>> - d_add(de, inode);
>
> Uhh... Why would you reorder them like that?
Hm… The thinking at the time was to always attach inode to dentry, so even in case of error
it's not really leaked. Though I guess that also makes dentry visible which is not ideal.
>> Also any particular reason to prefer d_splice_alias over d_add? Did not we already determine there
>> were no other aliases.
>
> No unhashed aliases with the same name and parent. Seriously, d_splice_alias()
> will do exactly the same thing as d_add() except for the situations where
> d_add() would've created multiple dentries over the same directory inode.
> And it's not more costly outside of that case...
Actually I guess it's kind of important with d_exact_alias, since it skips any hashed dentries,
but in fact invalid Lustre dentries are only marked with a flag inside that we then
validate during d_compare.
ll_find_alias was not ignoring hashed entries so it all worked out, but with
d_exact_alias we also need the d_splice_alias to certainly find those hashed entries too, we get
a lot of hits in it too according to my tests.
After looking into it a bit more… d_splice_alias would pick any alias there might be,
unhashed or whatever. So what's the importance of obtaining an exact (but unhashed) alias first,
since if we don't find it we are still going to pick first one from the alias list (both in nfsd
and in Lustre with your patches)? Just because the exact unhashed alias might be a better fit
and we are preferring it? Any other reason I don't realize yet?
I guess this looks pretty good at the moment. I'll do some additional multinode (and nfs reexport)
testing to ensure it all holds up nicely just in case.
Thanks!
Bye,
Oleg--
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
