Re: xfs: Assertion failed in xfs_ag_resv_init()

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 6529 bytes --]

On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > You could send this patch to the stable list, but my guess is that
> > > they'd prefer a straight backport of all three commits...
> > 
> > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > four conflicting files. The conflicts are trivial to resolve (git
> > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > (xfs: create a function to query all records in a btree) is needed as
> > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > to common file) on top of that gives non-trivial conflicts.
> 
> Ah, I suspected that might happen.  Backports are hard. :(
> 
> I suppose one saving grace of the patch you sent is that it'll likely
> break the build if anyone ever /does/ attempt a backport of those first
> two commits.  Perhaps that is the most practical way forward.
> 
> > So, for automatic backporting we would need to cherry-pick even more,
> > and each backported commit should be tested of course. Given this, do
> > you still think Greg prefers a rather large set of straight backports
> > over the simple commit that just pulls in the missing function?
> 
> I think you'd have to ask him that, if you decide not to send
> yesterday's patch.

Let's try. I've added a sentence to the commit message which explains
why a straight backport is not practical, and how to proceed if anyone
wants to backport the earlier commits.

Greg: Under the given circumstances, would you be willing to accept
the patch below for 4.9?

Thanks
Andre
---
commit 6a12a8bda15d5223103df76b8f92cc277c2f5e69
Author: Dave Chinner <dchinner@redhat.com>
Date:   Mon Nov 19 13:31:08 2018 -0800

    xfs: finobt AG reserves don't consider last AG can be a runt
    
    This is a backport of upstream commit c08768977b9 and the part of
    21ec54168b36 which is needed by c08768977b9 to fix a ENOSPC issue
    due to a bug in the finobt per-ag space reservation code. The bug
    was observed on an almost empty 100T large filesystem whose last AG
    happened to be very small.
    
    A direct backport of the above mentioned two commits is not possible
    because the second commit relies on further infrastructure that was
    introduced earlier. If anyone ever attempts to backport those earlier
    commits, this commit has to be reverted first so that the earlier
    commits apply, and c08768977b9 has to be applied on top of that.
    
    Commit log of c08768977b9 follows.
    
            commit c08768977b9a65cab9bcfd1ba30ffb686b2b7c69
            Author: Dave Chinner <dchinner@redhat.com>
            Date:   Mon Nov 19 13:31:08 2018 -0800
    
                xfs: finobt AG reserves don't consider last AG can be a runt
    
                The last AG may be very small comapred to all other AGs, and hence
                AG reservations based on the superblock AG size may actually consume
                more space than the AG actually has. This results on assert failures
                like:
    
                XFS: Assertion failed: xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <= pag->pagf_freeblks + pag->pagf_flcount, file: fs/xfs/libxfs/xfs_ag_resv.c, line: 319
                [   48.932891]  xfs_ag_resv_init+0x1bd/0x1d0
                [   48.933853]  xfs_fs_reserve_ag_blocks+0x37/0xb0
                [   48.934939]  xfs_mountfs+0x5b3/0x920
                [   48.935804]  xfs_fs_fill_super+0x462/0x640
                [   48.936784]  ? xfs_test_remount_options+0x60/0x60
                [   48.937908]  mount_bdev+0x178/0x1b0
                [   48.938751]  mount_fs+0x36/0x170
                [   48.939533]  vfs_kern_mount.part.43+0x54/0x130
                [   48.940596]  do_mount+0x20e/0xcb0
                [   48.941396]  ? memdup_user+0x3e/0x70
                [   48.942249]  ksys_mount+0xba/0xd0
                [   48.943046]  __x64_sys_mount+0x21/0x30
                [   48.943953]  do_syscall_64+0x54/0x170
                [   48.944835]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
    
                Hence we need to ensure the finobt per-ag space reservations take
                into account the size of the last AG rather than treat it like all
                the other full size AGs.
    
                Note that both refcountbt and rmapbt already take the size of the AG
                into account via reading the AGF length directly.
    
                Signed-off-by: Dave Chinner <dchinner@redhat.com>
                Reviewed-by: Christoph Hellwig <hch@lst.de>
                Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
                Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
    
    Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
    Tested-by: Andre Noll <maan@tuebingen.mpg.de>

diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index b9c351ff0422..33905989929e 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -502,17 +502,33 @@ xfs_inobt_rec_check_count(
 }
 #endif	/* DEBUG */
 
+/* Find the size of the AG, in blocks. */
+static xfs_agblock_t
+xfs_ag_block_count(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno)
+{
+	ASSERT(agno < mp->m_sb.sb_agcount);
+
+	if (agno < mp->m_sb.sb_agcount - 1)
+		return mp->m_sb.sb_agblocks;
+	return mp->m_sb.sb_dblocks - (agno * mp->m_sb.sb_agblocks);
+}
+
 static xfs_extlen_t
 xfs_inobt_max_size(
-	struct xfs_mount	*mp)
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno)
 {
+	xfs_agblock_t		agblocks = xfs_ag_block_count(mp, agno);
+
 	/* Bail out if we're uninitialized, which can happen in mkfs. */
 	if (mp->m_inobt_mxr[0] == 0)
 		return 0;
 
 	return xfs_btree_calc_size(mp, mp->m_inobt_mnr,
-		(uint64_t)mp->m_sb.sb_agblocks * mp->m_sb.sb_inopblock /
-				XFS_INODES_PER_CHUNK);
+				(uint64_t)agblocks * mp->m_sb.sb_inopblock /
+					XFS_INODES_PER_CHUNK);
 }
 
 static int
