[SRU][Xenial][PATCH 0/2] LP#1728489: tar -x sometimes fails on overlayfs

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

[SRU][Xenial][PATCH 0/2] LP#1728489: tar -x sometimes fails on overlayfs

Daniel Axtens
[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 couple of
dependencies.

[Fix]
For Zesty, backport the entire series.
For Xenial, where a full backport is not feasible, backport the key
commit and the short list of dependencies.

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

One option to reduce the regression potential on Zesty is to reduce
the set of patches applied - rather than including the whole series we
could include just the patches to solve this bug, which are much
easier to inspect for correctness.

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

 fs/overlayfs/dir.c       | 29 ++++++++++++++++++++++++++++-
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 17 +++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)

--
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 1/2] ovl: check if all layers are on the same fs

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

BugLink: https://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/super.c     | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 5ab50cd102af..fdd7ea8b96f3 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -164,6 +164,7 @@ static inline int ovl_do_whiteout(struct inode *dir, struct dentry *dentry,
 }
 
 const struct cred *ovl_override_creds(struct super_block *sb);
+struct super_block *ovl_same_sb(struct super_block *sb);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 u64 ovl_dentry_version_get(struct dentry *dentry);
 void ovl_dentry_version_inc(struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 6c3077473fa8..9d8e5278a199 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -44,6 +44,8 @@ struct ovl_fs {
  /* pathnames of lower and upper dirs, for show_options */
  struct ovl_config config;
  struct cred *mounter_creds;
+ /* sb common to all layers */
+ struct super_block *same_sb;
 };
 
 struct ovl_dir_cache;
@@ -430,6 +432,13 @@ static const struct dentry_operations ovl_reval_dentry_operations = {
  .d_weak_revalidate = ovl_dentry_weak_revalidate,
 };
 
+struct super_block *ovl_same_sb(struct super_block *sb)
+{
+ struct ovl_fs *ofs = sb->s_fs_info;
+
+ return ofs->same_sb;
+}
+
 static struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 {
  size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
@@ -1157,11 +1166,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;
--
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 2/2] ovl: persistent inode number for directories

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

BugLink: https://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 | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 6e0c7274b0f2..7b8bbd05e9a0 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -176,8 +176,35 @@ static int ovl_dir_getattr(struct vfsmount *mnt, struct dentry *dentry,
  if (err)
  return err;
 
+ /*
+ * 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)
+ return err;
+
+ 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
--
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
|

ACK: [SRU][Xenial][PATCH 0/2] LP#1728489: tar -x sometimes fails on overlayfs

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

> [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 couple of
> dependencies.
>
> [Fix]
> For Zesty, backport the entire series.
> For Xenial, where a full backport is not feasible, backport the key
> commit and the short list of dependencies.
>
> [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.
>
> One option to reduce the regression potential on Zesty is to reduce
> the set of patches applied - rather than including the whole series we
> could include just the patches to solve this bug, which are much
> easier to inspect for correctness.
>
> Amir Goldstein (2):
>   ovl: check if all layers are on the same fs
>   ovl: persistent inode number for directories
>
>  fs/overlayfs/dir.c       | 29 ++++++++++++++++++++++++++++-
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/super.c     | 17 +++++++++++++++++
>  3 files changed, 46 insertions(+), 1 deletion(-)
>
Acked-by: Stefan Bader <[hidden email]>



--
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 0/2] LP#1728489: tar -x sometimes fails on overlayfs

Kleber Souza
In reply to this post by Daniel Axtens
A v2 of this patchset has been sent to the mailing list.

On 10/30/17 07:56, Daniel Axtens wrote:

> [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 couple of
> dependencies.
>
> [Fix]
> For Zesty, backport the entire series.
> For Xenial, where a full backport is not feasible, backport the key
> commit and the short list of dependencies.
>
> [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.
>
> One option to reduce the regression potential on Zesty is to reduce
> the set of patches applied - rather than including the whole series we
> could include just the patches to solve this bug, which are much
> easier to inspect for correctness.
>
> Amir Goldstein (2):
>   ovl: check if all layers are on the same fs
>   ovl: persistent inode number for directories
>
>  fs/overlayfs/dir.c       | 29 ++++++++++++++++++++++++++++-
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/super.c     | 17 +++++++++++++++++
>  3 files changed, 46 insertions(+), 1 deletion(-)
>

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

Re: NACK: [SRU][Xenial][PATCH 0/2] LP#1728489: tar -x sometimes fails on overlayfs

Stefan Bader-2
On 15.11.2017 11:43, Kleber Souza wrote:

> A v2 of this patchset has been sent to the mailing list.
>
> On 10/30/17 07:56, Daniel Axtens wrote:
>> [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 couple of
>> dependencies.
>>
>> [Fix]
>> For Zesty, backport the entire series.
>> For Xenial, where a full backport is not feasible, backport the key
>> commit and the short list of dependencies.
>>
>> [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.
>>
>> One option to reduce the regression potential on Zesty is to reduce
>> the set of patches applied - rather than including the whole series we
>> could include just the patches to solve this bug, which are much
>> easier to inspect for correctness.
>>
>> Amir Goldstein (2):
>>   ovl: check if all layers are on the same fs
>>   ovl: persistent inode number for directories
>>
>>  fs/overlayfs/dir.c       | 29 ++++++++++++++++++++++++++++-
>>  fs/overlayfs/overlayfs.h |  1 +
>>  fs/overlayfs/super.c     | 17 +++++++++++++++++
>>  3 files changed, 46 insertions(+), 1 deletion(-)
>>
>
Not sure the v2 was really Xenial but agreed its confusing...


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

Re: NACK: [SRU][Xenial][PATCH 0/2] LP#1728489: tar -x sometimes fails on overlayfs

Daniel Axtens
Hi all,

Apologies - my screw-up.

The aim is to have a 2 patch series for both Xenial and Zesty.

I will resubmit both with the correct headers and will look for ways to tighten up my personal processes so as not to be so confusing in future.

Regards,
Daniel

On Wed, Nov 15, 2017 at 9:51 PM, Stefan Bader <[hidden email]> wrote:
On 15.11.2017 11:43, Kleber Souza wrote:
> A v2 of this patchset has been sent to the mailing list.
>
> On 10/30/17 07:56, Daniel Axtens wrote:
>> [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 couple of
>> dependencies.
>>
>> [Fix]
>> For Zesty, backport the entire series.
>> For Xenial, where a full backport is not feasible, backport the key
>> commit and the short list of dependencies.
>>
>> [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.
>>
>> One option to reduce the regression potential on Zesty is to reduce
>> the set of patches applied - rather than including the whole series we
>> could include just the patches to solve this bug, which are much
>> easier to inspect for correctness.
>>
>> Amir Goldstein (2):
>>   ovl: check if all layers are on the same fs
>>   ovl: persistent inode number for directories
>>
>>  fs/overlayfs/dir.c       | 29 ++++++++++++++++++++++++++++-
>>  fs/overlayfs/overlayfs.h |  1 +
>>  fs/overlayfs/super.c     | 17 +++++++++++++++++
>>  3 files changed, 46 insertions(+), 1 deletion(-)
>>
>
Not sure the v2 was really Xenial but agreed its confusing...



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