[SRU][Xenial][PATCH v2 0/2]

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][PATCH v2 0/2]

Daniel Axtens
BugLink: https://bugs.launchpad.net/bugs/1728489

[SRU Justification]

[Impact]

A user is seeing failures from extracting tar archives on overlay
filesystems on the 4.4 kernel in constrained environments. The error
presents as:

`tar: ./deps/0/bin: Directory renamed before its status could be extracted`

Following this thread
(https://www.spinics.net/lists/linux-unionfs/msg00856.html), it appears
that this occurs when entries in the kernel's inode cache are
reclaimed, and subsequent lookups return new inode numbers.

Further testing showed that when setting
`/proc/sys/vm/vfs_cache_pressure` to 0 (don't allow the kernel to
reclaim inode cache entries due to memory pressure) the error does not
recur, supporting the hypothesis that cache entries are being
evicted. However, this setting may lead to a kernel OOM so is not a
reasonable workaround even temporarily.

The error cannot be reproduced on a 4.13 kernel, due to the series at
https://www.spinics.net/lists/linux-fsdevel/msg110235.html. The
particular relevant commit is
b7a807dc2010334e62e0afd89d6f7a8913eb14ff, which needs a dependency.

[Fix]

Backport the key commit and the dependency.

[Testcase]

# Testing this bug

The testcase for this particular bug is simple - create an overlay
filesystem with all layers on the same underlying file system, and
then see if the inode of a directory is constant across dropping the
caches:

mkdir -p /upper/upper /upper/work /lower
mount -t overlay none /mnt -o lowerdir=/lower,upperdir=/upper/upper,workdir=/upper/work
cd /mnt
mkdir a
stat a # observe inode number
echo 2 > /proc/sys/vm/drop_caches
stat a # compare inode number

If the inode number is the same, the fix is successful.

# Regression testing

I have run the unionmount test suite from
https://git.infradead.org/users/dhowells/unionmount-testsuite.git in
overlay mode (./run --ov), and verified that it still passes.

(The series cover letter mentions a fork of the test suite at
https://github.com/amir73il/unionmount-testsuite/commits/overlayfs-devel. I
have *not* attempted to get this running: it assumes a range of
changes that are not present in our kernels.)

[Regression Potential]

As this changes overlayfs, there is potential for regression in the
form of unexpected breakages to overlaysfs behaviour.

I think this is adequately addressed by the regression testing.

[Notes]
v2: drop patches that are not strpctly required
    minor change to patch 2 to not rely on ORIGIN type, as in Xenial

Amir Goldstein (2):
  ovl: check if all layers are on the same fs
  ovl: persistent inode number for directories

 fs/overlayfs/dir.c       | 36 ++++++++++++++++++++++++++++++++----
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/ovl_entry.h |  2 ++
 fs/overlayfs/super.c     |  8 ++++++++
 fs/overlayfs/util.c      |  7 +++++++
 5 files changed, 50 insertions(+), 4 deletions(-)

--
2.11.0


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

[SRU][Xenial][PATCH v2 1/2] ovl: check if all layers are on the same fs

Daniel Axtens
From: Amir Goldstein <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1728489

Some features can only work when all layers are on the same fs.  Test this
condition during mount time, so features can check them later.

Add helper ovl_same_sb() to return the common super block in case all
layers are on the same fs.

Signed-off-by: Amir Goldstein <[hidden email]>
Signed-off-by: Miklos Szeredi <[hidden email]>
(backported from commit 7bcd74b98d7bac3e5149894caaf72de6989af7f0)
Signed-off-by: Daniel Axtens <[hidden email]>
---
 fs/overlayfs/overlayfs.h | 1 +
 fs/overlayfs/ovl_entry.h | 2 ++
 fs/overlayfs/super.c     | 8 ++++++++
 fs/overlayfs/util.c      | 7 +++++++
 4 files changed, 18 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index f14fb7b56b09..ee05d77ad7cd 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -154,6 +154,7 @@ int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
 struct dentry *ovl_workdir(struct dentry *dentry);
 const struct cred *ovl_override_creds(struct super_block *sb);
+struct super_block *ovl_same_sb(struct super_block *sb);
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
 bool ovl_dentry_remote(struct dentry *dentry);
 bool ovl_dentry_weird(struct dentry *dentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index d14bca1850d9..1894ce31c20b 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -27,6 +27,8 @@ struct ovl_fs {
  struct ovl_config config;
  /* creds of process who forced instantiation of super block */
  const struct cred *creator_cred;
+ /* sb common to all layers */
+ struct super_block *same_sb;
 };
 
 /* private information held for every overlayfs dentry */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ed2607904a1a..18c8bb3baff5 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -865,11 +865,19 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
  ufs->lower_mnt[ufs->numlower] = mnt;
  ufs->numlower++;
+
+ /* Check if all lower layers are on same sb */
+ if (i == 0)
+ ufs->same_sb = mnt->mnt_sb;
+ else if (ufs->same_sb != mnt->mnt_sb)
+ ufs->same_sb = NULL;
  }
 
  /* If the upper fs is nonexistent, we mark overlayfs r/o too */
  if (!ufs->upper_mnt)
  sb->s_flags |= MS_RDONLY;
+ else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
+ ufs->same_sb = NULL;
 
  if (remote)
  sb->s_d_op = &ovl_reval_dentry_operations;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 952286f4826c..9889daaae78b 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -39,6 +39,13 @@ const struct cred *ovl_override_creds(struct super_block *sb)
  return override_creds(ofs->creator_cred);
 }
 
