From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jordi Pujol Subject: Re: overlayfs patches for ovl_copy_up & ovl_rename Date: Mon, 9 May 2011 08:38:04 +0200 Message-ID: <201105090838.04534.jordipujolp@gmail.com> References: <201105041659.38427.jordipujolp@gmail.com> <871v0da54v.fsf@tucsk.pomaz.szeredi.hu> Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org To: Miklos Szeredi Return-path: Received: from mail-wy0-f174.google.com ([74.125.82.174]:57544 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751937Ab1EIGiM (ORCPT ); Mon, 9 May 2011 02:38:12 -0400 Received: by wya21 with SMTP id 21so3625940wya.19 for ; Sun, 08 May 2011 23:38:11 -0700 (PDT) In-Reply-To: <871v0da54v.fsf@tucsk.pomaz.szeredi.hu> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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