[PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

Hi all!

During LSFMM 2022, I have had an opportunity to speak with developers
from several different companies that showed interest in collaborating
on the effort of improving the state of xfs code in LTS kernels.

I would like to kick-off this effort for the 5.10 LTS kernel, in the
hope that others will join me in the future to produce a better common
baseline for everyone to build on.

This is the first of 6 series of stable patch candidates that
I collected from xfs releases v5.11..v5.18 [1].

My intention is to post the parts for review on the xfs list on
a ~weekly basis and forward them to stable only after xfs developers
have had the chance to review the selection.

I used a gadget that I developed "b4 rn" that produces high level
"release notes" with references to the posted patch series and also
looks for mentions of fstest names in the discussions on lore.
I then used an elimination process to select the stable tree candidate
patches. The selection process is documented in the git log of [1].

After I had candidates, Luis has helped me to set up a kdevops testing
environment on a server that Samsung has contributed to the effort.
Luis and I have spent a considerable amount of time to establish the
expunge lists that produce stable baseline results for v5.10.y [2].
Eventually, we ran the auto group test over 100 times to sanitize the
baseline, on the following configurations:
reflink_normapbt (default), reflink, reflink_1024, nocrc, nocrc_512.

The patches in this part are from circa v5.11 release.
They have been through 36 auto group runs with the configs listed above
and no regressions from baseline were observed.

At least two of the fixes have regression tests in fstests that were used
to verify the fix. I also annotated [3] the fix commits in the tests.

I would like to thank Luis for his huge part in this still ongoing effort
and I would like to thank Samsung for contributing the hardware resources
to drive this effort.

Your inputs on the selection in this part and in upcoming parts [1]
are most welcome!

Thanks,
Amir.

[1] https://github.com/amir73il/b4/blob/xfs-5.10.y/xfs-5.10..5.17-fixes.rst
[2] https://github.com/linux-kdevops/kdevops/tree/master/workflows/fstests/expunges/5.10.105/xfs/unassigned
[3] https://lore.kernel.org/fstests/20220520143249.2103631-1-amir73il@gmail.com/

Darrick J. Wong (3):
  xfs: detect overflows in bmbt records
  xfs: fix the forward progress assertion in xfs_iwalk_run_callbacks
  xfs: fix an ABBA deadlock in xfs_rename

Kaixu Xia (1):
  xfs: show the proper user quota options

 fs/xfs/libxfs/xfs_bmap.c    |  5 +++++
 fs/xfs/libxfs/xfs_dir2.h    |  2 --
 fs/xfs/libxfs/xfs_dir2_sf.c |  2 +-
 fs/xfs/xfs_inode.c          | 42 ++++++++++++++++++++++---------------
 fs/xfs/xfs_iwalk.c          |  2 +-
 fs/xfs/xfs_super.c          | 10 +++++----
 6 files changed, 38 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATH 5.10 1/4] xfs: detect overflows in bmbt records

[Amir Goldstein]

From: "Darrick J. Wong" <darrick.wong@oracle.com>

commit acf104c2331c1ba2a667e65dd36139d1555b1432 upstream.

Detect file block mappings with a blockcount that's either so large that
integer overflows occur or are zero, because neither are valid in the
filesystem.  Worse yet, attempting directory modifications causes the
iext code to trip over the bmbt key handling and takes the filesystem
down.  We can fix most of this by preventing the bad metadata from
entering the incore structures in the first place.

