xfs: always free inline data before resetting inode fork during ifree

PATCH In review Darrick J. Wong 9 participants 34 messages Nov 23, 2017 – Apr 3, 2018 XFS filesystem

lore

Share & tools

lore

Darrick J. Wong

author

2017-11-23 06:01 UTC #0 lore raw

PATCH 1/1 xfs: always free inline data before resetting inode fork during ifree

From: Darrick J. Wong <[email protected]>

In xfs_ifree, we reset the data/attr forks to extents format without bothering to free any inline data buffer that might still be around after all the blocks have been truncated off the file. Prior to commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") nobody noticed because the leftover inline data after truncation was small enough to fit inside the inline buffer inside the fork itself.

However, now that we've removed the inline buffer, we /always/ have to free the inline data buffer or else we leak them like crazy. This test was found by turning on kmemleak for generic/001 or generic/388.

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/xfs_inode.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

fs/xfs/xfs_inode.c +21 -0

index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
+
+	ifp = XFS_IFORK_PTR(ip, whichfork);
+	xfs_idata_realloc(ip, -ifp->if_bytes, whichfork);
+}
+
+/*
  * This is called to return an inode to the inode free list.
  * The inode should already be truncated to 0 length and have
  * no pages associated with it.  This routine also assumes that

@@ -2437,6 +2455,9 @@ xfs_ifree(
 	if (error)
 		return error;
 
+	xfs_ifree_local_data(ip, XFS_DATA_FORK);
+	xfs_ifree_local_data(ip, XFS_ATTR_FORK);
+
 	VFS_I(ip)->i_mode = 0;		/* mark incore inode as free */
 	ip->i_d.di_flags = 0;
 	ip->i_d.di_dmevmask = 0;

Christoph Hellwig

Reviewed-by on v1 1/1

2017-11-23 08:14 UTC #1 lore raw

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Luis R. Rodriguez

2018-03-23 01:30 UTC #2 lore raw

In reply to Darrick J. Wong

Quoted 29 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
In xfs_ifree, we reset the data/attr forks to extents format without bothering to free any inline data buffer that might still be around after all the blocks have been truncated off the file. Prior to commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") nobody noticed because the leftover inline data after truncation was small enough to fit inside the inline buffer inside the fork itself.
However, now that we've removed the inline buffer, we /always/ have to free the inline data buffer or else we leak them like crazy. This test was found by turning on kmemleak for generic/001 or generic/388.
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/xfs_inode.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
fs/xfs/xfs_inode.c +2 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.

fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415

Quoted 10 lines

fs/xfs/xfs_inode.c +10 -0

+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;

I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?

Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?

Luis

Darrick J. Wong

author

2018-03-23 03:55 UTC #3 lore raw

Quoted 37 lines · Luis R. Rodriguez

Quoted 36 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
In xfs_ifree, we reset the data/attr forks to extents format without bothering to free any inline data buffer that might still be around after all the blocks have been truncated off the file. Prior to commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") nobody noticed because the leftover inline data after truncation was small enough to fit inside the inline buffer inside the fork itself.
However, now that we've removed the inline buffer, we /always/ have to free the inline data buffer or else we leak them like crazy. This test was found by turning on kmemleak for generic/001 or generic/388.
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/xfs_inode.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;

Quoted 8 lines

fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?

Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.

Quoted 2 lines

Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?

An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.

--D

Quoted 5 lines

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez

2018-03-23 17:08 UTC #4 lore raw

Quoted 28 lines · Darrick J. Wong

Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?

Quoted 2 lines

Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.

Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.

Quoted 7 lines

Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.

Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.

Luis

Darrick J. Wong

author

2018-03-23 17:26 UTC #5 lore raw

Quoted 37 lines · Luis R. Rodriguez

Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 3 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?

Quoted 6 lines

Quoted 4 lines
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.

You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.

(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)

--D

Quoted 5 lines

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez

2018-03-23 18:23 UTC #6 lore raw

Quoted 43 lines · Darrick J. Wong

Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.

Quoted 7 lines

Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)

Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").

Sasha, Greg,

Can you confirm if the algorithm was used in this case?

Since both commits are merged on v4.15, this is a non-issue on >= 4.15.

I do wonder if other XFS folks are *at least* aware that the auto-selection algorithm now currently merging patches onto stable for XFS?

FWIW I just finished completing review *all* the other stable commits merged on XFS on v4.14 *and* v4.13.y and this was the only one that cried out as fishy... so I would not use this as a reason to say we shouldn't use it for XFS, specially in lieu of any formal active process which we can count on always takes place for XFS stable patches. In fact, I'd say that if the auto-selection algorithm was used we should be able to fine tune it with a bit more subsystem involvement. I can at least volunteer to help try to review the candidate patches that AUTOSEL comes up with (any others?), but note I'm new to XFS... I can also think of a few modifications to the algorithm but which I can make in a separate thread. Anyway, provided this is reasonable with others, then perhaps we can keep using it for XFS?

[0] https://soarsmu.github.io/papers/icse12-patch.pdf
[1] https://lkml.org/lkml/2017/11/21/486

Luis

Greg Kroah-Hartman

2018-03-24 09:06 UTC #7 lore raw

Quoted 61 lines · Luis R. Rodriguez

Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)
Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").

Quoted 3 lines

Sasha, Greg,
Can you confirm if the algorithm was used in this case?

No idea.

I think xfs should just be added to the "blacklist" so that it is not even looked at for these types of auto-selected patches. Much like the i915 driver currently is handled (it too is ignored for these patches due to objections from the maintainers of it.)

thanks,

greg k-h

Darrick J. Wong

author

2018-03-24 17:21 UTC #8 lore raw

Quoted 64 lines · Greg Kroah-Hartman

Quoted 63 lines · Luis R. Rodriguez
Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)
Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").
Sasha, Greg,

Quoted 8 lines

Quoted 1 line
Can you confirm if the algorithm was used in this case?
No idea.
I think xfs should just be added to the "blacklist" so that it is not even looked at for these types of auto-selected patches. Much like the i915 driver currently is handled (it too is ignored for these patches due to objections from the maintainers of it.)

Just out of curiosity, how does this autoselection mechanism work today? If it's smart enough to cherry pick patches, apply them to a kernel, build the kernel and run xfstests, and propose the patches if nothing weird happened, then I'd be interested in looking further. I've nothing against algorithmic selection per se, but I'd want to know more about the data sets and parameters that feed the algorithm.

I did receive the AUTOSEL tagged patches a few days ago, but I couldn't figure out what automated regression testing, if any, had been done; or whether the patch submission was asking if we wanted it put into 4.14 or if it was a declaration that they were on their way in. Excuse me for being behind the times, but I'd gotten accustomed xfs patches only ending up in the stable kernels because we'd deliberately put them there. :)

If blacklisting xfs is more convenient then I'm happy to continue things as they were.

--D

Quoted 7 lines

thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sasha Levin

2018-03-26 04:54 UTC #9 lore raw

Quoted 74 lines · Darrick J. Wong

Quoted 64 lines · Greg Kroah-Hartman
Quoted 63 lines · Luis R. Rodriguez
Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)
Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").
Sasha, Greg,
Quoted 8 lines
Quoted 1 line
Can you confirm if the algorithm was used in this case?
No idea.
I think xfs should just be added to the "blacklist" so that it is not even looked at for these types of auto-selected patches. Much like the i915 driver currently is handled (it too is ignored for these patches due to objections from the maintainers of it.)

Quoted 6 lines

Just out of curiosity, how does this autoselection mechanism work today? If it's smart enough to cherry pick patches, apply them to a kernel, build the kernel and run xfstests, and propose the patches if nothing weird happened, then I'd be interested in looking further. I've nothing against algorithmic selection per se, but I'd want to know more about the data sets and parameters that feed the algorithm.

It won't go beyond build testing.

Quoted 4 lines

I did receive the AUTOSEL tagged patches a few days ago, but I couldn't figure out what automated regression testing, if any, had been done; or whether the patch submission was asking if we wanted it put into 4.14 or if it was a declaration that they were on their way in. Excuse me

There would be (at least) 3 different mails involved in this process:

1. You'd get a mail from me, proposing this patch for stable. We give at least 1 week (but usually closer to 2) to comment on whether this patch should or should not go in stable.

2. If no objections were received, Greg would add it to his queue and you'd get another mail about that.

3. A few more days later, Greg would release that stable tree and you'd get another mail.

Quoted 6 lines

for being behind the times, but I'd gotten accustomed xfs patches only ending up in the stable kernels because we'd deliberately put them there. :)
If blacklisting xfs is more convenient then I'm happy to continue things as they were.

No problem with blacklisting subsystems if maintainers prefer it that way, but the i915 case was slightly different as their development process was very quirky and testing was complex, so they asked to just keep doing their own selection for stable.

However, looking at stable history, it seems that no patch from fs/xfs/ was proposed for stable for about half a year now, which is something that the autoselection project is trying to help with.

A different flow I'm working on for this is to send an email as a reply to the original patch submission to lkml if the patch is selected by the network, including details about which trees it was applied to and build results. I think it might work better for subsystems such as xfs.

--
Thanks,
Sasha

Darrick J. Wong

author

2018-03-26 06:48 UTC #10 lore raw

Quoted 75 lines · Sasha Levin

Quoted 74 lines · Darrick J. Wong
Quoted 72 lines · Greg Kroah-Hartman
Quoted 61 lines · Luis R. Rodriguez
Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)
Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").
Quoted 3 lines
Sasha, Greg,
Can you confirm if the algorithm was used in this case?
No idea.
I think xfs should just be added to the "blacklist" so that it is not even looked at for these types of auto-selected patches. Much like the i915 driver currently is handled (it too is ignored for these patches due to objections from the maintainers of it.)

Quoted 8 lines

Quoted 6 lines
Just out of curiosity, how does this autoselection mechanism work today? If it's smart enough to cherry pick patches, apply them to a kernel, build the kernel and run xfstests, and propose the patches if nothing weird happened, then I'd be interested in looking further. I've nothing against algorithmic selection per se, but I'd want to know more about the data sets and parameters that feed the algorithm.
It won't go beyond build testing.

No further regression testing ==> please blacklist XFS.

We will continue our current practices w.r.t. stable.

--D

Quoted 45 lines

