xfs: Assertion failed in xfs_ag_resv_init()

xfs: Assertion failed in xfs_ag_resv_init()

[Andre Noll]

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

Hi

I'm hitting the assertion below when mounting an xfs filesystem
stored on a thin LV. The mount command segfaults, the machine
is unusable afterwards and requires a hard reset. This is 100%
reproducible. xfs_repair did not report any inconsistencies and did
not fix the issue.

[  546.622715] XFS (dm-6): Mounting V5 Filesystem
[  546.867893] XFS (dm-6): Ending clean mount
[  546.898846] XFS: Assertion failed: xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + xfs_perag_resv(pag, XFS_AG_RESV_AGFL)->ar_reserved <= pag->pagf_freeblks + pag->pagf_flcount, file: /ebio/maan/scm/OTHER/linux/fs/xfs/libxfs/xfs_ag_resv.c, line: 308
[  546.899089] ------------[ cut here ]------------
[  546.899177] kernel BUG at /ebio/maan/scm/OTHER/linux/fs/xfs/xfs_message.c:113!
[  546.899303] invalid opcode: 0000 [#1] SMP
[  546.899392] CPU: 6 PID: 3196 Comm: mount Not tainted 4.9.171 #16
[  546.899485] Hardware name: Supermicro Super Server/H11SSL-i, BIOS 1.0c 10/04/2018
[  546.899611] task: ffff881ffb56de00 task.stack: ffffc9000dd04000
[  546.899704] RIP: 0010:[<ffffffff8130c81b>]  [<ffffffff8130c81b>] assfail+0x1b/0x20
[  546.899882] RSP: 0018:ffffc9000dd07c98  EFLAGS: 00010282
[  546.899972] RAX: 00000000ffffffea RBX: ffff881ff519c000 RCX: 0000000000000000
[  546.900069] RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffff8192384b
[  546.900167] RBP: ffffc9000dd07c98 R08: 0000000000000000 R09: 0000000000000000
[  546.900264] R10: 000000000000000a R11: f000000000000000 R12: ffff881ffbbe0000
[  546.900360] R13: 0000000000000064 R14: ffff881ffbbe0000 R15: 0000000000000000
[  546.900458] FS:  00007fec47b56080(0000) GS:ffff88201fa00000(0000) knlGS:0000000000000000
[  546.900585] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  546.900677] CR2: 00007fec4633b000 CR3: 00000007f6aa1000 CR4: 00000000003406f0
[  546.900773] Stack:
[  546.900852]  ffffc9000dd07cd0 ffffffff812dd46d 0000000000000064 0000000000000000
[  546.901157]  0000000000000064 ffff881ff519c000 0000000000000000 ffffc9000dd07d08
[  546.901462]  ffffffff812faac5 ffff881ffbbe0000 ffff881ffbbe0640 ffff881ffbbe0928
[  546.901766] Call Trace:
[  546.901850]  [<ffffffff812dd46d>] xfs_ag_resv_init+0x16d/0x180
[  546.901947]  [<ffffffff812faac5>] xfs_fs_reserve_ag_blocks+0x35/0xb0
[  546.902041]  [<ffffffff8130de21>] xfs_mountfs+0x891/0x9c0
[  546.902133]  [<ffffffff8131433d>] xfs_fs_fill_super+0x3fd/0x550
[  546.902229]  [<ffffffff8113ede7>] mount_bdev+0x177/0x1b0
[  546.902321]  [<ffffffff81313f40>] ? xfs_finish_flags+0x130/0x130
[  546.902415]  [<ffffffff813126e0>] xfs_fs_mount+0x10/0x20
[  546.902505]  [<ffffffff8113efff>] mount_fs+0xf/0xa0
[  546.902598]  [<ffffffff81159328>] vfs_kern_mount.part.11+0x58/0x100
[  546.902692]  [<ffffffff8115b5f0>] do_mount+0x1a0/0xc50
[  546.902784]  [<ffffffff8110860d>] ? memdup_user+0x3d/0x70
[  546.902876]  [<ffffffff8115c395>] SyS_mount+0x55/0xe0
[  546.902968]  [<ffffffff810018e6>] do_syscall_64+0x56/0xc0
[  546.903063]  [<ffffffff8169771b>] entry_SYSCALL_64_after_swapgs+0x58/0xc2
[  546.903159] Code: 48 c7 c7 10 04 95 81 e8 c4 42 d4 ff 5d c3 66 90 55 48 89 f1 41 89 d0 48 c7 c6 40 04 95 81 48 89 fa 31 ff 48 89 e5 e8 65 fa ff ff <0f> 0b 0f 1f 00 55 48 63 f6 49 89 f9 41 b8 01 00 00 00 b9 10 00 
[  546.906798] RIP  [<ffffffff8130c81b>] assfail+0x1b/0x20
[  546.906934]  RSP <ffffc9000dd07c98>
[  546.907029] ---[ end trace deeb8384ab04a23c ]---

