[RFC PATCH V2] ovl:fix rmdir problem on impure dir

[RFC PATCH V2] ovl:fix rmdir problem on impure dir

[zhangyi (F)]

There is another problem about impure upper dir which have left
whiteout files(the first one is whiteout exposure). If an
"impure && not merged" upper dir have left whiteout files
(last mount created), ovl cannot clear this dir when we
removing it, which may lead to rmdir fail or temp file left
in workdir.

Reproducer:
  mkdir lower upper work merge
  mkdir -p lower/dir
  touch lower/dir/a
  mount -t overlay overlay -olowerdir=lower,upperdir=upper,\
    workdir=work merge
  rm merge/dir/a
  umount merge
  rm -rf lower/*
  touch lower/dir  (*)
  mount -t overlay overlay -olowerdir=lower,upperdir=upper,\
    workdir=work merge
  rm -rf merge/dir

Syslog dump:
  overlayfs: cleanup of 'work/#7' failed (-39)

(*): if we are not creat this file, the result is different:
  rm: cannot remove "dir/": Directory not empty

This patch add impure flag on parent dir in ovl_remove_and_whiteout()
to indicate the dir is impure, and then check and clean it when
removing this dir.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

---
V2:
 - fix comment in ovl_remove_and_whiteout()
 - post reproducer to xfstest

---
 fs/overlayfs/dir.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index a63a716..9bedad0 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -336,7 +336,8 @@ static struct dentry *ovl_check_empty_and_clear(struct dentry *dentry)
 	 * Doesn't matter, since copy-up can't create a non-empty directory
 	 * from an empty one.
 	 */
-	if (OVL_TYPE_UPPER(type) && OVL_TYPE_MERGE(type))
+	if (OVL_TYPE_UPPER(type) && (OVL_TYPE_MERGE(type) ||
+				     ovl_dentry_is_impure(dentry)))
 		ret = ovl_clear_empty(dentry, &list);
 
 out_free:
@@ -633,6 +634,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
 			goto out;
 	}
 
+	/* Mark parent "impure" because it may now contain whiteouts */
+	err = ovl_set_impure(dentry->d_parent, upperdir);
+	if (err)
+		goto out_dput;
+
 	err = ovl_lock_rename_workdir(workdir, upperdir);
 	if (err)
 		goto out_dput;
@@ -690,8 +696,9 @@ static int ovl_remove_upper(struct dentry *dentry, bool is_dir)
 	struct dentry *opaquedir = NULL;
 	int err;
 
