[PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: rework how shiftfs opens files

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: rework how shiftfs opens files

Christian Brauner-3
From: Christian Brauner <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1846265

This commit simplifies how shiftfs open files, both regular files an
directories.

In the first iteration, we implemented a kmem cache for struct
shiftfs_file_info which stashed away a struct path and the struct file
for the underlay. The path however was never used anywhere so the struct
shiftfs_file_info and therefore the whole kmem cache can go away.
Instead we move to the same model as overlayfs and just stash away the
struct file for the underlay in file->private_data of the shiftfs struct
file.
Addtionally, we split the .open method for files and directories.
Similar to overlayfs .open for regular files uses open_with_fake_path()
which ensures that it doesn't contribute to the open file count (since
this would mean we'd count double). The .open method for directories
however used dentry_open() which contributes to the open file count.

The basic logic for opening files is unchanged. The main point is to
ensure that a reference to the underlay's dentry is kept through struct
path.

Various bits and pieces of this were cooked up in discussions Seth and I
had in Paris.

Signed-off-by: Christian Brauner <[hidden email]>
---
 fs/shiftfs.c | 105 +++++++++++++++++++++++----------------------------
 1 file changed, 47 insertions(+), 58 deletions(-)

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index a21cb473e000..55bb32b611f2 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -31,13 +31,6 @@ struct shiftfs_super_info {
  unsigned int passthrough_mark;
 };
 
-struct shiftfs_file_info {
- struct path realpath;
- struct file *realfile;
-};
-
-struct kmem_cache *shiftfs_file_info_cache;
-
 static void shiftfs_fill_inode(struct inode *inode, unsigned long ino,
        umode_t mode, dev_t dev, struct dentry *dentry);
 
@@ -1042,21 +1035,21 @@ static const struct inode_operations shiftfs_symlink_inode_operations = {
 };
 
 static struct file *shiftfs_open_realfile(const struct file *file,
-  struct path *realpath)
+  struct inode *realinode)
 {
- struct file *lowerf;
- const struct cred *oldcred;
+ struct file *realfile;
+ const struct cred *old_cred;
  struct inode *inode = file_inode(file);
- struct inode *loweri = realpath->dentry->d_inode;
+ struct dentry *lowerd = file->f_path.dentry->d_fsdata;
  struct shiftfs_super_info *info = inode->i_sb->s_fs_info;
+ struct path realpath = { .mnt = info->mnt, .dentry = lowerd };
 
- oldcred = shiftfs_override_creds(inode->i_sb);
- /* XXX: open_with_fake_path() not gauranteed to stay around, if
- * removed use dentry_open() */
- lowerf = open_with_fake_path(realpath, file->f_flags, loweri, info->creator_cred);
- revert_creds(oldcred);
+ old_cred = shiftfs_override_creds(inode->i_sb);
+ realfile = open_with_fake_path(&realpath, file->f_flags, realinode,
+       info->creator_cred);
+ revert_creds(old_cred);
 
- return lowerf;
+ return realfile;
 }
 
 #define SHIFTFS_SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT)