To see why the assertion triggers, I added

        xfs_warn(NULL, "a: %u", xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved);
        xfs_warn(NULL, "b: %u", xfs_perag_resv(pag, XFS_AG_RESV_AGFL)->ar_reserved);
        xfs_warn(NULL, "c: %u", pag->pagf_freeblks);
        xfs_warn(NULL, "d: %u", pag->pagf_flcount);

right before the ASSERT() in xfs_ag_resv.c. Looks like
pag->pagf_freeblks is way too small:

[  149.777035] XFS: a: 267367
[  149.777036] XFS: b: 0
[  149.777036] XFS: c: 6388
[  149.777037] XFS: d: 4

Fortunately, this is new hardware which is not yet in production use,
and the filesystem in question only contains a few dummy files. So
I can test patches.

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

[Darrick J. Wong]

On Tue, Apr 30, 2019 at 02:14:20PM +0200, Andre Noll wrote:
> Hi
> 
> I'm hitting the assertion below when mounting an xfs filesystem
> stored on a thin LV. The mount command segfaults, the machine
> is unusable afterwards and requires a hard reset. This is 100%
> reproducible. xfs_repair did not report any inconsistencies and did
> not fix the issue.
> 
> [  546.622715] XFS (dm-6): Mounting V5 Filesystem
> [  546.867893] XFS (dm-6): Ending clean mount
> [  546.898846] XFS: Assertion failed: xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved + xfs_perag_resv(pag, XFS_AG_RESV_AGFL)->ar_reserved <= pag->pagf_freeblks + pag->pagf_flcount, file: /ebio/maan/scm/OTHER/linux/fs/xfs/libxfs/xfs_ag_resv.c, line: 308
> [  546.899089] ------------[ cut here ]------------
> [  546.899177] kernel BUG at /ebio/maan/scm/OTHER/linux/fs/xfs/xfs_message.c:113!
> [  546.899303] invalid opcode: 0000 [#1] SMP
> [  546.899392] CPU: 6 PID: 3196 Comm: mount Not tainted 4.9.171 #16
> [  546.899485] Hardware name: Supermicro Super Server/H11SSL-i, BIOS 1.0c 10/04/2018
> [  546.899611] task: ffff881ffb56de00 task.stack: ffffc9000dd04000
> [  546.899704] RIP: 0010:[<ffffffff8130c81b>]  [<ffffffff8130c81b>] assfail+0x1b/0x20
> [  546.899882] RSP: 0018:ffffc9000dd07c98  EFLAGS: 00010282
> [  546.899972] RAX: 00000000ffffffea RBX: ffff881ff519c000 RCX: 0000000000000000
> [  546.900069] RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffff8192384b
> [  546.900167] RBP: ffffc9000dd07c98 R08: 0000000000000000 R09: 0000000000000000
> [  546.900264] R10: 000000000000000a R11: f000000000000000 R12: ffff881ffbbe0000
> [  546.900360] R13: 0000000000000064 R14: ffff881ffbbe0000 R15: 0000000000000000
> [  546.900458] FS:  00007fec47b56080(0000) GS:ffff88201fa00000(0000) knlGS:0000000000000000
> [  546.900585] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  546.900677] CR2: 00007fec4633b000 CR3: 00000007f6aa1000 CR4: 00000000003406f0
> [  546.900773] Stack:
> [  546.900852]  ffffc9000dd07cd0 ffffffff812dd46d 0000000000000064 0000000000000000
> [  546.901157]  0000000000000064 ffff881ff519c000 0000000000000000 ffffc9000dd07d08
> [  546.901462]  ffffffff812faac5 ffff881ffbbe0000 ffff881ffbbe0640 ffff881ffbbe0928
> [  546.901766] Call Trace:
> [  546.901850]  [<ffffffff812dd46d>] xfs_ag_resv_init+0x16d/0x180
> [  546.901947]  [<ffffffff812faac5>] xfs_fs_reserve_ag_blocks+0x35/0xb0
> [  546.902041]  [<ffffffff8130de21>] xfs_mountfs+0x891/0x9c0
> [  546.902133]  [<ffffffff8131433d>] xfs_fs_fill_super+0x3fd/0x550
> [  546.902229]  [<ffffffff8113ede7>] mount_bdev+0x177/0x1b0
> [  546.902321]  [<ffffffff81313f40>] ? xfs_finish_flags+0x130/0x130
> [  546.902415]  [<ffffffff813126e0>] xfs_fs_mount+0x10/0x20
> [  546.902505]  [<ffffffff8113efff>] mount_fs+0xf/0xa0
> [  546.902598]  [<ffffffff81159328>] vfs_kern_mount.part.11+0x58/0x100
> [  546.902692]  [<ffffffff8115b5f0>] do_mount+0x1a0/0xc50
> [  546.902784]  [<ffffffff8110860d>] ? memdup_user+0x3d/0x70
> [  546.902876]  [<ffffffff8115c395>] SyS_mount+0x55/0xe0
> [  546.902968]  [<ffffffff810018e6>] do_syscall_64+0x56/0xc0
> [  546.903063]  [<ffffffff8169771b>] entry_SYSCALL_64_after_swapgs+0x58/0xc2
> [  546.903159] Code: 48 c7 c7 10 04 95 81 e8 c4 42 d4 ff 5d c3 66 90 55 48 89 f1 41 89 d0 48 c7 c6 40 04 95 81 48 89 fa 31 ff 48 89 e5 e8 65 fa ff ff <0f> 0b 0f 1f 00 55 48 63 f6 49 89 f9 41 b8 01 00 00 00 b9 10 00 
> [  546.906798] RIP  [<ffffffff8130c81b>] assfail+0x1b/0x20
> [  546.906934]  RSP <ffffc9000dd07c98>
> [  546.907029] ---[ end trace deeb8384ab04a23c ]---
> 
> To see why the assertion triggers, I added
> 
>         xfs_warn(NULL, "a: %u", xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved);
>         xfs_warn(NULL, "b: %u", xfs_perag_resv(pag, XFS_AG_RESV_AGFL)->ar_reserved);
>         xfs_warn(NULL, "c: %u", pag->pagf_freeblks);
>         xfs_warn(NULL, "d: %u", pag->pagf_flcount);
> 
> right before the ASSERT() in xfs_ag_resv.c. Looks like
> pag->pagf_freeblks is way too small:
> 
> [  149.777035] XFS: a: 267367
> [  149.777036] XFS: b: 0
> [  149.777036] XFS: c: 6388
> [  149.777037] XFS: d: 4
> 
> Fortunately, this is new hardware which is not yet in production use,
> and the filesystem in question only contains a few dummy files. So
> I can test patches.

