Jordi Pujol <jordipujolp@xxxxxxxxx> 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
---
fs/overlayfs/dir.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Index: linux-2.6/fs/overlayfs/dir.c
===================================================================
--- linux-2.6.orig/fs/overlayfs/dir.c 2011-05-10 18:13:03.000000000 +0200
+++ linux-2.6/fs/overlayfs/dir.c 2011-05-10 18:16:13.000000000 +0200
@@ -512,6 +512,9 @@ static int ovl_rename(struct inode *oldd
old_upperdir = ovl_dentry_upper(old->d_parent);
new_upperdir = ovl_dentry_upper(new->d_parent);
+ (void) dget(old_upperdir);
+ (void) dget(new_upperdir);
+
trap = lock_rename(new_upperdir, old_upperdir);
olddentry = ovl_dentry_upper(old);
@@ -571,6 +574,13 @@ static int ovl_rename(struct inode *oldd
dput(newdentry);
out_unlock:
unlock_rename(new_upperdir, old_upperdir);
+
+ WARN_ON(old_upperdir->d_count == 1);
+ WARN_ON(new_upperdir->d_count == 1);
+
+ dput(old_upperdir);
+ dput(new_upperdir);
+
return err;
}
