[PATCH] ovl: fix visible whiteout on not merged dir

[PATCH] ovl: fix visible whiteout on not merged dir

[zhangyi (F)]

When we mount overlayfs which have whiteout on the directory that
are not merged(single lower or upper layer only), the whiteout will
be visible on the merge layer, because of readdir on this directory
is simply handled by the underlying directory.

Reproducer:

mkdir -p low/dir up work merge
mknod low/dir/test c 0 0

mount -t overlay -o lowerdir=low,upperdir=up,workdir=work overlayfs merge

ls -l merge/dir

The out put will be:

  ls: cannot access merge/dir/test: No such file or directory
  total 0
  c????????? ? ? ? ?            ? test

It is same that have whiteout on the not merged upper directory or 
the opaque directory.

This patch fix this by add a flag and check whiteout, if the directory
don't have whiteout, it handled by theunderlying directory later,
otherwise the same to merged directories.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/readdir.c   | 21 ++++++++++++++++++---
 fs/overlayfs/util.c      | 15 +++++++++++++++
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 741dc0b..2ffb7ec 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -161,6 +161,8 @@ static inline struct inode *ovl_inode_real(struct inode *inode, bool *is_upper)
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
+void ovl_set_dir_pure(struct dentry *dentry, bool pure);
+bool ovl_dir_pure(struct dentry *dentry);
 struct ovl_dir_cache *ovl_dir_cache(struct dentry *dentry);
 void ovl_set_dir_cache(struct dentry *dentry, struct ovl_dir_cache *cache);
 bool ovl_dentry_is_opaque(struct dentry *dentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 59614fa..8c32d9d 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -41,6 +41,7 @@ struct ovl_entry {
 			const char *redirect;
 			bool opaque;
 			bool copying;
+			bool pure;
 		};
 		struct rcu_head rcu;
 	};
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index f241b4e..ca97e46 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -45,11 +45,13 @@ struct ovl_readdir_data {
 	int count;
 	int err;
 	bool d_type_supported;
+	bool have_whiteout;
 };
 
 struct ovl_dir_file {
 	bool is_real;
 	bool is_upper;
+	bool is_pure;
 	struct ovl_dir_cache *cache;
 	struct list_head *cursor;
 	struct file *realfile;
@@ -218,6 +220,8 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
 			dentry = lookup_one_len(p->name, dir, p->len);
 			if (!IS_ERR(dentry)) {
 				p->is_whiteout = ovl_is_whiteout(dentry);
+				if (p->is_whiteout)
+					rdd->have_whiteout = true;
 				dput(dentry);
 			}
 		}
@@ -271,6 +275,9 @@ static void ovl_dir_reset(struct file *file)
 	WARN_ON(!od->is_real && !OVL_TYPE_MERGE(type));
 	if (od->is_real && OVL_TYPE_MERGE(type))
 		od->is_real = false;
+
+	if (od->is_real)
+		od->is_pure = ovl_dir_pure(dentry);
 }
 
 static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list)
@@ -283,6 +290,7 @@ static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list)
 		.list = list,
 		.root = RB_ROOT,
 		.is_lowest = false,
+		.have_whiteout = false,
 	};
 	int idx, next;
 
@@ -304,6 +312,10 @@ static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list)
 			list_del(&rdd.middle);
 		}
 	}
+
+	if (!OVL_TYPE_MERGE(ovl_path_type(dentry)))
+		ovl_set_dir_pure(dentry, !rdd.have_whiteout);
+
 	return err;
 }
 
@@ -362,7 +374,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	if (!ctx->pos)
 		ovl_dir_reset(file);
 