The assert (and your very helpful debugging xfs_warns) indicate that for
the kernel was trying to reserve 267,367 blocks to guarantee space for
metadata btrees in an allocation group (AG) that has only 6,392 blocks
remaining.

This per-AG block reservation exists to avoid running out of space for
metadata in worst case situations (needing space midway through a
transaction on a nearly full fs).  The assert your machine hit is a
debugging warning to alert developers to the per-AG block reservation
system deciding that it won't be able to handle all cases.

Hmmm, what features does this filesystem have enabled?

Given that XFS_AG_RESV_METADATA > 0 and there's no warning about the
experimental reflink feature, that implies that the free inode btree
(finobt) feature is enabled?

The awkward thing about the finobt reservation is that it was added long
after the finobt feature was enabled, to fix a corner case in that code.
If you're coming from an older kernel, there might not be enough free
space in the AG to guarantee space for the finobt.

(Maybe we ought to turn that ASSERT into a xfs_warn or something...?)

In any case, if you're /not/ trying to debug the XFS code itself, you
could set CONFIG_XFS_DEBUG=n to turn off all the programmer debugging
pieces (which will improve fs performance substantially).

If you want all the verbose debugging checks without the kernel hang
behavior you could set CONFIG_XFS_DEBUG=n and CONFIG_XFS_WARN=y.