-	/* Redirect dir can be !ovl_lower_positive && OVL_TYPE_MERGE */
-	if (is_dir && ovl_dentry_get_redirect(dentry)) {
+	/* Redirect/Impure dir can be !ovl_lower_positive && not clean */
+	if (is_dir && (ovl_dentry_get_redirect(dentry) ||
+		       ovl_dentry_is_impure(dentry))) {
 		opaquedir = ovl_check_empty_and_clear(dentry);
 		err = PTR_ERR(opaquedir);
 		if (IS_ERR(opaquedir))
-- 
1.8.3.1

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[Miklos Szeredi]

On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> There is another problem about impure upper dir which have left
> whiteout files(the first one is whiteout exposure). If an
> "impure && not merged" upper dir have left whiteout files
> (last mount created), ovl cannot clear this dir when we
> removing it, which may lead to rmdir fail or temp file left
> in workdir.
>
> Reproducer:
>   mkdir lower upper work merge
>   mkdir -p lower/dir
>   touch lower/dir/a
>   mount -t overlay overlay -olowerdir=lower,upperdir=upper,\
>     workdir=work merge
>   rm merge/dir/a
>   umount merge
>   rm -rf lower/*
>   touch lower/dir  (*)
>   mount -t overlay overlay -olowerdir=lower,upperdir=upper,\
>     workdir=work merge
>   rm -rf merge/dir
>
> Syslog dump:
>   overlayfs: cleanup of 'work/#7' failed (-39)
>
> (*): if we are not creat this file, the result is different:
>   rm: cannot remove "dir/": Directory not empty
>
> This patch add impure flag on parent dir in ovl_remove_and_whiteout()
> to indicate the dir is impure, and then check and clean it when
> removing this dir.

I still don't like adding the impure flag because of a whiteout.
Impure dir means it contains a copied up object.  In fact a valid (but
probably not worthwhile) optimizition would be to remove the impure
flag on removal of the last copied up object.

What's wrong with testing for (origin || merge) to see if we need to
check for whiteouts?

Thanks,
Miklos

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[Amir Goldstein]

On Mon, Jun 19, 2017 at 10:59 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> There is another problem about impure upper dir which have left
>> whiteout files(the first one is whiteout exposure). If an
>> "impure && not merged" upper dir have left whiteout files
>> (last mount created), ovl cannot clear this dir when we
>> removing it, which may lead to rmdir fail or temp file left
>> in workdir.
>>
>> Reproducer:
>>   mkdir lower upper work merge
>>   mkdir -p lower/dir
>>   touch lower/dir/a
>>   mount -t overlay overlay -olowerdir=lower,upperdir=upper,\
>>     workdir=work merge
>>   rm merge/dir/a
>>   umount merge
>>   rm -rf lower/*
>>   touch lower/dir  (*)
>>   mount -t overlay overlay -olowerdir=lower,upperdir=upper,\
>>     workdir=work merge
>>   rm -rf merge/dir
>>
>> Syslog dump:
>>   overlayfs: cleanup of 'work/#7' failed (-39)
>>
>> (*): if we are not creat this file, the result is different:
>>   rm: cannot remove "dir/": Directory not empty
>>
>> This patch add impure flag on parent dir in ovl_remove_and_whiteout()
>> to indicate the dir is impure, and then check and clean it when
>> removing this dir.
>
> I still don't like adding the impure flag because of a whiteout.
> Impure dir means it contains a copied up object.  In fact a valid (but
> probably not worthwhile) optimizition would be to remove the impure
> flag on removal of the last copied up object.
>
> What's wrong with testing for (origin || merge) to see if we need to
> check for whiteouts?
>

Fine by me.

Yi,

Please note that you cannot use the test OVL_TYPE_ORIGIN(type)
in ovl_dir_is_real(), because that flag is only set when the lower
dentry is found by lookup. Instead, you should explicitly check for
existence of OVL_XATTR_ORIGIN.
Note that even ovl_get_origin_fh() is too strict, because it discards
empty (unkown) origin, but for your use case, unknown origin is also
candidate to contain whiteouts.

Amir.

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[zhangyi (F)]

On 2017/6/19 16:28, Amir Goldstein wrote:
> On Mon, Jun 19, 2017 at 10:59 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>> I still don't like adding the impure flag because of a whiteout.
>> Impure dir means it contains a copied up object.  In fact a valid (but
>> probably not worthwhile) optimizition would be to remove the impure
>> flag on removal of the last copied up object.
>>
>> What's wrong with testing for (origin || merge) to see if we need to
>> check for whiteouts?
>>
> 
> Fine by me.
> 
> Yi,
> 
> Please note that you cannot use the test OVL_TYPE_ORIGIN(type)
> in ovl_dir_is_real(), because that flag is only set when the lower
> dentry is found by lookup. Instead, you should explicitly check for
> existence of OVL_XATTR_ORIGIN.
> Note that even ovl_get_origin_fh() is too strict, because it discards
> empty (unkown) origin, but for your use case, unknown origin is also
> candidate to contain whiteouts.
> 

I realize that impure flag is not quite suitable. If we use (origin || merge)
to see whether we need to check whiteouts or not, I think we still have a
problem.

Thought: In my current test case, testdir was copyuped when we delete
the test file, so it have OVL_XATTR_ORIGIN (it's OK), but if the testdir
is a merged dir originally, it will not be copied up and the OVL_XATTR_ORIGIN
will not be set.

So, if we want to use (origin || merge), we have to set OVL_XATTR_ORIGIN for
the merged parent dir when it create whiteout, any side effects?

Thanks,
ZhangYi.

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[Miklos Szeredi]

On Mon, Jun 19, 2017 at 1:19 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/6/19 16:28, Amir Goldstein wrote:
>> On Mon, Jun 19, 2017 at 10:59 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>> I still don't like adding the impure flag because of a whiteout.
>>> Impure dir means it contains a copied up object.  In fact a valid (but
>>> probably not worthwhile) optimizition would be to remove the impure
>>> flag on removal of the last copied up object.
>>>
>>> What's wrong with testing for (origin || merge) to see if we need to
>>> check for whiteouts?
>>>
>>
>> Fine by me.
>>
>> Yi,
>>
>> Please note that you cannot use the test OVL_TYPE_ORIGIN(type)
>> in ovl_dir_is_real(), because that flag is only set when the lower
>> dentry is found by lookup. Instead, you should explicitly check for
>> existence of OVL_XATTR_ORIGIN.
>> Note that even ovl_get_origin_fh() is too strict, because it discards
>> empty (unkown) origin, but for your use case, unknown origin is also
>> candidate to contain whiteouts.
>>
>
> I realize that impure flag is not quite suitable. If we use (origin || merge)
> to see whether we need to check whiteouts or not, I think we still have a
> problem.
>
> Thought: In my current test case, testdir was copyuped when we delete
> the test file, so it have OVL_XATTR_ORIGIN (it's OK), but if the testdir
> is a merged dir originally, it will not be copied up and the OVL_XATTR_ORIGIN
> will not be set.
>
> So, if we want to use (origin || merge), we have to set OVL_XATTR_ORIGIN for
> the merged parent dir when it create whiteout, any side effects?

It's done for the root of the overlay by Amir's patchset, but that
probably should be done generically for all merged directories not
already having an origin marking.   It could be done at lookup time,
or at whiteout time.  Not sure which is better.

Thanks,
Miklos

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[Amir Goldstein]

On Mon, Jun 19, 2017 at 2:44 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, Jun 19, 2017 at 1:19 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2017/6/19 16:28, Amir Goldstein wrote:
>>> On Mon, Jun 19, 2017 at 10:59 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>
>>>> I still don't like adding the impure flag because of a whiteout.
>>>> Impure dir means it contains a copied up object.  In fact a valid (but
>>>> probably not worthwhile) optimizition would be to remove the impure
>>>> flag on removal of the last copied up object.
>>>>
>>>> What's wrong with testing for (origin || merge) to see if we need to
>>>> check for whiteouts?
>>>>
>>>
>>> Fine by me.
>>>
>>> Yi,
>>>
>>> Please note that you cannot use the test OVL_TYPE_ORIGIN(type)
>>> in ovl_dir_is_real(), because that flag is only set when the lower
>>> dentry is found by lookup. Instead, you should explicitly check for
>>> existence of OVL_XATTR_ORIGIN.
>>> Note that even ovl_get_origin_fh() is too strict, because it discards
>>> empty (unkown) origin, but for your use case, unknown origin is also
>>> candidate to contain whiteouts.
>>>
>>
>> I realize that impure flag is not quite suitable. If we use (origin || merge)
>> to see whether we need to check whiteouts or not, I think we still have a
>> problem.
>>
>> Thought: In my current test case, testdir was copyuped when we delete
>> the test file, so it have OVL_XATTR_ORIGIN (it's OK), but if the testdir
>> is a merged dir originally, it will not be copied up and the OVL_XATTR_ORIGIN
>> will not be set.
>>
>> So, if we want to use (origin || merge), we have to set OVL_XATTR_ORIGIN for
>> the merged parent dir when it create whiteout, any side effects?
>
> It's done for the root of the overlay by Amir's patchset, but that
> probably should be done generically for all merged directories not
> already having an origin marking.   It could be done at lookup time,
> or at whiteout time.  Not sure which is better.
>

Checking merge dir origin is something I planned to do anyway, because
it is needed for NFS export (only verified lower can be indexed).
In fact, I already did it, but yanked it out of the current series because
it is not needed for indexing non-dirs, see:
https://github.com/amir73il/linux/commit/9af404799ba4ba4d08e147c7d54f6bcef0527bc9#diff-643262c1d5b2cba0bc9500e531831c12R481

On top of my current ovl-hardlinks branch, the following change in lookup
would get the required origin mark on merge dirs and code will
be inline with upcoming NFS export changes:

@@ -416,6 +478,17 @@ struct dentry *ovl_lookup(struct inode *dir,
struct dentry *dentry,
                if (err)
                        goto out_put;

+               /* Verify that uppermost lower matches the copy up origin fh */
+               if (this && upperdentry && !ctr &&
+                   ovl_indexdir(dentry->d_sb))) {
+                       err = ovl_verify_set_origin(upperdentry, lowerpath.mnt,
+                                               this, "merge", true);
+                       if (err && err != -ENODATA) {
+                               dput(this);
+                               break;
+                       }
+               }
+
                if (!this)
                        continue;




Amir.

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[zhangyi (F)]

On 2017/6/19 20:07, Amir Goldstein Wrote:
> On Mon, Jun 19, 2017 at 2:44 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Mon, Jun 19, 2017 at 1:19 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> On 2017/6/19 16:28, Amir Goldstein wrote:
>>>> On Mon, Jun 19, 2017 at 10:59 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>>> On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>> It's done for the root of the overlay by Amir's patchset, but that
>> probably should be done generically for all merged directories not
>> already having an origin marking.   It could be done at lookup time,
>> or at whiteout time.  Not sure which is better.
>>
> 
> Checking merge dir origin is something I planned to do anyway, because
> it is needed for NFS export (only verified lower can be indexed).
> In fact, I already did it, but yanked it out of the current series because
> it is not needed for indexing non-dirs, see:
> https://github.com/amir73il/linux/commit/9af404799ba4ba4d08e147c7d54f6bcef0527bc9#diff-643262c1d5b2cba0bc9500e531831c12R481
> 
> On top of my current ovl-hardlinks branch, the following change in lookup
> would get the required origin mark on merge dirs and code will
> be inline with upcoming NFS export changes:
> 
> @@ -416,6 +478,17 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>                 if (err)
>                         goto out_put;
> 
> +               /* Verify that uppermost lower matches the copy up origin fh */
> +               if (this && upperdentry && !ctr &&
> +                   ovl_indexdir(dentry->d_sb))) {
> +                       err = ovl_verify_set_origin(upperdentry, lowerpath.mnt,
> +                                               this, "merge", true);
> +                       if (err && err != -ENODATA) {
> +                               dput(this);
> +                               break;
> +                       }
> +               }
> +
>                 if (!this)
>                         continue;
> 