-	if (od->is_real)
+	if (od->is_real && likely(od->is_pure))
 		return iterate_dir(od->realfile, ctx);
 
 	if (!od->cache) {
@@ -396,7 +408,7 @@ static loff_t ovl_dir_llseek(struct file *file, loff_t offset, int origin)
 	if (!file->f_pos)
 		ovl_dir_reset(file);
 
-	if (od->is_real) {
+	if (od->is_real && likely(od->is_pure)) {
 		res = vfs_llseek(od->realfile, offset, origin);
 		file->f_pos = od->realfile->f_pos;
 	} else {
@@ -490,12 +502,14 @@ static int ovl_dir_open(struct inode *inode, struct file *file)
 	struct file *realfile;
 	struct ovl_dir_file *od;
 	enum ovl_path_type type;
+	struct dentry *dentry;
 
 	od = kzalloc(sizeof(struct ovl_dir_file), GFP_KERNEL);
 	if (!od)
 		return -ENOMEM;
 
-	type = ovl_path_real(file->f_path.dentry, &realpath);
+	dentry = file->f_path.dentry;
+	type = ovl_path_real(dentry, &realpath);
 	realfile = ovl_path_open(&realpath, file->f_flags);
 	if (IS_ERR(realfile)) {
 		kfree(od);
@@ -504,6 +518,7 @@ static int ovl_dir_open(struct inode *inode, struct file *file)
 	od->realfile = realfile;
 	od->is_real = !OVL_TYPE_MERGE(type);
 	od->is_upper = OVL_TYPE_UPPER(type);
+	od->is_pure = ovl_dir_pure(dentry);
 	file->private_data = od;
 
 	return 0;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 6e610a2..1638913 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -146,6 +146,21 @@ struct dentry *ovl_dentry_real(struct dentry *dentry)
 	return realdentry;
 }
 
+void ovl_set_dir_pure(struct dentry *dentry, bool pure)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+
+	if (d_is_dir(dentry))
+		oe->pure = pure;
+}
+
+bool ovl_dir_pure(struct dentry *dentry)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+
+	return oe->pure;
+}
+
 struct ovl_dir_cache *ovl_dir_cache(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
-- 
1.8.3.1

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[Amir Goldstein]

On Wed, May 3, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> When we mount overlayfs which have whiteout on the directory that
> are not merged(single lower or upper layer only), the whiteout will
> be visible on the merge layer, because of readdir on this directory
> is simply handled by the underlying directory.
>

Hi Zhang,

I am interested in one specific use case of visible whiteouts in non-merge
dir and that is the use case of a lower dir that has been deleted leaving
upper with possible visible whiteouts.

With some of my patches related to constant inode numbers, that use case
could be fixed simply by replacing OVL_TYPE_MERGE in readdir.c with
OVL_TYPE_COPYUP. Every dir that has *ever* been copied up is marked
with an xattr overlay.origin, which may or may not be uptodate, but will
forever indicate that this is a 'suspect impure' directory.

My question is whether this solution is sufficient to cover your use cases
and if not, where and how did those whiteouts get to your lower/upper
impure directory?

You patch does provide extra optimization for 'purifying' a 'suspect impure'
directory, but:
1. Not sure if that optimization is that important.
2. Upcoming changes related to constant inode numbers will have to use
    ovl_dir_read_merged() for 'suspect impure' dir, not only if that dir may
    contain whiteouts, but also if that dir may contain copyups, namely files
    with overlay.origin, which may need to report non-real d_ino.

[...]

> @@ -271,6 +275,9 @@ static void ovl_dir_reset(struct file *file)
>         WARN_ON(!od->is_real && !OVL_TYPE_MERGE(type));
>         if (od->is_real && OVL_TYPE_MERGE(type))
>                 od->is_real = false;
> +
> +       if (od->is_real)
> +               od->is_pure = ovl_dir_pure(dentry);
>  }
>

[...]

> @@ -362,7 +374,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
>         if (!ctx->pos)
>                 ovl_dir_reset(file);
>
> -       if (od->is_real)
> +       if (od->is_real && likely(od->is_pure))
>                 return iterate_dir(od->realfile, ctx);
>

[...]

> @@ -396,7 +408,7 @@ static loff_t ovl_dir_llseek(struct file *file, loff_t offset, int origin)
>         if (!file->f_pos)
>                 ovl_dir_reset(file);
>
> -       if (od->is_real) {
> +       if (od->is_real && likely(od->is_pure)) {
>                 res = vfs_llseek(od->realfile, offset, origin);
>                 file->f_pos = od->realfile->f_pos;

[...]

> @@ -504,6 +518,7 @@ static int ovl_dir_open(struct inode *inode, struct file *file)
>         od->realfile = realfile;
>         od->is_real = !OVL_TYPE_MERGE(type);
>         od->is_upper = OVL_TYPE_UPPER(type);
> +       od->is_pure = ovl_dir_pure(dentry);

Isn't is_real (or is_pure) redundant?, i.e.:
            od->is_real = !OVL_TYPE_MERGE(type) && ovl_dir_pure(dentry);

You always && them together and the only place you use them separately
is ovl_dir_reset(), but you also check OVL_TYPE_MERGE(type) and
ovl_dir_pure(dentry) in that function anyway, so you could just reset
od->real to the definition above.

Cheers,
Amir.

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[zhangyi (F)]

on 2017/5/3 16:38, Amir Goldstein wrote:
> On Wed, May 3, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> When we mount overlayfs which have whiteout on the directory that
>> are not merged(single lower or upper layer only), the whiteout will
>> be visible on the merge layer, because of readdir on this directory
>> is simply handled by the underlying directory.
>>
> 
> Hi Zhang,
> 
> I am interested in one specific use case of visible whiteouts in non-merge
> dir and that is the use case of a lower dir that has been deleted leaving
> upper with possible visible whiteouts.
> 
> With some of my patches related to constant inode numbers, that use case
> could be fixed simply by replacing OVL_TYPE_MERGE in readdir.c with
> OVL_TYPE_COPYUP. Every dir that has *ever* been copied up is marked
> with an xattr overlay.origin, which may or may not be uptodate, but will
> forever indicate that this is a 'suspect impure' directory.
> 
> My question is whether this solution is sufficient to cover your use cases
> and if not, where and how did those whiteouts get to your lower/upper
> impure directory?
> 
> You patch does provide extra optimization for 'purifying' a 'suspect impure'
> directory, but:
> 1. Not sure if that optimization is that important.
> 2. Upcoming changes related to constant inode numbers will have to use
>     ovl_dir_read_merged() for 'suspect impure' dir, not only if that dir may
>     contain whiteouts, but also if that dir may contain copyups, namely files
>     with overlay.origin, which may need to report non-real d_ino.
> 

  I think there are three(or more) cases can cause this problem:
  1. Some one create whiteouts in lower/upper's single subdir manually(as my
     reproducer show) and then mount overlayfs;
  2. User create whiteouts in upper's opaque dir manualy and remount
     overlayfs.
     For example:
     mkdir -p low/dir up/dir work merge
     mount -t overlay -o lowerdir=low,upperdir=up,workdir=work overlayfs merge
     rm -rf merge/dir
     mkdir merge/dir
     umount merge
     mknod up/dir/aa c 0 0
     mount -t overlay -o lowerdir=low,upperdir=up,workdir=work overlayfs merge
     ls -l merge/dir

        ls: cannot access merge/dir/aa: No such file or directory
        total 0
        c????????? ? ? ? ?            ? aa

  3. User clean lower dir and remount overlayfs (as you interested in).

  If we use OVL_TYPE_COPYUP in readdir.c, we can find out 'suspect impure'
  directory in the third case only, we still can not make sure dir have
  whiteout or not. Especially in the first case, we even haven't any
  information to deduce it's 'suspect' or not. (Am I miss something ?)

  I want to find a way to fix this and minimize the performance impact,
  so I use the merged flow in ovl_iterate() to 'purifying' the dir is 'pure'
  or not, if this dir is not merged and don't have whiteout, no need to create
  dir cache.

  I didn't find a better way, if it's not important, can we use the merged
  flow to all directories and remove the is_real flag (not handled by
  underlying directory)? or other better solutions?

> Isn't is_real (or is_pure) redundant?, i.e.:
>             od->is_real = !OVL_TYPE_MERGE(type) && ovl_dir_pure(dentry);
> 
> You always && them together and the only place you use them separately
> is ovl_dir_reset(), but you also check OVL_TYPE_MERGE(type) and
> ovl_dir_pure(dentry) in that function anyway, so you could just reset
> od->real to the definition above.
> 
  Indeed, thanks for your suggestion.

Thanks,
zhangyi

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[Amir Goldstein]

On Wed, May 3, 2017 at 2:25 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>
>
> on 2017/5/3 16:38, Amir Goldstein wrote:
>> On Wed, May 3, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> When we mount overlayfs which have whiteout on the directory that
>>> are not merged(single lower or upper layer only), the whiteout will
>>> be visible on the merge layer, because of readdir on this directory
>>> is simply handled by the underlying directory.
>>>
>>
>> Hi Zhang,
>>
>> I am interested in one specific use case of visible whiteouts in non-merge
>> dir and that is the use case of a lower dir that has been deleted leaving
>> upper with possible visible whiteouts.
>>
>> With some of my patches related to constant inode numbers, that use case
>> could be fixed simply by replacing OVL_TYPE_MERGE in readdir.c with
>> OVL_TYPE_COPYUP. Every dir that has *ever* been copied up is marked
>> with an xattr overlay.origin, which may or may not be uptodate, but will
>> forever indicate that this is a 'suspect impure' directory.
>>
>> My question is whether this solution is sufficient to cover your use cases
>> and if not, where and how did those whiteouts get to your lower/upper
>> impure directory?
>>
>> You patch does provide extra optimization for 'purifying' a 'suspect impure'
>> directory, but:
>> 1. Not sure if that optimization is that important.
>> 2. Upcoming changes related to constant inode numbers will have to use
>>     ovl_dir_read_merged() for 'suspect impure' dir, not only if that dir may
>>     contain whiteouts, but also if that dir may contain copyups, namely files
>>     with overlay.origin, which may need to report non-real d_ino.
>>
>
>   I think there are three(or more) cases can cause this problem:
>   1. Some one create whiteouts in lower/upper's single subdir manually(as my
>      reproducer show) and then mount overlayfs;
>   2. User create whiteouts in upper's opaque dir manualy and remount
>      overlayfs.
>      For example:
>      mkdir -p low/dir up/dir work merge
>      mount -t overlay -o lowerdir=low,upperdir=up,workdir=work overlayfs merge
>      rm -rf merge/dir
>      mkdir merge/dir
>      umount merge
>      mknod up/dir/aa c 0 0
>      mount -t overlay -o lowerdir=low,upperdir=up,workdir=work overlayfs merge
>      ls -l merge/dir
>
>         ls: cannot access merge/dir/aa: No such file or directory
>         total 0
>         c????????? ? ? ? ?            ? aa
>
>   3. User clean lower dir and remount overlayfs (as you interested in).
>
>   If we use OVL_TYPE_COPYUP in readdir.c, we can find out 'suspect impure'
>   directory in the third case only, we still can not make sure dir have
>   whiteout or not. Especially in the first case, we even haven't any
>   information to deduce it's 'suspect' or not. (Am I miss something ?)
>

Of course it is *possible* to hand craft use cases 1 and 2, but so what?
I am asking for *your* use case. You must have a reason to propose this
purification that is not a user hand crafted impure lower/upper, don't you?
So I am asking of for your use case, OVL_TYPE_COPYUP is enough.
If you need a test branch to test if OVL_TYPE_COPYUP
answers your needs, I can prepare one for you.

>   I want to find a way to fix this and minimize the performance impact,
>   so I use the merged flow in ovl_iterate() to 'purifying' the dir is 'pure'
>   or not, if this dir is not merged and don't have whiteout, no need to create
>   dir cache.
>

If you are able to argue that cases 1 and 2 are important, then your proposed
solution seems fine to me, but if you will also want to benefit from constant
d_ino going forward, then it won't be enough.

>   I didn't find a better way, if it's not important, can we use the merged
>   flow to all directories and remove the is_real flag (not handled by
>   underlying directory)? or other better solutions?
>

Removing the is_real flag would cause performance overhead and bloated
memory usage (for large dir cache), so it is unlikely to be adopted as default
behavior. If you can convince that there is demand for purification, then
perhaps the only way would be to introduce a mount option to ignore the
is_real flag.


Cheers,
Amir.

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[zhangyi (F)]

On 2017/5/3 20:05, Amir Goldstein wrote:
> On Wed, May 3, 2017 at 2:25 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> on 2017/5/3 16:38, Amir Goldstein wrote:
>>> On Wed, May 3, 2017 at 10:39 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:

>>> I am interested in one specific use case of visible whiteouts in non-merge
>>> dir and that is the use case of a lower dir that has been deleted leaving
>>> upper with possible visible whiteouts.
>>>
>>> With some of my patches related to constant inode numbers, that use case
>>> could be fixed simply by replacing OVL_TYPE_MERGE in readdir.c with
>>> OVL_TYPE_COPYUP. Every dir that has *ever* been copied up is marked
>>> with an xattr overlay.origin, which may or may not be uptodate, but will
>>> forever indicate that this is a 'suspect impure' directory.
>>>
>>> My question is whether this solution is sufficient to cover your use cases
>>> and if not, where and how did those whiteouts get to your lower/upper
>>> impure directory?
>>>
>>> You patch does provide extra optimization for 'purifying' a 'suspect impure'
>>> directory, but:
>>> 1. Not sure if that optimization is that important.
>>> 2. Upcoming changes related to constant inode numbers will have to use
>>>     ovl_dir_read_merged() for 'suspect impure' dir, not only if that dir may
>>>     contain whiteouts, but also if that dir may contain copyups, namely files
>>>     with overlay.origin, which may need to report non-real d_ino.
>>>
>>
>>   I think there are three(or more) cases can cause this problem:
>>   1. Some one create whiteouts in lower/upper's single subdir manually(as my
>>      reproducer show) and then mount overlayfs;
>>   2. User create whiteouts in upper's opaque dir manualy and remount
>>      overlayfs.
>>      For example:
>>      mkdir -p low/dir up/dir work merge
>>      mount -t overlay -o lowerdir=low,upperdir=up,workdir=work overlayfs merge
>>      rm -rf merge/dir
>>      mkdir merge/dir
>>      umount merge
>>      mknod up/dir/aa c 0 0
>>      mount -t overlay -o lowerdir=low,upperdir=up,workdir=work overlayfs merge
>>      ls -l merge/dir
>>
>>         ls: cannot access merge/dir/aa: No such file or directory
>>         total 0
>>         c????????? ? ? ? ?            ? aa
>>
>>   3. User clean lower dir and remount overlayfs (as you interested in).
>>
>>   If we use OVL_TYPE_COPYUP in readdir.c, we can find out 'suspect impure'
>>   directory in the third case only, we still can not make sure dir have
>>   whiteout or not. Especially in the first case, we even haven't any
>>   information to deduce it's 'suspect' or not. (Am I miss something ?)
>>
> 
> Of course it is *possible* to hand craft use cases 1 and 2, but so what?
> I am asking for *your* use case. You must have a reason to propose this
> purification that is not a user hand crafted impure lower/upper, don't you?
> So I am asking of for your use case, OVL_TYPE_COPYUP is enough.
> If you need a test branch to test if OVL_TYPE_COPYUP
> answers your needs, I can prepare one for you.
> 

  Hi Amir:

  Thanks for your valuable time.

  I just found this problem inadvertently when I did some tests.
  Indeed, cases 1 and 2 may not happen unless user hand crafted, and
  I don't have these use cases.:)

  I saw your patches pushed to overlayfs-constino.v5 last night and V3.
  We can use OVL_TYPE_COPYUP to figure out dir that has *ever* been copied up,
  and use the merged flow in ovl_iterate() for these directories.

  I have another two idears, please assess:

  1. I think we can use an xattr 'overlay.whiteout' to count the whiteouts
     the OVL_TYPE_COPYUP dir have. We increase count in ovl_remove_and_whiteout()
     and decrease count in ovl_create_over_whiteout(), so we can make sure
     the dir have created whiteout or not more accurate.(is it necessary ?)

  2. Can we read underlying dir entry and get rid of whiteout every time for
     the 'impure' dircetory, not execute the merged flow ? It will save
     memory usage.