--D

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

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Andre Noll]

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

On Tue, Apr 30, 08:11, Darrick J. Wong wrote
> > To see why the assertion triggers, I added
> > 
> >         xfs_warn(NULL, "a: %u", xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved);
> >         xfs_warn(NULL, "b: %u", xfs_perag_resv(pag, XFS_AG_RESV_AGFL)->ar_reserved);
> >         xfs_warn(NULL, "c: %u", pag->pagf_freeblks);
> >         xfs_warn(NULL, "d: %u", pag->pagf_flcount);
> > 
> > right before the ASSERT() in xfs_ag_resv.c. Looks like
> > pag->pagf_freeblks is way too small:
> > 
> > [  149.777035] XFS: a: 267367
> > [  149.777036] XFS: b: 0
> > [  149.777036] XFS: c: 6388
> > [  149.777037] XFS: d: 4
> > 
> > Fortunately, this is new hardware which is not yet in production use,
> > and the filesystem in question only contains a few dummy files. So
> > I can test patches.
> 
> The assert (and your very helpful debugging xfs_warns) indicate that for
> the kernel was trying to reserve 267,367 blocks to guarantee space for
> metadata btrees in an allocation group (AG) that has only 6,392 blocks
> remaining.
> 
> This per-AG block reservation exists to avoid running out of space for
> metadata in worst case situations (needing space midway through a
> transaction on a nearly full fs).  The assert your machine hit is a
> debugging warning to alert developers to the per-AG block reservation
> system deciding that it won't be able to handle all cases.

So, consider yourself alerted :)

> Hmmm, what features does this filesystem have enabled?

With CONFIG_XFS_DEBUG=n the mount succeeded, and xfs_info says

	meta-data=/dev/mapper/zeal-tst   isize=512    agcount=101, agsize=268435392 blks
		 =                       sectsz=4096  attr=2, projid32bit=1
		 =                       crc=1        finobt=1 spinodes=0 rmapbt=0
		 =                       reflink=0
	data     =                       bsize=4096   blocks=26843545600, imaxpct=1
		 =                       sunit=64     swidth=1024 blks
	naming   =version 2              bsize=4096   ascii-ci=0 ftype=1
	log      =internal               bsize=4096   blocks=521728, version=2
		 =                       sectsz=4096  sunit=1 blks, lazy-count=1
	realtime =none                   extsz=4096   blocks=0, rtextents=0

> Given that XFS_AG_RESV_METADATA > 0 and there's no warning about the
> experimental reflink feature, that implies that the free inode btree
> (finobt) feature is enabled?

yep: no reflink, but finobt.

> The awkward thing about the finobt reservation is that it was added long
> after the finobt feature was enabled, to fix a corner case in that code.
> If you're coming from an older kernel, there might not be enough free
> space in the AG to guarantee space for the finobt.

No, this machine and its storage is new, and never ran a kernel other
than 4.9.x. The filesystem was created with mkfs.xfs of xfsprogs
version 4.9.0+nmu1ubuntu2, which ships with Ubuntu-18.04.

Isn't it surprising to run into ENOSPC on an almost empty 100T
large filesystem? If so, do you think the issue could be related to
dm-thin? Another explanation would be that the assert condition is
broken, for example because pag->pagf_freeblks is not uptodate.

> In any case, if you're /not/ trying to debug the XFS code itself, you
> could set CONFIG_XFS_DEBUG=n to turn off all the programmer debugging
> pieces (which will improve fs performance substantially).
> 
> If you want all the verbose debugging checks without the kernel hang
> behavior you could set CONFIG_XFS_DEBUG=n and CONFIG_XFS_WARN=y.

