On Mon, Nov 6, 2017 at 11:06 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Mon, Nov 6, 2017 at 11:17 AM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: >> On Thu, Nov 2, 2017 at 9:38 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >>> ovl_rename() updates dir cache version for impure old parent if an entry >>> with copy up origin is moved into old parent, but it did not update >>> cache version if the entry moved out of old parent has a copy up origin. >>> >>> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> >>> --- >>> fs/overlayfs/dir.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c >>> index ef533198be45..26aef3b5f007 100644 >>> --- a/fs/overlayfs/dir.c >>> +++ b/fs/overlayfs/dir.c >>> @@ -1075,9 +1075,10 @@ static int ovl_rename(struct inode *olddir, struct dentry *old, >>> drop_nlink(d_inode(new)); >>> } >>> >>> - ovl_dentry_version_inc(old->d_parent, >>> - !overwrite && ovl_type_origin(new)); >>> + ovl_dentry_version_inc(old->d_parent, ovl_type_origin(old)); >>> ovl_dentry_version_inc(new->d_parent, ovl_type_origin(old)); >>> + if (!overwrite) >>> + ovl_dentry_version_inc(old->d_parent, ovl_type_origin(new)); >> >> How about the opposite (i.e. newdir losing impurity)? That can happen two ways: >> >> - overwriting copied up file (new) with pure one (old) >> - exchange copied up file (new) with pure one (old) >> >> Also I'd merge the two version increments into one, although that >> doesn't really matter: >> >> - ovl_dentry_version_inc(old->d_parent, >> - !overwrite && ovl_type_origin(new)); >> - ovl_dentry_version_inc(new->d_parent, ovl_type_origin(old)); >> + ovl_dentry_version_inc(old->d_parent, ovl_type_origin(old) || >> + (!overwrite && ovl_type_origin(new))); >> + ovl_dentry_version_inc(new->d_parent, >> + ovl_type_origin(old) || ovl_type_origin(new)); >> >> Fixed up the patch, but tell me if I'm missing something here. >> > > I think you are. > Both my version and your version are wrong because the argument > 'impurity' should really be 'impurity_change' > > so if we have: > oldimpurity = ovl_type_origin(old); > newimpurity = d_inode(new) ? ovl_type_origin(new) : false; > > version increment should really be something like this: > ovl_dentry_version_inc(old->d_parent, oldimpurity ^ (overwrite ? > newimpurity : false)); > ovl_dentry_version_inc(new->d_parent, oldimpurity ^ newimpurity); > > > I think... We are not only interested in change of state, but rather if the cache is valid or not. If an impurity is removed and another added, then that needs to be noted as well. Note: strictly speaking we don't need to invalidate the cache on entry removal (that's why things worked fine before this patch). And in fact recalculating the cache on every removal of an impurity might well cause worse performance than not doing that. So this could be further optimized to differentiate between removal and addition of impurity and only "garbage collect" old caches occasionally. But lets not get lost in such details for now. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