Thanks,
ZhangYi

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[Amir Goldstein]

On Thu, May 4, 2017 at 11:36 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:

>   Hi Amir:
>
>   Thanks for your valuable time.
>
>   I just found this problem inadvertently when I did some tests.
>   Indeed, cases 1 and 2 may not happen unless user hand crafted, and
>   I don't have these use cases.:)

That's what I thought.

>
>   I saw your patches pushed to overlayfs-constino.v5 last night and V3.
>   We can use OVL_TYPE_COPYUP to figure out dir that has *ever* been copied up,
>   and use the merged flow in ovl_iterate() for these directories.
>

Yes, but keep in mind that OVL_TYPE_COPYUP from v3 did not make it into v5
and it's not trivial to add it back.

After I rebase my OVL_TYPE_COPYUP patches over v5 I'll make the patch
to replace is_real = !OVL_TYPE_MERGE() with !OVL_TYPE_COPYUP()
and let you know so you can test it.

>   I have another two ideas, please assess:
>
>   1. I think we can use an xattr 'overlay.whiteout' to count the whiteouts
>      the OVL_TYPE_COPYUP dir have. We increase count in ovl_remove_and_whiteout()
>      and decrease count in ovl_create_over_whiteout(), so we can make sure
>      the dir have created whiteout or not more accurate.(is it necessary ?)

