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

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

[zhangyi (F)]

Hi All:

I found another problem about impure upper dir which have left
whiteout files. 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>
---
 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..9535e2d 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 non-pure upper */
+	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] ovl:fix rmdir problem on impure dir

[Amir Goldstein]

On Fri, Jun 16, 2017 at 6:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Hi All:
>
> I found another problem about impure upper dir which have left
> whiteout files. 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

Can you please post an xfstest with that reproducer.
A good starting point is test overlay/012

>
> 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>

Apart of the one comment below, you may add

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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..9535e2d 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 non-pure upper */
> +       err = ovl_set_impure(dentry->d_parent, upperdir);
> +       if (err)
> +               goto out_dput;
> +

This should be done before ovl_whiteout().
You rather have false positive impure than the other way around.
Also please fix comment to "... may now contain whiteouts".

>         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] ovl:fix rmdir problem on impure dir

[zhangyi (F)]

On 2017/6/16 14:02, Amir Goldstein Wrote:
> On Fri, Jun 16, 2017 at 6:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> Hi All:
>>
>> I found another problem about impure upper dir which have left
>> whiteout files. 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
> 
> Can you please post an xfstest with that reproducer.
> A good starting point is test overlay/012
> 

Okay，I will write a new xfstest case with this reproducer, thanks!

>> ---
>>  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..9535e2d 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 non-pure upper */
>> +       err = ovl_set_impure(dentry->d_parent, upperdir);
>> +       if (err)
>> +               goto out_dput;
>> +
> 
> This should be done before ovl_whiteout().
> You rather have false positive impure than the other way around.
> Also please fix comment to "... may now contain whiteouts".
> 

Thanks for the review comments. I will fix it.

The first suggestion I don't quite understand. As you said, ovl_set_impure()
should done before ovl_whiteout()，I understand that have false impure is
doesn't matter if subsequent code return error. My patch call ovl_set_impure()
before ovl_lock_rename_workdir() is also before ovl_whiteout().

Do I misunderstand or do you mean just before ovl_whiteout() ?

Thanks,

ZhangYi

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

[Amir Goldstein]

On Fri, Jun 16, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/6/16 14:02, Amir Goldstein Wrote:
>> On Fri, Jun 16, 2017 at 6:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> Hi All:
>>>
>>> I found another problem about impure upper dir which have left
>>> whiteout files. 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
>>
>> Can you please post an xfstest with that reproducer.
>> A good starting point is test overlay/012
>>
>
> Okay，I will write a new xfstest case with this reproducer, thanks!
>
>>> ---
>>>  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..9535e2d 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 non-pure upper */
>>> +       err = ovl_set_impure(dentry->d_parent, upperdir);
>>> +       if (err)
>>> +               goto out_dput;
>>> +
>>
>> This should be done before ovl_whiteout().
>> You rather have false positive impure than the other way around.
>> Also please fix comment to "... may now contain whiteouts".
>>
>
> Thanks for the review comments. I will fix it.
>
> The first suggestion I don't quite understand. As you said, ovl_set_impure()
> should done before ovl_whiteout()，I understand that have false impure is
> doesn't matter if subsequent code return error. My patch call ovl_set_impure()
> before ovl_lock_rename_workdir() is also before ovl_whiteout().
>
> Do I misunderstand or do you mean just before ovl_whiteout() ?

I misread. Your patch is correct

>

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

[Miklos Szeredi]

On Fri, Jun 16, 2017 at 10:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Jun 16, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2017/6/16 14:02, Amir Goldstein Wrote:
>>> On Fri, Jun 16, 2017 at 6:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>> Hi All:
>>>>
>>>> I found another problem about impure upper dir which have left
>>>> whiteout files. 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.

Thanks for the report and patch.

I think the unlink failures are indicative of a bigger problem,
though: we optimize directory reading (and empty checking) based on
the MERGE flag.  This results in the whiteout being returned in the
directory listing in your test case.