It looks good to me. Do you have a plan to push this code? I can fix this whiteout
problem base on it.

Thanks,
ZhangYi.

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[Amir Goldstein]

On Tue, Jun 20, 2017 at 9:10 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/6/19 20:07, Amir Goldstein Wrote:
>> On Mon, Jun 19, 2017 at 2:44 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Mon, Jun 19, 2017 at 1:19 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>> On 2017/6/19 16:28, Amir Goldstein wrote:
>>>>> On Mon, Jun 19, 2017 at 10:59 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>>>> On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>> It's done for the root of the overlay by Amir's patchset, but that
>>> probably should be done generically for all merged directories not
>>> already having an origin marking.   It could be done at lookup time,
>>> or at whiteout time.  Not sure which is better.
>>>
>>
>> Checking merge dir origin is something I planned to do anyway, because
>> it is needed for NFS export (only verified lower can be indexed).
>> In fact, I already did it, but yanked it out of the current series because
>> it is not needed for indexing non-dirs, see:
>> https://github.com/amir73il/linux/commit/9af404799ba4ba4d08e147c7d54f6bcef0527bc9#diff-643262c1d5b2cba0bc9500e531831c12R481
>>
>> On top of my current ovl-hardlinks branch, the following change in lookup
>> would get the required origin mark on merge dirs and code will
>> be inline with upcoming NFS export changes:
>>
>> @@ -416,6 +478,17 @@ struct dentry *ovl_lookup(struct inode *dir,
>> struct dentry *dentry,
>>                 if (err)
>>                         goto out_put;
>>
>> +               /* Verify that uppermost lower matches the copy up origin fh */
>> +               if (this && upperdentry && !ctr &&
>> +                   ovl_indexdir(dentry->d_sb))) {
>> +                       err = ovl_verify_set_origin(upperdentry, lowerpath.mnt,
>> +                                               this, "merge", true);
>> +                       if (err && err != -ENODATA) {
>> +                               dput(this);
>> +                               break;
>> +                       }
>> +               }
>> +
>>                 if (!this)
>>                         continue;
>>
>
> It looks good to me. Do you have a plan to push this code? I can fix this whiteout
> problem base on it.
>