Found by setting blockcount=0 in a directory data fork mapping and
watching the fireworks.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index d9a692484eae..de9c27ef68d8 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6229,6 +6229,11 @@ xfs_bmap_validate_extent(
 	xfs_fsblock_t		endfsb;
 	bool			isrt;
 
+	if (irec->br_startblock + irec->br_blockcount <= irec->br_startblock)
+		return __this_address;
+	if (irec->br_startoff + irec->br_blockcount <= irec->br_startoff)
+		return __this_address;
+
 	isrt = XFS_IS_REALTIME_INODE(ip);
 	endfsb = irec->br_startblock + irec->br_blockcount - 1;
 	if (isrt && whichfork == XFS_DATA_FORK) {
-- 
2.25.1

[PATH 5.10 2/4] xfs: show the proper user quota options

[Amir Goldstein]

From: Kaixu Xia <kaixuxia@tencent.com>

commit 237d7887ae723af7d978e8b9a385fdff416f357b upstream.

The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
seems wrong, Fix it and show proper options.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_super.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e3e229e52512..5ebd6cdc44a7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -199,10 +199,12 @@ xfs_fs_show_options(
 		seq_printf(m, ",swidth=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
 
-	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
-		seq_puts(m, ",usrquota");
-	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
-		seq_puts(m, ",uqnoenforce");
+	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
+		if (mp->m_qflags & XFS_UQUOTA_ENFD)
+			seq_puts(m, ",usrquota");
+		else
+			seq_puts(m, ",uqnoenforce");
+	}
 
 	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_PQUOTA_ENFD)
-- 
2.25.1

[PATH 5.10 3/4] xfs: fix the forward progress assertion in xfs_iwalk_run_callbacks

[Amir Goldstein]

From: "Darrick J. Wong" <darrick.wong@oracle.com>

commit a5336d6bb2d02d0e9d4d3c8be04b80b8b68d56c8 upstream.

In commit 27c14b5daa82 we started tracking the last inode seen during an
inode walk to avoid infinite loops if a corrupt inobt record happens to
have a lower ir_startino than the record preceeding it.  Unfortunately,
the assertion trips over the case where there are completely empty inobt
records (which can happen quite easily on 64k page filesystems) because
we advance the tracking cursor without actually putting the empty record
into the processing buffer.  Fix the assert to allow for this case.

Reported-by: zlang@redhat.com
Fixes: 27c14b5daa82 ("xfs: ensure inobt record walks always make forward progress")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Zorro Lang <zlang@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_iwalk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
index 2a45138831e3..eae3aff9bc97 100644
--- a/fs/xfs/xfs_iwalk.c
+++ b/fs/xfs/xfs_iwalk.c
@@ -363,7 +363,7 @@ xfs_iwalk_run_callbacks(
 	/* Delete cursor but remember the last record we cached... */
 	xfs_iwalk_del_inobt(tp, curpp, agi_bpp, 0);
 	irec = &iwag->recs[iwag->nr_recs - 1];
-	ASSERT(next_agino == irec->ir_startino + XFS_INODES_PER_CHUNK);
+	ASSERT(next_agino >= irec->ir_startino + XFS_INODES_PER_CHUNK);
 
 	error = xfs_iwalk_ag_recs(iwag);
 	if (error)
-- 
2.25.1

[PATH 5.10 4/4] xfs: fix an ABBA deadlock in xfs_rename

[Amir Goldstein]

From: "Darrick J. Wong" <darrick.wong@oracle.com>

commit 6da1b4b1ab36d80a3994fd4811c8381de10af604 upstream.

When overlayfs is running on top of xfs and the user unlinks a file in
the overlay, overlayfs will create a whiteout inode and ask xfs to
"rename" the whiteout file atop the one being unlinked.  If the file
being unlinked loses its one nlink, we then have to put the inode on the
unlinked list.

This requires us to grab the AGI buffer of the whiteout inode to take it
off the unlinked list (which is where whiteouts are created) and to grab
the AGI buffer of the file being deleted.  If the whiteout was created
in a higher numbered AG than the file being deleted, we'll lock the AGIs
in the wrong order and deadlock.

Therefore, grab all the AGI locks we think we'll need ahead of time, and
in order of increasing AG number per the locking rules.

Reported-by: wenli xie <wlxie7296@gmail.com>
Fixes: 93597ae8dac0 ("xfs: Fix deadlock between AGI and AGF when target_ip exists in xfs_rename()")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.h    |  2 --
 fs/xfs/libxfs/xfs_dir2_sf.c |  2 +-
 fs/xfs/xfs_inode.c          | 42 ++++++++++++++++++++++---------------
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index e55378640b05..d03e6098ded9 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -47,8 +47,6 @@ extern int xfs_dir_lookup(struct xfs_trans *tp, struct xfs_inode *dp,
 extern int xfs_dir_removename(struct xfs_trans *tp, struct xfs_inode *dp,
 				struct xfs_name *name, xfs_ino_t ino,
 				xfs_extlen_t tot);
-extern bool xfs_dir2_sf_replace_needblock(struct xfs_inode *dp,
-				xfs_ino_t inum);
 extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp,
 				struct xfs_name *name, xfs_ino_t inum,
 				xfs_extlen_t tot);
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 2463b5d73447..8c4f76bba88b 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -1018,7 +1018,7 @@ xfs_dir2_sf_removename(
 /*
  * Check whether the sf dir replace operation need more blocks.
  */
-bool
+static bool
 xfs_dir2_sf_replace_needblock(
 	struct xfs_inode	*dp,
 	xfs_ino_t		inum)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 2bfbcf28b1bd..e958b1c74561 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3152,7 +3152,7 @@ xfs_rename(
 	struct xfs_trans	*tp;
 	struct xfs_inode	*wip = NULL;		/* whiteout inode */
 	struct xfs_inode	*inodes[__XFS_SORT_INODES];
-	struct xfs_buf		*agibp;
+	int			i;
 	int			num_inodes = __XFS_SORT_INODES;
 	bool			new_parent = (src_dp != target_dp);
 	bool			src_is_directory = S_ISDIR(VFS_I(src_ip)->i_mode);
@@ -3265,6 +3265,30 @@ xfs_rename(
 		}
 	}
 
+	/*
+	 * Lock the AGI buffers we need to handle bumping the nlink of the
+	 * whiteout inode off the unlinked list and to handle dropping the
+	 * nlink of the target inode.  Per locking order rules, do this in
+	 * increasing AG order and before directory block allocation tries to
+	 * grab AGFs because we grab AGIs before AGFs.
+	 *
+	 * The (vfs) caller must ensure that if src is a directory then
+	 * target_ip is either null or an empty directory.
+	 */
+	for (i = 0; i < num_inodes && inodes[i] != NULL; i++) {
+		if (inodes[i] == wip ||
+		    (inodes[i] == target_ip &&
+		     (VFS_I(target_ip)->i_nlink == 1 || src_is_directory))) {
+			struct xfs_buf	*bp;
+			xfs_agnumber_t	agno;
+
+			agno = XFS_INO_TO_AGNO(mp, inodes[i]->i_ino);
+			error = xfs_read_agi(mp, tp, agno, &bp);
+			if (error)
+				goto out_trans_cancel;
+		}
+	}
+
 	/*
 	 * Directory entry creation below may acquire the AGF. Remove
 	 * the whiteout from the unlinked list first to preserve correct
@@ -3317,22 +3341,6 @@ xfs_rename(
 		 * In case there is already an entry with the same
 		 * name at the destination directory, remove it first.
 		 */
-
-		/*
-		 * Check whether the replace operation will need to allocate
-		 * blocks.  This happens when the shortform directory lacks
-		 * space and we have to convert it to a block format directory.
-		 * When more blocks are necessary, we must lock the AGI first
-		 * to preserve locking order (AGI -> AGF).
-		 */
-		if (xfs_dir2_sf_replace_needblock(target_dp, src_ip->i_ino)) {
-			error = xfs_read_agi(mp, tp,
-					XFS_INO_TO_AGNO(mp, target_ip->i_ino),
-					&agibp);
-			if (error)
-				goto out_trans_cancel;
-		}
-
 		error = xfs_dir_replace(tp, target_dp, target_name,
 					src_ip->i_ino, spaceres);
 		if (error)
-- 
2.25.1

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Darrick J. Wong]

On Wed, May 25, 2022 at 02:17:11PM +0300, Amir Goldstein wrote:
> Hi all!
> 
> During LSFMM 2022, I have had an opportunity to speak with developers
> from several different companies that showed interest in collaborating
> on the effort of improving the state of xfs code in LTS kernels.
> 
> I would like to kick-off this effort for the 5.10 LTS kernel, in the
> hope that others will join me in the future to produce a better common
> baseline for everyone to build on.
> 
> This is the first of 6 series of stable patch candidates that
> I collected from xfs releases v5.11..v5.18 [1].
> 
> My intention is to post the parts for review on the xfs list on
> a ~weekly basis and forward them to stable only after xfs developers
> have had the chance to review the selection.
> 
> I used a gadget that I developed "b4 rn" that produces high level
> "release notes" with references to the posted patch series and also
> looks for mentions of fstest names in the discussions on lore.
> I then used an elimination process to select the stable tree candidate
> patches. The selection process is documented in the git log of [1].
> 
> After I had candidates, Luis has helped me to set up a kdevops testing
> environment on a server that Samsung has contributed to the effort.
> Luis and I have spent a considerable amount of time to establish the
> expunge lists that produce stable baseline results for v5.10.y [2].
> Eventually, we ran the auto group test over 100 times to sanitize the
> baseline, on the following configurations:
> reflink_normapbt (default), reflink, reflink_1024, nocrc, nocrc_512.
> 
> The patches in this part are from circa v5.11 release.
> They have been through 36 auto group runs with the configs listed above
> and no regressions from baseline were observed.

Woot!

> At least two of the fixes have regression tests in fstests that were used
> to verify the fix. I also annotated [3] the fix commits in the tests.
> 
> I would like to thank Luis for his huge part in this still ongoing effort
> and I would like to thank Samsung for contributing the hardware resources
> to drive this effort.
> 
> Your inputs on the selection in this part and in upcoming parts [1]
> are most welcome!

/me wonders if you need commit 9a5280b312e2 xfs: reorder iunlink remove
operation in xfs_ifree ?  Or did that one already get pulled in?

The changes proposed look reasonable to me, and moreso with the testing
to prove it.  Minor nit: patchsets should be tagged "PATCH", not "PATH".

Thanks to you and Luis for doing this work! :)

> Thanks,
> Amir.
> 
> [1] https://github.com/amir73il/b4/blob/xfs-5.10.y/xfs-5.10..5.17-fixes.rst
> [2] https://github.com/linux-kdevops/kdevops/tree/master/workflows/fstests/expunges/5.10.105/xfs/unassigned

/me looks and sees a large collection of expunge lists, along with
comments about how often failures occur and/or reasons.  Neat!

Leah mentioned on the ext4 call this morning that she would have found
it helpful to know (before she started working on 5.15 backports) which
tests were of the flaky variety so that she could better prioritize the
time she had to look into fstests failures.  (IOWS: saw a test fail a
small percentage of the time and then burned a lot of machine time only
to figure out that 5.15.0 also failed a percentage of th time).

We talked about where it would be most useful for maintainers and QA
people to store their historical pass/fail data, before settling on
"somewhere public where everyone can review their colleagues' notes" and
"somewhere minimizing commit friction".  At the time, we were thinking
about having people contribute their notes directly to the fstests
source code, but I guess Luis has been doing that in the kdevops repo
for a few years now.

So, maybe there?

--D

> [3] https://lore.kernel.org/fstests/20220520143249.2103631-1-amir73il@gmail.com/
> 
> Darrick J. Wong (3):
>   xfs: detect overflows in bmbt records
>   xfs: fix the forward progress assertion in xfs_iwalk_run_callbacks
>   xfs: fix an ABBA deadlock in xfs_rename
> 
> Kaixu Xia (1):
>   xfs: show the proper user quota options
> 
>  fs/xfs/libxfs/xfs_bmap.c    |  5 +++++
>  fs/xfs/libxfs/xfs_dir2.h    |  2 --
>  fs/xfs/libxfs/xfs_dir2_sf.c |  2 +-
>  fs/xfs/xfs_inode.c          | 42 ++++++++++++++++++++++---------------
>  fs/xfs/xfs_iwalk.c          |  2 +-
>  fs/xfs/xfs_super.c          | 10 +++++----
>  6 files changed, 38 insertions(+), 25 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Luis Chamberlain]

On Thu, May 26, 2022 at 10:27:41AM -0700, Darrick J. Wong wrote:
> /me looks and sees a large collection of expunge lists, along with
> comments about how often failures occur and/or reasons.  Neat!
> 
> Leah mentioned on the ext4 call this morning that she would have found
> it helpful to know (before she started working on 5.15 backports) which
> tests were of the flaky variety so that she could better prioritize the
> time she had to look into fstests failures.  (IOWS: saw a test fail a
> small percentage of the time and then burned a lot of machine time only
> to figure out that 5.15.0 also failed a percentage of th time).

See my proposal to try to make this easier to parse:

https://lore.kernel.org/all/YoW0ZC+zM27Pi0Us@bombadil.infradead.org/

> We talked about where it would be most useful for maintainers and QA
> people to store their historical pass/fail data, before settling on
> "somewhere public where everyone can review their colleagues' notes" and
> "somewhere minimizing commit friction".  At the time, we were thinking
> about having people contribute their notes directly to the fstests
> source code, but I guess Luis has been doing that in the kdevops repo
> for a few years now.
> 
> So, maybe there?

For now sure, I'm happy to add others the linux-kdevops org on github
and they get immediate write access to the repo. This is working well
so far. Long term we need to decide if we want to spin off the
expunge list as a separate effort and make it a git subtree (note
it is different than a git sub module). Another example of a use case
for a git subtree, to use it as an example, is the way I forked
kconfig from Linux into a standalone git tree so to allow any project
to bump kconfig code with just one command. So different projects
don't need to fork kconfig as they do today.

The value in doing the git subtree for expunges is any runner can use
it. I did design kdevops though to run on *any* cloud, and support
local virtualization tech like libvirt and virtualbox.

The linux-kdevops git org also has other projects which both fstest
and blktests depend on, so for example dbench which I had forked and
cleaned up a while ago. It may make sense to share keeping oddball
efforts like thse which are no longer maintained in this repo.

There is other tech I'm evaluating for this sort of collaborative test
efforts such as ledgers, but that is in its infancy at this point in
time. I have a sense though it may be a good outlet for collection of
test artifacts in a decentralized way and also allow *any* entity to
participate in bringing confidence to stable kernel branches or dev
branches prior to release.

  Luis

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

On Thu, May 26, 2022 at 9:44 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Thu, May 26, 2022 at 10:27:41AM -0700, Darrick J. Wong wrote:
> > /me looks and sees a large collection of expunge lists, along with
> > comments about how often failures occur and/or reasons.  Neat!
> >
> > Leah mentioned on the ext4 call this morning that she would have found
> > it helpful to know (before she started working on 5.15 backports) which
> > tests were of the flaky variety so that she could better prioritize the
> > time she had to look into fstests failures.  (IOWS: saw a test fail a
> > small percentage of the time and then burned a lot of machine time only
> > to figure out that 5.15.0 also failed a percentage of th time).
>
> See my proposal to try to make this easier to parse:
>
> https://lore.kernel.org/all/YoW0ZC+zM27Pi0Us@bombadil.infradead.org/
>
> > We talked about where it would be most useful for maintainers and QA
> > people to store their historical pass/fail data, before settling on
> > "somewhere public where everyone can review their colleagues' notes" and
> > "somewhere minimizing commit friction".  At the time, we were thinking
> > about having people contribute their notes directly to the fstests
> > source code, but I guess Luis has been doing that in the kdevops repo
> > for a few years now.
> >
> > So, maybe there?
>
> For now sure, I'm happy to add others the linux-kdevops org on github
> and they get immediate write access to the repo. This is working well
> so far. Long term we need to decide if we want to spin off the
> expunge list as a separate effort and make it a git subtree (note
> it is different than a git sub module). Another example of a use case
> for a git subtree, to use it as an example, is the way I forked
> kconfig from Linux into a standalone git tree so to allow any project
> to bump kconfig code with just one command. So different projects
> don't need to fork kconfig as they do today.
>
> The value in doing the git subtree for expunges is any runner can use
> it. I did design kdevops though to run on *any* cloud, and support
> local virtualization tech like libvirt and virtualbox.
>
> The linux-kdevops git org also has other projects which both fstest
> and blktests depend on, so for example dbench which I had forked and
> cleaned up a while ago. It may make sense to share keeping oddball
> efforts like thse which are no longer maintained in this repo.
>
> There is other tech I'm evaluating for this sort of collaborative test
> efforts such as ledgers, but that is in its infancy at this point in
> time. I have a sense though it may be a good outlet for collection of
> test artifacts in a decentralized way and also allow *any* entity to
> participate in bringing confidence to stable kernel branches or dev
> branches prior to release.
>

There are few problems I noticed with the current workflow.

1. It will not scale to maintain this in git as more and more testers
start using kdepops and adding more and more fs and configs and distros.
How many more developers you want to give push access to linux-kdevops?
I don't know how test labs report to KernelCI, but we need to look at their
model and not invent the wheel.

2. kdevops is very focused on stabilizing the baseline fast, which is
very good, but there is no good process of getting a test out of expunge list.
We have a very strong suspicion that some of the tests that we put in
expunge lists failed due to some setup issue in the host OS that caused
NVME IO errors in the guests. I tried to put that into comments when
I noticed that, but I am afraid there may have been other tests that are
falsely accused of failing. All developers make those mistakes in their
own expunge lists, but if we start propagating those mistakes to the world,
it becomes an issue.

For those two reasons I think that the model to aspire to should be
composed of a database where absolutely everyone can post data
point to in the form of facts (i.e. the test failed after N runs on this kernel
and this hardware...) and another process, partly AI, partly human to
digest all those facts into a knowledge base that is valuable and
maintained by experts. Much easier said than done...

Thanks,
Amir.

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Luis Chamberlain]

