Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1520513 > unrolled thread

Re: [PATCH 3/3] ovl: redirect on rename-dir

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] ovl: redirect on rename-dir Amir Goldstein <amir73il@gmail.com> - 2016-11-13 11:10 +0100
    Re: [PATCH 3/3] ovl: redirect on rename-dir Amir Goldstein <amir73il@gmail.com> - 2016-11-14 17:30 +0100
      Re: [PATCH 3/3] ovl: redirect on rename-dir Miklos Szeredi <miklos@szeredi.hu> - 2016-11-16 23:10 +0100
        Re: [PATCH 3/3] ovl: redirect on rename-dir Amir Goldstein <amir73il@gmail.com> - 2016-11-18 16:40 +0100
          Re: [PATCH 3/3] ovl: redirect on rename-dir Amir Goldstein <amir73il@gmail.com> - 2016-11-20 13:00 +0100
          Re: [PATCH 3/3] ovl: redirect on rename-dir Miklos Szeredi <miklos@szeredi.hu> - 2016-11-21 11:00 +0100
            Re: [PATCH 3/3] ovl: redirect on rename-dir Amir Goldstein <amir73il@gmail.com> - 2016-11-21 11:20 +0100
              Re: [PATCH 3/3] ovl: redirect on rename-dir Miklos Szeredi <mszeredi@redhat.com> - 2016-11-21 11:20 +0100
                Re: [PATCH 3/3] ovl: redirect on rename-dir Amir Goldstein <amir73il@gmail.com> - 2016-11-22 14:50 +0100

#1520513 — Re: [PATCH 3/3] ovl: redirect on rename-dir

On Fri, Nov 11, 2016 at 12:39 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> New version is at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #redirect
>
> News:
> - it actually should work in all cases
> - when rename is not cross directory, just store the new name instead
> of a full path, as suggested by Amir
> - when redirect path is too long fall back to EXDEV (the max length
> should probably be a module param)
>

Looks goods, except for the case of change from relative to absolute
redirect of the victim dentry. IIUC, ovl_set_redirect() will return immediately
because ovl_dentry_is_redirect() and will not get to setting the absolute
redirect.

It passed my sanity tests (including recycle test) and on top of my copy up
lock changes.

You can add Reviewed-by/Tested-by me to
  1380846 ovl: redirect on rename-dir

> About turning the feature on or off.  Yes, maybe the empty checking is
> too complicated.  Going one simpler:
>
>  - default to old behavior, turn on with mount option
>  - add module option and kernel compile option to turn on the feature by default
>

I don't see these changes on your branch.
Are these your plans for a future version?

Thanks,
Amir.

[toc] | [next] | [standalone]

#1521824

On Sun, Nov 13, 2016 at 12:00 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Nov 11, 2016 at 12:39 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> New version is at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #redirect
>>
>> News:
>> - it actually should work in all cases
>> - when rename is not cross directory, just store the new name instead
>> of a full path, as suggested by Amir
>> - when redirect path is too long fall back to EXDEV (the max length
>> should probably be a module param)
>>
>
> Looks goods, except for the case of change from relative to absolute
> redirect of the victim dentry. IIUC, ovl_set_redirect() will return immediately
> because ovl_dentry_is_redirect() and will not get to setting the absolute
> redirect.
>

I added some more tests to catch this problem at:
https://github.com/amir73il/unionmount-testsuite.git #ovl_rename_dir

In the new version, upper recycling is optional and you can set a bound
to the number of layers. This is needed to catch the bug because the
scenario is:
- Rename populated dir in same dir
- Move the populated dir to another dir
- Re-mount
- Populated dir is empty

The following test run demonstrates it:

$ sudo ./run --ov=0 rename-move-dir
...
TEST rename-move-dir.py:37: Rename populated dir and move into another
 ./run --rename /mnt/a/dir102 /mnt/a/dir102x
 ./run --rename /mnt/a/dir102 /mnt/a/dir102x -E ENOENT
 ./run --rename /mnt/a/dir102x /mnt/a/empty102/dir102
 ./run --rename /mnt/a/dir102x /mnt/a/empty102/dir102 -E ENOENT
 ./run --open-file /mnt/a/dir102 -r -d -E ENOENT
 ./run --open-file /mnt/a/dir102x -r -d -E ENOENT
 ./run --open-file /mnt/a/empty102/dir102 -r -d