It's not going to be easy to update overlay.whiteout 'atomically',
because you need to update it either before or after creating/removing
whiteout and both possibilities can get it out of sync.

A simpler optimization would be to set an 'overlay.impure' xattr on a directory
*before* creating the first whiteout AND before creating/moving a COPYUP object
into the dir (so only creating uppers leave the upper dir pure).

So there is no process of purifying a dir, but an upper dir is pure until proven
otherwise by ovl_set_dir_impure() (or by finding 'overlay.impure').

With that optimization, we can set:
is_real = !OVL_TYPE_MERGE() && !ovl_dir_impure();

very similar to your suggestion (but with a twist)

For my use case (lower dirs can disappear underneath) this sort of
optimization can be useful.

>
>   2. Can we read underlying dir entry and get rid of whiteout every time for
>      the 'impure' dircetory, not execute the merged flow ? It will save
>      memory usage.
>

Did not understand this questions.

Cheers,
Amir.

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[zhangyi (F)]

On 2017/5/4 17:03, Amir Goldstein wrote:
> On Thu, May 4, 2017 at 11:36 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>>   2. Can we read underlying dir entry and get rid of whiteout every time for
>>      the 'impure' dircetory, not execute the merged flow ? It will save
>>      memory usage.
>>
> Did not understand this questions.
> 