Fortunately we now have a way to identify directories that may contain
whiteouts: the "overlay.origin" xattr.  So I think we should switch to
using that instead of the MERGE flag for deciding whether we can use
real readdir and if we need to check for emptiness by iterating the
directory.

Thanks,
Miklos

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

[Miklos Szeredi]

On Fri, Jun 16, 2017 at 11:50 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jun 16, 2017 at 10:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Jun 16, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> On 2017/6/16 14:02, Amir Goldstein Wrote:
>>>> On Fri, Jun 16, 2017 at 6:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>> Hi All:
>>>>>
>>>>> I found another problem about impure upper dir which have left
>>>>> whiteout files. 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.
>
> Thanks for the report and patch.
>
> I think the unlink failures are indicative of a bigger problem,
> though: we optimize directory reading (and empty checking) based on
> the MERGE flag.  This results in the whiteout being returned in the
> directory listing in your test case.
>
> Fortunately we now have a way to identify directories that may contain
> whiteouts: the "overlay.origin" xattr.  So I think we should switch to
> using that instead of the MERGE flag for deciding whether we can use
             ^^^^^^^^^
That should be "in addition to" not "instead of".

Thanks,
Miklos

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

[zhangyi (F)]

On 2017/6/16 17:57, Miklos Szeredi wrote:
> On Fri, Jun 16, 2017 at 11:50 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Fri, Jun 16, 2017 at 10:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Fri, Jun 16, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>> On 2017/6/16 14:02, Amir Goldstein Wrote:
>>>>> On Fri, Jun 16, 2017 at 6:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>>> Hi All:
>>>>>>
>>>>>> I found another problem about impure upper dir which have left
>>>>>> whiteout files. 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.
>>
>> Thanks for the report and patch.
>>
>> I think the unlink failures are indicative of a bigger problem,
>> though: we optimize directory reading (and empty checking) based on
>> the MERGE flag.  This results in the whiteout being returned in the
>> directory listing in your test case.
>>
>> Fortunately we now have a way to identify directories that may contain
>> whiteouts: the "overlay.origin" xattr.  So I think we should switch to
>> using that instead of the MERGE flag for deciding whether we can use
>              ^^^^^^^^^
> That should be "in addition to" not "instead of".
> 

I think these have been done by Amir in his patch "ovl: consistent st_ino/d_ino" on June 1.  :)


Thanks,

ZhangYi

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

[Amir Goldstein]

On Fri, Jun 16, 2017 at 12:50 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jun 16, 2017 at 10:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Jun 16, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> On 2017/6/16 14:02, Amir Goldstein Wrote:
>>>> On Fri, Jun 16, 2017 at 6:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>> Hi All:
>>>>>
>>>>> I found another problem about impure upper dir which have left
>>>>> whiteout files. 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.
>
> Thanks for the report and patch.
>
> I think the unlink failures are indicative of a bigger problem,
> though: we optimize directory reading (and empty checking) based on
> the MERGE flag.  This results in the whiteout being returned in the
> directory listing in your test case.

But not only. Now we already check MERGE || (!UPPER && !IMPURE),
so this patch DOES address both empty checking and whiteout filtering
in readdir.

>
> Fortunately we now have a way to identify directories that may contain
> whiteouts: the "overlay.origin" xattr.  So I think we should switch to
> using that instead of the MERGE flag for deciding whether we can use
> real readdir and if we need to check for emptiness by iterating the
> directory.
>

Well, IMPURE can be !ORIGIN and then its a waste to check whiteouts,
but that is a rare case (ORIGIN object moved into pure upper dir).
OTOH, ORIGIN can be !IMPURE (only new files created in merge dir)
and I think that is a more common case, so overall, if we have to choose
from existing flags and not add new ones (i.e. HAS_WHITEOUTS yuck)
then IMO marking IMPURE on whiteout and checking only IMPURE on
readdir is the better option.

Amir.