/mnt/a/empty102/dir102/a: File is missing


Amir.

[toc] | [prev] | [next] | [standalone]

#1523879

On Mon, Nov 14, 2016 at 5:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Sun, Nov 13, 2016 at 12:00 PM, Amir Goldstein <amir73il@gmail.com> wrote:

>> Looks goods, except for the case of change from relative to absolute
>> redirect of the victim dentry. IIUC, ovl_set_redirect() will return immediately
>> because ovl_dentry_is_redirect() and will not get to setting the absolute
>> redirect.
>>
>
> I added some more tests to catch this problem at:
> https://github.com/amir73il/unionmount-testsuite.git #ovl_rename_dir

Thanks for testing.

Force pushed updated version to the usual place:

   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #redirect

This also has the xattr feature thing replaced with mount option,
module param and kernel config option.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1525468

On Thu, Nov 17, 2016 at 12:00 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Nov 14, 2016 at 5:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Sun, Nov 13, 2016 at 12:00 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
> >> Looks goods, except for the case of change from relative to absolute
> >> redirect of the victim dentry. IIUC, ovl_set_redirect() will return immediately
> >> because ovl_dentry_is_redirect() and will not get to setting the absolute
> >> redirect.
> >>
> >
> > I added some more tests to catch this problem at:
> > https://github.com/amir73il/unionmount-testsuite.git #ovl_rename_dir
>
> Thanks for testing.
>
> Force pushed updated version to the usual place:
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #redirect
>

Found one typo and one bug in error that can cause crash on dput(ERR_PTR(err)):

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 21ddac7..0daac51 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -15,7 +15,7 @@ config OVERLAY_FS_REDIRECT_DIR
        help
          If this config option is enabled then overlay filesystems will use
          redirects when renaming directories by default.  In this case it is