Quoted 4 lines
I did receive the AUTOSEL tagged patches a few days ago, but I couldn't figure out what automated regression testing, if any, had been done; or whether the patch submission was asking if we wanted it put into 4.14 or if it was a declaration that they were on their way in. Excuse me
There would be (at least) 3 different mails involved in this process:
1. You'd get a mail from me, proposing this patch for stable. We give at least 1 week (but usually closer to 2) to comment on whether this patch should or should not go in stable.
2. If no objections were received, Greg would add it to his queue and you'd get another mail about that.
3. A few more days later, Greg would release that stable tree and you'd get another mail.
Quoted 6 lines
for being behind the times, but I'd gotten accustomed xfs patches only ending up in the stable kernels because we'd deliberately put them there. :)
If blacklisting xfs is more convenient then I'm happy to continue things as they were.
No problem with blacklisting subsystems if maintainers prefer it that way, but the i915 case was slightly different as their development process was very quirky and testing was complex, so they asked to just keep doing their own selection for stable.
However, looking at stable history, it seems that no patch from fs/xfs/ was proposed for stable for about half a year now, which is something that the autoselection project is trying to help with.
A different flow I'm working on for this is to send an email as a reply to the original patch submission to lkml if the patch is selected by the network, including details about which trees it was applied to and build results. I think it might work better for subsystems such as xfs.
--
Thanks,
Sasha--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez

2018-03-26 17:39 UTC #11 lore raw

In reply to Sasha Levin

Quoted 75 lines · Sasha Levin

Quoted 74 lines · Darrick J. Wong
Quoted 72 lines · Greg Kroah-Hartman
Quoted 61 lines · Luis R. Rodriguez
Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)
Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").
Quoted 3 lines
Sasha, Greg,
Can you confirm if the algorithm was used in this case?
No idea.
I think xfs should just be added to the "blacklist" so that it is not even looked at for these types of auto-selected patches. Much like the i915 driver currently is handled (it too is ignored for these patches due to objections from the maintainers of it.)

Quoted 8 lines

Quoted 6 lines
Just out of curiosity, how does this autoselection mechanism work today? If it's smart enough to cherry pick patches, apply them to a kernel, build the kernel and run xfstests, and propose the patches if nothing weird happened, then I'd be interested in looking further. I've nothing against algorithmic selection per se, but I'd want to know more about the data sets and parameters that feed the algorithm.
It won't go beyond build testing.

This is one area where we can improve things.

Perhaps the typical use case for the kernel for looking for stable fixes is to groom for good candidate fixes using some heuristics, test compile and if there is no issue merge. It would be not so different than a human using their own experience of some sort using a pattern to do the same. However the big difference for filesystems is we *need* proper testing.

The issue with filesystems is the consequence of a bug are much more severe than just a panic. Any corner case issue can really be much more critical than just crashing a system. Ruining your filesystem is simply unforgivable, specially if all that was missing was a proper regression test, and we already have infrastructure for it.

The good news is that we have proper a proper testing suite to help out, and as per my inspection 0-day already embraces a few tests. We can do a bit more here for expanding on these... but also use a baseline for testing proposed XFS stable fixes. For now I suggest a bit of manual work and I can volunteer to lead this on, with a transparent mechanism to enable other folks to reproduce similar testing in an easy way. I had in mind a mechanism to automate this long term, and I started working on something, so I can use that to start this effort, and publish what I have soon.

But that would not be enough, we need proper testing and careful oversight. I can volunteer to review the patches manually (any others?) and if any questions come up bring them up and check / verify with others.

The new autoselection truly is state of the art and we'd be silly to not address corner cases for subsystems to help take advantage of the candidate patches it provides. My inspection so far revealed only one possible issue fix, that is not bad at all.

Quoted 5 lines

Quoted 4 lines
I did receive the AUTOSEL tagged patches a few days ago, but I couldn't figure out what automated regression testing, if any, had been done; or whether the patch submission was asking if we wanted it put into 4.14 or if it was a declaration that they were on their way in. Excuse me

Quoted 8 lines

There would be (at least) 3 different mails involved in this process:
1. You'd get a mail from me, proposing this patch for stable. We give at least 1 week (but usually closer to 2) to comment on whether this patch should or should not go in stable.
2. If no objections were received, Greg would add it to his queue and you'd get another mail about that.

Is there a temporary tree with all the patches already merged by any chance? That would help with testing the queue of candidate fixes.

Quoted 2 lines

3. A few more days later, Greg would release that stable tree and you'd get another mail.

What I propose is for XFS we add a few more steps:

1a. Ensure the fixes are in a temporary tree
1b. Add me to the list of reviewers
1c. I'll run some tests against it compared to a baseline which would be
pre-established
1d. While the tests are running do a manual inspection of the patches 1e. Only if patches produce no regressions with the established baseline and get
a Reviewed-by by at least one developer do we publish to stable

I can work on establishing the baseline with the community next.

Quoted 11 lines

Quoted 6 lines
for being behind the times, but I'd gotten accustomed xfs patches only ending up in the stable kernels because we'd deliberately put them there. :)
If blacklisting xfs is more convenient then I'm happy to continue things as they were.
No problem with blacklisting subsystems if maintainers prefer it that way, but the i915 case was slightly different as their development process was very quirky and testing was complex, so they asked to just keep doing their own selection for stable.

filesystems are the same -- the issue is the risk of an issue is much more severe.

Quoted 3 lines

However, looking at stable history, it seems that no patch from fs/xfs/ was proposed for stable for about half a year now, which is something that the autoselection project is trying to help with.

Indeed. I have addressed the stable question on XFS in person with at the small BoF at Vault last year, and with other folks later. The current process may work for some... but I think we can do better and it just requires volunteers, and I think using the auto-selection process to come up with *candidates* is a great opportunity, specially in light of the patches I have seen so far already merged.

My testing of them also proved no regressions (modulo I skipped the one I pointed out on this thread.

Quoted 4 lines

A different flow I'm working on for this is to send an email as a reply to the original patch submission to lkml if the patch is selected by the network, including details about which trees it was applied to and build results. I think it might work better for subsystems such as xfs.

We need testing. Without testing this cannot fly.

Luis

Dave Chinner

2018-03-25 22:33 UTC #12 lore raw

In reply to Luis R. Rodriguez

Quoted 52 lines · Luis R. Rodriguez

Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+		return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)

Quoted 8 lines

Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").

Yikes. That sets off all my "how to break filesysetms for fun and profit" alarm bells. This is like playing russian roulette with all our user's data. XFS fixes that look like they are simple often have subtle dependencies in them that automated backports won't ever be able to understand, and if we don't get that right, we break stuff.

Filesystems aren't like drivers or memory management - you can't reboot to fix a filesystem corruption or data loss bug. User's tend to care a lot more about their data and cat photos than they do about how often the dodgy hardware they bought on ebay needs OS rebooting to get working again..

Quoted 2 lines

I do wonder if other XFS folks are *at least* aware that the auto-selection algorithm now currently merging patches onto stable for XFS?

No I wasn't aware that this was happening. I'm kinda shit scared right now hearing about how automated backports of random kernel patches are being done with minimal oversight and no visibility to the subsystem developers. When did this start happening?

At this point I'd be much more comfortable if XFS was blacklisted until there's solid subsystem developer visibility of the iautomated backports, not to mention a solid set of automated regression testing backing this automated backport proceedure.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Sasha Levin

2018-03-26 23:54 UTC #13 lore raw

Quoted 62 lines · Dave Chinner

Quoted 52 lines · Luis R. Rodriguez
Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+   struct xfs_inode        *ip,
+   int                     whichfork)
+{
+   struct xfs_ifork        *ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+   if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+           return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)
Quoted 8 lines
Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").

Quoted 6 lines

Yikes. That sets off all my "how to break filesysetms for fun and profit" alarm bells. This is like playing russian roulette with all our user's data. XFS fixes that look like they are simple often have subtle dependencies in them that automated backports won't ever be able to understand, and if we don't get that right, we break stuff.

On the other hand, XFS has a few commits that fix possible corruptions, that have never ended up in a stable tree. Isn't it just as bad ("playing roulette") for users?

Quoted 5 lines

Filesystems aren't like drivers or memory management - you can't reboot to fix a filesystem corruption or data loss bug. User's tend to care a lot more about their data and cat photos than they do about how often the dodgy hardware they bought on ebay needs OS rebooting to get working again..

Thank you for your input Dave. Let me give you the background for why I've been doing it this way up to now to explain my reasoning, and where I was wrong.

After I've built the initial framework for this, I ran it through a set of kernel versions and ended up with a massively large set of commits that were detected as bug fixes but didn't end up getting in stable trees.

My thinking back then was that I can start building branches of proposed commits, send them to their respective authors/maintainers, and remove any patches that maintainers objected to their inclusion. This process ran for a few months, and a few thousand patches (between all kernel trees) went in this way. The rate of rejection (either when a maintainer objects to an inclusion, or a regression discovered after a kernel was released) was on par (and even lower) than the one for "regular" patches tagged for stable.

What I didn't account for correctly was that people are way more busier than I have expected, so a lot of these mails were lost or ignored in mailboxes, so some of these patches never received review before going in a stable tree.

I'm trying to fix this with a different approach (more below).

Quoted 7 lines

Quoted 2 lines
I do wonder if other XFS folks are *at least* aware that the auto-selection algorithm now currently merging patches onto stable for XFS?
No I wasn't aware that this was happening. I'm kinda shit scared right now hearing about how automated backports of random kernel patches are being done with minimal oversight and no visibility to the subsystem developers. When did this start happening?

About half a year ago. I'm not sure about the no visibility part - maintainers and authors would receive at least 3 mails for each patch that got in this way, and would have at least a week (usually a lot more) to object to the inclusion. Did you not receive any mails from me?

Quoted 4 lines

At this point I'd be much more comfortable if XFS was blacklisted until there's solid subsystem developer visibility of the iautomated backports, not to mention a solid set of automated regression testing backing this automated backport proceedure.

I'll be winding down what I'm trying to do now, and will be trying to address these concerns from maintainers in a few different ways.

I've started working on a framework to automate reviews of sent patches to lkml by my framework, this will allow me to do the following:

- I would send a reply to the original patch sent to LKML within a
few hours for patches that have a high probability for being a bug fix rather than sending a brand new mail a few months after this patch made it upstream. This will help reviews as this commit is still fresh in the author+maintainers head.
- I will include the results of builds for various build testing (I
got that working now). At this point I suspect this will mostly help Greg with patches that are already sent with stable tags.
- This will turn into an opt-in rather than opt-out, but it will be
extremely easy to opt in (something like replying with "ack" to have that patch included in the proposed stable branches).
- In the future, I'd also like to create a per-subsystem testing
procedure (so for example, for xfs - run xfstest). I'll try working with maintainers of each subsystem to create something they're happy with. Given this discussion, I'll make XFS my first attempt at this :)

The mails will look something like this (an example based on a recent XFS commit):

Quoted 24 lines

From: Sasha Levin <[email protected]>
To: Sasha Levin <[email protected]>
To: [email protected], "Darrick J . Wong" <[email protected]>
Cc: Brian Foster <[email protected]>, [email protected]
Subject: Re: [PATCH] xfs: Correctly invert xfs_buftarg LRU isolation logic
In-Reply-To: <[email protected]>
References: <[email protected]>
Hi Vratislav Bendel,
[This is an automated email]
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: OK!
v4.14.29: OK!
v4.9.89: OK!
v4.4.123: OK!
v4.1.50: OK!
v3.18.101: OK!

