Re: overlayfs patches for ovl_copy_up & ovl_rename

[Jordi Pujol <jordipujolp@gmail.com>]

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