On Thu, May 26, 2022 at 09:59:19PM +0300, Amir Goldstein wrote:
> On Thu, May 26, 2022 at 9:44 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Thu, May 26, 2022 at 10:27:41AM -0700, Darrick J. Wong wrote:
> > > /me looks and sees a large collection of expunge lists, along with
> > > comments about how often failures occur and/or reasons.  Neat!
> > >
> > > Leah mentioned on the ext4 call this morning that she would have found
> > > it helpful to know (before she started working on 5.15 backports) which
> > > tests were of the flaky variety so that she could better prioritize the
> > > time she had to look into fstests failures.  (IOWS: saw a test fail a
> > > small percentage of the time and then burned a lot of machine time only
> > > to figure out that 5.15.0 also failed a percentage of th time).
> >
> > See my proposal to try to make this easier to parse:
> >
> > https://lore.kernel.org/all/YoW0ZC+zM27Pi0Us@bombadil.infradead.org/
> >
> > > We talked about where it would be most useful for maintainers and QA
> > > people to store their historical pass/fail data, before settling on
> > > "somewhere public where everyone can review their colleagues' notes" and
> > > "somewhere minimizing commit friction".  At the time, we were thinking
> > > about having people contribute their notes directly to the fstests
> > > source code, but I guess Luis has been doing that in the kdevops repo
> > > for a few years now.
> > >
> > > So, maybe there?
> >
> > For now sure, I'm happy to add others the linux-kdevops org on github
> > and they get immediate write access to the repo. This is working well
> > so far. Long term we need to decide if we want to spin off the
> > expunge list as a separate effort and make it a git subtree (note
> > it is different than a git sub module). Another example of a use case
> > for a git subtree, to use it as an example, is the way I forked
> > kconfig from Linux into a standalone git tree so to allow any project
> > to bump kconfig code with just one command. So different projects
> > don't need to fork kconfig as they do today.
> >
> > The value in doing the git subtree for expunges is any runner can use
> > it. I did design kdevops though to run on *any* cloud, and support
> > local virtualization tech like libvirt and virtualbox.
> >
> > The linux-kdevops git org also has other projects which both fstest
> > and blktests depend on, so for example dbench which I had forked and
> > cleaned up a while ago. It may make sense to share keeping oddball
> > efforts like thse which are no longer maintained in this repo.
> >
> > There is other tech I'm evaluating for this sort of collaborative test
> > efforts such as ledgers, but that is in its infancy at this point in
> > time. I have a sense though it may be a good outlet for collection of
> > test artifacts in a decentralized way and also allow *any* entity to
> > participate in bringing confidence to stable kernel branches or dev
> > branches prior to release.
> >
> 
> There are few problems I noticed with the current workflow.
> 
> 1. It will not scale to maintain this in git as more and more testers
> start using kdepops and adding more and more fs and configs and distros.

You say that but do not explain why you think this is the case.
Quite the contrary, I don't think so and I'll expain why. Let's
just stic to the expunge list as that is what matters here in this
context.

The expunge list is already divided by target kernels if using upstream
kernels by directory. So this applies to any stable kernel, vanilla
kernels, or linux-next. Folks working on these kernels would very likely
be collaborating just as you and I have.

Distro kernels also have their own directory, and so they'd very likely
collaborate.

> How many more developers you want to give push access to linux-kdevops?

Only those really collaborating, the idea is not to give access to the
world here. The challenge I'm thinking about for the future though is
how to scale this beyond just those few in a meaningful way in such a
way that you don't limit your scope of evaluation only to what resources
these folks have.

That is a research question and beyond the scope of just using git in a
shared linux repo.

> I don't know how test labs report to KernelCI, but we need to look at their
> model and not invent the wheel.

I looked at and to say the least I was not in any way shape or form
drawn to it or what it was using. You are free to look at it too.

The distributed aspect is what I don't agree with, and it is why I am
evaluating alternative decentralized technologies for the future.

It relies on a LAVA, Linaro Automated Validation Architecture. The
project home page to LAVA [0], mentions "LAVA is an automated validation
architecture primarily aimed at testing deployments of systems based
around the Linux kernel on ARM devices, specifically ARMv7 and later".
The SOC [1] page however now lists x86, but it is not the main focus of
the project. You can add a new test lab and add new tests, these tests
are intended to be public. If running tests for private consumption
you’d have to set up your own backend and front end. All this and the
experience with the results page was enough for me to decide this
wasn't an immediate good fit for automation for fstests and blktests
when I started considering this for enterprise Linux.

[0] https://git.lavasoftware.org
[1] https://linux.kernelci.org/soc/

It does not mean one cannot use a centralized methodology to share an
expunge list / artifacts, etc for fstests or blktests. A shared expunge
set on linux-kdevops organization is a *simple* centralized way to do that
to start off with, and if you limit access to folks who collaborate on
one directory (as you kind of do in Linux development with maintainers)
you avoid merge conflicts. We're not at a point yet where we're going to
have 100 folks who want access to say the v5.10.y directory for expunges
for XFS for example.... it's just you and me right now. Likewise for
other filesystems it would be similar. But from a research perspective
it does invite one to consider how to scale this in a sensible way
beyond those. When I looked at kernelci, I didn't personally think that
was an optimal way to scale, but that is beyond the scope of the simple
ramp up we're still discussing.

> 2. kdevops is very focused on stabilizing the baseline fast, 

Although it does help with this, I still think there is small efforts to
help automate this further in the future. A runner should be able to
spin this off without intervention if possible. Today upon failures it
requires manual verification, adding a new failure to an expunge list,
etc.  We can do better, and the goal is to slowly automate each of those
menial tasks which today we do manually. Building a completely new
baseline without manual intervention I think is possible and we should
strive towards it slowly and carefully.

> which is
> very good, but there is no good process of getting a test out of expunge list.

Yes, *part* of this involves a nice atomic task which can be dedicated to a
runner. So this goal alone needs to broken up in to parts:

a) Is this task still failing? --> easily automated today
b) How can we avoid this to fail --> not easily autmated today

As for a), a simple dedicated guest could for example evaluate a target kernel
on a fileystem configuration and run through each expunge and *verify* it
is indeed still failing. If it is not, and there is high confidence that
this is the case (say it verified that it is not failing many times), then
clearly the issue may have been fixed (say a stable kernel update) and
the task can inform us of that.

Thas task b) requires years, and years of work.

> We have a very strong suspicion that some of the tests that we put in
> expunge lists failed due to some setup issue in the host OS that caused
> NVME IO errors in the guests.

We already know that qemu with qcow2 nvme files does incur some delays
when doing full swing drive discards and this can cause some of these
nvme IO errors (timeouts). We now also are aware that the odds of this
timeout happening twice is also low but *is* possible. We *also* now
know that when two consecutive nvme timeoutes happen due to this it can
also *somehow* trigger an RCU false positive for blktests in some corner
cases when testing ZNS [0] but this was *what* made us realize that this
issue was a qemu issue and the qemu nvme maintainer has noted that
this needs to be fixed in qemu.

[0] https://lkml.kernel.org/r/YliZ9M6QWISXvhAJ@bombadil.infradead.org

But these sorts of qemu bugs should should not cause filesystem
issues. We also already know that this is a qemu bug and that this will
be fixed in the long term. Upon review with the qemu nvme maintainer
the way kdevops uses nvme is not incorrect.