Sorry, I didn't make it clear. I mean if we set is_real = !OVL_TYPE_COPYUP()
or is_real = !OVL_TYPE_MERGE() && !ovl_dir_impure() (with optimization),
it will create ovl_dir_cache and ovl_cache_entry when we read COPYUP directory
or 'impure' dircetory in ovl_iterate() flow (as merged dir does). This will
cause bloated memory usage and some performance overhead, because this dir
is only exist in one layer(not merged), so I think it's don't need ovl_dir_cache.
(cost too much ?)

Can we find a way to avoid cache use, I find two ways(maybe too complex):
1. Make a wapper ovl_iterate_dir_impure(), it read the underlaying dir entry
   and get rid of whiteout entry, and use this function to deal the COPYUP or
   or 'impure' branch in ovl_iterate() like follow example:

static int ovl_iterate(struct file *file, struct dir_context *ctx)

     	if (od->is_real)
		return iterate_dir(od->realfile, ctx);
+	else if (!OVL_TYPE_MERGE && COPYUP)
+		return ovl_iterate_dir_impure(od->realfile, ctx);

2. Add a flag in struct dir_context, let underlying filesystem to return what we
   want to read, like follow example:

#define DIR_CONTEXT_HIDE_WHITEOUT  (1 << 0)

struct dir_context {
	const filldir_t actor;
+	unsigned int flag;
	loff_t pos;
};