-         still possible possible to turn off redirects globally with the
+         still possible to turn off redirects globally with the
          "redirect_dir=off" module option or on a filesystem instance basis
          with the "redirect_dir=off" mount option.

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5eaa9f9..a19fc5c 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -105,11 +105,12 @@ static int ovl_lookup_single(struct dentry
*base, struct ovl_lookup_data *d,

        this = lookup_one_len_unlocked(name, base, namelen);
        if (IS_ERR(this)) {
-               if (PTR_ERR(this) == -ENOENT ||
-                   PTR_ERR(this) == -ENAMETOOLONG) {
+               err = PTR_ERR(this);
+               if (err == -ENOENT || err == -ENAMETOOLONG) {
                        this = NULL;
+                       goto out;
                }
-               goto out;
+               return err;
        }
        if (!this->d_inode)
                goto put_and_out;

> This also has the xattr feature thing replaced with mount option,
> module param and kernel config option.
>

I like the kernel config/module param/mount option for
enabling/disabling the feature.

But I still think that we should write the features xattr on the first
redirect rename.
The features xattr tell us what can be found on the layer, so we would
be wise to
keep it around for all sorts of backward compatibility aspect.

Amir.

[toc] | [prev] | [next] | [standalone]

#1526224

On Fri, Nov 18, 2016 at 5:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Nov 17, 2016 at 12:00 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> On Mon, Nov 14, 2016 at 5:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > On Sun, Nov 13, 2016 at 12:00 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> >> Looks goods, except for the case of change from relative to absolute
>> >> redirect of the victim dentry. IIUC, ovl_set_redirect() will return immediately
>> >> because ovl_dentry_is_redirect() and will not get to setting the absolute
>> >> redirect.
>> >>
>> >
>> > I added some more tests to catch this problem at:
>> > https://github.com/amir73il/unionmount-testsuite.git #ovl_rename_dir
>>
>> Thanks for testing.
>>
>> Force pushed updated version to the usual place:
>>
>>    git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #redirect
>>
>
> Found one typo and one bug in error that can cause crash on dput(ERR_PTR(err)):
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 21ddac7..0daac51 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -15,7 +15,7 @@ config OVERLAY_FS_REDIRECT_DIR
>         help
>           If this config option is enabled then overlay filesystems will use
>           redirects when renaming directories by default.  In this case it is
> -         still possible possible to turn off redirects globally with the
> +         still possible to turn off redirects globally with the
>           "redirect_dir=off" module option or on a filesystem instance basis
>           with the "redirect_dir=off" mount option.
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 5eaa9f9..a19fc5c 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -105,11 +105,12 @@ static int ovl_lookup_single(struct dentry
> *base, struct ovl_lookup_data *d,
>
>         this = lookup_one_len_unlocked(name, base, namelen);
>         if (IS_ERR(this)) {
> -               if (PTR_ERR(this) == -ENOENT ||
> -                   PTR_ERR(this) == -ENAMETOOLONG) {
> +               err = PTR_ERR(this);
> +               if (err == -ENOENT || err == -ENAMETOOLONG) {
>                         this = NULL;
> +                       goto out;
>                 }
> -               goto out;
> +               return err;
>         }
>         if (!this->d_inode)
>                 goto put_and_out;
>

I just realized that this bug is already in overlayfs-next, so posted
a patch to fix it.

>> This also has the xattr feature thing replaced with mount option,
>> module param and kernel config option.
>>
>
> I like the kernel config/module param/mount option for
> enabling/disabling the feature.
>
> But I still think that we should write the features xattr on the first
> redirect rename.
> The features xattr tell us what can be found on the layer, so we would
> be wise to
> keep it around for all sorts of backward compatibility aspect.
>
> Amir.

[toc] | [prev] | [next] | [standalone]

#1526495

On Fri, Nov 18, 2016 at 4:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> Found one typo and one bug in error that can cause crash on dput(ERR_PTR(err)):

Thanks.

Fixes force pushed to overlayfs-next.

Also pushed the redirect patches to overlayfs-next, as they seem to
have matured enough.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1526518

On Mon, Nov 21, 2016 at 11:54 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Nov 18, 2016 at 4:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> Found one typo and one bug in error that can cause crash on dput(ERR_PTR(err)):
>
> Thanks.
>
> Fixes force pushed to overlayfs-next.

All right. I had the (wrong) impression the next was not a rewindable branch.

>
> Also pushed the redirect patches to overlayfs-next, as they seem to
> have matured enough.
>

Agreed.

Amir.

[toc] | [prev] | [next] | [standalone]

#1526527

On Mon, Nov 21, 2016 at 11:13 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Nov 21, 2016 at 11:54 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Fri, Nov 18, 2016 at 4:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>>> Found one typo and one bug in error that can cause crash on dput(ERR_PTR(err)):
>>
>> Thanks.
>>
>> Fixes force pushed to overlayfs-next.
>
> All right. I had the (wrong) impression the next was not a rewindable branch.

That depends on how many devel branches are broken by such an action.
In case of overlayfs-next, there doesn't appear to be too much of
that, so for now I feel free to mess with it.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1527511

On Mon, Nov 21, 2016 at 12:16 PM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> On Mon, Nov 21, 2016 at 11:13 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Mon, Nov 21, 2016 at 11:54 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Fri, Nov 18, 2016 at 4:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>
>>>> Found one typo and one bug in error that can cause crash on dput(ERR_PTR(err)):
>>>
>>> Thanks.
>>>
>>> Fixes force pushed to overlayfs-next.
>>
>> All right. I had the (wrong) impression the next was not a rewindable branch.
>
> That depends on how many devel branches are broken by such an action.
> In case of overlayfs-next, there doesn't appear to be too much of
> that, so for now I feel free to mess with it.
>

All right. Just posted another minor fix to display redirect=xx on /proc/mounts
when it is due. Feel free to squash or apply or whatever.

*Strictly* FYI, here is something I have been working on, on top of
redirect_dir,
which I need for one of our use cases.
Since it is working and passed all the sanity tests, I am letting you all know
about it in case it's relevant for anyone else.
Not even going to post the patches yet, just a link and an abstract.

Amir.

https://github.com/amir73il/linux.git #redirect_fh

===============================
  ovl: redirect merged dir by file handle on copy up

When mounted with mount option redirect_dir=fh,
every copy up of lower directory stores the lower
dir file handle in redirect xattr of upper dir.

After the redirect at copy up, renaming upper merged
directory requires no further action.

This method has some advantages over absolute path redirect:
- it is more compact in stored xattr size
- it is not limited by lengths of full paths
- lookup redirect is more efficient for very nested directories

It also has some disadvantages over absolute path redirect:
- it requires setting the redirect xattr for all layers of
  merged dirs
- it requires that all lower layers are on the same file system,
  which support exportfs ops
- file handles will become stale if overlay lower directories
  where to be copied to another location

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web