Quoted 5 lines

Please reply with "ack" to have this patch included in the appropriate stable trees.
--
Thanks,
Sasha

If you look at the recent history for fs/xfs, there were no commits in the past half a year or so that were submitted to any stable tree in the "traditional" way. There are no XFS fixes in the 4.14 LTS tree besides the ones submitted with the autoselection method. This is not finger pointing at XFS, but rather at the -stable process itself. It's difficult to keep track on which branches authors need to test their patches on, what sort of tests they need to do, and how they should tag their commits. In quite a few cases the effort to properly tag a commit for stable takes more effort that writing the code for that commit, which deters people from working with stable.

Thanks again for your input.

Michal Hocko

2018-03-27 07:06 UTC #14 lore raw

On Mon 26-03-18 19:54:31, Sasha Levin wrote:
[...]

Quoted 5 lines

About half a year ago. I'm not sure about the no visibility part - maintainers and authors would receive at least 3 mails for each patch that got in this way, and would have at least a week (usually a lot more) to object to the inclusion. Did you not receive any mails from me?

Well, I was aware of your emails yet my free time didn't really allow me to follow those patch bombs. I responded only when some email subject hit my eyes as being non-stable candidate. So by no means the MM backports were reviewed by me. And considering how hard it is to get any review for MM patches in general I strongly suspect that others didn't review either.

In general I am quite skeptical about the automagic backports selections, to be honest. MM patches should be reasonably good at selecting stable backports and adding more patches on top just risks regressions.

Signature · 2 lines

Michal Hocko
SUSE Labs

Luis R. Rodriguez

2018-03-27 19:54 UTC #15 lore raw

Quoted 8 lines · Michal Hocko

So by no means the MM backports were reviewed by me. And considering how hard it is to get any review for MM patches in general I strongly suspect that others didn't review either.
In general I am quite skeptical about the automagic backports selections, to be honest. MM patches should be reasonably good at selecting stable backports and adding more patches on top just risks regressions.

BTW other than suggesting we needing *actual review* of the MM patches, are there known unit tests which could be run as well? Thinking long term.

Luis

Michal Hocko

2018-03-28 13:21 UTC #16 lore raw

Quoted 5 lines · Luis R. Rodriguez

Quoted 4 lines · Michal Hocko
So by no means the MM backports were reviewed by me. And considering how hard it is to get any review for MM patches in general I strongly suspect that others didn't review either.

Quoted 7 lines

Quoted 4 lines
In general I am quite skeptical about the automagic backports selections, to be honest. MM patches should be reasonably good at selecting stable backports and adding more patches on top just risks regressions.
BTW other than suggesting we needing *actual review* of the MM patches, are there known unit tests which could be run as well? Thinking long term.

There are some in selftests but most fixes are quite hard to get a specialized testcase for. Rememeber the MM is a pile of heuristics to handle large scale of workloads.

Signature · 2 lines

Michal Hocko
SUSE Labs

Sasha Levin

2018-03-28 19:33 UTC #17 lore raw

Quoted 11 lines · Michal Hocko

Quoted 10 lines · Luis R. Rodriguez
Quoted 8 lines · Michal Hocko
So by no means the MM backports were reviewed by me. And considering how hard it is to get any review for MM patches in general I strongly suspect that others didn't review either.
In general I am quite skeptical about the automagic backports selections, to be honest. MM patches should be reasonably good at selecting stable backports and adding more patches on top just risks regressions.

Quoted 6 lines

Quoted 2 lines
BTW other than suggesting we needing *actual review* of the MM patches, are there known unit tests which could be run as well? Thinking long term.
There are some in selftests but most fixes are quite hard to get a specialized testcase for. Rememeber the MM is a pile of heuristics to handle large scale of workloads.

Would running mmtests for each patch help here at all?

--
Thanks,
Sasha

Michal Hocko

2018-03-29 07:01 UTC #18 lore raw

Quoted 12 lines · Sasha Levin

Quoted 11 lines · Michal Hocko
Quoted 10 lines · Luis R. Rodriguez
Quoted 8 lines · Michal Hocko
So by no means the MM backports were reviewed by me. And considering how hard it is to get any review for MM patches in general I strongly suspect that others didn't review either.
In general I am quite skeptical about the automagic backports selections, to be honest. MM patches should be reasonably good at selecting stable backports and adding more patches on top just risks regressions.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
BTW other than suggesting we needing *actual review* of the MM patches, are there known unit tests which could be run as well? Thinking long term.
There are some in selftests but most fixes are quite hard to get a specialized testcase for. Rememeber the MM is a pile of heuristics to handle large scale of workloads.
Would running mmtests for each patch help here at all?

mmtests are more for performance than regression/correctness testing AFAIR.

Signature · 2 lines

Michal Hocko
SUSE Labs

Sasha Levin

2018-03-28 01:11 UTC #19 lore raw

In reply to Michal Hocko

Quoted 8 lines · Michal Hocko

On Mon 26-03-18 19:54:31, Sasha Levin wrote:
[...]
Quoted 5 lines
About half a year ago. I'm not sure about the no visibility part - maintainers and authors would receive at least 3 mails for each patch that got in this way, and would have at least a week (usually a lot more) to object to the inclusion. Did you not receive any mails from me?

Quoted 6 lines

Well, I was aware of your emails yet my free time didn't really allow me to follow those patch bombs. I responded only when some email subject hit my eyes as being non-stable candidate. So by no means the MM backports were reviewed by me. And considering how hard it is to get any review for MM patches in general I strongly suspect that others didn't review either.

Being aware is different than saying it was snuck in without anyone knowing.

Quoted 4 lines

In general I am quite skeptical about the automagic backports selections, to be honest. MM patches should be reasonably good at selecting stable backports and adding more patches on top just risks regressions.

Okay, let's take mm/ as a subsystem that is doing a good job with submitting stuff to -stable.

There were 40 patches in total going into the 4.14 LTS tree that touched mm/. Out of those, 5 were selected via the "automagic" process:

Quoted 5 lines

647a37ec1a17 mm/frame_vector.c: release a semaphore in 'get_vaddr_frames()' d91c3f2e540f mm/early_ioremap: Fix boot hang with earlyprintk=efi,keep b2ba0bd34695 kmemleak: add scheduling point to kmemleak_scan() 5ca94e03675a slub: fix sysfs duplicate filename creation when slub_debug=O 342ee8775800 mm, x86/mm: Fix performance regression in get_user_pages_fast()

The only "questionable" one here I see is the performance regression one, which I decided to keep in because it's a rather severe one in a common code path.

Even good subsystems might sometimes miss a patch or two.

But yes, I've received less response from maintainers than I expected, so I'll switch it to an opt-in model for new patches that go upstream.

--
Thanks,
Sasha

Michal Hocko

2018-03-28 13:20 UTC #20 lore raw

Quoted 16 lines · Sasha Levin

Quoted 8 lines · Michal Hocko
On Mon 26-03-18 19:54:31, Sasha Levin wrote:
[...]
Quoted 5 lines
About half a year ago. I'm not sure about the no visibility part - maintainers and authors would receive at least 3 mails for each patch that got in this way, and would have at least a week (usually a lot more) to object to the inclusion. Did you not receive any mails from me?
Quoted 6 lines
Well, I was aware of your emails yet my free time didn't really allow me to follow those patch bombs. I responded only when some email subject hit my eyes as being non-stable candidate. So by no means the MM backports were reviewed by me. And considering how hard it is to get any review for MM patches in general I strongly suspect that others didn't review either.

Quoted 2 lines

Being aware is different than saying it was snuck in without anyone knowing.

Well, you have to realize that we are receiving tons of emails and if there are large piles appearing in my inbox I tend to delete them all if they fall into certain pattern categories.

But just to be absolutely clear. I do not want to accuse anybody from sneaking changes into the stable tree behind maintainers backs.

Quoted 18 lines

Quoted 5 lines
In general I am quite skeptical about the automagic backports selections, to be honest. MM patches should be reasonably good at selecting stable backports and adding more patches on top just risks regressions.
Okay, let's take mm/ as a subsystem that is doing a good job with submitting stuff to -stable.
There were 40 patches in total going into the 4.14 LTS tree that touched mm/. Out of those, 5 were selected via the "automagic" process:
Quoted 5 lines
647a37ec1a17 mm/frame_vector.c: release a semaphore in 'get_vaddr_frames()' d91c3f2e540f mm/early_ioremap: Fix boot hang with earlyprintk=efi,keep b2ba0bd34695 kmemleak: add scheduling point to kmemleak_scan() 5ca94e03675a slub: fix sysfs duplicate filename creation when slub_debug=O 342ee8775800 mm, x86/mm: Fix performance regression in get_user_pages_fast()

Quoted 5 lines

The only "questionable" one here I see is the performance regression one, which I decided to keep in because it's a rather severe one in a common code path.
Even good subsystems might sometimes miss a patch or two.

Ohh, absolutely. But if anybody is going to hit the issue, it would be easier to spot with the upstream kernel having the fix already. Most of above patches have good chance of never being hit or at least not that harmful.

Quoted 2 lines

But yes, I've received less response from maintainers than I expected, so I'll switch it to an opt-in model for new patches that go upstream.

OK, that would certainly be more conservative and to some degree safer. I have had that feeling that stable simply has to rely on maintainers others we are operating on the pure luck mode. Just the fact that the patch applies doesn't mean it is correct to be backported and it is out of reasonable to expect stable tree maintainers to evaluate that.

So as much as I appreciate the work you and others are doing (the automagic part is actually and interesting concept for sure) the less is sometimes more...

Signature · 2 lines

Michal Hocko
SUSE Labs

Dave Chinner

2018-03-28 03:32 UTC #21 lore raw

In reply to Sasha Levin

Quoted 70 lines · Sasha Levin