@@ -558,7 +574,7 @@ xfs_finobt_calc_reserves(
 	if (error)
 		return error;
 
-	*ask += xfs_inobt_max_size(mp);
+	*ask += xfs_inobt_max_size(mp, agno);
 	*used += tree_len;
 	return 0;
 }
-- 
Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Greg Kroah-Hartman]

On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
> On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > > You could send this patch to the stable list, but my guess is that
> > > > they'd prefer a straight backport of all three commits...
> > > 
> > > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > > four conflicting files. The conflicts are trivial to resolve (git
> > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > > (xfs: create a function to query all records in a btree) is needed as
> > > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > > to common file) on top of that gives non-trivial conflicts.
> > 
> > Ah, I suspected that might happen.  Backports are hard. :(
> > 
> > I suppose one saving grace of the patch you sent is that it'll likely
> > break the build if anyone ever /does/ attempt a backport of those first
> > two commits.  Perhaps that is the most practical way forward.
> > 
> > > So, for automatic backporting we would need to cherry-pick even more,
> > > and each backported commit should be tested of course. Given this, do
> > > you still think Greg prefers a rather large set of straight backports
> > > over the simple commit that just pulls in the missing function?
> > 
> > I think you'd have to ask him that, if you decide not to send
> > yesterday's patch.
> 
> Let's try. I've added a sentence to the commit message which explains
> why a straight backport is not practical, and how to proceed if anyone
> wants to backport the earlier commits.
> 
> Greg: Under the given circumstances, would you be willing to accept
> the patch below for 4.9?

If the xfs maintainers say this is ok, it is fine with me.

thanks,

greg k-h

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
> On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
> > On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > > > You could send this patch to the stable list, but my guess is that
> > > > > they'd prefer a straight backport of all three commits...
> > > > 
> > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > > > four conflicting files. The conflicts are trivial to resolve (git
> > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > > > (xfs: create a function to query all records in a btree) is needed as
> > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > > > to common file) on top of that gives non-trivial conflicts.
> > > 
> > > Ah, I suspected that might happen.  Backports are hard. :(
> > > 
> > > I suppose one saving grace of the patch you sent is that it'll likely
> > > break the build if anyone ever /does/ attempt a backport of those first
> > > two commits.  Perhaps that is the most practical way forward.
> > > 
> > > > So, for automatic backporting we would need to cherry-pick even more,
> > > > and each backported commit should be tested of course. Given this, do
> > > > you still think Greg prefers a rather large set of straight backports
> > > > over the simple commit that just pulls in the missing function?
> > > 
> > > I think you'd have to ask him that, if you decide not to send
> > > yesterday's patch.
> > 
> > Let's try. I've added a sentence to the commit message which explains
> > why a straight backport is not practical, and how to proceed if anyone
> > wants to backport the earlier commits.
> > 
> > Greg: Under the given circumstances, would you be willing to accept
> > the patch below for 4.9?
> 
> If the xfs maintainers say this is ok, it is fine with me.

Darrick said, he's in favor of the patch, so I guess I can add his
Acked-by. Would you also like to see the ack from Dave (the author
of the original commit)?

Anything else that needs to be changed?

Thanks
Andre
-- 
Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Darrick J. Wong]

On Wed, May 01, 2019 at 07:51:29PM +0200, Andre Noll wrote:
> On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
> > On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
> > > On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > > > > You could send this patch to the stable list, but my guess is that
> > > > > > they'd prefer a straight backport of all three commits...
> > > > > 
> > > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > > > > four conflicting files. The conflicts are trivial to resolve (git
> > > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > > > > (xfs: create a function to query all records in a btree) is needed as
> > > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > > > > to common file) on top of that gives non-trivial conflicts.
> > > > 
> > > > Ah, I suspected that might happen.  Backports are hard. :(
> > > > 
> > > > I suppose one saving grace of the patch you sent is that it'll likely
> > > > break the build if anyone ever /does/ attempt a backport of those first
> > > > two commits.  Perhaps that is the most practical way forward.
> > > > 
> > > > > So, for automatic backporting we would need to cherry-pick even more,
> > > > > and each backported commit should be tested of course. Given this, do
> > > > > you still think Greg prefers a rather large set of straight backports
> > > > > over the simple commit that just pulls in the missing function?
> > > > 
> > > > I think you'd have to ask him that, if you decide not to send
> > > > yesterday's patch.
> > > 
> > > Let's try. I've added a sentence to the commit message which explains
> > > why a straight backport is not practical, and how to proceed if anyone
> > > wants to backport the earlier commits.
> > > 
> > > Greg: Under the given circumstances, would you be willing to accept
> > > the patch below for 4.9?
> > 
> > If the xfs maintainers say this is ok, it is fine with me.
> 
> Darrick said, he's in favor of the patch, so I guess I can add his
> Acked-by. Would you also like to see the ack from Dave (the author
> of the original commit)?

FWIW it seems fine to me, though Dave [cc'd] might have stronger opinions...

Acked-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Anything else that needs to be changed?
> 
> Thanks
> Andre
> -- 
> Max Planck Institute for Developmental Biology
> Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
> http://people.tuebingen.mpg.de/maan/

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Dave Chinner]

On Wed, May 01, 2019 at 12:28:22PM -0700, Darrick J. Wong wrote:
> On Wed, May 01, 2019 at 07:51:29PM +0200, Andre Noll wrote:
> > On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
> > > On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
> > > > On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > > > > > You could send this patch to the stable list, but my guess is that
> > > > > > > they'd prefer a straight backport of all three commits...
> > > > > > 
> > > > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > > > > > four conflicting files. The conflicts are trivial to resolve (git
> > > > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > > > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > > > > > (xfs: create a function to query all records in a btree) is needed as
> > > > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > > > > > to common file) on top of that gives non-trivial conflicts.
> > > > > 
> > > > > Ah, I suspected that might happen.  Backports are hard. :(
> > > > > 
> > > > > I suppose one saving grace of the patch you sent is that it'll likely
> > > > > break the build if anyone ever /does/ attempt a backport of those first
> > > > > two commits.  Perhaps that is the most practical way forward.
> > > > > 
> > > > > > So, for automatic backporting we would need to cherry-pick even more,
> > > > > > and each backported commit should be tested of course. Given this, do
> > > > > > you still think Greg prefers a rather large set of straight backports
> > > > > > over the simple commit that just pulls in the missing function?
> > > > > 
> > > > > I think you'd have to ask him that, if you decide not to send
> > > > > yesterday's patch.
> > > > 
> > > > Let's try. I've added a sentence to the commit message which explains
> > > > why a straight backport is not practical, and how to proceed if anyone
> > > > wants to backport the earlier commits.
> > > > 
> > > > Greg: Under the given circumstances, would you be willing to accept
> > > > the patch below for 4.9?
> > > 
> > > If the xfs maintainers say this is ok, it is fine with me.
> > 
> > Darrick said, he's in favor of the patch, so I guess I can add his
> > Acked-by. Would you also like to see the ack from Dave (the author
> > of the original commit)?
> 
> FWIW it seems fine to me, though Dave [cc'd] might have stronger opinions...

Only thing I care about is whether it is QA'd properly. Greg, Sasha,
is the 4.9 stable kernel having fstests run on it as part of the
release gating?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 08:11:07AM +1000, Dave Chinner wrote:
> On Wed, May 01, 2019 at 12:28:22PM -0700, Darrick J. Wong wrote:
> > On Wed, May 01, 2019 at 07:51:29PM +0200, Andre Noll wrote:
> > > On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
> > > > On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
> > > > > On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > > > > > > You could send this patch to the stable list, but my guess is that
> > > > > > > > they'd prefer a straight backport of all three commits...
> > > > > > > 
> > > > > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > > > > > > four conflicting files. The conflicts are trivial to resolve (git
> > > > > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > > > > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > > > > > > (xfs: create a function to query all records in a btree) is needed as
> > > > > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > > > > > > to common file) on top of that gives non-trivial conflicts.
> > > > > > 
> > > > > > Ah, I suspected that might happen.  Backports are hard. :(
> > > > > > 
> > > > > > I suppose one saving grace of the patch you sent is that it'll likely
> > > > > > break the build if anyone ever /does/ attempt a backport of those first
> > > > > > two commits.  Perhaps that is the most practical way forward.
> > > > > > 
> > > > > > > So, for automatic backporting we would need to cherry-pick even more,
> > > > > > > and each backported commit should be tested of course. Given this, do
> > > > > > > you still think Greg prefers a rather large set of straight backports
> > > > > > > over the simple commit that just pulls in the missing function?
> > > > > > 
> > > > > > I think you'd have to ask him that, if you decide not to send
> > > > > > yesterday's patch.
> > > > > 
> > > > > Let's try. I've added a sentence to the commit message which explains
> > > > > why a straight backport is not practical, and how to proceed if anyone
> > > > > wants to backport the earlier commits.
> > > > > 
> > > > > Greg: Under the given circumstances, would you be willing to accept
> > > > > the patch below for 4.9?
> > > > 
> > > > If the xfs maintainers say this is ok, it is fine with me.
> > > 
> > > Darrick said, he's in favor of the patch, so I guess I can add his
> > > Acked-by. Would you also like to see the ack from Dave (the author
> > > of the original commit)?
> > 
> > FWIW it seems fine to me, though Dave [cc'd] might have stronger opinions...
> 
> Only thing I care about is whether it is QA'd properly. Greg, Sasha,
> is the 4.9 stable kernel having fstests run on it as part of the
> release gating?

I do not know about fstests, I know Linaro was looking into doing it as
part of the test suites that they verify before I do a release.  But I
doubt it's run on an XFS filesystem.

Sasha was doing some work in this area though, Sasha?

greg k-h

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 01:44:40PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 02, 2019 at 08:11:07AM +1000, Dave Chinner wrote:
> > On Wed, May 01, 2019 at 12:28:22PM -0700, Darrick J. Wong wrote:
> > > On Wed, May 01, 2019 at 07:51:29PM +0200, Andre Noll wrote:
> > > > On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
> > > > > On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
> > > > > > On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > > > > > > > You could send this patch to the stable list, but my guess is that
> > > > > > > > > they'd prefer a straight backport of all three commits...
> > > > > > > > 
> > > > > > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > > > > > > > four conflicting files. The conflicts are trivial to resolve (git
> > > > > > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > > > > > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > > > > > > > (xfs: create a function to query all records in a btree) is needed as
> > > > > > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > > > > > > > to common file) on top of that gives non-trivial conflicts.
> > > > > > > 
> > > > > > > Ah, I suspected that might happen.  Backports are hard. :(
> > > > > > > 
> > > > > > > I suppose one saving grace of the patch you sent is that it'll likely
> > > > > > > break the build if anyone ever /does/ attempt a backport of those first
> > > > > > > two commits.  Perhaps that is the most practical way forward.
> > > > > > > 
> > > > > > > > So, for automatic backporting we would need to cherry-pick even more,
> > > > > > > > and each backported commit should be tested of course. Given this, do
> > > > > > > > you still think Greg prefers a rather large set of straight backports
> > > > > > > > over the simple commit that just pulls in the missing function?
> > > > > > > 
> > > > > > > I think you'd have to ask him that, if you decide not to send
> > > > > > > yesterday's patch.
> > > > > > 
> > > > > > Let's try. I've added a sentence to the commit message which explains
> > > > > > why a straight backport is not practical, and how to proceed if anyone
> > > > > > wants to backport the earlier commits.
> > > > > > 
> > > > > > Greg: Under the given circumstances, would you be willing to accept
> > > > > > the patch below for 4.9?
> > > > > 
> > > > > If the xfs maintainers say this is ok, it is fine with me.
> > > > 
> > > > Darrick said, he's in favor of the patch, so I guess I can add his
> > > > Acked-by. Would you also like to see the ack from Dave (the author
> > > > of the original commit)?
> > > 
> > > FWIW it seems fine to me, though Dave [cc'd] might have stronger opinions...
> > 
> > Only thing I care about is whether it is QA'd properly. Greg, Sasha,
> > is the 4.9 stable kernel having fstests run on it as part of the
> > release gating?
> 
> I do not know about fstests, I know Linaro was looking into doing it as
> part of the test suites that they verify before I do a release.  But I
> doubt it's run on an XFS filesystem.
> 
> Sasha was doing some work in this area though, Sasha?

[actually adding Sasha to the email thread this time...]

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Sasha Levin]

On Thu, May 02, 2019 at 08:11:07AM +1000, Dave Chinner wrote:
>On Wed, May 01, 2019 at 12:28:22PM -0700, Darrick J. Wong wrote:
>> On Wed, May 01, 2019 at 07:51:29PM +0200, Andre Noll wrote:
>> > On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
>> > > On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
>> > > > On Wed, May 01, 08:36, Darrick J. Wong wrote
>> > > > > > > You could send this patch to the stable list, but my guess is that
>> > > > > > > they'd prefer a straight backport of all three commits...
>> > > > > >
>> > > > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
>> > > > > > four conflicting files. The conflicts are trivial to resolve (git
>> > > > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
>> > > > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
>> > > > > > (xfs: create a function to query all records in a btree) is needed as
>> > > > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
>> > > > > > to common file) on top of that gives non-trivial conflicts.
>> > > > >
>> > > > > Ah, I suspected that might happen.  Backports are hard. :(
>> > > > >
>> > > > > I suppose one saving grace of the patch you sent is that it'll likely
>> > > > > break the build if anyone ever /does/ attempt a backport of those first
>> > > > > two commits.  Perhaps that is the most practical way forward.
>> > > > >
>> > > > > > So, for automatic backporting we would need to cherry-pick even more,
>> > > > > > and each backported commit should be tested of course. Given this, do
>> > > > > > you still think Greg prefers a rather large set of straight backports
>> > > > > > over the simple commit that just pulls in the missing function?
>> > > > >
>> > > > > I think you'd have to ask him that, if you decide not to send
>> > > > > yesterday's patch.
>> > > >
>> > > > Let's try. I've added a sentence to the commit message which explains
>> > > > why a straight backport is not practical, and how to proceed if anyone
>> > > > wants to backport the earlier commits.
>> > > >
>> > > > Greg: Under the given circumstances, would you be willing to accept
>> > > > the patch below for 4.9?
>> > >
>> > > If the xfs maintainers say this is ok, it is fine with me.
>> >
>> > Darrick said, he's in favor of the patch, so I guess I can add his
>> > Acked-by. Would you also like to see the ack from Dave (the author
>> > of the original commit)?
>>
>> FWIW it seems fine to me, though Dave [cc'd] might have stronger opinions...
>
>Only thing I care about is whether it is QA'd properly. Greg, Sasha,
>is the 4.9 stable kernel having fstests run on it as part of the
>release gating?

I test only for 5.1 and 4.19 and this point. I don't have a solid
baseline for anything older.

--
Thanks,
Sasha

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Sasha Levin]

On Thu, May 02, 2019 at 01:44:40PM +0200, Greg Kroah-Hartman wrote:
>On Thu, May 02, 2019 at 08:11:07AM +1000, Dave Chinner wrote:
>> On Wed, May 01, 2019 at 12:28:22PM -0700, Darrick J. Wong wrote:
>> > On Wed, May 01, 2019 at 07:51:29PM +0200, Andre Noll wrote:
>> > > On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
>> > > > On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
>> > > > > On Wed, May 01, 08:36, Darrick J. Wong wrote
>> > > > > > > > You could send this patch to the stable list, but my guess is that
>> > > > > > > > they'd prefer a straight backport of all three commits...
>> > > > > > >
>> > > > > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
>> > > > > > > four conflicting files. The conflicts are trivial to resolve (git
>> > > > > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
>> > > > > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
>> > > > > > > (xfs: create a function to query all records in a btree) is needed as
>> > > > > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
>> > > > > > > to common file) on top of that gives non-trivial conflicts.
>> > > > > >
>> > > > > > Ah, I suspected that might happen.  Backports are hard. :(
>> > > > > >
>> > > > > > I suppose one saving grace of the patch you sent is that it'll likely
>> > > > > > break the build if anyone ever /does/ attempt a backport of those first
>> > > > > > two commits.  Perhaps that is the most practical way forward.
>> > > > > >
>> > > > > > > So, for automatic backporting we would need to cherry-pick even more,
>> > > > > > > and each backported commit should be tested of course. Given this, do
>> > > > > > > you still think Greg prefers a rather large set of straight backports
>> > > > > > > over the simple commit that just pulls in the missing function?
>> > > > > >
>> > > > > > I think you'd have to ask him that, if you decide not to send
>> > > > > > yesterday's patch.
>> > > > >
>> > > > > Let's try. I've added a sentence to the commit message which explains
>> > > > > why a straight backport is not practical, and how to proceed if anyone
>> > > > > wants to backport the earlier commits.
>> > > > >
>> > > > > Greg: Under the given circumstances, would you be willing to accept
>> > > > > the patch below for 4.9?
>> > > >
>> > > > If the xfs maintainers say this is ok, it is fine with me.
>> > >
>> > > Darrick said, he's in favor of the patch, so I guess I can add his
>> > > Acked-by. Would you also like to see the ack from Dave (the author
>> > > of the original commit)?
>> >
>> > FWIW it seems fine to me, though Dave [cc'd] might have stronger opinions...
>>
>> Only thing I care about is whether it is QA'd properly. Greg, Sasha,
>> is the 4.9 stable kernel having fstests run on it as part of the
>> release gating?
>
>I do not know about fstests, I know Linaro was looking into doing it as
>part of the test suites that they verify before I do a release.  But I
>doubt it's run on an XFS filesystem.
>
>Sasha was doing some work in this area though, Sasha?

My biggest blocker at this point that it is extremely difficult to get a
baseline for a kernel version.

Even after adding all the "known" failures to the expunge list, I ket
hitting issues that reproduced once every 100 runs, and once those are
expunged I started hitting even rarer stuff.

While there's no actual real difficulty in building these expunge lists,
it ended up being somewhat of a loop where I couldn't establish a solid
baseline since random things kept breaking.

--
Thanks,
Sasha

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 09:20:27AM -0400, Sasha Levin wrote:
> On Thu, May 02, 2019 at 01:44:40PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 02, 2019 at 08:11:07AM +1000, Dave Chinner wrote:
> > > On Wed, May 01, 2019 at 12:28:22PM -0700, Darrick J. Wong wrote:
> > > > On Wed, May 01, 2019 at 07:51:29PM +0200, Andre Noll wrote:
> > > > > On Wed, May 01, 19:15, Greg Kroah-Hartman wrote
> > > > > > On Wed, May 01, 2019 at 06:59:33PM +0200, Andre Noll wrote:
> > > > > > > On Wed, May 01, 08:36, Darrick J. Wong wrote
> > > > > > > > > > You could send this patch to the stable list, but my guess is that
> > > > > > > > > > they'd prefer a straight backport of all three commits...
> > > > > > > > >
> > > > > > > > > Hm, cherry-picking the first commit onto 4.9,171 already gives
> > > > > > > > > four conflicting files. The conflicts are trivial to resolve (git
> > > > > > > > > cherry-pick -xX theirs 21ec54168b36 does it), but that doesn't
> > > > > > > > > compile because xfs_btree_query_all() is missing.  So e9a2599a249ed
> > > > > > > > > (xfs: create a function to query all records in a btree) is needed as
> > > > > > > > > well. But then, applying 86210fbebae (xfs: move various type verifiers
> > > > > > > > > to common file) on top of that gives non-trivial conflicts.
> > > > > > > >
> > > > > > > > Ah, I suspected that might happen.  Backports are hard. :(
> > > > > > > >
> > > > > > > > I suppose one saving grace of the patch you sent is that it'll likely
> > > > > > > > break the build if anyone ever /does/ attempt a backport of those first
> > > > > > > > two commits.  Perhaps that is the most practical way forward.
> > > > > > > >
> > > > > > > > > So, for automatic backporting we would need to cherry-pick even more,
> > > > > > > > > and each backported commit should be tested of course. Given this, do
> > > > > > > > > you still think Greg prefers a rather large set of straight backports
> > > > > > > > > over the simple commit that just pulls in the missing function?
> > > > > > > >
> > > > > > > > I think you'd have to ask him that, if you decide not to send
> > > > > > > > yesterday's patch.
> > > > > > >
> > > > > > > Let's try. I've added a sentence to the commit message which explains
> > > > > > > why a straight backport is not practical, and how to proceed if anyone
> > > > > > > wants to backport the earlier commits.
> > > > > > >
> > > > > > > Greg: Under the given circumstances, would you be willing to accept
> > > > > > > the patch below for 4.9?
> > > > > >
> > > > > > If the xfs maintainers say this is ok, it is fine with me.
> > > > >
> > > > > Darrick said, he's in favor of the patch, so I guess I can add his
> > > > > Acked-by. Would you also like to see the ack from Dave (the author
> > > > > of the original commit)?
> > > >
> > > > FWIW it seems fine to me, though Dave [cc'd] might have stronger opinions...
> > > 
> > > Only thing I care about is whether it is QA'd properly. Greg, Sasha,
> > > is the 4.9 stable kernel having fstests run on it as part of the
> > > release gating?
> > 
> > I do not know about fstests, I know Linaro was looking into doing it as
> > part of the test suites that they verify before I do a release.  But I
> > doubt it's run on an XFS filesystem.
> > 
> > Sasha was doing some work in this area though, Sasha?
> 
> My biggest blocker at this point that it is extremely difficult to get a
> baseline for a kernel version.
> 
> Even after adding all the "known" failures to the expunge list, I ket
> hitting issues that reproduced once every 100 runs, and once those are
> expunged I started hitting even rarer stuff.
> 
> While there's no actual real difficulty in building these expunge lists,
> it ended up being somewhat of a loop where I couldn't establish a solid
> baseline since random things kept breaking.

Ok, then how about we hold off on this patch for 4.9.y then.  "no one"
should be using 4.9.y in a "server system" anymore, unless you happen to
have an enterprise kernel based on it.  So we should be fine as the
users of the older kernels don't run xfs.

thanks,

greg k-h

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Thu, May 02, 16:10, Greg Kroah-Hartman wrote
> Ok, then how about we hold off on this patch for 4.9.y then.  "no one"
> should be using 4.9.y in a "server system" anymore, unless you happen to
> have an enterprise kernel based on it.  So we should be fine as the
> users of the older kernels don't run xfs.

Well, we do run xfs on top of bcache on vanilla 4.9 kernels on a few
dozen production servers here. Mainly because we ran into all sorts
of issues with newer kernels (not necessary related to xfs). 4.9,
OTOH, appears to be rock solid for our workload.

But I'm OK with holding off on the patch. According to Darrick, the
issue that is fixed by the patch should not cause serious problems
anyway if xfs debugging is turned off.

Best
Andre
-- 
Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 05:27:36PM +0200, Andre Noll wrote:
> On Thu, May 02, 16:10, Greg Kroah-Hartman wrote
> > Ok, then how about we hold off on this patch for 4.9.y then.  "no one"
> > should be using 4.9.y in a "server system" anymore, unless you happen to
> > have an enterprise kernel based on it.  So we should be fine as the
> > users of the older kernels don't run xfs.
> 
> Well, we do run xfs on top of bcache on vanilla 4.9 kernels on a few
> dozen production servers here. Mainly because we ran into all sorts
> of issues with newer kernels (not necessary related to xfs). 4.9,
> OTOH, appears to be rock solid for our workload.

Great, but what is wrong with 4.14.y or better yet, 4.19.y?  Do those
also work for your workload?  If not, we should fix that, and soon :)

I would _STRONGLY_ recommend moving of of 4.9 on any non-SoC-based
system at this point in time, there should not be any reason to stick
with it, unless you are paying a company to provide support for it.

thanks,

greg k-h

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

On Thu, May 02, 18:52, Greg Kroah-Hartman wrote
> On Thu, May 02, 2019 at 05:27:36PM +0200, Andre Noll wrote:
> > On Thu, May 02, 16:10, Greg Kroah-Hartman wrote
> > > Ok, then how about we hold off on this patch for 4.9.y then.  "no one"
> > > should be using 4.9.y in a "server system" anymore, unless you happen to
> > > have an enterprise kernel based on it.  So we should be fine as the
> > > users of the older kernels don't run xfs.
> > 
> > Well, we do run xfs on top of bcache on vanilla 4.9 kernels on a few
> > dozen production servers here. Mainly because we ran into all sorts
> > of issues with newer kernels (not necessary related to xfs). 4.9,
> > OTOH, appears to be rock solid for our workload.
> 
> Great, but what is wrong with 4.14.y or better yet, 4.19.y?  Do those
> also work for your workload?  If not, we should fix that, and soon :)

Some months ago we tried 4.14 and it was a real disaster: random
crashes with nothing in the logs on the file servers and unkillable
hung processes on the compute machines. The thing is, I can't afford
an extended downtime of these production systems, or test patches, or
enable debugging options which slow down the systems too much. Also,
10 of the compute nodes load the nvidia module, so all bets are off
anyway. But we've seen the hung processes also on the non-gpu nodes
where the nvidia module is not loaded.

As for 4.19, xfs on bcache was broken until a couple of weeks
ago. Meanwhile the fix (e578f90d8a9c) went in, so I benchmarked 4.19.x
on one system briefly. To my surprise the results were *worse* than
with 4.9. This seems to be another cache bypass issue, but I need to
have a closer look, and more reliable numbers.

> I would _STRONGLY_ recommend moving of of 4.9 on any non-SoC-based
> system at this point in time, there should not be any reason to stick
> with it, unless you are paying a company to provide support for it.

That's really bad news :(

Thanks for sharing your thoughts about the future of 4.9, though. I'll
try to spend some time on the bcache issue on 4.19.

Best
Andre
-- 
Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Sasha Levin]

On Thu, May 02, 2019 at 07:45:16PM +0200, Andre Noll wrote:
>On Thu, May 02, 18:52, Greg Kroah-Hartman wrote
>> On Thu, May 02, 2019 at 05:27:36PM +0200, Andre Noll wrote:
>> > On Thu, May 02, 16:10, Greg Kroah-Hartman wrote
>> > > Ok, then how about we hold off on this patch for 4.9.y then.  "no one"
>> > > should be using 4.9.y in a "server system" anymore, unless you happen to
>> > > have an enterprise kernel based on it.  So we should be fine as the
>> > > users of the older kernels don't run xfs.
>> >
>> > Well, we do run xfs on top of bcache on vanilla 4.9 kernels on a few
>> > dozen production servers here. Mainly because we ran into all sorts
>> > of issues with newer kernels (not necessary related to xfs). 4.9,
>> > OTOH, appears to be rock solid for our workload.
>>
>> Great, but what is wrong with 4.14.y or better yet, 4.19.y?  Do those
>> also work for your workload?  If not, we should fix that, and soon :)
>
>Some months ago we tried 4.14 and it was a real disaster: random
>crashes with nothing in the logs on the file servers and unkillable
>hung processes on the compute machines. The thing is, I can't afford
>an extended downtime of these production systems, or test patches, or
>enable debugging options which slow down the systems too much. Also,
>10 of the compute nodes load the nvidia module, so all bets are off
>anyway. But we've seen the hung processes also on the non-gpu nodes
>where the nvidia module is not loaded.
>
>As for 4.19, xfs on bcache was broken until a couple of weeks
>ago. Meanwhile the fix (e578f90d8a9c) went in, so I benchmarked 4.19.x
>on one system briefly. To my surprise the results were *worse* than
>with 4.9. This seems to be another cache bypass issue, but I need to
>have a closer look, and more reliable numbers.

Is this something you can reproduce outside of those 10 magical
machines?

--
Thanks,
Sasha