Hello,
A Dijous 05 Maig 2011 11:06:08, Miklos Szeredi va escriure:
> Looks good, but I'm only willing to look at performance issues after the
> bugs have been fixed.
This also locks the whole path before doing the copy, that could be better
also, give it a try,
Pass me the list of pending bugs and I will help to debug, if I can
> >
> > old_upperdir = ovl_dentry_upper(old->d_parent);
> > new_upperdir = ovl_dentry_upper(new->d_parent);
> >
> > + (void) dget(old_upperdir);
> > + (void) dget(new_upperdir);
> > +
>
> This should not be necessary. vfs_rename() locks old and new parents
> which means taking an extra ref is totally superfluous. If this does
> make some difference then there's something very sinister going on an we
> need to investigate further.
>
> > trap = lock_rename(new_upperdir, old_upperdir);
> >
> > olddentry = ovl_dentry_upper(old);
> >
> > - newdentry = ovl_dentry_upper(new);
> > - if (newdentry) {
> > - dget(newdentry);
> > + (void) dget(olddentry);
>
> Again, while we have a ref on old, we should have a ref on olddentry as
> well, so this shouldn't make a difference.
>
> > +
Maybe the key action is to take this reference as early as possible;
consider that this really works, and it looks the same that is done by
unionfs.
> > + if (!ovl_is_whiteout(new) &&
> > + (newdentry = ovl_dentry_upper(new))) {
> > + (void) dget(newdentry);
>
> This shouldn't make a difference either, ovl_dentry_upper() will return
> NULL for whiteouts.
Agree, this has not any impact on the final behaviour,
> Can you try introducing the above changes one by one to see which one,
> if any of them, makes a difference to your test case?
>
In my tests, locking the parent directories makes the right thing,
but maybe there are other cases that should be considered...
Thanks,
Jordi Pujol
Live never ending Tale
GNU/Linux Live forever!
http://livenet.selfip.com
--
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