Yes we can switch to raw format to skip the suboptimal way to do
discards, but we *want* to find more bugs, not less. We could
simply just make a new Kconfig entry on kdevops to enable users to use
raw files for the nvme drives for those that want to opt-out of these
timeouts for now.

> I tried to put that into comments when
> I noticed that, but I am afraid there may have been other tests that are
> falsely accused of failing.

There are two things we should consider in light of this:

c) We do need semantics for common exceptions to failures
d) We need an appreciation for why some of these exceptions may be
   real odd issues and it may take time to either fix them or
   to acknoledge they are non-issue somehow.

As for c) I had proposed a way to annotate failure rate, perhaps
we need a way to annotate these odd issues as well.

In my talk at LSFMM I mentioned how 25% of time *alone* on the test
automation effort consists of dealing with low hanging fruit. Since
companies are now trying to dedicate some resources towards stable
filesystem efforts it maybe worthy for them to consider this so that
they are aware that some of these oddball issues may end up with them
lurking in odd corners. I gave one example which took 8 months to root
cause on the blktests front alone at LSFMM.

> All developers make those mistakes in their
> own expunge lists, but if we start propagating those mistakes to the world,
> it becomes an issue.

Agreed, but note that the conversation is shifting from not sharing
expunges to possibly sharing some notion of expunges *somehow*. That is
a small step forward. I agree we need to address these semantics issues
and they are important, but without the will to share expunges there
would have been no point to address some of these common pain points.

> For those two reasons I think that the model to aspire to should be
> composed of a database where absolutely everyone can post data
> point to in the form of facts (i.e. the test failed after N runs on this kernel
> and this hardware...) and another process, partly AI, partly human to
> digest all those facts into a knowledge base that is valuable and
> maintained by experts. Much easier said than done...

Anything is possible, sure. A centralized database is one way to go
about some of these things. I'm however suspicious that perhaps there
is a better way, and am still evaluating a ledger as a way to scale
test results. Both paths can be taken, in fact. One does not negate
the other.

*For now*... I do think a simple repo with those who *are* collaborating
on expunges can share a simple repo as we have been doing for a few
months.

The need for scaling has to be addressed but for the long term of growth
of the endeavour.

  Luis

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

On Thu, May 26, 2022 at 8:27 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, May 25, 2022 at 02:17:11PM +0300, Amir Goldstein wrote:
> > Hi all!
> >
> > During LSFMM 2022, I have had an opportunity to speak with developers
> > from several different companies that showed interest in collaborating
> > on the effort of improving the state of xfs code in LTS kernels.
> >
> > I would like to kick-off this effort for the 5.10 LTS kernel, in the
> > hope that others will join me in the future to produce a better common
> > baseline for everyone to build on.
> >
> > This is the first of 6 series of stable patch candidates that
> > I collected from xfs releases v5.11..v5.18 [1].
> >
> > My intention is to post the parts for review on the xfs list on
> > a ~weekly basis and forward them to stable only after xfs developers
> > have had the chance to review the selection.
> >
> > I used a gadget that I developed "b4 rn" that produces high level
> > "release notes" with references to the posted patch series and also
> > looks for mentions of fstest names in the discussions on lore.
> > I then used an elimination process to select the stable tree candidate
> > patches. The selection process is documented in the git log of [1].
> >
> > After I had candidates, Luis has helped me to set up a kdevops testing
> > environment on a server that Samsung has contributed to the effort.
> > Luis and I have spent a considerable amount of time to establish the
> > expunge lists that produce stable baseline results for v5.10.y [2].
> > Eventually, we ran the auto group test over 100 times to sanitize the
> > baseline, on the following configurations:
> > reflink_normapbt (default), reflink, reflink_1024, nocrc, nocrc_512.
> >
> > The patches in this part are from circa v5.11 release.
> > They have been through 36 auto group runs with the configs listed above
> > and no regressions from baseline were observed.
>
> Woot!
>
> > At least two of the fixes have regression tests in fstests that were used
> > to verify the fix. I also annotated [3] the fix commits in the tests.
> >
> > I would like to thank Luis for his huge part in this still ongoing effort
> > and I would like to thank Samsung for contributing the hardware resources
> > to drive this effort.
> >
> > Your inputs on the selection in this part and in upcoming parts [1]
> > are most welcome!
>
> /me wonders if you need commit 9a5280b312e2 xfs: reorder iunlink remove
> operation in xfs_ifree ?  Or did that one already get pulled in?
>

Ok. I added it to my branch.
Note that 5.10.y was not getting any xfs fixes for 2 years, so for now I am
working my way up from v5.11 and not expediting any fixes, because if
downstream users waited for 2 years, they can wait a few more weeks.

My list of candidates was extracted at time of 5.18-rc1 and I intentionally
wanted to let the 5.18 patches soak before I consider them.
I will get to the 5.18 release when I am done posting fixes up to 5.17.

> The changes proposed look reasonable to me, and moreso with the testing
> to prove it.  Minor nit: patchsets should be tagged "PATCH", not "PATH".
>

Oy :)

I'll be sure to fix that when I post to stable.

Thanks,
Amir.

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Christoph Hellwig]

FYI, below is the 5.10-stable backport I have been testing earlier this
week that fixes a bugzilla reported hang:

https://bugzilla.kernel.org/show_bug.cgi?id=214767

I was just going to submit it to the stable maintaines today after
beeing out of the holiday, but if you want to add it to this queue
that is fine with me as well.

---
From 8e0464752b24f2b3919e8e92298759d116b283eb Mon Sep 17 00:00:00 2001
From: Dave Chinner <dchinner@redhat.com>
Date: Fri, 18 Jun 2021 08:21:51 -0700
Subject: xfs: Fix CIL throttle hang when CIL space used going backwards

commit 19f4e7cc819771812a7f527d7897c2deffbf7a00 upstream.

A hang with tasks stuck on the CIL hard throttle was reported and
largely diagnosed by Donald Buczek, who discovered that it was a
result of the CIL context space usage decrementing in committed
transactions once the hard throttle limit had been hit and processes
were already blocked.  This resulted in the CIL push not waking up
those waiters because the CIL context was no longer over the hard
throttle limit.

The surprising aspect of this was the CIL space usage going
backwards regularly enough to trigger this situation. Assumptions
had been made in design that the relogging process would only
increase the size of the objects in the CIL, and so that space would
only increase.

This change and commit message fixes the issue and documents the
result of an audit of the triggers that can cause the CIL space to
go backwards, how large the backwards steps tend to be, the
frequency in which they occur, and what the impact on the CIL
accounting code is.

Even though the CIL ctx->space_used can go backwards, it will only
do so if the log item is already logged to the CIL and contains a
space reservation for it's entire logged state. This is tracked by
the shadow buffer state on the log item. If the item is not
previously logged in the CIL it has no shadow buffer nor log vector,
and hence the entire size of the logged item copied to the log
vector is accounted to the CIL space usage. i.e.  it will always go
up in this case.

If the item has a log vector (i.e. already in the CIL) and the size
decreases, then the existing log vector will be overwritten and the
space usage will go down. This is the only condition where the space
usage reduces, and it can only occur when an item is already tracked
in the CIL. Hence we are safe from CIL space usage underruns as a
result of log items decreasing in size when they are relogged.

Typically this reduction in CIL usage occurs from metadata blocks
being free, such as when a btree block merge occurs or a directory
enter/xattr entry is removed and the da-tree is reduced in size.
This generally results in a reduction in size of around a single
block in the CIL, but also tends to increase the number of log
vectors because the parent and sibling nodes in the tree needs to be
updated when a btree block is removed. If a multi-level merge
occurs, then we see reduction in size of 2+ blocks, but again the
log vector count goes up.

The other vector is inode fork size changes, which only log the
current size of the fork and ignore the previously logged size when
the fork is relogged. Hence if we are removing items from the inode
fork (dir/xattr removal in shortform, extent record removal in
extent form, etc) the relogged size of the inode for can decrease.

No other log items can decrease in size either because they are a
fixed size (e.g. dquots) or they cannot be relogged (e.g. relogging
an intent actually creates a new intent log item and doesn't relog
the old item at all.) Hence the only two vectors for CIL context
size reduction are relogging inode forks and marking buffers active
in the CIL as stale.

Long story short: the majority of the code does the right thing and
handles the reduction in log item size correctly, and only the CIL
hard throttle implementation is problematic and needs fixing. This
patch makes that fix, as well as adds comments in the log item code
that result in items shrinking in size when they are relogged as a
clear reminder that this can and does happen frequently.

The throttle fix is based upon the change Donald proposed, though it
goes further to ensure that once the throttle is activated, it
captures all tasks until the CIL push issues a wakeup, regardless of
whether the CIL space used has gone back under the throttle
threshold.

This ensures that we prevent tasks reducing the CIL slightly under
the throttle threshold and then making more changes that push it
well over the throttle limit. This is acheived by checking if the
throttle wait queue is already active as a condition of throttling.
Hence once we start throttling, we continue to apply the throttle
until the CIL context push wakes everything on the wait queue.

We can use waitqueue_active() for the waitqueue manipulations and
checks as they are all done under the ctx->xc_push_lock. Hence the
waitqueue has external serialisation and we can safely peek inside
the wait queue without holding the internal waitqueue locks.

Many thanks to Donald for his diagnostic and analysis work to
isolate the cause of this hang.