Quoted 62 lines · Dave Chinner
Quoted 52 lines · Luis R. Rodriguez
Quoted 43 lines · Darrick J. Wong
Quoted 42 lines · Luis R. Rodriguez
Quoted 28 lines · Darrick J. Wong
Quoted 18 lines · Luis R. Rodriguez
Quoted 17 lines · Darrick J. Wong
fs/xfs/xfs_inode.c +9 -0
index 61d1cb7..8012741 100644
@@ -2401,6 +2401,24 @@ xfs_ifree_cluster(
 }
 
 /*
+ * Free any local-format buffers sitting around before we reset to
+ * extents format.
+ */
+static inline void
+xfs_ifree_local_data(
+   struct xfs_inode        *ip,
+   int                     whichfork)
+{
+   struct xfs_ifork        *ifp;
Quoted 8 lines
fs/xfs/xfs_inode.c xfs_ifree_cluster( · line 2415
Quoted 3 lines
fs/xfs/xfs_inode.c +3 -0
+
+   if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
+           return;
I'm new to all this so this was a bit hard to follow. I'm confused with how commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork") exacerbated the leak, isn't that commit about XFS_DINODE_FMT_EXTENTS?
Quoted 2 lines
Not specifically _EXTENTS, merely any fork (EXTENTS or LOCAL) whose incore data was small enough to fit in if_inline_ata.
Got it, I thought those were XFS_DINODE_FMT_EXTENTS by definition.
Quoted 7 lines
Quoted 2 lines
Did we have cases where the format was XFS_DINODE_FMT_LOCAL and yet ifp->if_u1.if_data == ifp->if_u2.if_inline_data ?
An empty directory is 6 bytes, which is what you get with a fresh mkdir or after deleting everything in the directory. Prior to the 43518812d2 patch we could get away with not even checking if we had to free if_data when deleting a directory because it fit within if_inline_data.
Quoted 7 lines
Quoted 1 line
Ah got it. So your fix *is* also applicable even prior to commit 43518812d2.
You'd have to modify the patch so that it doesn't try to kmem_free if_data if if_data == if_inline_data but otherwise (in theory) I think that the concept applies to pre-4.15 kernels.
(YMMV, please do run this through QA/kmemleak just in case I'm wrong, etc...)
Quoted 8 lines
Well... so we need a resolution and better get testing this already given that *I believe* the new auto-selection algorithm used to cherry pick patches onto stable for linux-4.14.y (covered on a paper [0] and when used, stable patches are prefixed with AUTOSEL, a recent discussion covered this in November 2017 [1]) recommended to merge your commit 98c4f78dcdd8 ("xfs: always free inline data before resetting inode fork during ifree") as stable commit 1eccdbd4836a41 on v4.14.17 *without* merging commit 43518812d2 ("xfs: remove support for inlining data/extents into the inode fork").
Quoted 6 lines
Yikes. That sets off all my "how to break filesysetms for fun and profit" alarm bells. This is like playing russian roulette with all our user's data. XFS fixes that look like they are simple often have subtle dependencies in them that automated backports won't ever be able to understand, and if we don't get that right, we break stuff.

Quoted 3 lines

On the other hand, XFS has a few commits that fix possible corruptions, that have never ended up in a stable tree. Isn't it just as bad ("playing roulette") for users?

No, because most corruption problems we fix are rarely seen by users. Those that are seen or considered a significant risk are backported as per the usual process. What we don't do is shovel things that *look like fixes* back in older kernels.

This is the third time in recent weeks where I've had to explain this. e.g:

https://marc.info/?l=linux-xfs&m=152103080002315&w=2

And note Christoph's followup:
https://marc.info/?l=linux-xfs&m=152103175702634&w=2

What's important to note is that the discussion in that thread lead to the patch being backported, validated and then included in Greg's stable tree.

Validating that backports to all the stable kernels is effectively a full time job in itself, and we simply don't have enough upstream developer resources available to do that. So it's a simple: if we don't have the resources to validate changes properly, then we *don't change the code*.

....

Quoted 8 lines

Quoted 7 lines
Quoted 2 lines
I do wonder if other XFS folks are *at least* aware that the auto-selection algorithm now currently merging patches onto stable for XFS?
No I wasn't aware that this was happening. I'm kinda shit scared right now hearing about how automated backports of random kernel patches are being done with minimal oversight and no visibility to the subsystem developers. When did this start happening?

Quoted 5 lines

About half a year ago. I'm not sure about the no visibility part - maintainers and authors would receive at least 3 mails for each patch that got in this way, and would have at least a week (usually a lot more) to object to the inclusion. Did you not receive any mails from me?

I'm not the XFS maintainer (haven't been for 18 months now), I don't subscribe to LKML anymore and none of my patches were selected for backports. So, no, I had no idea this was going on.

Quoted 4 lines

I've started working on a framework to automate reviews of sent patches to lkml by my framework, this will allow me to do the following:

Quoted 15 lines

- I would send a reply to the original patch sent to LKML within a
few hours for patches that have a high probability for being a bug fix rather than sending a brand new mail a few months after this patch made it upstream. This will help reviews as this commit is still fresh in the author+maintainers head.
- I will include the results of builds for various build testing (I
got that working now). At this point I suspect this will mostly help Greg with patches that are already sent with stable tags.
- This will turn into an opt-in rather than opt-out, but it will be
extremely easy to opt in (something like replying with "ack" to have that patch included in the proposed stable branches).
- In the future, I'd also like to create a per-subsystem testing
procedure (so for example, for xfs - run xfstest). I'll try working with maintainers of each subsystem to create something they're happy with. Given this discussion, I'll make XFS my first attempt at this :)

How much time are your test rigs going to be able to spend running xfstests? A single pass on a single filesysetm config on spinning disks will take 3-4 hours of run time. And we have at least 4 common configs that need validation (v4, v4 w/ 512b block size, v5 (defaults), and v5 w/ reflink+rmap) and so you're looking at a minimum 12-24 hours of machine test time per kernel you'd need to test.

And that's just for XFS. There's the same sort of basic configuration matrix test for ext4 (Ted does it via kvm-xfstests on GCE which, IIRC takes about 20 hours to run) and btrfs has similar test requirements. Then there's f2fs, overlay, etc.

You can probably start to see the scope of the validation problem stable kernels pose, and this is just for filesystem changes....

Quoted 28 lines

The mails will look something like this (an example based on a recent XFS commit):
Quoted 25 lines
From: Sasha Levin <[email protected]>
To: Sasha Levin <[email protected]>
To: [email protected], "Darrick J . Wong" <[email protected]>
Cc: Brian Foster <[email protected]>, [email protected]
Subject: Re: [PATCH] xfs: Correctly invert xfs_buftarg LRU isolation logic
In-Reply-To: <[email protected]>
References: <[email protected]>
Hi Vratislav Bendel,
[This is an automated email]
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: OK!
v4.14.29: OK!
v4.9.89: OK!
v4.4.123: OK!
v4.1.50: OK!
v3.18.101: OK!
Please reply with "ack" to have this patch included in the appropriate stable trees.

That might help, but the testing and validation is completely opaque. If I wanted to know what that "OK!" actually meant, where do I go to find that out?

Quoted 5 lines

If you look at the recent history for fs/xfs, there were no commits in the past half a year or so that were submitted to any stable tree in the "traditional" way. There are no XFS fixes in the 4.14 LTS tree besides the ones submitted with the autoselection method. This is not finger pointing at XFS, but rather at the -stable process itself.

It's not a reflection on the -stable process, it's a reflection on the amount of work validation of filesystem changes require. If we decide to do backports, the -stable process will work just fine for the mechanical code movement into the stable trees. It's all the extra stuff before and after that movement occurs that incurs the resource costs.

Quoted 6 lines

It's
difficult to keep track on which branches authors need to test their patches on, what sort of tests they need to do, and how they should tag their commits. In quite a few cases the effort to properly tag a commit for stable takes more effort that writing the code for that commit, which deters people from working with stable.

See the link I posted above - I explicitly address the overhead involved in adding "fixes" tags and identifying backport targets. And even without the overhead of having to add "fixes" tags, the broader point I'm making about effectively random selection of commits for backports is very relevant to the auto-backport magic we've just learnt about...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Sasha Levin

2018-03-28 19:30 UTC #22 lore raw

Quoted 7 lines · Dave Chinner

How much time are your test rigs going to be able to spend running xfstests? A single pass on a single filesysetm config on spinning disks will take 3-4 hours of run time. And we have at least 4 common configs that need validation (v4, v4 w/ 512b block size, v5 (defaults), and v5 w/ reflink+rmap) and so you're looking at a minimum 12-24 hours of machine test time per kernel you'd need to test.

No reason they can't run in parallel, right?

Quoted 24 lines

Quoted 24 lines
Quoted 24 lines
From: Sasha Levin <[email protected]>
To: Sasha Levin <[email protected]>
To: [email protected], "Darrick J . Wong" <[email protected]>
Cc: Brian Foster <[email protected]>, [email protected]
Subject: Re: [PATCH] xfs: Correctly invert xfs_buftarg LRU isolation logic
In-Reply-To: <[email protected]>
References: <[email protected]>
Hi Vratislav Bendel,
[This is an automated email]
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: OK!
v4.14.29: OK!
v4.9.89: OK!
v4.4.123: OK!
v4.1.50: OK!
v3.18.101: OK!

Quoted 5 lines

Quoted 1 line
Quoted 1 line
Please reply with "ack" to have this patch included in the appropriate stable trees.
That might help, but the testing and validation is completely opaque. If I wanted to know what that "OK!" actually meant, where do I go to find that out?

This is actually something I want maintainers to dictate. What sort of testing would make the XFS folks happy here? Right now I'm doing "./check 'xfs/*'" with xfstests. Is it sufficient? Anything else you'd like to see?

--
Thanks,
Sasha

Darrick J. Wong

author

2018-03-28 19:40 UTC #23 lore raw

Quoted 10 lines · Sasha Levin

Quoted 7 lines · Dave Chinner
How much time are your test rigs going to be able to spend running xfstests? A single pass on a single filesysetm config on spinning disks will take 3-4 hours of run time. And we have at least 4 common configs that need validation (v4, v4 w/ 512b block size, v5 (defaults), and v5 w/ reflink+rmap) and so you're looking at a minimum 12-24 hours of machine test time per kernel you'd need to test.
No reason they can't run in parallel, right?

Correct, parallelizing them turns horrifying long test runs into manageable quantities.

Quoted 26 lines

Quoted 26 lines
Quoted 25 lines
Quoted 25 lines
From: Sasha Levin <[email protected]>
To: Sasha Levin <[email protected]>
To: [email protected], "Darrick J . Wong" <[email protected]>
Cc: Brian Foster <[email protected]>, [email protected]
Subject: Re: [PATCH] xfs: Correctly invert xfs_buftarg LRU isolation logic
In-Reply-To: <[email protected]>
References: <[email protected]>
Hi Vratislav Bendel,
[This is an automated email]
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: OK!
v4.14.29: OK!
v4.9.89: OK!
v4.4.123: OK!
v4.1.50: OK!
v3.18.101: OK!
Please reply with "ack" to have this patch included in the appropriate stable trees.

Quoted 7 lines

Quoted 3 lines
That might help, but the testing and validation is completely opaque. If I wanted to know what that "OK!" actually meant, where do I go to find that out?
This is actually something I want maintainers to dictate. What sort of testing would make the XFS folks happy here? Right now I'm doing "./check 'xfs/*'" with xfstests. Is it sufficient? Anything else you'd like to see?

FWIW /me usually runs ./check '-g auto,quick,clone,dedupe,fsmap,rmap' with the following four mkfs configs:

MKFS_OPTIONS='-m reflink=1,rmapbt=1, -i sparse=1, -b size=1024,' MKFS_OPTIONS='-m reflink=1,rmapbt=1, -i sparse=1,' MKFS_OPTIONS='-m crc=0,reflink=0,rmapbt=0, -i sparse=0,' MKFS_OPTIONS='-m crc=0,reflink=0,rmapbt=0, -i sparse=0, -b size=512,'

Eventually I'll turn quotas on all the time too, time permitting.

--D

Quoted 7 lines

--
Thanks,
Sasha--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dave Chinner

2018-03-28 23:05 UTC #24 lore raw

In reply to Sasha Levin

Quoted 10 lines · Sasha Levin

Quoted 7 lines · Dave Chinner
How much time are your test rigs going to be able to spend running xfstests? A single pass on a single filesysetm config on spinning disks will take 3-4 hours of run time. And we have at least 4 common configs that need validation (v4, v4 w/ 512b block size, v5 (defaults), and v5 w/ reflink+rmap) and so you're looking at a minimum 12-24 hours of machine test time per kernel you'd need to test.
No reason they can't run in parallel, right?

Sure they can, if you've got the infrastructure to do it. e.g. putting concurrent test runs on the same spinning disk doesn't speed up the overall test run time by very much - they slow each other down as they contend for IO from the same spindle...

I have 5-6 configs on each of my test VMs that I use for validation. They all have the default config, all have a reflink enabledi config, and then have varing numbers of other unique configs according to how fast they run. i.e. it's tailored to "overnight" testing, so 12-16 hours of test run time.

With them all running in parallel, it takes about 16 hours to cover all the different configs. I could create more test VMs and run one config per VM, but that's slower than (due to resource contention) than running mutliple configs sequentially with limited concurrency. What is most efficient for your available resources will be different, so don't assume what works for me will work for you....

Quoted 26 lines

Quoted 26 lines
Quoted 25 lines
Quoted 25 lines
From: Sasha Levin <[email protected]>
To: Sasha Levin <[email protected]>
To: [email protected], "Darrick J . Wong" <[email protected]>
Cc: Brian Foster <[email protected]>, [email protected]
Subject: Re: [PATCH] xfs: Correctly invert xfs_buftarg LRU isolation logic
In-Reply-To: <[email protected]>
References: <[email protected]>
Hi Vratislav Bendel,
[This is an automated email]
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: OK!
v4.14.29: OK!
v4.9.89: OK!
v4.4.123: OK!
v4.1.50: OK!
v3.18.101: OK!
Please reply with "ack" to have this patch included in the appropriate stable trees.

Quoted 7 lines

Quoted 3 lines
That might help, but the testing and validation is completely opaque. If I wanted to know what that "OK!" actually meant, where do I go to find that out?
This is actually something I want maintainers to dictate. What sort of testing would make the XFS folks happy here? Right now I'm doing "./check 'xfs/*'" with xfstests. Is it sufficient? Anything else you'd like to see?

... and you're doing it wrong. This is precisely why being able to discover /exactly/ what you are testing and being able to browse the test results so we can find out if tests passed when a user reports a bug on a stable kernel.

The way you are running fstests skips more than half the test suite It also runs tests that are considered dangerous because they are likely to cause the test run to fail in some way (i.e. trigger an oops, hang the machine, leave a filesystem in an unmountable state, etc) and hence not complete a full pass.

"./check -g auto" runs the full "expected to pass" regression test suite for all configured test configurations. (i.e. all config sections listed in the configs/<host>.config file)

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Luis R. Rodriguez

2018-03-29 18:12 UTC #25 lore raw

Quoted 17 lines · Dave Chinner

Quoted 4 lines · Sasha Levin
This is actually something I want maintainers to dictate. What sort of testing would make the XFS folks happy here? Right now I'm doing "./check 'xfs/*'" with xfstests. Is it sufficient? Anything else you'd like to see?
... and you're doing it wrong. This is precisely why being able to discover /exactly/ what you are testing and being able to browse the test results so we can find out if tests passed when a user reports a bug on a stable kernel.
The way you are running fstests skips more than half the test suite It also runs tests that are considered dangerous because they are likely to cause the test run to fail in some way (i.e. trigger an oops, hang the machine, leave a filesystem in an unmountable state, etc) and hence not complete a full pass.

Quoted 3 lines

"./check -g auto" runs the full "expected to pass" regression test suite for all configured test configurations. (i.e. all config sections listed in the configs/<host>.config file)

ie, it would be safer to expect that an algorithmic auto-selection process for fixes for stable kernels should have direct input and involvement from subsystems for run-time testing and simply guessing or assuming won't suffice.

The days of just compile testing should be way over by now, and we should expect no less for stable kernels, *specially* if we start involving automation.

Would a way to *start* to address this long term for XFS or other filesystems for auto-selection long-term be a topic worth covering / addressing at LSF/MM?

Luis

Josef Bacik

2018-03-29 18:17 UTC #26 lore raw

Quoted 26 lines · Luis R. Rodriguez

Quoted 17 lines · Dave Chinner
Quoted 4 lines · Sasha Levin
This is actually something I want maintainers to dictate. What sort of testing would make the XFS folks happy here? Right now I'm doing "./check 'xfs/*'" with xfstests. Is it sufficient? Anything else you'd like to see?
... and you're doing it wrong. This is precisely why being able to discover /exactly/ what you are testing and being able to browse the test results so we can find out if tests passed when a user reports a bug on a stable kernel.
The way you are running fstests skips more than half the test suite It also runs tests that are considered dangerous because they are likely to cause the test run to fail in some way (i.e. trigger an oops, hang the machine, leave a filesystem in an unmountable state, etc) and hence not complete a full pass.
Quoted 3 lines
"./check -g auto" runs the full "expected to pass" regression test suite for all configured test configurations. (i.e. all config sections listed in the configs/<host>.config file)
ie, it would be safer to expect that an algorithmic auto-selection process for fixes for stable kernels should have direct input and involvement from subsystems for run-time testing and simply guessing or assuming won't suffice.

Quoted 7 lines

The days of just compile testing should be way over by now, and we should expect no less for stable kernels, *specially* if we start involving automation.
Would a way to *start* to address this long term for XFS or other filesystems for auto-selection long-term be a topic worth covering / addressing at LSF/MM?

It would be cool to tie tests to commit numbers for things where we're making sure a oops/hang doesn't happen again, but honestly I'm not sure it's worth the effort. Maybe this is my upstream bias showing, but I only ever run xfstests against something relatively close to linus, so I'm not super worried about ./check -g auto eating my box. I expect that if I run auto that everything minus the few flakey tests are going to pass.

Also TIL about configs/<host>.config, that's pretty fucking cool. Thanks,

Josef

Sasha Levin

2018-03-29 18:36 UTC #27 lore raw

Quoted 27 lines · Josef Bacik

Quoted 26 lines · Luis R. Rodriguez
Quoted 17 lines · Dave Chinner
Quoted 4 lines · Sasha Levin
This is actually something I want maintainers to dictate. What sort of testing would make the XFS folks happy here? Right now I'm doing "./check 'xfs/*'" with xfstests. Is it sufficient? Anything else you'd like to see?
... and you're doing it wrong. This is precisely why being able to discover /exactly/ what you are testing and being able to browse the test results so we can find out if tests passed when a user reports a bug on a stable kernel.
The way you are running fstests skips more than half the test suite It also runs tests that are considered dangerous because they are likely to cause the test run to fail in some way (i.e. trigger an oops, hang the machine, leave a filesystem in an unmountable state, etc) and hence not complete a full pass.
Quoted 3 lines
"./check -g auto" runs the full "expected to pass" regression test suite for all configured test configurations. (i.e. all config sections listed in the configs/<host>.config file)
ie, it would be safer to expect that an algorithmic auto-selection process for fixes for stable kernels should have direct input and involvement from subsystems for run-time testing and simply guessing or assuming won't suffice.

Quoted 6 lines

Quoted 6 lines
The days of just compile testing should be way over by now, and we should expect no less for stable kernels, *specially* if we start involving automation.
Would a way to *start* to address this long term for XFS or other filesystems for auto-selection long-term be a topic worth covering / addressing at LSF/MM?

If this is something the FS/MM folks would like to discuss I'd be happy to attend. I do plan on pushing this to other subsystems (and given that I'd support xfstests, fs/ is a good candidate) once the infrastructure + XFS specific stuff is done (hopefully later today).

Quoted 8 lines

It would be cool to tie tests to commit numbers for things where we're making sure a oops/hang doesn't happen again, but honestly I'm not sure it's worth the effort. Maybe this is my upstream bias showing, but I only ever run xfstests against something relatively close to linus, so I'm not super worried about ./check -g auto eating my box. I expect that if I run auto that everything minus the few flakey tests are going to pass.
Also TIL about configs/<host>.config, that's pretty fucking cool. Thanks,

On the other hand, from all the customers I've seen, none run anything "close" (<3 months delta?) from Linus. The "good" ones run 4.15.y, the worst are probably 4.1.y for upstream and the 2.6.32 RHEL ones.

So while making upstream bug-free is important, no one is actually using that code :)

By the time those customers end up using a 4.16 kernel, for example, they'll get all the fixes you're pushing in now, but they'll also be stuck with the new bugs that slipped in.

Either way, I've integrated xfstests to run the way Derrick and Dave recommended for every stable tree that I'm considering a backport of a patch to, I'll send another mail later today once I wrap it up.

--
Thanks,
Sasha

Sasha Levin

2018-03-30 02:47 UTC #28 lore raw

In reply to Dave Chinner

Quoted 16 lines · Dave Chinner

Quoted 3 lines · Sasha Levin
This is actually something I want maintainers to dictate. What sort of testing would make the XFS folks happy here? Right now I'm doing "./check 'xfs/*'" with xfstests. Is it sufficient? Anything else you'd like to see?
... and you're doing it wrong. This is precisely why being able to discover /exactly/ what you are testing and being able to browse the test results so we can find out if tests passed when a user reports a bug on a stable kernel.
The way you are running fstests skips more than half the test suite It also runs tests that are considered dangerous because they are likely to cause the test run to fail in some way (i.e. trigger an oops, hang the machine, leave a filesystem in an unmountable state, etc) and hence not complete a full pass.

Quoted 3 lines

"./check -g auto" runs the full "expected to pass" regression test suite for all configured test configurations. (i.e. all config sections listed in the configs/<host>.config file)

Great! With information from Darrick and yourself I've modified tests to be more relevant. Right now I run 4 configs for each stable kernel, but can add more or remove any - depends on what helps people analyse the results.

The complete VM serial logs as well as results/ from xfstests are also available and are linked from the email.

Here's an example of such email:

Quoted 53 lines

From: Sasha Levin <[email protected]>
To: Sasha Levin <[email protected]>
To: [email protected], "Darrick J . Wong" <[email protected]>
Cc: Brian Foster <[email protected]>, [email protected]
Subject: Re: [PATCH] xfs: Correctly invert xfs_buftarg LRU isolation logic
In-Reply-To: <[email protected]>
References: <[email protected]>
Hi Vratislav Bendel,
[This is an automated email]
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: Build OK!
v4.14.29: Build OK!
v4.9.89: Build OK!
v4.4.123: Build OK!
v4.1.50: Build OK!
v3.18.101: Build OK!
XFS Specific tests:
v4.15.12 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.15.12/tests/):
No tests completed!
v4.14.29 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.14.29/tests/):
No tests completed!
v4.9.89 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.9.89/tests/):
No tests completed!
v4.4.123 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.4.123/tests/):
    v4:
        Thu Mar 29 21:23:57 UTC 2018
        Interrupted!
        Passed all 0 tests
    v4_reflink:
        Thu Mar 29 21:24:37 UTC 2018
        Interrupted!
        Passed all 0 tests
v4.1.50 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.1.50/tests/):
    No tests completed!
v3.18.101 (http://stable-bot.westus2.cloudapp.azure.com/test/v3.18.101/tests/):
    v4:
        Thu Mar 29 21:30:40 UTC 2018
        Interrupted!
        Passed all 0 tests
    v4_reflink:
        Thu Mar 29 21:25:14 UTC 2018
        Interrupted!
        Passed all 0 tests

Quoted 5 lines

Please let us know if you'd like to have this patch included in a stable tree.
--
Thanks,
Sasha

Let me know if this would be good enough for now, and if there's anything else to add that'll be useful.

This brings me to the sad part of this mail: not a single stable kernel survived a run. Most are paniced, some are hanging, and some were killed because of KASan.

All have hit various warnings in fs/iomap.c, and kernels accross several versions hit the BUG at fs/xfs/xfs_message.c:113 (+-1 line)

4.15.12 is hitting a use-after-free in xfs_efi_release(). 4.14.29 and 4.9.89 seems to end up with corrupted memory (KASAN warnings) at or before generic/027.
And finally, 3.18.101 is pretty unhappy with sleeping functions called from atomic context.

--
Thanks,
Sasha

Luis R. Rodriguez

2018-03-30 19:49 UTC #29 lore raw

Quoted 6 lines · Sasha Levin

Quoted 4 lines · Dave Chinner
On Wed, Mar 28, 2018 at 07:30:06PM +0000, Sasha Levin wrote:
"./check -g auto" runs the full "expected to pass" regression test suite for all configured test configurations. (i.e. all config sections listed in the configs/<host>.config file)

Quoted 7 lines

Great! With information from Darrick and yourself I've modified tests to be more relevant. Right now I run 4 configs for each stable kernel, but can add more or remove any - depends on what helps people analyse the results.
This brings me to the sad part of this mail: not a single stable kernel survived a run. Most are paniced, some are hanging,

I expected this. The semantics over -g auto yielding "expected to pass" are relative. Perhaps its better described as "should pass"?

Quoted 6 lines

and some were killed
because of KASan.
All have hit various warnings in fs/iomap.c, and kernels accross several versions hit the BUG at fs/xfs/xfs_message.c:113 (+-1 line)

Quoted 7 lines

4.15.12 is hitting a use-after-free in xfs_efi_release(). 4.14.29 and 4.9.89 seems to end up with corrupted memory (KASAN warnings) at or before generic/027.
And finally, 3.18.101 is pretty unhappy with sleeping functions called from atomic context.
From my limited experience you have no option but to create an expunge list for

each failure for now, and then pass the expunge lists -- that in essence would define the stable baseline and you should expect this to be different per kernel release. If you upgrade tooling, it can also change the results, and likewise if you upgrade fstests.

If you define an expunge list you can then pass the list with the -E parameter, you can for instance categorize then failures by type and use a file for each type of failure, whether that's a triage list or a type of common failure. Format can be:

test # comments are ignored

Since you may want to database this somehow, perhaps a format that lists some tracking for it or other heuristics:

generic/388 # bug#12345 - 1/300 run fails

I'd recommend to just add all failures to one large expunge list for now, and later you can split / sort them them as needed.

The idea later is that any failure later would be a regression. What would be good is to test a stable kernel prior to the auto-selection and use that as baseline, then bump the kernel and ensure no regressions were created.

A dicey corner issue is that of tests which are supposed to "pass" but yet can fail every blue moon. For instance I've been running into one-off failures with generic/388 -- but only if I run it over 300 times.

As such the baseline IMHO should also track these as just failures, however it will be often picked up as a regression first. The only way to rule this out is to loop test the same test prior to a kernel update and ensure it wasn't a regression -- ie, that it *was* still failing before.

This is why all this work is rather full time'ish. There is no way around it, it will take time to establish a baseline from fstests for a filesystem. There will also be a lot of odd ins and outs of each filesystem.

Luis

Sasha Levin

2018-04-02 00:35 UTC #30 lore raw

Quoted 55 lines · Luis R. Rodriguez

Quoted 6 lines · Sasha Levin
Quoted 4 lines · Dave Chinner
On Wed, Mar 28, 2018 at 07:30:06PM +0000, Sasha Levin wrote:
"./check -g auto" runs the full "expected to pass" regression test suite for all configured test configurations. (i.e. all config sections listed in the configs/<host>.config file)
Quoted 7 lines
Great! With information from Darrick and yourself I've modified tests to be more relevant. Right now I run 4 configs for each stable kernel, but can add more or remove any - depends on what helps people analyse the results.
This brings me to the sad part of this mail: not a single stable kernel survived a run. Most are paniced, some are hanging,
I expected this. The semantics over -g auto yielding "expected to pass" are relative. Perhaps its better described as "should pass"?
Quoted 11 lines
and some were killed
because of KASan.
All have hit various warnings in fs/iomap.c, and kernels accross several versions hit the BUG at fs/xfs/xfs_message.c:113 (+-1 line)
4.15.12 is hitting a use-after-free in xfs_efi_release(). 4.14.29 and 4.9.89 seems to end up with corrupted memory (KASAN warnings) at or before generic/027.
And finally, 3.18.101 is pretty unhappy with sleeping functions called from atomic context.
From my limited experience you have no option but to create an expunge list for each failure for now, and then pass the expunge lists -- that in essence would define the stable baseline and you should expect this to be different per kernel release. If you upgrade tooling, it can also change the results, and likewise if you upgrade fstests.
If you define an expunge list you can then pass the list with the -E parameter, you can for instance categorize then failures by type and use a file for each type of failure, whether that's a triage list or a type of common failure. Format can be:
test # comments are ignored
Since you may want to database this somehow, perhaps a format that lists some tracking for it or other heuristics:
generic/388 # bug#12345 - 1/300 run fails
I'd recommend to just add all failures to one large expunge list for now, and later you can split / sort them them as needed.
The idea later is that any failure later would be a regression. What would be good is to test a stable kernel prior to the auto-selection and use that as baseline, then bump the kernel and ensure no regressions were created.

Quoted 8 lines

A dicey corner issue is that of tests which are supposed to "pass" but yet can fail every blue moon. For instance I've been running into one-off failures with generic/388 -- but only if I run it over 300 times.
As such the baseline IMHO should also track these as just failures, however it will be often picked up as a regression first. The only way to rule this out is to loop test the same test prior to a kernel update and ensure it wasn't a regression -- ie, that it *was* still failing before.

Thanks for the pointers!

Quoted 3 lines

This is why all this work is rather full time'ish. There is no way around it, it will take time to establish a baseline from fstests for a filesystem. There will also be a lot of odd ins and outs of each filesystem.

Right, but the way I see it, no one actually uses upstream. If anything, it's a development branch, and the "real" users pick up one of the stable trees to work with. So while there seems to be a lot of effort dedicated to new features or fixing upstream bugs, not enough people care that no one won't see those fixes for a few years.

--
Thanks,
Sasha

Dave Chinner

2018-03-31 22:02 UTC #31 lore raw

In reply to Sasha Levin

Quoted 14 lines · Sasha Levin

Quoted 13 lines · Dave Chinner
On Wed, Mar 28, 2018 at 07:30:06PM +0000, Sasha Levin wrote:
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: Build OK!
v4.14.29: Build OK!
v4.9.89: Build OK!
v4.4.123: Build OK!
v4.1.50: Build OK!
v3.18.101: Build OK!

Quoted 4 lines

Quoted 4 lines
XFS Specific tests:
v4.15.12 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.15.12/tests/):
No tests completed!

Can you capture the actual check command output into it's own file? That tells us at a glance which tests succeed or failed.

So I'm looking at the v5.log file:

....
echo 'export MKFS_OPTIONS='\''-m crc=0,reflink=0,rmapbt=0, -i sparse=0,'\'''
....

FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 autosel 4.15.12+
MKFS_OPTIONS  -- -f -m crc=0,reflink=0,rmapbt=0, -i sparse=0,

/dev/vdb
MOUNT_OPTIONS -- /dev/vdb /mnt2

That's not testing v5 filesystems. That's turned off crcs, and so is testing a v4 filesystem. You'll see this on filesysetms that don't support reflink:

[not run] Reflink not supported by test filesystem type: xfs

Also, you need to make the test filesystem to match the options the test run is configured with (i.e. v4, v5, reflink, etc) otherwise half the tests don't exercise the expected config.

[not run] src/dbtest not built

[not run] chacl command not found

[not run] xfs_io set_encpolicy support is missing

You need to update your userspace.

And the test run has not completed. It's run to:

generic/430 [11172.480621] run fstests generic/430 at 2018-03-30 00:20:12
+ scp -i /home/sasha/ssh/id_rsa -P 10022 -r [email protected]:/root/xfstests-dev/results /home/sasha/data/results/test/v4.15.12/tests//v5/
+ az vm delete -y --resource-group sasha-auto-stable --name sasha-worker-629016242-vm

generic/430 and then stopped. There's still another ~50 tests in the generic group to run, and then there's the shared and XFS subdirs to run, too. So there's still something wrong in the way you are setting up/installing fstests here....

Quoted 10 lines

Quoted 10 lines
v4.14.29 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.14.29/tests/):
No tests completed!
v4.9.89 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.9.89/tests/):
No tests completed!
v4.4.123 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.4.123/tests/):
    v4:
        Thu Mar 29 21:23:57 UTC 2018
        Interrupted!
        Passed all 0 tests
    v4_reflink:

There's no such configuration as "v4 reflink". reflink is only available on v5 (crc enabled) filesystems on kernels >=4.10 (IIRC). Perhaps you've mislabelled them?

Quoted 8 lines

Let me know if this would be good enough for now, and if there's anything else to add that'll be useful.
This brings me to the sad part of this mail: not a single stable kernel survived a run. Most are paniced, some are hanging, and some were killed because of KASan.
All have hit various warnings in fs/iomap.c,

Normal - the dmesg filter in the test harness catches those and ignores them if they are known/expected to occur.

Quoted 2 lines

and kernels accross several
versions hit the BUG at fs/xfs/xfs_message.c:113 (+-1 line)

That's an ASSERT() failure, indicating a fatal error. e.g:

Stuff like this (from
http://stable-bot.westus2.cloudapp.azure.com/test/v4.9.89/tests/v4_reflink.log)

.....
generic/083 [ 4443.536212] run fstests generic/083 at 2018-03-29 22:32:17

[ 4444.557989] XFS (vdb): Unmounting Filesystem
[ 4445.498461] XFS (vdb): EXPERIMENTAL reverse mapping btree feature enabled. Use at your own risk!
[ 4445.505860] XFS (vdb): EXPERIMENTAL reflink feature enabled. Use at your own risk!
[ 4445.513090] XFS (vdb): Mounting V5 Filesystem
[ 4445.531284] XFS (vdb): Ending clean mount
[ 4458.087406] XFS: Assertion failed: xfs_is_reflink_inode(ip), file: fs/xfs/xfs_reflink.c, line: 509

[snip stack trace]

Indicate a problem that should not be occurring. It's debug an triage time - there's some problem that needs backports to fix. I doubt anyone in XFS land has time to do this on top of everything else we alreayd have to do...

Quoted 1 line

4.15.12 is hitting a use-after-free in xfs_efi_release().

Debug and triage time.

Quoted 2 lines

4.14.29 and 4.9.89 seems to end up with corrupted memory (KASAN warnings) at or before generic/027.

More debug and triage time.

Quoted 2 lines

And finally, 3.18.101 is pretty unhappy with sleeping functions called from atomic context.

Needle in a haystack :/

So this is just basic XFS validation, and it's throwing problems up all over the place. Now do you see why we've been saying maintaining stable backports for XFS is pretty much a full time job for someone?

And keep in mind this is just one filesystem. You're going to end up with the same issues on ext4 and btrfs - the regression tests are going to show up all sorts of problems that have been fixed in the upstream kernels but never backported....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Sasha Levin

2018-04-02 00:32 UTC #32 lore raw

Quoted 33 lines · Dave Chinner

Quoted 14 lines · Sasha Levin
Quoted 13 lines · Dave Chinner
On Wed, Mar 28, 2018 at 07:30:06PM +0000, Sasha Levin wrote:
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: Build OK!
v4.14.29: Build OK!
v4.9.89: Build OK!
v4.4.123: Build OK!
v4.1.50: Build OK!
v3.18.101: Build OK!
Quoted 4 lines
Quoted 4 lines
XFS Specific tests:
v4.15.12 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.15.12/tests/):
No tests completed!
Can you capture the actual check command output into it's own file? That tells us at a glance which tests succeed or failed.
So I'm looking at the v5.log file:
....
echo 'export MKFS_OPTIONS='\''-m crc=0,reflink=0,rmapbt=0, -i sparse=0,'\'''
....
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 autosel 4.15.12+
MKFS_OPTIONS  -- -f -m crc=0,reflink=0,rmapbt=0, -i sparse=0,

Quoted 8 lines

/dev/vdb
MOUNT_OPTIONS -- /dev/vdb /mnt2
That's not testing v5 filesystems. That's turned off crcs, and so is testing a v4 filesystem. You'll see this on filesysetms that don't support reflink:
[not run] Reflink not supported by test filesystem type: xfs

I mixed up the configs here. Basically I have 4 different ones (provided by Darrick):

Quoted 11 lines

Also, you need to make the test filesystem to match the options the test run is configured with (i.e. v4, v5, reflink, etc) otherwise half the tests don't exercise the expected config.
[not run] src/dbtest not built
[not run] chacl command not found
[not run] xfs_io set_encpolicy support is missing
You need to update your userspace.

I thought xfsprogs in Ubuntu 18.04 is more recent, but is really 1 year old. Instead, I'm just building it from source now.

Quoted 10 lines

And the test run has not completed. It's run to:
generic/430 [11172.480621] run fstests generic/430 at 2018-03-30 00:20:12
+ scp -i /home/sasha/ssh/id_rsa -P 10022 -r [email protected]:/root/xfstests-dev/results /home/sasha/data/results/test/v4.15.12/tests//v5/
+ az vm delete -y --resource-group sasha-auto-stable --name sasha-worker-629016242-vm
generic/430 and then stopped. There's still another ~50 tests in the generic group to run, and then there's the shared and XFS subdirs to run, too. So there's still something wrong in the way you are setting up/installing fstests here....

I had a timeout I forgot about. There tests do take a while... :)

Quoted 60 lines

Quoted 10 lines
Quoted 10 lines
v4.14.29 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.14.29/tests/):
No tests completed!
v4.9.89 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.9.89/tests/):
No tests completed!
v4.4.123 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.4.123/tests/):
    v4:
        Thu Mar 29 21:23:57 UTC 2018
        Interrupted!
        Passed all 0 tests
    v4_reflink:
There's no such configuration as "v4 reflink". reflink is only available on v5 (crc enabled) filesystems on kernels >=4.10 (IIRC). Perhaps you've mislabelled them?
Quoted 8 lines
Let me know if this would be good enough for now, and if there's anything else to add that'll be useful.
This brings me to the sad part of this mail: not a single stable kernel survived a run. Most are paniced, some are hanging, and some were killed because of KASan.
All have hit various warnings in fs/iomap.c,
Normal - the dmesg filter in the test harness catches those and ignores them if they are known/expected to occur.
Quoted 2 lines
and kernels accross several
versions hit the BUG at fs/xfs/xfs_message.c:113 (+-1 line)
That's an ASSERT() failure, indicating a fatal error. e.g:
Stuff like this (from
http://stable-bot.westus2.cloudapp.azure.com/test/v4.9.89/tests/v4_reflink.log)
.....
generic/083 [ 4443.536212] run fstests generic/083 at 2018-03-29 22:32:17
[ 4444.557989] XFS (vdb): Unmounting Filesystem
[ 4445.498461] XFS (vdb): EXPERIMENTAL reverse mapping btree feature enabled. Use at your own risk!
[ 4445.505860] XFS (vdb): EXPERIMENTAL reflink feature enabled. Use at your own risk!
[ 4445.513090] XFS (vdb): Mounting V5 Filesystem
[ 4445.531284] XFS (vdb): Ending clean mount
[ 4458.087406] XFS: Assertion failed: xfs_is_reflink_inode(ip), file: fs/xfs/xfs_reflink.c, line: 509
[snip stack trace]
Indicate a problem that should not be occurring. It's debug an triage time - there's some problem that needs backports to fix. I doubt anyone in XFS land has time to do this on top of everything else we alreayd have to do...
Quoted 1 line
4.15.12 is hitting a use-after-free in xfs_efi_release().
Debug and triage time.
Quoted 2 lines
4.14.29 and 4.9.89 seems to end up with corrupted memory (KASAN warnings) at or before generic/027.
More debug and triage time.

Quoted 8 lines

Quoted 2 lines
And finally, 3.18.101 is pretty unhappy with sleeping functions called from atomic context.
Needle in a haystack :/
So this is just basic XFS validation, and it's throwing problems up all over the place. Now do you see why we've been saying maintaining stable backports for XFS is pretty much a full time job for someone?

Maintaining stable backports is indeed lots of work, but I feel that a lot of progress can be made by automating parts of it.

Quoted 4 lines

And keep in mind this is just one filesystem. You're going to end up with the same issues on ext4 and btrfs - the regression tests are going to show up all sorts of problems that have been fixed in the upstream kernels but never backported....

Right, but this effort will both make it harder for new regressions to creep in, and will make it easier to backport fixes for these issues much easier.

I've tried running it on current mainline (that was about 12 hours before v4.16 was released), and saw some failures there, in particular the use-after-free which seems to be caused by generic/388:

[25302.342348] ==================================================================
[25302.348851] BUG: KASAN: use-after-free in xfs_rui_release+0x1ce/0x220
[25302.355334] Read of size 4 at addr ffff8801600f3630 by task fsstress/6274
[25302.360678]
[25302.360678] CPU: 7 PID: 6274 Comm: fsstress Not tainted 4.16.0-rc7+ #22
[25302.360678] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[25302.360678] Call Trace:
[25302.360678]  dump_stack+0xd6/0x184
[25302.360678]  ? _atomic_dec_and_lock+0x2cc/0x2cc
[25302.360678]  ? show_regs_print_info+0x5/0x5
[25302.360678]  print_address_description+0x75/0x430
[25302.360678]  ? xfs_rui_release+0x1ce/0x220
[25302.360678]  kasan_report+0x1d8/0x460
[25302.360678]  ? xfs_rui_release+0x1ce/0x220
[25302.360678]  xfs_rui_release+0x1ce/0x220
[25302.407137]  ? xfs_rui_copy_format+0x100/0x100
[25302.407137]  xfs_defer_trans_abort+0x1e6/0xac0
[25302.407137]  ? xfs_defer_intake_work+0xad0/0xad0
[25302.407137]  ? xfs_trans_free_items+0x2e0/0x2e0
[25302.407137]  ? __lock_is_held+0xcb/0x1a0
[25302.407137]  xfs_defer_trans_roll+0x727/0xe60
[25302.407137]  ? xfs_defer_trans_abort+0xac0/0xac0
[25302.407137]  ? xfs_bmap_add_extent_hole_delay+0x1320/0x1320
[25302.407137]  ? __lock_is_held+0xcb/0x1a0
[25302.407137]  xfs_defer_finish+0x2a6/0x2140
[25302.407137]  ? xfs_trans_log_inode+0xa5/0x5b0
[25302.407137]  ? xfs_defer_bjoin+0x100/0x100
[25302.407137]  ? xfs_trans_ichgtime+0x1a0/0x1a0
[25302.448302]  ? save_stack+0x89/0xb0
[25302.448302]  ? xfs_bmapi_write+0x275c/0x4e20
[25302.448302]  ? do_syscall_64+0x24b/0x8b0
[25302.460213]  ? entry_SYSCALL_64_after_hwframe+0x42/0xb7
[25302.460213]  ? __bfs+0xaa0/0xaf0
[25302.460213]  ? xfs_bmapi_reserve_delalloc+0xd20/0xd20
[25302.460213]  ? __lock_is_held+0xcb/0x1a0
[25302.460213]  ? __lock_is_held+0xcb/0x1a0
[25302.460213]  ? rcu_read_lock_sched_held+0x102/0x120
[25302.482063]  ? xfs_defer_init+0x433/0x5f0
[25302.482063]  ? xfs_trans_unreserve_and_mod_sb+0xc10/0xc10
[25302.490331]  xfs_alloc_file_space+0x53a/0xf90
[25302.490331]  ? xfs_prepare_shift+0x160/0x160
[25302.498803]  ? lock_acquire+0x1c0/0x600
[25302.498803]  ? xfs_ilock+0x42b/0x670
[25302.498803]  ? lock_contended+0x1330/0x1330
[25302.498803]  ? down_write_nested+0xf2/0x190
[25302.498803]  ? _down_write_nest_lock+0x190/0x190
[25302.498803]  ? xfs_get_cowextsz_hint+0xa0/0xa0
[25302.498803]  ? xfs_break_layouts+0x2c/0x350
[25302.498803]  xfs_file_fallocate+0x5c6/0xac0
[25302.498803]  ? xfs_update_prealloc_flags+0x370/0x370
[25302.498803]  ? __lock_is_held+0xcb/0x1a0
[25302.498803]  ? rcu_read_lock_sched_held+0x102/0x120
[25302.498803]  ? rcu_sync_lockdep_assert+0xbc/0x150
[25302.498803]  vfs_fallocate+0x2f5/0x930
[25302.498803]  ioctl_preallocate+0x1dc/0x320
[25302.498803]  ? vfs_ioctl+0xf0/0xf0
[25302.550185]  do_vfs_ioctl+0xfe4/0x1690
[25302.550185]  ? cp_new_stat+0x66a/0x8e0
[25302.550185]  ? inode_get_bytes+0xc0/0xc0
[25302.550185]  ? ioctl_preallocate+0x320/0x320
[25302.550185]  ? fget_raw+0x10/0x10
[25302.550185]  ? vfs_statx_fd+0x3f/0x70
[25302.550185]  ? SYSC_newfstat+0x7c/0xd0
[25302.570302]  ? vfs_statx_fd+0x70/0x70
[25302.570302]  SyS_ioctl+0x6f/0x80
[25302.570302]  ? do_vfs_ioctl+0x1690/0x1690
[25302.570302]  do_syscall_64+0x24b/0x8b0
[25302.570302]  ? exit_to_usermode_loop+0xdf/0x1f0
[25302.570302]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[25302.570302]  ? syscall_return_slowpath+0x560/0x560
[25302.590209]  ? syscall_return_slowpath+0x21f/0x560
[25302.590209]  ? entry_SYSCALL_64_after_hwframe+0x52/0xb7
[25302.600302]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[25302.600302]  entry_SYSCALL_64_after_hwframe+0x42/0xb7
[25302.607106] RIP: 0033:0x7fb0455315d7
[25302.607106] RSP: 002b:00007fffc620fef8 EFLAGS: 00000202 ORIG_RAX: 0000000000000010
[25302.607106] RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007fb0455315d7
[25302.607106] RDX: 00007fffc620ff20 RSI: 000000004030582a RDI: 0000000000000003
[25302.607106] RBP: 00000000000000dd R08: 000000000000007d R09: feff54bcff603065
[25302.607106] R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000051873
[25302.607106] R13: 0000000051eb851f R14: 00007fffc62104b6 R15: 000055be2ca263e0
[25302.607106]
[25302.607106] Allocated by task 6274:
[25302.607106]  kasan_kmalloc+0xa0/0xd0
[25302.649054]  kmem_cache_alloc+0x109/0x450
[25302.649054]  kmem_zone_alloc+0x67/0x180
[25302.649054]  xfs_rui_init+0xbd/0x2e0
[25302.649054]  xfs_rmap_update_create_intent+0x43/0xd0
[25302.660185]  xfs_defer_intake_work+0x184/0xad0
[25302.660185]  xfs_defer_finish+0x29b/0x2140
[25302.660185]  xfs_alloc_file_space+0x53a/0xf90
[25302.660185]  xfs_file_fallocate+0x5c6/0xac0
[25302.660185]  vfs_fallocate+0x2f5/0x930
[25302.680347]  ioctl_preallocate+0x1dc/0x320
[25302.680347]  do_vfs_ioctl+0xfe4/0x1690
[25302.680347]  SyS_ioctl+0x6f/0x80
[25302.680347]  do_syscall_64+0x24b/0x8b0
[25302.680347]  entry_SYSCALL_64_after_hwframe+0x42/0xb7
[25302.680347]
[25302.680347] Freed by task 6274:
[25302.680347]  __kasan_slab_free+0x136/0x180
[25302.700464]  kmem_cache_free+0xe7/0x4b0
[25302.700464]  xfs_trans_free_items+0x198/0x2e0
[25302.700464]  __xfs_trans_commit+0x27f/0xcc0
[25302.711238]  xfs_trans_roll+0x17b/0x2a0
[25302.713745]  xfs_defer_trans_roll+0x6ad/0xe60
[25302.713745]  xfs_defer_finish+0x2a6/0x2140
[25302.713745]  xfs_alloc_file_space+0x53a/0xf90
[25302.713745]  xfs_file_fallocate+0x5c6/0xac0
[25302.713745]  vfs_fallocate+0x2f5/0x930
[25302.713745]  ioctl_preallocate+0x1dc/0x320
[25302.713745]  do_vfs_ioctl+0xfe4/0x1690
[25302.713745]  SyS_ioctl+0x6f/0x80
[25302.713745]  do_syscall_64+0x24b/0x8b0
[25302.713745]  entry_SYSCALL_64_after_hwframe+0x42/0xb7
[25302.713745]
[25302.713745] The buggy address belongs to the object at ffff8801600f35a8
[25302.713745]  which belongs to the cache xfs_rui_item of size 680
[25302.757350] The buggy address is located 136 bytes inside of
[25302.757350]  680-byte region [ffff8801600f35a8, ffff8801600f3850)
[25302.757350] The buggy address belongs to the page:
[25302.757350] page:ffffea0005803c00 count:1 mapcount:0 mapping:0000000000000000 index:0x0 compound_mapcount: 0
[25302.757350] flags: 0x2fffc0000008100(slab|head)
[25302.757350] raw: 02fffc0000008100 0000000000000000 0000000000000000 0000000180140014
[25302.757350] raw: ffffea000e4da600 0000000500000005 ffff8803a707c300 0000000000000000
[25302.757350] page dumped because: kasan: bad access detected
[25302.757350]
[25302.757350] Memory state around the buggy address:
[25302.757350]  ffff8801600f3500: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc
[25302.757350]  ffff8801600f3580: fc fc fc fc fc fb fb fb fb fb fb fb fb fb fb fb
[25302.757350] >ffff8801600f3600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[25302.757350]                                      ^
[25302.757350]  ffff8801600f3680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[25302.757350]  ffff8801600f3700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[25302.757350] ==================================================================

--
Thanks,
Sasha

Dave Chinner

2018-04-03 01:46 UTC #33 lore raw

Quoted 43 lines · Sasha Levin

Quoted 33 lines · Dave Chinner
Quoted 14 lines · Sasha Levin
Quoted 13 lines · Dave Chinner
On Wed, Mar 28, 2018 at 07:30:06PM +0000, Sasha Levin wrote:
This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 6.4845)
The bot has tested the following trees: v4.15.12, v4.14.29, v4.9.89, v4.4.123, v4.1.50, v3.18.101.
v4.15.12: Build OK!
v4.14.29: Build OK!
v4.9.89: Build OK!
v4.4.123: Build OK!
v4.1.50: Build OK!
v3.18.101: Build OK!
Quoted 4 lines
Quoted 4 lines
XFS Specific tests:
v4.15.12 (http://stable-bot.westus2.cloudapp.azure.com/test/v4.15.12/tests/):
No tests completed!
Can you capture the actual check command output into it's own file? That tells us at a glance which tests succeed or failed.
So I'm looking at the v5.log file:
....
echo 'export MKFS_OPTIONS='\''-m crc=0,reflink=0,rmapbt=0, -i sparse=0,'\'''
....
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 autosel 4.15.12+
MKFS_OPTIONS  -- -f -m crc=0,reflink=0,rmapbt=0, -i sparse=0,
Quoted 8 lines
/dev/vdb
MOUNT_OPTIONS -- /dev/vdb /mnt2
That's not testing v5 filesystems. That's turned off crcs, and so is testing a v4 filesystem. You'll see this on filesysetms that don't support reflink:
[not run] Reflink not supported by test filesystem type: xfs

Quoted 7 lines

I mixed up the configs here. Basically I have 4 different ones (provided by Darrick):
MKFS_OPTIONS='-m reflink=1,rmapbt=1, -i sparse=1, -b size=1024,' MKFS_OPTIONS='-m reflink=1,rmapbt=1, -i sparse=1,' MKFS_OPTIONS='-m crc=0,reflink=0,rmapbt=0, -i sparse=0,' MKFS_OPTIONS='-m crc=0,reflink=0,rmapbt=0, -i sparse=0, -b size=512,'

That doesn't actually test the default options that most users will currently be using. It tests a new format relatively few people are using in production, and the old format that people are slowly moving away from. If you run mkfs.xfs without any options right now you get

MKFS_OPTIONS='-m crc=1,reflink=0,rmapbt=0, -i sparse=0'

Quoted 6 lines

Quoted 3 lines
So this is just basic XFS validation, and it's throwing problems up all over the place. Now do you see why we've been saying maintaining stable backports for XFS is pretty much a full time job for someone?
Maintaining stable backports is indeed lots of work, but I feel that a lot of progress can be made by automating parts of it.

Yup, but the sorting out the results of the automated tests is the part that takes all the developer time and resources, not the running of the tests...

Quoted 8 lines

Quoted 4 lines
And keep in mind this is just one filesystem. You're going to end up with the same issues on ext4 and btrfs - the regression tests are going to show up all sorts of problems that have been fixed in the upstream kernels but never backported....
Right, but this effort will both make it harder for new regressions to creep in, and will make it easier to backport fixes for these issues much easier.

Assuming someone has the time to actually look at all these problems....

Quoted 13 lines

I've tried running it on current mainline (that was about 12 hours before v4.16 was released), and saw some failures there, in particular the use-after-free which seems to be caused by generic/388:

[25302.342348] ==================================================================
[25302.348851] BUG: KASAN: use-after-free in xfs_rui_release+0x1ce/0x220
[25302.355334] Read of size 4 at addr ffff8801600f3630 by task fsstress/6274
[25302.360678]
[25302.360678] CPU: 7 PID: 6274 Comm: fsstress Not tainted 4.16.0-rc7+ #22
[25302.360678] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[25302.360678] Call Trace:
[25302.360678]  dump_stack+0xd6/0x184
[25302.360678]  ? _atomic_dec_and_lock+0x2cc/0x2cc

Quoted 8 lines

[25302.360678]  ? show_regs_print_info+0x5/0x5
[25302.360678]  print_address_description+0x75/0x430
[25302.360678]  ? xfs_rui_release+0x1ce/0x220
[25302.360678]  kasan_report+0x1d8/0x460
[25302.360678]  ? xfs_rui_release+0x1ce/0x220
[25302.360678]  xfs_rui_release+0x1ce/0x220
[25302.407137]  ? xfs_rui_copy_format+0x100/0x100
[25302.407137]  xfs_defer_trans_abort+0x1e6/0xac0

I'm sure this was fixed. It's simply a case of having the code call the release function rather than freeing the item it directly on abort.

But I can't find the patch, commit, etc.

Ah, a drive-by bug fix back to the EFI code in January, didn't pass review, was not followed up on with fixes required. Just posted a patch to fix it.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]