Jordi Pujol <jordipujolp@gmail.com> writes:

> 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

Currently the only pending bug is the one you reported.

>> > 
>> >  	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.

The fact that the same is done by unionfs doesn't make it right and it
also doesn't explain why unionfs does this.

>
>> > +	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...

Can you please try the following patch then?  Does any of the WARN_ONs
trigger when you observe the bad behavior?

Thanks,
Miklos