Reported-and-tested-by: Donald Buczek <buczek@molgen.mpg.de>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf_item.c   | 37 ++++++++++++++++++-------------------
 fs/xfs/xfs_inode_item.c | 14 ++++++++++++++
 fs/xfs/xfs_log_cil.c    | 22 +++++++++++++++++-----
 3 files changed, 49 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 0356f2e340a10e..8c6e26d62ef288 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -56,14 +56,12 @@ xfs_buf_log_format_size(
 }
 
 /*
- * This returns the number of log iovecs needed to log the
- * given buf log item.
+ * Return the number of log iovecs and space needed to log the given buf log
+ * item segment.
  *
- * It calculates this as 1 iovec for the buf log format structure
- * and 1 for each stretch of non-contiguous chunks to be logged.
- * Contiguous chunks are logged in a single iovec.
- *
- * If the XFS_BLI_STALE flag has been set, then log nothing.
+ * It calculates this as 1 iovec for the buf log format structure and 1 for each
+ * stretch of non-contiguous chunks to be logged.  Contiguous chunks are logged
+ * in a single iovec.
  */
 STATIC void
 xfs_buf_item_size_segment(
@@ -119,11 +117,8 @@ xfs_buf_item_size_segment(
 }
 
 /*
- * This returns the number of log iovecs needed to log the given buf log item.
- *
- * It calculates this as 1 iovec for the buf log format structure and 1 for each
- * stretch of non-contiguous chunks to be logged.  Contiguous chunks are logged
- * in a single iovec.
+ * Return the number of log iovecs and space needed to log the given buf log
+ * item.
  *
  * Discontiguous buffers need a format structure per region that is being
  * logged. This makes the changes in the buffer appear to log recovery as though
@@ -133,7 +128,11 @@ xfs_buf_item_size_segment(
  * what ends up on disk.
  *
  * If the XFS_BLI_STALE flag has been set, then log nothing but the buf log
- * format structures.
+ * format structures. If the item has previously been logged and has dirty
+ * regions, we do not relog them in stale buffers. This has the effect of
+ * reducing the size of the relogged item by the amount of dirty data tracked
+ * by the log item. This can result in the committing transaction reducing the
+ * amount of space being consumed by the CIL.
  */
 STATIC void
 xfs_buf_item_size(
@@ -147,9 +146,9 @@ xfs_buf_item_size(
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 	if (bip->bli_flags & XFS_BLI_STALE) {
 		/*
-		 * The buffer is stale, so all we need to log
-		 * is the buf log format structure with the
-		 * cancel flag in it.
+		 * The buffer is stale, so all we need to log is the buf log
+		 * format structure with the cancel flag in it as we are never
+		 * going to replay the changes tracked in the log item.
 		 */
 		trace_xfs_buf_item_size_stale(bip);
 		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
@@ -164,9 +163,9 @@ xfs_buf_item_size(
 
 	if (bip->bli_flags & XFS_BLI_ORDERED) {
 		/*
-		 * The buffer has been logged just to order it.
-		 * It is not being included in the transaction
-		 * commit, so no vectors are used at all.
+		 * The buffer has been logged just to order it. It is not being
+		 * included in the transaction commit, so no vectors are used at
+		 * all.
 		 */
 		trace_xfs_buf_item_size_ordered(bip);
 		*nvecs = XFS_LOG_VEC_ORDERED;
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 17e20a6d8b4e27..6ff91e5bf3cd73 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -28,6 +28,20 @@ static inline struct xfs_inode_log_item *INODE_ITEM(struct xfs_log_item *lip)
 	return container_of(lip, struct xfs_inode_log_item, ili_item);
 }
 
+/*
+ * The logged size of an inode fork is always the current size of the inode
+ * fork. This means that when an inode fork is relogged, the size of the logged
+ * region is determined by the current state, not the combination of the
+ * previously logged state + the current state. This is different relogging
+ * behaviour to most other log items which will retain the size of the
+ * previously logged changes when smaller regions are relogged.
+ *
+ * Hence operations that remove data from the inode fork (e.g. shortform
+ * dir/attr remove, extent form extent removal, etc), the size of the relogged
+ * inode gets -smaller- rather than stays the same size as the previously logged
+ * size and this can result in the committing transaction reducing the amount of
+ * space being consumed by the CIL.
+ */
 STATIC void
 xfs_inode_item_data_fork_size(
 	struct xfs_inode_log_item *iip,
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index b0ef071b3cb53a..cd5c04dabe2e18 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -668,9 +668,14 @@ xlog_cil_push_work(
 	ASSERT(push_seq <= ctx->sequence);
 
 	/*
-	 * Wake up any background push waiters now this context is being pushed.
+	 * As we are about to switch to a new, empty CIL context, we no longer
+	 * need to throttle tasks on CIL space overruns. Wake any waiters that
+	 * the hard push throttle may have caught so they can start committing
+	 * to the new context. The ctx->xc_push_lock provides the serialisation
+	 * necessary for safely using the lockless waitqueue_active() check in
+	 * this context.
 	 */
-	if (ctx->space_used >= XLOG_CIL_BLOCKING_SPACE_LIMIT(log))
+	if (waitqueue_active(&cil->xc_push_wait))
 		wake_up_all(&cil->xc_push_wait);
 
 	/*
@@ -907,7 +912,7 @@ xlog_cil_push_background(
 	ASSERT(!list_empty(&cil->xc_cil));
 
 	/*
-	 * don't do a background push if we haven't used up all the
+	 * Don't do a background push if we haven't used up all the
 	 * space available yet.
 	 */
 	if (cil->xc_ctx->space_used < XLOG_CIL_SPACE_LIMIT(log)) {
@@ -931,9 +936,16 @@ xlog_cil_push_background(
 
 	/*
 	 * If we are well over the space limit, throttle the work that is being
-	 * done until the push work on this context has begun.
+	 * done until the push work on this context has begun. Enforce the hard
+	 * throttle on all transaction commits once it has been activated, even
+	 * if the committing transactions have resulted in the space usage
+	 * dipping back down under the hard limit.
+	 *
+	 * The ctx->xc_push_lock provides the serialisation necessary for safely
+	 * using the lockless waitqueue_active() check in this context.
 	 */
-	if (cil->xc_ctx->space_used >= XLOG_CIL_BLOCKING_SPACE_LIMIT(log)) {
+	if (cil->xc_ctx->space_used >= XLOG_CIL_BLOCKING_SPACE_LIMIT(log) ||
+	    waitqueue_active(&cil->xc_push_wait)) {
 		trace_xfs_log_cil_wait(log, cil->xc_ctx->ticket);
 		ASSERT(cil->xc_ctx->space_used < log->l_logsize);
 		xlog_wait(&cil->xc_push_wait, &cil->xc_push_lock);
-- 
2.30.2

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

On Fri, May 27, 2022 at 9:06 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> FYI, below is the 5.10-stable backport I have been testing earlier this
> week that fixes a bugzilla reported hang:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=214767
>
> I was just going to submit it to the stable maintaines today after
> beeing out of the holiday, but if you want to add it to this queue
> that is fine with me as well.
>

Let me take it for a short spin in out xfs stable test environment, since
this env has caught one regression with an allegedly safe fix.
This env has also VMs with old xfsprogs, which is kind of important to
test since those LTS patches may end up in distros with old xfsprogs.

If all is well, I'll send your patch later today to stable maintainers
with this first for-5.10 series.

> ---
> From 8e0464752b24f2b3919e8e92298759d116b283eb Mon Sep 17 00:00:00 2001
> From: Dave Chinner <dchinner@redhat.com>
> Date: Fri, 18 Jun 2021 08:21:51 -0700
> Subject: xfs: Fix CIL throttle hang when CIL space used going backwards
>

Damn! this patch slipped through my process (even though I did see
the correspondence on the list).

My (human) process has eliminated the entire 38 patch series
https://lore.kernel.org/linux-xfs/20210603052240.171998-1-david@fromorbit.com/
without noticing the fix that was inside it.

I did read every cover letter carefully for optimization series that I
eliminated
to look for important fixes and more than once I did pick singular fix patches
from optimization/cleanup series.

In this case, I guess Dave was not aware of the severity of the bug fixed
when he wrote the cover letter(?), but the fact that the series is not only
an improvement was not mentioned.

It's good that we have many vectors to find stable fixes :)

Cheers,
Amir.

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Dave Chinner]

On Fri, May 27, 2022 at 10:01:48AM +0300, Amir Goldstein wrote:
> On Fri, May 27, 2022 at 9:06 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > FYI, below is the 5.10-stable backport I have been testing earlier this
> > week that fixes a bugzilla reported hang:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=214767
> >
> > I was just going to submit it to the stable maintaines today after
> > beeing out of the holiday, but if you want to add it to this queue
> > that is fine with me as well.
> >
> 
> Let me take it for a short spin in out xfs stable test environment, since
> this env has caught one regression with an allegedly safe fix.
> This env has also VMs with old xfsprogs, which is kind of important to
> test since those LTS patches may end up in distros with old xfsprogs.
> 
> If all is well, I'll send your patch later today to stable maintainers
> with this first for-5.10 series.
> 
> > ---
> > From 8e0464752b24f2b3919e8e92298759d116b283eb Mon Sep 17 00:00:00 2001
> > From: Dave Chinner <dchinner@redhat.com>
> > Date: Fri, 18 Jun 2021 08:21:51 -0700
> > Subject: xfs: Fix CIL throttle hang when CIL space used going backwards
> >
> 
> Damn! this patch slipped through my process (even though I did see
> the correspondence on the list).
> 
> My (human) process has eliminated the entire 38 patch series
> https://lore.kernel.org/linux-xfs/20210603052240.171998-1-david@fromorbit.com/
> without noticing the fix that was inside it.

The first two times it was in much smaller patch series (5 and 8
patches total).


Also, you probably need to search for commit IDs on the list, too,
because this discussion was held in November about backporting the
fix to 5.10 stable kernels:

Subject: Help deciding about backported patch (kernel bug 214767, 19f4e7cc8197 xfs: Fix CIL throttle hang when CIL space used going backwards)
https://lore.kernel.org/linux-xfs/C1EC87A2-15B4-45B1-ACE2-F225E9E30DA9@flyingcircus.io/

> In this case, I guess Dave was not aware of the severity of the bug fixed

I was very aware of the severity of the problem, and I don't need
anyone trying to tell me what I should have been doing 18 months
ago.

It simply wasn't a severe bug. We had one user reporting it, and the
when I found the bug I realised that it was a zero-day thinko in
delayed logging accounting I made back in 2010 (~2.6.38 timeframe,
IIRC).  IOWs, it took 10 years before we got the first indication
there was a deep, dark corner case bug lurking in that code.

The time between first post of the bug fix and merge was about 6
months, so it also wasn't considered serious by anyone at the time
as it missed 2 whole kernel releases before it was reviewed and
merged...

There's been a small handful of user reports of this bug since (e.g
the bz above and the backport discussions), but it's pretty clear
that this bug is not (and never has been) a widespread issue.  It
just doesn't fit any of the criteria for a severe bug.

Backport candidate: yes. Severe: absolutely not.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

On Fri, May 27, 2022 at 12:08 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Fri, May 27, 2022 at 10:01:48AM +0300, Amir Goldstein wrote:
> > On Fri, May 27, 2022 at 9:06 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > FYI, below is the 5.10-stable backport I have been testing earlier this
> > > week that fixes a bugzilla reported hang:
> > >
> > > https://bugzilla.kernel.org/show_bug.cgi?id=214767
> > >
> > > I was just going to submit it to the stable maintaines today after
> > > beeing out of the holiday, but if you want to add it to this queue
> > > that is fine with me as well.
> > >
> >
> > Let me take it for a short spin in out xfs stable test environment, since
> > this env has caught one regression with an allegedly safe fix.
> > This env has also VMs with old xfsprogs, which is kind of important to
> > test since those LTS patches may end up in distros with old xfsprogs.
> >
> > If all is well, I'll send your patch later today to stable maintainers
> > with this first for-5.10 series.
> >
> > > ---
> > > From 8e0464752b24f2b3919e8e92298759d116b283eb Mon Sep 17 00:00:00 2001
> > > From: Dave Chinner <dchinner@redhat.com>
> > > Date: Fri, 18 Jun 2021 08:21:51 -0700
> > > Subject: xfs: Fix CIL throttle hang when CIL space used going backwards
> > >
> >
> > Damn! this patch slipped through my process (even though I did see
> > the correspondence on the list).
> >
> > My (human) process has eliminated the entire 38 patch series
> > https://lore.kernel.org/linux-xfs/20210603052240.171998-1-david@fromorbit.com/
> > without noticing the fix that was inside it.
>
> The first two times it was in much smaller patch series (5 and 8
> patches total).
>

The tool I was using tried to find the latest reference in lore
to have the closest reference to what was eventually merged
under the assumption that the last cover letter revision is the
best place to look at for overview.

In this case, however, there was an actual pull request for
the final version, but my tool wasn't aware of this practice yet,
so wasnt look for it.

>
> Also, you probably need to search for commit IDs on the list, too,
> because this discussion was held in November about backporting the
> fix to 5.10 stable kernels:
>
> Subject: Help deciding about backported patch (kernel bug 214767, 19f4e7cc8197 xfs: Fix CIL throttle hang when CIL space used going backwards)
> https://lore.kernel.org/linux-xfs/C1EC87A2-15B4-45B1-ACE2-F225E9E30DA9@flyingcircus.io/
>

Yes, as a human I was aware of this correspondence.
As a human I also forgot about it, so having the tool search
the lists for followup by commit id sounds like a good idea.

> > In this case, I guess Dave was not aware of the severity of the bug fixed
>
> I was very aware of the severity of the problem, and I don't need
> anyone trying to tell me what I should have been doing 18 months
> ago.
>

I will make a note of that.
I wasn't trying to pass judgement on you.
I was trying to analyse what needed fixing in my process as this is
the first time I employed it and this was a case study of something
that went wrong in my process.
forgive me if I got carried away with trying to read your thoughts
about the bug.

> It simply wasn't a severe bug. We had one user reporting it, and the
> when I found the bug I realised that it was a zero-day thinko in
> delayed logging accounting I made back in 2010 (~2.6.38 timeframe,
> IIRC).  IOWs, it took 10 years before we got the first indication
> there was a deep, dark corner case bug lurking in that code.
>
> The time between first post of the bug fix and merge was about 6
> months, so it also wasn't considered serious by anyone at the time
> as it missed 2 whole kernel releases before it was reviewed and
> merged...
>
> There's been a small handful of user reports of this bug since (e.g
> the bz above and the backport discussions), but it's pretty clear
> that this bug is not (and never has been) a widespread issue.  It
> just doesn't fit any of the criteria for a severe bug.
>
> Backport candidate: yes. Severe: absolutely not.
>

Understood.

In the future, if you are writing a cover letter for an improvement
series or internal pull request and you know there is a backport
candidate inside, if you happen to remember to mention it, it would
be of great help to me.

Thanks!
Amir.

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Luis Chamberlain]

On Fri, May 27, 2022 at 03:24:02PM +0300, Amir Goldstein wrote:
> On Fri, May 27, 2022 at 12:08 PM Dave Chinner <david@fromorbit.com> wrote:
> > Backport candidate: yes. Severe: absolutely not.
> In the future, if you are writing a cover letter for an improvement
> series or internal pull request and you know there is a backport
> candidate inside, if you happen to remember to mention it, it would
> be of great help to me.

Amir, since you wrote a tool enhancement to scrape for possible
candidates, *if* we defined some sort of meta-data to describe
this sort of stuff on the cover letter:

Backport candidate: yes. Severe: absolutely not

It would be useful when scraping. Therefore, leaving the effort
to try to backport / feasibility to others. This would be different
than a stable Cc tag, as those have a high degree of certainty.

How about something like:

Backport-candidate: yes
Impact: low

  Luis

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Darrick J. Wong]

On Fri, May 27, 2022 at 08:40:14AM -0700, Luis Chamberlain wrote:
> On Fri, May 27, 2022 at 03:24:02PM +0300, Amir Goldstein wrote:
> > On Fri, May 27, 2022 at 12:08 PM Dave Chinner <david@fromorbit.com> wrote:
> > > Backport candidate: yes. Severe: absolutely not.
> > In the future, if you are writing a cover letter for an improvement
> > series or internal pull request and you know there is a backport
> > candidate inside, if you happen to remember to mention it, it would
> > be of great help to me.
> 
> Amir, since you wrote a tool enhancement to scrape for possible
> candidates, *if* we defined some sort of meta-data to describe
> this sort of stuff on the cover letter:
> 
> Backport candidate: yes. Severe: absolutely not
> 
> It would be useful when scraping. Therefore, leaving the effort
> to try to backport / feasibility to others. This would be different
> than a stable Cc tag, as those have a high degree of certainty.
> 
> How about something like:
> 
> Backport-candidate: yes
> Impact: low

This particular patch (because we've recently hit it internally too)
would have been one of those:

Stable-Soak: January 2022

patches that I was talking about.

--D

>   Luis

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Dave Chinner]

On Fri, May 27, 2022 at 08:40:14AM -0700, Luis Chamberlain wrote:
> On Fri, May 27, 2022 at 03:24:02PM +0300, Amir Goldstein wrote:
> > On Fri, May 27, 2022 at 12:08 PM Dave Chinner <david@fromorbit.com> wrote:
> > > Backport candidate: yes. Severe: absolutely not.
> > In the future, if you are writing a cover letter for an improvement
> > series or internal pull request and you know there is a backport
> > candidate inside, if you happen to remember to mention it, it would
> > be of great help to me.

That's what "fixes" and "cc: stable" tags in the commit itself are
for, not the cover letter.

> Amir, since you wrote a tool enhancement to scrape for possible
> candidates, *if* we defined some sort of meta-data to describe
> this sort of stuff on the cover letter:
> 
> Backport candidate: yes. Severe: absolutely not
> 
> It would be useful when scraping. Therefore, leaving the effort
> to try to backport / feasibility to others. This would be different
> than a stable Cc tag, as those have a high degree of certainty.
> 
> How about something like:
> 
> Backport-candidate: yes
> Impact: low

No.

This placing the responsibility/burden on upstream developers to
classify everything they do that *might* get backported regardless
of the context the change is being made under. Stable maintainers
have the benefit of hindsight, user bug reports, etc and can make
much better determinations of whether any particular change should
be backported.

Indeed, a single user reporting a bug doesn't make the fix an
automatic backport candidate. The fix might be too complex to
backport, it might have serious dependency requirements, it might be
specific to a really niche configuration or workload, the user might
be running their own custom kernel and does their own backports, it
might be impossible for anyone but the person fixing the bug to
validate that the fix works correctly and it might take days or
weeks of effort to perform that validation, etc.

IOWs, there are many reasons that bug fixes may not be considered
backport candidates at the time they are fixed and committed. If
circumstances change 6 months later and only then it becomes clear
that we need to backport the fix, then that is not a problem the
upstream process can solve. Upstream has not done anything wrong
here, nor could they have known that the initial classification of
"rare, whacky, almost no exposure anywhere" might be completely
wrong because of something else they did not know about at the
time...

IMO, placing the responsibility on upstream developers to accurately
identify and classify every possible commit that might be a backport
candidate for the benefit of stable kernel maintainers is completely
upside down.

Identifying the fixes for problems that that stable kernel users are
hitting is the responsibility of the stable kernel maintainers. A
stable kernel maintainer is not just a role of "do backports and
test kernels". The stable kernel maintainer must also provide front
line user support so they are aware of the bugs those users are
hitting that *haven't already been identified* as affecting users of
those kernels. Stable kernel maintainers are *always* going to miss
bug fixes or changes that happen upstream that unintentionally fix
or mitigate problems that occur in older kernels.

Hence stable maintainers need to start from the position of "users
will always hit stuff we haven't fixed", not start from the position
of "upstream developers should tell us what we should fix". Many of
the upstream bug fixes come from user bug reports on stable kernels,
not from users on bleeding edge upstream kernels. Stable kernel
maintainers need to be triaging these bug reports and determining if
the problem is fixed upstream and the commit that fixes it, or if it
hasn't been fixed working with upstream to get the bugs fixed and
then backported.

If the upstream developer knows that it is a regression fix or a new
bug that likely needs to be backported, then we already have
"Fixes:" and "cc: stable" for communicating "please backport this"
back down the stack. Anything more is just asking us to make wild
guesses about an unknown future.

Cheers,

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

On Sat, May 28, 2022 at 2:42 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Fri, May 27, 2022 at 08:40:14AM -0700, Luis Chamberlain wrote:
> > On Fri, May 27, 2022 at 03:24:02PM +0300, Amir Goldstein wrote:
> > > On Fri, May 27, 2022 at 12:08 PM Dave Chinner <david@fromorbit.com> wrote:
> > > > Backport candidate: yes. Severe: absolutely not.
> > > In the future, if you are writing a cover letter for an improvement
> > > series or internal pull request and you know there is a backport
> > > candidate inside, if you happen to remember to mention it, it would
> > > be of great help to me.
>
> That's what "fixes" and "cc: stable" tags in the commit itself are
> for, not the cover letter.

Cover letter is an overview of the work.
A good cover letter includes an overview of the individual patches
in the context of the whole work as your cover letter did:

Summary of series:

Patches Modifications
------- -------------
1-7: log write FUA/FLUSH optimisations
8: bug fix
9-11: Async CIL pushes
12-25: xlog_write() rework
26-39: CIL commit scalability

So it was lapse of judgement on my part or carelessness that made me
eliminate the series without noting patch #8.

Furthermore, the subject of the patch has Fix and trailer has
Reported-and-tested-by:
so auto candidate selection would have picked it up easily, but my scripts
only looked for the obvious Fixes: tag inside the eliminated series, so that
is a problem with my process that I need to improve.

So the blame is entirely on me! not on you!

And yet.
"bug fix"?
Really?

I may not have been expecting more of other developers.
But I consider you to be one of the best when it comes to analyzing and
documenting complex and subtle code, so forgive me for expecting more.
I am not talking about severity or rareness or other things that may change
in time perspective. I am talking about describing the changes in a way that
benefits the prospective consumers of the cover letter (i.e. me).

It makes me sad that you are being defensive about this, because I wish
to be able to provide feedback to developers without having deal with a
responses like:
"I don't need anyone trying to tell me what I should have been doing"

Not every suggestion for an improvement is an attack.
You do not have to agree with my suggestions nor to adopt them,
but please do not dismiss them, because they come in good faith.

Thanks,
Amir.

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Dave Chinner]

On Sat, May 28, 2022 at 08:00:48AM +0300, Amir Goldstein wrote:
> On Sat, May 28, 2022 at 2:42 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Fri, May 27, 2022 at 08:40:14AM -0700, Luis Chamberlain wrote:
> > > On Fri, May 27, 2022 at 03:24:02PM +0300, Amir Goldstein wrote:
> > > > On Fri, May 27, 2022 at 12:08 PM Dave Chinner <david@fromorbit.com> wrote:
> > > > > Backport candidate: yes. Severe: absolutely not.
> > > > In the future, if you are writing a cover letter for an improvement
> > > > series or internal pull request and you know there is a backport
> > > > candidate inside, if you happen to remember to mention it, it would
> > > > be of great help to me.
> >
> > That's what "fixes" and "cc: stable" tags in the commit itself are
> > for, not the cover letter.
> 
> Cover letter is an overview of the work.
> A good cover letter includes an overview of the individual patches
> in the context of the whole work as your cover letter did:
> 
> Summary of series:
> 
> Patches Modifications
> ------- -------------
> 1-7: log write FUA/FLUSH optimisations
> 8: bug fix
> 9-11: Async CIL pushes
> 12-25: xlog_write() rework
> 26-39: CIL commit scalability
> 
> So it was lapse of judgement on my part or carelessness that made me
> eliminate the series without noting patch #8.
> 
> Furthermore, the subject of the patch has Fix and trailer has
> Reported-and-tested-by:
> so auto candidate selection would have picked it up easily, but my scripts
> only looked for the obvious Fixes: tag inside the eliminated series, so that
> is a problem with my process that I need to improve.
>
> So the blame is entirely on me! not on you!

I can feel a "But..." is about to arrive....

> And yet.
> "bug fix"?
> Really?

... and there's the passive-aggressive blame-shift.

As you just said yourself, all the information you required was in
both the cover letter and the patch, but you missed them both. You
also missed the other 3 times this patch was posted to the list,
too. Hence even if that cover letter said "patch 8: CIL log space
overrun bug fix", it wouldn't have made any difference because of
the process failures on your side.

So what's the point you're trying to make with this comment? What is
the problem it demonstrates that we need to address? We can't be
much more explicit than "patch X is a bug fix" in a cover letter, so
what are you expecting us to do differently?

> I may not have been expecting more of other developers.
> But I consider you to be one of the best when it comes to analyzing and
> documenting complex and subtle code, so forgive me for expecting more.

That's not very fair.  If you are going to hold me to a high bar,
then you need to hold everyone to that same high bar.....

> It makes me sad that you are being defensive about this, because I wish

.... because people tend to get defensive when they feel like they
are being singled out repeatedly for things that nobody else is
getting called out for.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

On Wed, Jun 1, 2022 at 7:31 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Sat, May 28, 2022 at 08:00:48AM +0300, Amir Goldstein wrote:
> > On Sat, May 28, 2022 at 2:42 AM Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > On Fri, May 27, 2022 at 08:40:14AM -0700, Luis Chamberlain wrote:
> > > > On Fri, May 27, 2022 at 03:24:02PM +0300, Amir Goldstein wrote:
> > > > > On Fri, May 27, 2022 at 12:08 PM Dave Chinner <david@fromorbit.com> wrote:
> > > > > > Backport candidate: yes. Severe: absolutely not.
> > > > > In the future, if you are writing a cover letter for an improvement
> > > > > series or internal pull request and you know there is a backport
> > > > > candidate inside, if you happen to remember to mention it, it would
> > > > > be of great help to me.
> > >
> > > That's what "fixes" and "cc: stable" tags in the commit itself are
> > > for, not the cover letter.
> >
> > Cover letter is an overview of the work.
> > A good cover letter includes an overview of the individual patches
> > in the context of the whole work as your cover letter did:
> >
> > Summary of series:
> >
> > Patches Modifications
> > ------- -------------
> > 1-7: log write FUA/FLUSH optimisations
> > 8: bug fix
> > 9-11: Async CIL pushes
> > 12-25: xlog_write() rework
> > 26-39: CIL commit scalability
> >
> > So it was lapse of judgement on my part or carelessness that made me
> > eliminate the series without noting patch #8.
> >
> > Furthermore, the subject of the patch has Fix and trailer has
> > Reported-and-tested-by:
> > so auto candidate selection would have picked it up easily, but my scripts
> > only looked for the obvious Fixes: tag inside the eliminated series, so that
> > is a problem with my process that I need to improve.
> >
> > So the blame is entirely on me! not on you!
>
> I can feel a "But..." is about to arrive....
>
> > And yet.
> > "bug fix"?
> > Really?
>
> ... and there's the passive-aggressive blame-shift.
>

Not very passive ;)

> As you just said yourself, all the information you required was in
> both the cover letter and the patch, but you missed them both. You
> also missed the other 3 times this patch was posted to the list,
> too. Hence even if that cover letter said "patch 8: CIL log space
> overrun bug fix", it wouldn't have made any difference because of
> the process failures on your side.
>

That's fair.

> So what's the point you're trying to make with this comment? What is
> the problem it demonstrates that we need to address? We can't be
> much more explicit than "patch X is a bug fix" in a cover letter, so
> what are you expecting us to do differently?
>

It is a bit hard for me to express my expectations in technical terms,
because you are right - you did provide all the information.

At a very high level, I would very much like the authors of patches
and cover letters to consider the people working on backports
when they are describing their work.

There are many people working on backports on every major
company/distro so I think I am not alone in this request.

The best I can do is point to an example patch set that
I eliminated from back port for being a "rework" and where it
was made very clear that the patch set contains an independent bug fix:
https://lore.kernel.org/all/20210121154526.1852176-1-bfoster@redhat.com/

> > I may not have been expecting more of other developers.
> > But I consider you to be one of the best when it comes to analyzing and
> > documenting complex and subtle code, so forgive me for expecting more.
>
> That's not very fair.  If you are going to hold me to a high bar,
> then you need to hold everyone to that same high bar.....
>

Holding everyone to your standards is not going to be easy,
but I will try :)

> > It makes me sad that you are being defensive about this, because I wish
>
> .... because people tend to get defensive when they feel like they
> are being singled out repeatedly for things that nobody else is
> getting called out for.
>

I am sorry that I made you feel this way.

The trigger for this thread was a patch that I missed.
Doing the post mortem, I was raising the postulate [with a (?)]
that the author (who happened to be you) was not considering
the bug to be severe, which you had later confirmed.

After that, the conversation just escalated for the wrong reasons.
I was trying to make a general plea for more consideration in
the people that do backporting work and it came out wrong as
an attack on you. I need to watch my phrasing more carefully.

I sincerely apologize. It was not my intention at all.
I hope we can move past this and avoid clashes in the future.

Moving on...

I was thinking later that there is another point of failure in the
backport process that is demonstrated in this incident -
An elephant in the room even -
We have no *standard* way to mark a patch as a bug fix,
so everyone is using their own scripts and regex magic.

Fixes: is a standard that works, but it does not apply in many
cases, for example:

1. Zero day (some mark those as Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"))
2. Hard work to figure out the fix commit
3. Discourage AUTOSEL

I am not sure if the patch in question was not annotated with Fixes:
for one or more of the above(?).

Regarding AUTOSEL, I hope this is not a problem anymore, because
AUTOSEL has been told to stay away from xfs code for a while now.

Regarding Zero day, I think this is a common case that deserves
attention by annotation.

Any annotation along the lines of Fixes: ????/0000
could cover cases #1 and #2 and may work with some existing auto
selection scripts without having to change them.

Granted, it may also break some existing script by making them
unselect a fix for a commit they cannot find.

Maybe instead of annotating what the fix fixes, annotation could
be somehow related to how the bug was detected, on which version,
with which test? not reproducible?

Thoughts?

Thanks,
Amir.

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Theodore Ts'o]

On Wed, Jun 01, 2022 at 10:10:11AM +0300, Amir Goldstein wrote:
> At a very high level, I would very much like the authors of patches
> and cover letters to consider the people working on backports
> when they are describing their work.
> 
> There are many people working on backports on every major
> company/distro so I think I am not alone in this request.

So while there are people on my time that are working on backports,
and need to support a variety of kernels for diffeernt varieties of
production, I'm also an upstream maintainer so I see both sides of the
issue.

Yes, there are a lot of people who are working on backports.  But at
the same time, many backport efforts are being done by companies that
have a profit motive for supporting these production/product kernels,
and so it is often easier to get funding, in the form of head count
allocations, for supporting these production/product kernels, than it
is to get funding to support upstream kernel work, from a comparative
point of view.  Take a look at how many kernel engineers at Red Hat,
SuSE, etc., work on supporting their revenue product, versus those who
work on the upstream kernel.

There is a reason why historically, we've optimized for the upstream
maintainers, and not for the product kernels.  After all, companies
are getting paid $$$ for the product kernels.  If companies funded a
more head count to work on making life easier for stable backports,
that would be great.  But otherwise, requests like this end up coming
as effective unfunded mandates on upstream developers' time.

(And it's not stable kernel backports; it's also Syzbot reports,
Luis's failures found by using loop devices, etc.  If I had an
infinite amount of time, or if I have personal time on weekends where
I'm looking for something extra to do for fun, great.  But otherwise,
someone has to fund these efforts, or it's just going to make upstream
developers get irritated at worst, and not pay a lot of attention to
these requests at best.)

The reality is that if a backport is associated with a revenue
product, it's much easier to justify getting headcount when it comes
time to fighting the company budget headcount wargames, and I've seen
this at many different companies.  It's just the way of the world.

> I was thinking later that there is another point of failure in the
> backport process that is demonstrated in this incident -
> An elephant in the room even -
> We have no *standard* way to mark a patch as a bug fix,
> so everyone is using their own scripts and regex magic.
> 
> Fixes: is a standard that works, but it does not apply in many
> cases, for example:
> 
> 1. Zero day (some mark those as Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"))
> 2. Hard work to figure out the fix commit
> 3. Discourage AUTOSEL

For security fixes, just marking a bug as fixing a security bug, even
if you try to obfuscate the Fixes tag, is often enough to tip off a
potential attacker.   So I would consider:

    Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"))

to Not Be A Solution for security related patches.  The real fix here
is to put the commit id in the CVE, and to have kernel security teams
monitoring the CVE feeds looking for bugs that need to be remediated
post haste.  If you sell to the US Federal government, FedRAMP RA-5d
has some interesting mitigation timeline requirements depending on the
CVE Security Score.  And for people who are getting paid to sell into
that particular set of customers, they presumably have built into
their pricing model the cost of having a security team do that work.

Given the tight requirements for mitigating CVE's with a CVESS > 7.0,
you probably can't afford to wait for a LTS kernel to get those
patches into a revenue product (especially once you include time to QA
the updated kernel), so it should hopefully be not that hard to find a
product security team willing to identify commits that need to be
backported into the LTS Stable kernel to address security issues.
Espceially for high severity CVE's, they won't need to worry about
unduly giving their competitors in that market segment a free ride.  :-)


The hard work to figure out the fix commit is a real one, and this is
an example of where the interests of upstream and people who want to
do backports come into partial conflict.  The more we do code cleanup,
refactoring, etc., to reduce technical debt --- something which is of
great interest to upstream developers --- the harder it is to
idetntify the fixes tag, and the harder it is to backport to bug and
security fixes after the tech debt reduction commit has gone in.  So
someone who only cares about backports into product kernels, to the
exclusion of all else, would want to discourage tech debt reudction
commits.  Except they know that won't fly, and they would be flamed to
a crisp if they try.  :-)

I suppose one workaround is if an upstream developer is too tired or
too harried to figure out the correct value of a fixes tag, one cheesy
way around it would be to use:

    Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"))

to basically mean, this fixes a bug, but I'm not sure where it is, and
it's long enough ago that maybe it's better if we ask the backport
folks to figure out the dependency if the patch doesn't apply cleanly
as to whether or not they need to do the code archeology to figure out
if it applies to an ancient kernel like 4.19 or 5.10, because again,
the product backport folks are likely to outnumber the upstream
developers, and the product backport folks are linked to revenue
streams.

So I would argue that maybe a more sustainable approach is to find a
way for the product backport folks to work together to take load off
of the upstream developers.  I'm sure there will be times when there
are some easy things that upstream folks can do to make things better,
but trying to move all or most of the burden onto the upstream
developers is as much of an unfunded mandate as Syzbot is.  :-/

	      	      	    	     	     - Ted

Re: [PATH 5.10 0/4] xfs stable candidate patches for 5.10.y (part 1)

[Amir Goldstein]

> > I was thinking later that there is another point of failure in the
> > backport process that is demonstrated in this incident -
> > An elephant in the room even -
> > We have no *standard* way to mark a patch as a bug fix,
> > so everyone is using their own scripts and regex magic.
> >
> > Fixes: is a standard that works, but it does not apply in many
> > cases, for example:
> >
> > 1. Zero day (some mark those as Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"))
> > 2. Hard work to figure out the fix commit
> > 3. Discourage AUTOSEL
>
> For security fixes, just marking a bug as fixing a security bug, even
> if you try to obfuscate the Fixes tag, is often enough to tip off a
> potential attacker.   So I would consider:
>
>     Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"))
>
> to Not Be A Solution for security related patches.  The real fix here

I miswrote. I meant fixes for bugs that existed since day 1.
The annotation above is adequate, but is also a bit ugly IMO.

>
>
> The hard work to figure out the fix commit is a real one, and this is
> an example of where the interests of upstream and people who want to
> do backports come into partial conflict.  The more we do code cleanup,
> refactoring, etc., to reduce technical debt --- something which is of
> great interest to upstream developers --- the harder it is to
> idetntify the fixes tag, and the harder it is to backport to bug and
> security fixes after the tech debt reduction commit has gone in.  So
> someone who only cares about backports into product kernels, to the
> exclusion of all else, would want to discourage tech debt reudction
> commits.  Except they know that won't fly, and they would be flamed to
> a crisp if they try.  :-)
>
> I suppose one workaround is if an upstream developer is too tired or
> too harried to figure out the correct value of a fixes tag, one cheesy
> way around it would be to use:
>
>     Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"))
>

I am sure that we can be more expressive than that :)

I suggested:
  Fixes: ????

Not as a joke. It is more descriptive than the above which could really
mean "I know this bug existed since day 1"

> to basically mean, this fixes a bug, but I'm not sure where it is, and
> it's long enough ago that maybe it's better if we ask the backport
> folks to figure out the dependency if the patch doesn't apply cleanly
> as to whether or not they need to do the code archeology to figure out
> if it applies to an ancient kernel like 4.19 or 5.10, because again,
> the product backport folks are likely to outnumber the upstream
> developers, and the product backport folks are linked to revenue
> streams.
>
> So I would argue that maybe a more sustainable approach is to find a
> way for the product backport folks to work together to take load off
> of the upstream developers.  I'm sure there will be times when there
> are some easy things that upstream folks can do to make things better,
> but trying to move all or most of the burden onto the upstream
> developers is as much of an unfunded mandate as Syzbot is.  :-/
>

The funny thing is that my rant was about a cover letter written
by an upstream developer, so unless what you mean is that backport
teams should invest in review and make those comments during
review, then it's too late by the time the backport team got to do their job.

You raise the resources problem and I understand that, but
I am not looking for a solution that creates more work for upstream
maintainers. Quite the contrary.

I am simply looking for a standard way to express
"attention backport teams".

What I am looking for is a solution for this problem:

Developer/maintainer has information that can be very helpful to backport
teams.

The information is known at the time of the writing and it requires no extra
effort on their part, but is not mentioned in a standard way because there
is no standard way that does not incur extra work.

Some maintainers that I am working with are very diligent about requiring Fixes:
when applicable.

It may be because of $$$ as you say, but I have a more romantic interpretation.
I believe it is because they care about the code they are developing and they
understand that mainstream is not a product and has no real user.

Unless maintainers care about downstream products, real users will perceive
their code as bad quality - no matter whose responsibility it was to
backport the
fixes or point out to the relevant fixes.

Thanks,
Amir.