static int ovl_iterate(struct file *file, struct dir_context *ctx)

	if (od->is_real)   /* is_real = !OVL_TYPE_MERGE() */
		return iterate_dir(od->realfile, ctx);

And then let underlying filesystem deal with DIR_CONTEXT_HIDE_WHITEOUT
flag. If take on this solution, we can cover all cases, OVL_TYPE_COPYUP
and pure check are also not required. It changes too greate, but has
a good expansibility (flag can be used by other demand in the future).

Thanks,
ZhangYi.

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[zhangyi (F)]

on 2017/5/4 20:10, zhangyi (F) wrote:
> On 2017/5/4 17:03, Amir Goldstein wrote:
>> On Thu, May 4, 2017 at 11:36 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>>   2. Can we read underlying dir entry and get rid of whiteout every time for
>>>      the 'impure' dircetory, not execute the merged flow ? It will save
>>>      memory usage.
>>>
>> Did not understand this questions.
>>
> 
> Sorry, I didn't make it clear. I mean if we set is_real = !OVL_TYPE_COPYUP()
> or is_real = !OVL_TYPE_MERGE() && !ovl_dir_impure() (with optimization),
> it will create ovl_dir_cache and ovl_cache_entry when we read COPYUP directory
> or 'impure' dircetory in ovl_iterate() flow (as merged dir does). This will
> cause bloated memory usage and some performance overhead, because this dir
> is only exist in one layer(not merged), so I think it's don't need ovl_dir_cache.
> (cost too much ?)
> 
> Can we find a way to avoid cache use, I find two ways(maybe too complex):
> 1. Make a wapper ovl_iterate_dir_impure(), it read the underlaying dir entry
>    and get rid of whiteout entry, and use this function to deal the COPYUP or
>    or 'impure' branch in ovl_iterate() like follow example:
> 
> static int ovl_iterate(struct file *file, struct dir_context *ctx)
> 
>      	if (od->is_real)
> 		return iterate_dir(od->realfile, ctx);
> +	else if (!OVL_TYPE_MERGE && COPYUP)
> +		return ovl_iterate_dir_impure(od->realfile, ctx);
> 
> 2. Add a flag in struct dir_context, let underlying filesystem to return what we
>    want to read, like follow example:
> 
> #define DIR_CONTEXT_HIDE_WHITEOUT  (1 << 0)
> 
> struct dir_context {
> 	const filldir_t actor;
> +	unsigned int flag;
> 	loff_t pos;
> };
> 
> static int ovl_iterate(struct file *file, struct dir_context *ctx)
> 