+struct super_block *ovl_same_sb(struct super_block *sb)
+{
+ struct ovl_fs *ofs = sb->s_fs_info;
+
+ return ofs->same_sb;
+}
+
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 {
  size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
--
2.11.0


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

[SRU][Xenial][PATCH v2 2/2] ovl: persistent inode number for directories

Daniel Axtens
In reply to this post by Daniel Axtens
From: Amir Goldstein <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1728489

stat(2) on overlay directories reports the overlay temp inode
number, which is constant across copy up, but is not persistent.

When all layers are on the same fs, report the copy up origin inode
number for directories.

This inode number is persistent, unique across the overlay mount and
constant across copy up.

Signed-off-by: Amir Goldstein <[hidden email]>
Signed-off-by: Miklos Szeredi <[hidden email]>
(backported from commit b7a807dc2010334e62e0afd89d6f7a8913eb14ff)
Signed-off-by: Daniel Axtens <[hidden email]>
---
 fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 2e1f16d22c16..9c3421e74d95 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -149,12 +149,38 @@ static int ovl_dir_getattr(struct vfsmount *mnt, struct dentry *dentry,
  type = ovl_path_real(dentry, &realpath);
  old_cred = ovl_override_creds(dentry->d_sb);
  err = vfs_getattr(&realpath, stat);
- revert_creds(old_cred);
  if (err)
- return err;
+ goto out;
+
+ /*
+ * When all layers are on the same fs, use the copy-up-origin st_ino,
+ * which is persistent, unique and constant across copy up.
+ *
+ * Otherwise the pair {real st_ino; overlay st_dev} is not unique, so
+ * use the non persistent overlay st_ino.
+ */
+ if (ovl_same_sb(dentry->d_sb)) {
+ if (OVL_TYPE_MERGE(type) && OVL_TYPE_UPPER(type)) {
+ struct kstat lowerstat;
+
+ ovl_path_lower(dentry, &realpath);
+ err = vfs_getattr(&realpath, &lowerstat);
+ if (err)
+ goto out;
+
+ WARN_ON_ONCE(stat->dev != lowerstat.dev);
+ stat->ino = lowerstat.ino;
+ }
+ } else {
+ stat->ino = dentry->d_inode->i_ino;
+ }
 
+ /*
+ * Always use the overlay st_dev for directories, so 'find -xdev' will
+ * scan the entire overlay mount and won't cross the overlay mount
+ * boundaries.
+ */
  stat->dev = dentry->d_sb->s_dev;
- stat->ino = dentry->d_inode->i_ino;
 
  /*
  * It's probably not worth it to count subdirs to get the
@@ -163,8 +189,10 @@ static int ovl_dir_getattr(struct vfsmount *mnt, struct dentry *dentry,
  */
  if (OVL_TYPE_MERGE(type))
  stat->nlink = 1;
+out:
+ revert_creds(old_cred);
 
- return 0;
+ return err;
 }
 
 /* Common operations required to be done after creation of file on upper */
--
2.11.0


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

Re: [SRU][Xenial][PATCH v2 0/2]

Stefan Bader-2
In reply to this post by Daniel Axtens
On 08.11.2017 07:41, Daniel Axtens wrote:

> BugLink: https://bugs.launchpad.net/bugs/1728489
>
> [SRU Justification]
>
> [Impact]
>
> A user is seeing failures from extracting tar archives on overlay
> filesystems on the 4.4 kernel in constrained environments. The error
> presents as:
>
> `tar: ./deps/0/bin: Directory renamed before its status could be extracted`
>
> Following this thread
> (https://www.spinics.net/lists/linux-unionfs/msg00856.html), it appears
> that this occurs when entries in the kernel's inode cache are
> reclaimed, and subsequent lookups return new inode numbers.
>
> Further testing showed that when setting
> `/proc/sys/vm/vfs_cache_pressure` to 0 (don't allow the kernel to
> reclaim inode cache entries due to memory pressure) the error does not
> recur, supporting the hypothesis that cache entries are being
> evicted. However, this setting may lead to a kernel OOM so is not a
> reasonable workaround even temporarily.
>
> The error cannot be reproduced on a 4.13 kernel, due to the series at
> https://www.spinics.net/lists/linux-fsdevel/msg110235.html. The
> particular relevant commit is
> b7a807dc2010334e62e0afd89d6f7a8913eb14ff, which needs a dependency.
>
> [Fix]
>
> Backport the key commit and the dependency.
>
> [Testcase]
>
> # Testing this bug
>
> The testcase for this particular bug is simple - create an overlay
> filesystem with all layers on the same underlying file system, and
> then see if the inode of a directory is constant across dropping the
> caches:
>
> mkdir -p /upper/upper /upper/work /lower
> mount -t overlay none /mnt -o lowerdir=/lower,upperdir=/upper/upper,workdir=/upper/work
> cd /mnt
> mkdir a
> stat a # observe inode number
> echo 2 > /proc/sys/vm/drop_caches
> stat a # compare inode number
>
> If the inode number is the same, the fix is successful.
>
> # Regression testing
>
> I have run the unionmount test suite from
> https://git.infradead.org/users/dhowells/unionmount-testsuite.git in
> overlay mode (./run --ov), and verified that it still passes.
>
> (The series cover letter mentions a fork of the test suite at
> https://github.com/amir73il/unionmount-testsuite/commits/overlayfs-devel. I
> have *not* attempted to get this running: it assumes a range of
> changes that are not present in our kernels.)
>
> [Regression Potential]
>
> As this changes overlayfs, there is potential for regression in the
> form of unexpected breakages to overlaysfs behaviour.
>
> I think this is adequately addressed by the regression testing.
>
> [Notes]
> v2: drop patches that are not strpctly required
>     minor change to patch 2 to not rely on ORIGIN type, as in Xenial
>
> Amir Goldstein (2):
>   ovl: check if all layers are on the same fs
>   ovl: persistent inode number for directories
>
>  fs/overlayfs/dir.c       | 36 ++++++++++++++++++++++++++++++++----
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/ovl_entry.h |  2 ++
>  fs/overlayfs/super.c     |  8 ++++++++
>  fs/overlayfs/util.c      |  7 +++++++
>  5 files changed, 50 insertions(+), 4 deletions(-)
>
Hmm, could it be that most of the Xenials should be Zestys?

-Stefan


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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

NACK: [SRU][Xenial][PATCH v2 0/2]

Kleber Souza
In reply to this post by Daniel Axtens
This patch series has been resent to the mailing-list.

Kleber

On 11/08/17 07:41, Daniel Axtens wrote:

> BugLink: https://bugs.launchpad.net/bugs/1728489
>
> [SRU Justification]
>
> [Impact]
>
> A user is seeing failures from extracting tar archives on overlay
> filesystems on the 4.4 kernel in constrained environments. The error
> presents as:
>
> `tar: ./deps/0/bin: Directory renamed before its status could be extracted`
>
> Following this thread
> (https://www.spinics.net/lists/linux-unionfs/msg00856.html), it appears
> that this occurs when entries in the kernel's inode cache are
> reclaimed, and subsequent lookups return new inode numbers.
>
> Further testing showed that when setting
> `/proc/sys/vm/vfs_cache_pressure` to 0 (don't allow the kernel to
> reclaim inode cache entries due to memory pressure) the error does not
> recur, supporting the hypothesis that cache entries are being
> evicted. However, this setting may lead to a kernel OOM so is not a
> reasonable workaround even temporarily.
>
> The error cannot be reproduced on a 4.13 kernel, due to the series at
> https://www.spinics.net/lists/linux-fsdevel/msg110235.html. The
> particular relevant commit is
> b7a807dc2010334e62e0afd89d6f7a8913eb14ff, which needs a dependency.
>
> [Fix]
>
> Backport the key commit and the dependency.
>
> [Testcase]
>
> # Testing this bug
>
> The testcase for this particular bug is simple - create an overlay
> filesystem with all layers on the same underlying file system, and
> then see if the inode of a directory is constant across dropping the
> caches:
>
> mkdir -p /upper/upper /upper/work /lower
> mount -t overlay none /mnt -o lowerdir=/lower,upperdir=/upper/upper,workdir=/upper/work
> cd /mnt
> mkdir a
> stat a # observe inode number
> echo 2 > /proc/sys/vm/drop_caches
> stat a # compare inode number
>
> If the inode number is the same, the fix is successful.
>
> # Regression testing
>
> I have run the unionmount test suite from
> https://git.infradead.org/users/dhowells/unionmount-testsuite.git in
> overlay mode (./run --ov), and verified that it still passes.
>
> (The series cover letter mentions a fork of the test suite at
> https://github.com/amir73il/unionmount-testsuite/commits/overlayfs-devel. I
> have *not* attempted to get this running: it assumes a range of
> changes that are not present in our kernels.)
>
> [Regression Potential]
>
> As this changes overlayfs, there is potential for regression in the
> form of unexpected breakages to overlaysfs behaviour.
>
> I think this is adequately addressed by the regression testing.
>
> [Notes]
> v2: drop patches that are not strpctly required
>     minor change to patch 2 to not rely on ORIGIN type, as in Xenial
>
> Amir Goldstein (2):
>   ovl: check if all layers are on the same fs
>   ovl: persistent inode number for directories
>
>  fs/overlayfs/dir.c       | 36 ++++++++++++++++++++++++++++++++----
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/ovl_entry.h |  2 ++
>  fs/overlayfs/super.c     |  8 ++++++++
>  fs/overlayfs/util.c      |  7 +++++++
>  5 files changed, 50 insertions(+), 4 deletions(-)
>

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