@@ -1096,8 +1089,7 @@ static int shiftfs_change_flags(struct file *file, unsigned int flags)
 
 static int shiftfs_real_fdget(const struct file *file, struct fd *lowerfd)
 {
- struct shiftfs_file_info *file_info = file->private_data;
- struct file *realfile = file_info->realfile;
+ struct file *realfile = file->private_data;
 
  lowerfd->flags = 0;
  lowerfd->file = realfile;
@@ -1111,51 +1103,57 @@ static int shiftfs_real_fdget(const struct file *file, struct fd *lowerfd)
 
 static int shiftfs_open(struct inode *inode, struct file *file)
 {
- struct shiftfs_super_info *ssi = inode->i_sb->s_fs_info;
- struct shiftfs_file_info *file_info;
  struct file *realfile;
- struct path *realpath;
 
- file_info = kmem_cache_zalloc(shiftfs_file_info_cache, GFP_KERNEL);
- if (!file_info)
- return -ENOMEM;
-
- realpath = &file_info->realpath;
- realpath->mnt = ssi->mnt;
- realpath->dentry = file->f_path.dentry->d_fsdata;
-
- realfile = shiftfs_open_realfile(file, realpath);
- if (IS_ERR(realfile)) {
- kmem_cache_free(shiftfs_file_info_cache, file_info);
+ realfile = shiftfs_open_realfile(file, inode->i_private);
+ if (IS_ERR(realfile))
  return PTR_ERR(realfile);
- }
 
- file->private_data = file_info;
+ file->private_data = realfile;
  /* For O_DIRECT dentry_open() checks f_mapping->a_ops->direct_IO. */
  file->f_mapping = realfile->f_mapping;
 
- file_info->realfile = realfile;
  return 0;
 }
 
-static int shiftfs_release(struct inode *inode, struct file *file)
+static int shiftfs_dir_open(struct inode *inode, struct file *file)
 {
- struct shiftfs_file_info *file_info = file->private_data;
+ struct file *realfile;
+ const struct cred *oldcred;
+ struct dentry *lowerd = file->f_path.dentry->d_fsdata;
+ struct shiftfs_super_info *info = inode->i_sb->s_fs_info;
+ struct path realpath = { .mnt = info->mnt, .dentry = lowerd };
+
+ oldcred = shiftfs_override_creds(file->f_path.dentry->d_sb);
+ realfile = dentry_open(&realpath, file->f_flags | O_NOATIME,
+       info->creator_cred);
+ revert_creds(oldcred);
+ if (IS_ERR(realfile))
+ return PTR_ERR(realfile);
 
- if (file_info) {
- if (file_info->realfile)
- fput(file_info->realfile);
+ file->private_data = realfile;
 
- kmem_cache_free(shiftfs_file_info_cache, file_info);
- }
+ return 0;
+}
+
+static int shiftfs_release(struct inode *inode, struct file *file)
+{
+ struct file *realfile = file->private_data;
+
+ if (realfile)
+ fput(realfile);
 
  return 0;
 }
 
+static int shiftfs_dir_release(struct inode *inode, struct file *file)
+{
+ return shiftfs_release(inode, file);
+}
+
 static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence)
 {
- struct shiftfs_file_info *file_info = file->private_data;
- struct file *realfile = file_info->realfile;
+ struct file *realfile = file->private_data;
 
  return vfs_llseek(realfile, offset, whence);
 }
@@ -1274,8 +1272,7 @@ static int shiftfs_fsync(struct file *file, loff_t start, loff_t end,
 
 static int shiftfs_mmap(struct file *file, struct vm_area_struct *vma)
 {
- struct shiftfs_file_info *file_info = file->private_data;
- struct file *realfile = file_info->realfile;
+ struct file *realfile = file->private_data;
  const struct cred *oldcred;
  int ret;
 
@@ -1671,8 +1668,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx)
 {
  const struct cred *oldcred;
  int err = -ENOTDIR;
- struct shiftfs_file_info *file_info = file->private_data;
- struct file *realfile = file_info->realfile;
+ struct file *realfile = file->private_data;
 
  oldcred = shiftfs_override_creds(file->f_path.dentry->d_sb);
  err = iterate_dir(realfile, ctx);
@@ -1698,13 +1694,13 @@ const struct file_operations shiftfs_file_operations = {
 };
 
 const struct file_operations shiftfs_dir_operations = {
+ .open = shiftfs_dir_open,
+ .release = shiftfs_dir_release,
  .compat_ioctl = shiftfs_compat_ioctl,
  .fsync = shiftfs_fsync,
  .iterate_shared = shiftfs_iterate_shared,
  .llseek = shiftfs_dir_llseek,
- .open = shiftfs_open,
  .read = generic_read_dir,
- .release = shiftfs_release,
  .unlocked_ioctl = shiftfs_ioctl,
 };
 
@@ -2106,19 +2102,12 @@ static struct file_system_type shiftfs_type = {
 
 static int __init shiftfs_init(void)
 {
- shiftfs_file_info_cache = kmem_cache_create(
- "shiftfs_file_info_cache", sizeof(struct shiftfs_file_info), 0,
- SLAB_RECLAIM_ACCOUNT | SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT | SLAB_MEM_SPREAD, NULL);
- if (!shiftfs_file_info_cache)
- return -ENOMEM;
-
  return register_filesystem(&shiftfs_type);
 }
 
 static void __exit shiftfs_exit(void)
 {
  unregister_filesystem(&shiftfs_type);
- kmem_cache_destroy(shiftfs_file_info_cache);
 }
 
 MODULE_ALIAS_FS("shiftfs");
--
2.23.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2][SRU][Disco] UBUNTU: SAUCE: overlayfs: allow with shiftfs as underlay

Christian Brauner-3
BugLink: https://bugs.launchpad.net/bugs/1846272

In commit [1] we enabled overlayfs on top of shiftfs. This approach was
buggy since it let to a regression for some standard overlayfs workloads
(cf. [2]).
In our original approach in [1] Seth and I concluded that running
overlayfs on top of shiftfs was not possible because of the way
overlayfs is currently opening files. The fact that it did not pass down
the dentry of shiftfs but rather it's own caused shiftfs to be confused
since it stashes away necessary information in d_fsdata.
Our solution was to modify open_with_fake_path() to also take a dentry
as an argument, then change overlayfs to pass in the shiftfs dentry
which then would override the dentry in the passed in struct path in
open_with_fake_path().
However, this led to a regression for some standard overlayfs workloads
(cf. [2]).
After various discussions involving Seth and myself in Paris we
concluded the reason for the regression was that we effectively created
a struct path that was comprised of the vfsmount of the overlayfs dentry
and the dentry of shiftfs. This is obviously broken.
The fix is to a) not modify open_with_fake_path() and b) change
overlayfs to do what shiftfs is doing, namely correctly setup the struct
path such that vfsmount and dentry match and are both from shiftfs.
Note, that overlayfs already does this for the .open method for
directories. It just did not do it for the .open method for regular
files leading to this issue. The reason why this hasn't been a problem
for overlayfs so far is that it didn't allow running on top of
filesystems that make use of d_fsdata _implicitly_ by disallowing any
filesystem that is itself an overlay, or has revalidate methods for it's
dentries as those usually have d_fsdata set up. Any other filesystem
falling in this category would have suffered from the same problem.