Sorry, I forgot add:

	ctx->flag |= DIR_CONTEXT_HIDE_WHITEOUT;

> 	if (od->is_real)   /* is_real = !OVL_TYPE_MERGE() */
> 		return iterate_dir(od->realfile, ctx);
> 
> And then let underlying filesystem deal with DIR_CONTEXT_HIDE_WHITEOUT
> flag. If take on this solution, we can cover all cases, OVL_TYPE_COPYUP
> and pure check are also not required. It changes too greate, but has
> a good expansibility (flag can be used by other demand in the future).
> 

Thanks,
ZhangYi.

Re: [PATCH] ovl: fix visible whiteout on not merged dir

[Amir Goldstein]

On Thu, May 4, 2017 at 3:10 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/5/4 17:03, Amir Goldstein wrote:
>> On Thu, May 4, 2017 at 11:36 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>>   2. Can we read underlying dir entry and get rid of whiteout every time for
>>>      the 'impure' dircetory, not execute the merged flow ? It will save
>>>      memory usage.
>>>
>> Did not understand this questions.
>>
>
> Sorry, I didn't make it clear. I mean if we set is_real = !OVL_TYPE_COPYUP()
> or is_real = !OVL_TYPE_MERGE() && !ovl_dir_impure() (with optimization),
> it will create ovl_dir_cache and ovl_cache_entry when we read COPYUP directory
> or 'impure' dircetory in ovl_iterate() flow (as merged dir does). This will
> cause bloated memory usage and some performance overhead, because this dir
> is only exist in one layer(not merged), so I think it's don't need ovl_dir_cache.
> (cost too much ?)

Does it though? cache is released when dir fd is closed.

>
> Can we find a way to avoid cache use, I find two ways(maybe too complex):
> 1. Make a wapper ovl_iterate_dir_impure(), it read the underlaying dir entry
>    and get rid of whiteout entry, and use this function to deal the COPYUP or
>    or 'impure' branch in ovl_iterate() like follow example:
>

This is interesting. It does cost a bit more to iterate with whiteout filter
like this because it requires getattr() while iterating char devices, but that's
not so bad I guess.
Filtering COPYUP like this is going to be more expensive.

> static int ovl_iterate(struct file *file, struct dir_context *ctx)
>
>         if (od->is_real)
>                 return iterate_dir(od->realfile, ctx);
> +       else if (!OVL_TYPE_MERGE && COPYUP)
> +               return ovl_iterate_dir_impure(od->realfile, ctx);
>
> 2. Add a flag in struct dir_context, let underlying filesystem to return what we
>    want to read, like follow example:
>
> #define DIR_CONTEXT_HIDE_WHITEOUT  (1 << 0)
>
> struct dir_context {
>         const filldir_t actor;
> +       unsigned int flag;
>         loff_t pos;
> };
>
> static int ovl_iterate(struct file *file, struct dir_context *ctx)
>
>         if (od->is_real)   /* is_real = !OVL_TYPE_MERGE() */
>                 return iterate_dir(od->realfile, ctx);
>
> And then let underlying filesystem deal with DIR_CONTEXT_HIDE_WHITEOUT
> flag. If take on this solution, we can cover all cases, OVL_TYPE_COPYUP
> and pure check are also not required. It changes too greate, but has
> a good expansibility (flag can be used by other demand in the future).
>

I wish filesystems would be so pleased to make these changes for overlayfs
it's less likely that you can get far with this idea ;-)

Amir.