Sure, debugging will be turned off when the machine goes into production
mode. For stress testing new hardware I prefer to leave it on, though.

Anyways, do you believe that the assert is just an overzealous check
to inform developers about a corner case that never triggers under
normal circumstances, or is this an issue that will come back to hurt
plenty when the assert is ignored due to CONFIG_XFS_DEBUG=n?

One more data point: After booting into a CONFIG_XFS_DEBUG=n kernel,
mounting and unmounting the filesystem, and booting back into the
CONFIG_XFS_DEBUG=y kernel, the assert still triggers.

Thanks for your help
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 Tue, Apr 30, 2019 at 06:25:06PM +0200, Andre Noll wrote:
> On Tue, Apr 30, 08:11, Darrick J. Wong wrote
> > > To see why the assertion triggers, I added
> > > 
> > >         xfs_warn(NULL, "a: %u", xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved);
> > >         xfs_warn(NULL, "b: %u", xfs_perag_resv(pag, XFS_AG_RESV_AGFL)->ar_reserved);
> > >         xfs_warn(NULL, "c: %u", pag->pagf_freeblks);
> > >         xfs_warn(NULL, "d: %u", pag->pagf_flcount);
> > > 
> > > right before the ASSERT() in xfs_ag_resv.c. Looks like
> > > pag->pagf_freeblks is way too small:
> > > 
> > > [  149.777035] XFS: a: 267367
> > > [  149.777036] XFS: b: 0
> > > [  149.777036] XFS: c: 6388
> > > [  149.777037] XFS: d: 4
> > > 
> > > Fortunately, this is new hardware which is not yet in production use,
> > > and the filesystem in question only contains a few dummy files. So
> > > I can test patches.
> > 
> > The assert (and your very helpful debugging xfs_warns) indicate that for
> > the kernel was trying to reserve 267,367 blocks to guarantee space for
> > metadata btrees in an allocation group (AG) that has only 6,392 blocks
> > remaining.
> > 
> > This per-AG block reservation exists to avoid running out of space for
> > metadata in worst case situations (needing space midway through a
> > transaction on a nearly full fs).  The assert your machine hit is a
> > debugging warning to alert developers to the per-AG block reservation
> > system deciding that it won't be able to handle all cases.
> 
> So, consider yourself alerted :)
> 
> > Hmmm, what features does this filesystem have enabled?
> 
> With CONFIG_XFS_DEBUG=n the mount succeeded, and xfs_info says
> 
> 	meta-data=/dev/mapper/zeal-tst   isize=512    agcount=101, agsize=268435392 blks
> 		 =                       sectsz=4096  attr=2, projid32bit=1
> 		 =                       crc=1        finobt=1 spinodes=0 rmapbt=0
> 		 =                       reflink=0
> 	data     =                       bsize=4096   blocks=26843545600, imaxpct=1

Oh, wait, you have a 100T filesystem with a runt AG at the end due to
the raid striping...

26843545600 % 268435392 == 6400 blocks (in AG 100)

And that's why there's 6,392 free blocks in an AG and an attempted
reservation of 267,367 blocks.

Sorry, I misunderstood and thought this was a new-ish but nearly full
filesystem, not a completely new filesystem.