Seth managed to trigger the regression with the following script:
 #!/bin/bash

 utils=(bash cat)

 mkdir -p lower/proc upper work root
 for util in ${utils[@]}; do
  path="$(which $util)"
  dir="$(dirname $path)"
  mkdir -p "lower/$dir"
  cp -v "$path" "lower/$path"
  libs="$(ldd $path | egrep -o '(/usr)?/lib.*\.[0-9]')"
  for lib in $libs; do
  dir="$(dirname $lib)"
  mkdir -p "lower/$dir"
  cp -v "$lib" "lower/$lib"
  done
 done

 mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work nodev root
 mount -t proc nodev root/proc
 chroot root bash -c "cat /proc/self/maps"
 umount root/proc
 umount root

With the patch here applied the regression is not reproducible.

/* References */
[1]: 37430e430a14 ("UBUNTU: SAUCE: shiftfs: enable overlayfs on shiftfs")
[2]: https://bugs.launchpad.net/bugs/1838677

Signed-off-by: Christian Brauner <[hidden email]>
---
 fs/overlayfs/file.c  | 4 +++-
 fs/overlayfs/super.c | 5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 75d8d00fa087..71bf12eb70e6 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -27,13 +27,15 @@ static char ovl_whatisit(struct inode *inode, struct inode *realinode)
 static struct file *ovl_open_realfile(const struct file *file,
       struct inode *realinode)
 {
+ struct path realpath;
  struct inode *inode = file_inode(file);
  struct file *realfile;
  const struct cred *old_cred;
  int flags = file->f_flags | O_NOATIME | FMODE_NONOTIFY;
 
  old_cred = ovl_override_creds(inode->i_sb);
- realfile = open_with_fake_path(&file->f_path, flags, realinode,
+ ovl_path_real(file->f_path.dentry, &realpath);
+ realfile = open_with_fake_path(&realpath, flags, realinode,
        current_cred());
  revert_creds(old_cred);
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3bd48da819c2..b4fb0c402216 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -753,13 +753,14 @@ static int ovl_mount_dir(const char *name, struct path *path)
  ovl_unescape(tmp);
  err = ovl_mount_dir_noesc(tmp, path);
 
- if (!err)
- if (ovl_dentry_remote(path->dentry)) {
+ if (!err) {
+ if ((path->dentry->d_sb->s_magic != SHIFTFS_MAGIC) && ovl_dentry_remote(path->dentry)) {
  pr_err("overlayfs: filesystem on '%s' not supported as upperdir\n",
        tmp);
  path_put_init(path);
  err = -EINVAL;
  }
+ }
  kfree(tmp);
  }
  return err;
--
2.23.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: rework how shiftfs opens files

Stefan Bader-2
In reply to this post by Christian Brauner-3
On 01.10.19 22:42, Christian Brauner wrote:

> From: Christian Brauner <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1846265
>
> This commit simplifies how shiftfs open files, both regular files an
> directories.
>
> In the first iteration, we implemented a kmem cache for struct
> shiftfs_file_info which stashed away a struct path and the struct file
> for the underlay. The path however was never used anywhere so the struct
> shiftfs_file_info and therefore the whole kmem cache can go away.
> Instead we move to the same model as overlayfs and just stash away the
> struct file for the underlay in file->private_data of the shiftfs struct
> file.
> Addtionally, we split the .open method for files and directories.
> Similar to overlayfs .open for regular files uses open_with_fake_path()
> which ensures that it doesn't contribute to the open file count (since
> this would mean we'd count double). The .open method for directories
> however used dentry_open() which contributes to the open file count.
>
> The basic logic for opening files is unchanged. The main point is to
> ensure that a reference to the underlay's dentry is kept through struct
> path.
>
> Various bits and pieces of this were cooked up in discussions Seth and I
> had in Paris.
>
> Signed-off-by: Christian Brauner <[hidden email]>
> ---
This submission (and if it were 2 patches for the same reason I would like to
see a cover letter) mixes two different bug reports. And the first patch does
not really sound like a bug fix to me. So while it might be different as a late
addition for Eoan, I do not see the reason for SRU in Disco.

-Stefan

>  fs/shiftfs.c | 105 +++++++++++++++++++++++----------------------------
>  1 file changed, 47 insertions(+), 58 deletions(-)
>
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index a21cb473e000..55bb32b611f2 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -31,13 +31,6 @@ struct shiftfs_super_info {
>   unsigned int passthrough_mark;
>  };
>  
> -struct shiftfs_file_info {
> - struct path realpath;
> - struct file *realfile;
> -};
> -
> -struct kmem_cache *shiftfs_file_info_cache;
> -
>  static void shiftfs_fill_inode(struct inode *inode, unsigned long ino,
>         umode_t mode, dev_t dev, struct dentry *dentry);
>  
> @@ -1042,21 +1035,21 @@ static const struct inode_operations shiftfs_symlink_inode_operations = {
>  };
>  
>  static struct file *shiftfs_open_realfile(const struct file *file,
> -  struct path *realpath)
> +  struct inode *realinode)
>  {
> - struct file *lowerf;
> - const struct cred *oldcred;
> + struct file *realfile;
> + const struct cred *old_cred;
>   struct inode *inode = file_inode(file);
> - struct inode *loweri = realpath->dentry->d_inode;
> + struct dentry *lowerd = file->f_path.dentry->d_fsdata;
>   struct shiftfs_super_info *info = inode->i_sb->s_fs_info;
> + struct path realpath = { .mnt = info->mnt, .dentry = lowerd };
>  
> - oldcred = shiftfs_override_creds(inode->i_sb);
> - /* XXX: open_with_fake_path() not gauranteed to stay around, if
> - * removed use dentry_open() */
> - lowerf = open_with_fake_path(realpath, file->f_flags, loweri, info->creator_cred);
> - revert_creds(oldcred);
> + old_cred = shiftfs_override_creds(inode->i_sb);
> + realfile = open_with_fake_path(&realpath, file->f_flags, realinode,
> +       info->creator_cred);
> + revert_creds(old_cred);
>  
> - return lowerf;
> + return realfile;
>  }
>  
>  #define SHIFTFS_SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT)
> @@ -1096,8 +1089,7 @@ static int shiftfs_change_flags(struct file *file, unsigned int flags)
>  
>  static int shiftfs_real_fdget(const struct file *file, struct fd *lowerfd)
>  {
> - struct shiftfs_file_info *file_info = file->private_data;
> - struct file *realfile = file_info->realfile;
> + struct file *realfile = file->private_data;
>  
>   lowerfd->flags = 0;
>   lowerfd->file = realfile;
> @@ -1111,51 +1103,57 @@ static int shiftfs_real_fdget(const struct file *file, struct fd *lowerfd)
>  
>  static int shiftfs_open(struct inode *inode, struct file *file)
>  {
> - struct shiftfs_super_info *ssi = inode->i_sb->s_fs_info;
> - struct shiftfs_file_info *file_info;
>   struct file *realfile;
> - struct path *realpath;
>  
> - file_info = kmem_cache_zalloc(shiftfs_file_info_cache, GFP_KERNEL);
> - if (!file_info)
> - return -ENOMEM;
> -
> - realpath = &file_info->realpath;
> - realpath->mnt = ssi->mnt;
> - realpath->dentry = file->f_path.dentry->d_fsdata;
> -
> - realfile = shiftfs_open_realfile(file, realpath);
> - if (IS_ERR(realfile)) {
> - kmem_cache_free(shiftfs_file_info_cache, file_info);
> + realfile = shiftfs_open_realfile(file, inode->i_private);
> + if (IS_ERR(realfile))
>   return PTR_ERR(realfile);
> - }
>  
> - file->private_data = file_info;
> + file->private_data = realfile;
>   /* For O_DIRECT dentry_open() checks f_mapping->a_ops->direct_IO. */
>   file->f_mapping = realfile->f_mapping;
>  
> - file_info->realfile = realfile;
>   return 0;
>  }
>  
> -static int shiftfs_release(struct inode *inode, struct file *file)
> +static int shiftfs_dir_open(struct inode *inode, struct file *file)
>  {
> - struct shiftfs_file_info *file_info = file->private_data;
> + struct file *realfile;
> + const struct cred *oldcred;
> + struct dentry *lowerd = file->f_path.dentry->d_fsdata;
> + struct shiftfs_super_info *info = inode->i_sb->s_fs_info;
> + struct path realpath = { .mnt = info->mnt, .dentry = lowerd };
> +
> + oldcred = shiftfs_override_creds(file->f_path.dentry->d_sb);
> + realfile = dentry_open(&realpath, file->f_flags | O_NOATIME,
> +       info->creator_cred);
> + revert_creds(oldcred);
> + if (IS_ERR(realfile))
> + return PTR_ERR(realfile);
>  
> - if (file_info) {
> - if (file_info->realfile)
> - fput(file_info->realfile);
> + file->private_data = realfile;
>  
> - kmem_cache_free(shiftfs_file_info_cache, file_info);
> - }
> + return 0;
> +}
> +
> +static int shiftfs_release(struct inode *inode, struct file *file)
> +{
> + struct file *realfile = file->private_data;
> +
> + if (realfile)
> + fput(realfile);
>  
>   return 0;
>  }
>  
> +static int shiftfs_dir_release(struct inode *inode, struct file *file)
> +{
> + return shiftfs_release(inode, file);
> +}
> +
>  static loff_t shiftfs_dir_llseek(struct file *file, loff_t offset, int whence)
>  {
> - struct shiftfs_file_info *file_info = file->private_data;
> - struct file *realfile = file_info->realfile;
> + struct file *realfile = file->private_data;
>  
>   return vfs_llseek(realfile, offset, whence);
>  }
> @@ -1274,8 +1272,7 @@ static int shiftfs_fsync(struct file *file, loff_t start, loff_t end,
>  
>  static int shiftfs_mmap(struct file *file, struct vm_area_struct *vma)
>  {
> - struct shiftfs_file_info *file_info = file->private_data;
> - struct file *realfile = file_info->realfile;
> + struct file *realfile = file->private_data;
>   const struct cred *oldcred;
>   int ret;
>  
> @@ -1671,8 +1668,7 @@ static int shiftfs_iterate_shared(struct file *file, struct dir_context *ctx)
>  {
>   const struct cred *oldcred;
>   int err = -ENOTDIR;
> - struct shiftfs_file_info *file_info = file->private_data;
> - struct file *realfile = file_info->realfile;
> + struct file *realfile = file->private_data;
>  
>   oldcred = shiftfs_override_creds(file->f_path.dentry->d_sb);
>   err = iterate_dir(realfile, ctx);
> @@ -1698,13 +1694,13 @@ const struct file_operations shiftfs_file_operations = {
>  };
>  
>  const struct file_operations shiftfs_dir_operations = {
> + .open = shiftfs_dir_open,
> + .release = shiftfs_dir_release,
>   .compat_ioctl = shiftfs_compat_ioctl,
>   .fsync = shiftfs_fsync,
>   .iterate_shared = shiftfs_iterate_shared,
>   .llseek = shiftfs_dir_llseek,
> - .open = shiftfs_open,
>   .read = generic_read_dir,
> - .release = shiftfs_release,
>   .unlocked_ioctl = shiftfs_ioctl,
>  };
>  
> @@ -2106,19 +2102,12 @@ static struct file_system_type shiftfs_type = {
>  
>  static int __init shiftfs_init(void)
>  {
> - shiftfs_file_info_cache = kmem_cache_create(
> - "shiftfs_file_info_cache", sizeof(struct shiftfs_file_info), 0,
> - SLAB_RECLAIM_ACCOUNT | SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT | SLAB_MEM_SPREAD, NULL);
> - if (!shiftfs_file_info_cache)
> - return -ENOMEM;
> -
>   return register_filesystem(&shiftfs_type);
>  }
>  
>  static void __exit shiftfs_exit(void)
>  {
>   unregister_filesystem(&shiftfs_type);
> - kmem_cache_destroy(shiftfs_file_info_cache);
>  }
>  
>  MODULE_ALIAS_FS("shiftfs");
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment