On Mon, Nov 6, 2017 at 12:39 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > 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. Right... so you only have a bug in your patch calling ovl_type_origin(new) without checking d_inode(new) for increment of new->d_parent version. Thanks, Amir. -- 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