In that case, the patch you want is c08768977b9 ("xfs: finobt AG
reserves don't consider last AG can be a runt") which has not been
backported to 4.9.  That patch relies on a function introduced in
21ec54168b36 ("xfs: create block pointer check functions") and moved to
a different file in 86210fbebae6e ("xfs: move various type verifiers to
common file").

The c087 patch which will generate appropriately sized reservations for
the last AG if it is significantly smaller than the the other and should
fix the assertion failure.

--D

> 		 =                       sunit=64     swidth=1024 blks
> 	naming   =version 2              bsize=4096   ascii-ci=0 ftype=1
> 	log      =internal               bsize=4096   blocks=521728, version=2
> 		 =                       sectsz=4096  sunit=1 blks, lazy-count=1
> 	realtime =none                   extsz=4096   blocks=0, rtextents=0
> 
> > Given that XFS_AG_RESV_METADATA > 0 and there's no warning about the
> > experimental reflink feature, that implies that the free inode btree
> > (finobt) feature is enabled?
> 
> yep: no reflink, but finobt.
> 
> > The awkward thing about the finobt reservation is that it was added long
> > after the finobt feature was enabled, to fix a corner case in that code.
> > If you're coming from an older kernel, there might not be enough free
> > space in the AG to guarantee space for the finobt.
> 
> No, this machine and its storage is new, and never ran a kernel other
> than 4.9.x. The filesystem was created with mkfs.xfs of xfsprogs
> version 4.9.0+nmu1ubuntu2, which ships with Ubuntu-18.04.
> 
> Isn't it surprising to run into ENOSPC on an almost empty 100T
> large filesystem? If so, do you think the issue could be related to
> dm-thin? Another explanation would be that the assert condition is
> broken, for example because pag->pagf_freeblks is not uptodate.
> 
> > In any case, if you're /not/ trying to debug the XFS code itself, you
> > could set CONFIG_XFS_DEBUG=n to turn off all the programmer debugging
> > pieces (which will improve fs performance substantially).
> > 
> > If you want all the verbose debugging checks without the kernel hang
> > behavior you could set CONFIG_XFS_DEBUG=n and CONFIG_XFS_WARN=y.
> 
> Sure, debugging will be turned off when the machine goes into production
> mode. For stress testing new hardware I prefer to leave it on, though.
> 
> Anyways, do you believe that the assert is just an overzealous check
> to inform developers about a corner case that never triggers under
> normal circumstances, or is this an issue that will come back to hurt
> plenty when the assert is ignored due to CONFIG_XFS_DEBUG=n?
> 
> One more data point: After booting into a CONFIG_XFS_DEBUG=n kernel,
> mounting and unmounting the filesystem, and booting back into the
> CONFIG_XFS_DEBUG=y kernel, the assert still triggers.
> 
> Thanks for your help
> 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()

[Andre Noll]

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

On Tue, Apr 30, 10:40, Darrick J. Wong wrote
> > With CONFIG_XFS_DEBUG=n the mount succeeded, and xfs_info says
> > 
> > 	meta-data=/dev/mapper/zeal-tst   isize=512    agcount=101, agsize=268435392 blks
> > 		 =                       sectsz=4096  attr=2, projid32bit=1
> > 		 =                       crc=1        finobt=1 spinodes=0 rmapbt=0
> > 		 =                       reflink=0
> > 	data     =                       bsize=4096   blocks=26843545600, imaxpct=1
> 
> Oh, wait, you have a 100T filesystem with a runt AG at the end due to
> the raid striping...
> 
> 26843545600 % 268435392 == 6400 blocks (in AG 100)
> 
> And that's why there's 6,392 free blocks in an AG and an attempted
> reservation of 267,367 blocks.

Jup, that nails it.

> In that case, the patch you want is c08768977b9 ("xfs: finobt AG
> reserves don't consider last AG can be a runt") which has not been
> backported to 4.9.  That patch relies on a function introduced in
> 21ec54168b36 ("xfs: create block pointer check functions") and moved to
> a different file in 86210fbebae6e ("xfs: move various type verifiers to
> common file").
> 
> The c087 patch which will generate appropriately sized reservations for
> the last AG if it is significantly smaller than the the other and should
> fix the assertion failure.

Great. Thanks a lot for digging out these commits.

Would you be willing to support backporting this commit to
4.9.x? IOW, something like the below (against 4.9.171) which puts
xfs_inobt_max_size() into libxfs/xfs_ialloc_btree.c. Seems to work
fine.

Best
Andre
---
commit f847bda4d612744ff1812788417bd8df41a806d3
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.
    
    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()

[Darrick J. Wong]

On Tue, Apr 30, 2019 at 09:05:25PM +0200, Andre Noll wrote:
> On Tue, Apr 30, 10:40, Darrick J. Wong wrote
> > > With CONFIG_XFS_DEBUG=n the mount succeeded, and xfs_info says
> > > 
> > > 	meta-data=/dev/mapper/zeal-tst   isize=512    agcount=101, agsize=268435392 blks
> > > 		 =                       sectsz=4096  attr=2, projid32bit=1
> > > 		 =                       crc=1        finobt=1 spinodes=0 rmapbt=0
> > > 		 =                       reflink=0
> > > 	data     =                       bsize=4096   blocks=26843545600, imaxpct=1
> > 
> > Oh, wait, you have a 100T filesystem with a runt AG at the end due to
> > the raid striping...
> > 
> > 26843545600 % 268435392 == 6400 blocks (in AG 100)
> > 
> > And that's why there's 6,392 free blocks in an AG and an attempted
> > reservation of 267,367 blocks.
> 
> Jup, that nails it.
> 
> > In that case, the patch you want is c08768977b9 ("xfs: finobt AG
> > reserves don't consider last AG can be a runt") which has not been
> > backported to 4.9.  That patch relies on a function introduced in
> > 21ec54168b36 ("xfs: create block pointer check functions") and moved to
> > a different file in 86210fbebae6e ("xfs: move various type verifiers to
> > common file").
> > 
> > The c087 patch which will generate appropriately sized reservations for
> > the last AG if it is significantly smaller than the the other and should
> > fix the assertion failure.
> 
> Great. Thanks a lot for digging out these commits.
> 
> Would you be willing to support backporting this commit to
> 4.9.x? IOW, something like the below (against 4.9.171) which puts
> xfs_inobt_max_size() into libxfs/xfs_ialloc_btree.c. Seems to work
> fine.
> 
> Best
> Andre
> ---
> commit f847bda4d612744ff1812788417bd8df41a806d3
> 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.

You could send this patch to the stable list, but my guess is that
they'd prefer a straight backport of all three commits...

>     
>     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);
> +}

...because this piece ^^^ is going to cause gcc errors if anyone
(remember they have AI bots to do the mechanical parts now) backports
the original 21ec5 and 86210 commits and now there are two copies of
this function.

As for the general idea of supplying a xfs_ag_block_count function and
teaching xfs_inobt_max_size to use it, yes, I'd support that. :)

--D

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

Re: xfs: Assertion failed in xfs_ag_resv_init()

[Andre Noll]

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

On Tue, Apr 30, 12:18, Darrick J. Wong wrote
> > commit f847bda4d612744ff1812788417bd8df41a806d3
> > 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.
> 
> 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.

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 guess the important question is how much impact this issue
has on production systems (i.e., on CONFIG_XFS_DEBUG=n kernels,
where the assert statement is not compiled in). If the unpatched
xfs_inobt_max_size() is very unlikely to cause problems on such
systems, we might as well live with it.

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 Tue, Apr 30, 2019 at 11:07:24PM +0200, Andre Noll wrote:
> On Tue, Apr 30, 12:18, Darrick J. Wong wrote
> > > commit f847bda4d612744ff1812788417bd8df41a806d3
> > > 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.
> > 
> > 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.

> I guess the important question is how much impact this issue
> has on production systems (i.e., on CONFIG_XFS_DEBUG=n kernels,
> where the assert statement is not compiled in). If the unpatched
> xfs_inobt_max_size() is very unlikely to cause problems on such
> systems, we might as well live with it.

...but it's unlikely to cause problems since the allocator will probably
pass over that runt AG so long as the others have more space and it will
mostly stay empty.

(He says knocking on wood knowing that he's now tempted fate :P)

--D

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

[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()

[Andre Noll]

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

On Wed, May 01, 08:36, Darrick J. Wong wrote

> > I guess the important question is how much impact this issue
> > has on production systems (i.e., on CONFIG_XFS_DEBUG=n kernels,
> > where the assert statement is not compiled in). If the unpatched
> > xfs_inobt_max_size() is very unlikely to cause problems on such
> > systems, we might as well live with it.
> 
> ...but it's unlikely to cause problems since the allocator will probably
> pass over that runt AG so long as the others have more space and it will
> mostly stay empty.

That makes me feel better, as we have many such systems.  Thanks for
the assessment of risks.

> (He says knocking on wood knowing that he's now tempted fate :P)

One reason more to apply the patch :)

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