My plans were to defer it to next-next (v4.14) merge cycle along with
NFS export.

I suggest that you post a fix to the whiteout problem based on (MERGE
|| ORIGIN),
disregarding the issue of pre kernel v4.12 upper layers (MERGE && !ORIGIN).

After my current patch set lands on linux-next, or even after v4.13-rc1, we
can decide if it is worth while to merge the extra hunk above for the sake of
fixing whiteout problem with pre v4.12 upper layers.

Does your use case have to deal with migrating existing upper layers?

BTW, does you use case also care about other types of lower layer changes?
As part of the work for overlayfs snapshots and NFS export, I also plan to
post patches to follow renamed lower dir to their new location, instead of
treating the upper dir as !MERGE, see:
https://github.com/amir73il/linux/commits/ovl-verify-dir

Amir.

Re: [RFC PATCH V2] ovl:fix rmdir problem on impure dir

[zhangyi (F)]

on 2017/6/20 15:04, Amir Goldstein wrote:
> On Tue, Jun 20, 2017 at 9:10 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2017/6/19 20:07, Amir Goldstein Wrote:
>>> On Mon, Jun 19, 2017 at 2:44 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Mon, Jun 19, 2017 at 1:19 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>> On 2017/6/19 16:28, Amir Goldstein wrote:
>>>>>> On Mon, Jun 19, 2017 at 10:59 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>>>>> On Mon, Jun 19, 2017 at 6:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> 
> My plans were to defer it to next-next (v4.14) merge cycle along with
> NFS export.
> 
> I suggest that you post a fix to the whiteout problem based on (MERGE
> || ORIGIN),
> disregarding the issue of pre kernel v4.12 upper layers (MERGE && !ORIGIN).
> 

Okay, I will try to use current (MERGE || ORIGIN) to fix, the (MERGE && !ORIGIN)
issue will be fixed automatically after you merge your patchset. Thanks for your
suggestion.

> After my current patch set lands on linux-next, or even after v4.13-rc1, we
> can decide if it is worth while to merge the extra hunk above for the sake of
> fixing whiteout problem with pre v4.12 upper layers.
> 
> Does your use case have to deal with migrating existing upper layers?
> 

I don't have this use case this moment(maybe have in future), it's not urgent.

> BTW, does you use case also care about other types of lower layer changes?
> As part of the work for overlayfs snapshots and NFS export, I also plan to
> post patches to follow renamed lower dir to their new location, instead of
> treating the upper dir as !MERGE, see:
> https://github.com/amir73il/linux/commits/ovl-verify-dir

This case is interesting, I don't have this case too (not so sure), I will
think about this, thanks for reminding.

Thanks,
ZhangYi.