[PATCHSET 0/4] xfs: bug fixes for 6.4-rc1

[PATCHSET 0/4] xfs: bug fixes for 6.4-rc1

[Darrick J. Wong]

Hi all,

Here are some assorted bug fixes for 6.4:

 * A regression fix for the allocator refactoring that we did in 6.3.
 * Fix a bug that occurs when formatting an internal log with a stripe
   alignment such that there's free space before the start of the log
   but not after.
 * Make scrub actually take the MMAPLOCK (to lock out page faults) when
   scrubbing the COW fork
 * If we call FUNSHARE on a hole in the data fork, don't create a
   delalloc reservation in the cow fork for that hole.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=kernel-fixes-6.4
---
 fs/xfs/libxfs/xfs_ag.c   |   10 +++++-----
 fs/xfs/libxfs/xfs_bmap.c |    5 +++--
 fs/xfs/scrub/bmap.c      |    4 ++--
 fs/xfs/xfs_iomap.c       |    5 +++--
 4 files changed, 13 insertions(+), 11 deletions(-)

[PATCHSET 0/4] xfs: bug fixes for 6.4-rc1

[Darrick J. Wong]

Hi all,

Here are some assorted bug fixes for 6.4:

 * A regression fix for the allocator refactoring that we did in 6.3.
 * Fix a bug that occurs when formatting an internal log with a stripe
   alignment such that there's free space before the start of the log
   but not after.
 * Make scrub actually take the MMAPLOCK (to lock out page faults) when
   scrubbing the COW fork
 * If we call FUNSHARE on a hole in the data fork, don't create a
   delalloc reservation in the cow fork for that hole.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=kernel-fixes-6.4
---
 fs/xfs/libxfs/xfs_ag.c   |   10 +++++-----
 fs/xfs/libxfs/xfs_bmap.c |    5 +++--
 fs/xfs/scrub/bmap.c      |    4 ++--
 fs/xfs/xfs_iomap.c       |    5 +++--
 4 files changed, 13 insertions(+), 11 deletions(-)

[PATCH 1/4] xfs: don't unconditionally null args->pag in xfs_bmap_btalloc_at_eof

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

xfs/170 on a filesystem with su=128k,sw=4 produces this splat:

BUG: kernel NULL pointer dereference, address: 0000000000000010
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD 0 P4D 0
Oops: 0002 [#1] PREEMPT SMP
CPU: 1 PID: 4022907 Comm: dd Tainted: G        W          6.3.0-xfsx #2 6ebeeffbe9577d32
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-bu
RIP: 0010:xfs_perag_rele+0x10/0x70 [xfs]
RSP: 0018:ffffc90001e43858 EFLAGS: 00010217
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000100
RDX: ffffffffa054e717 RSI: 0000000000000005 RDI: 0000000000000000
RBP: ffff888194eea000 R08: 0000000000000000 R09: 0000000000000037
R10: ffff888100ac1cb0 R11: 0000000000000018 R12: 0000000000000000
R13: ffffc90001e43a38 R14: ffff888194eea000 R15: ffff888194eea000
FS:  00007f93d1a0e740(0000) GS:ffff88843fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 000000018a34f000 CR4: 00000000003506e0
Call Trace:
 <TASK>
 xfs_bmap_btalloc+0x1a7/0x5d0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
 xfs_bmapi_allocate+0xee/0x470 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
 xfs_bmapi_write+0x539/0x9e0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
 xfs_iomap_write_direct+0x1bb/0x2b0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
 xfs_direct_write_iomap_begin+0x51c/0x710 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
 iomap_iter+0x132/0x2f0
 __iomap_dio_rw+0x2f8/0x840
 iomap_dio_rw+0xe/0x30
 xfs_file_dio_write_aligned+0xad/0x180 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
 xfs_file_write_iter+0xfb/0x190 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
 vfs_write+0x2eb/0x410
 ksys_write+0x65/0xe0
 do_syscall_64+0x2b/0x80

This crash occurs under the "out_low_space" label.  We grabbed a perag
reference, passed it via args->pag into xfs_bmap_btalloc_at_eof, and
afterwards args->pag is NULL.  Fix the second function not to clobber
args->pag if the caller had passed one in.

Fixes: 85843327094f ("xfs: factor xfs_bmap_btalloc()")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index b512de0540d5..cd8870a16fd1 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3494,8 +3494,10 @@ xfs_bmap_btalloc_at_eof(
 		if (!caller_pag)
 			args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno));
 		error = xfs_alloc_vextent_exact_bno(args, ap->blkno);
-		if (!caller_pag)
+		if (!caller_pag) {
 			xfs_perag_put(args->pag);
+			args->pag = NULL;
+		}
 		if (error)
 			return error;
 
@@ -3505,7 +3507,6 @@ xfs_bmap_btalloc_at_eof(
 		 * Exact allocation failed. Reset to try an aligned allocation
 		 * according to the original allocation specification.
 		 */
-		args->pag = NULL;
 		args->alignment = stripe_align;
 		args->minlen = nextminlen;
 		args->minalignslop = 0;

[PATCH 2/4] xfs: set bnobt/cntbt numrecs correctly when formatting new AGs

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Through generic/300, I discovered that mkfs.xfs creates corrupt
filesystems when given these parameters:

# mkfs.xfs -d size=512M /dev/sda -f -d su=128k,sw=4 --unsupported
Filesystems formatted with --unsupported are not supported!!
meta-data=/dev/sda               isize=512    agcount=8, agsize=16352 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=1
         =                       reflink=1    bigtime=1 inobtcount=1 nrext64=1
data     =                       bsize=4096   blocks=130816, imaxpct=25
         =                       sunit=32     swidth=128 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=8192, version=2
         =                       sectsz=512   sunit=32 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
         =                       rgcount=0    rgsize=0 blks
Discarding blocks...Done.
# xfs_repair -n /dev/sda
Phase 1 - find and verify superblock...
        - reporting progress in intervals of 15 minutes
Phase 2 - using internal log
        - zero log...
        - 16:30:50: zeroing log - 16320 of 16320 blocks done
        - scan filesystem freespace and inode maps...
agf_freeblks 25, counted 0 in ag 4
sb_fdblocks 8823, counted 8798

The root cause of this problem is the numrecs handling in
xfs_freesp_init_recs, which is used to initialize a new AG.  Prior to
calling the function, we set up the new bnobt block with numrecs == 1
and rely on _freesp_init_recs to format that new record.  If the last
record created has a blockcount of zero, then it sets numrecs = 0.

That last bit isn't correct if the AG contains the log, the start of the
log is not immediately after the initial blocks due to stripe alignment,
and the end of the log is perfectly aligned with the end of the AG.  For
this case, we actually formatted a single bnobt record to handle the
free space before the start of the (stripe aligned) log, and incremented
arec to try to format a second record.  That second record turned out to
be unnecessary, so what we really want is to leave numrecs at 1.

The numrecs handling itself is overly complicated because a different
function sets numrecs == 1.  Change the bnobt creation code to start
with numrecs set to zero and only increment it after successfully
formatting a free space extent into the btree block.

Fixes: f327a00745ff ("xfs: account for log space when formatting new AGs")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_ag.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 1b078bbbf225..4481ce8ead9d 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -499,6 +499,7 @@ xfs_freesp_init_recs(
 			 */
 			arec->ar_blockcount = cpu_to_be32(start -
 						mp->m_ag_prealloc_blocks);
+			be16_add_cpu(&block->bb_numrecs, 1);
 			nrec = arec + 1;
 
 			/*
@@ -509,7 +510,6 @@ xfs_freesp_init_recs(
 					be32_to_cpu(arec->ar_startblock) +
 					be32_to_cpu(arec->ar_blockcount));
 			arec = nrec;
-			be16_add_cpu(&block->bb_numrecs, 1);
 		}
 		/*
 		 * Change record start to after the internal log
@@ -525,8 +525,8 @@ xfs_freesp_init_recs(
 	 */
 	arec->ar_blockcount = cpu_to_be32(id->agsize -
 					  be32_to_cpu(arec->ar_startblock));
-	if (!arec->ar_blockcount)
-		block->bb_numrecs = 0;
+	if (arec->ar_blockcount)
+		be16_add_cpu(&block->bb_numrecs, 1);
 }
 
 /*
@@ -538,7 +538,7 @@ xfs_bnoroot_init(
 	struct xfs_buf		*bp,
 	struct aghdr_init_data	*id)
 {
-	xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 1, id->agno);
+	xfs_btree_init_block(mp, bp, XFS_BTNUM_BNO, 0, 0, id->agno);
 	xfs_freesp_init_recs(mp, bp, id);
 }
 
@@ -548,7 +548,7 @@ xfs_cntroot_init(
 	struct xfs_buf		*bp,
 	struct aghdr_init_data	*id)
 {
-	xfs_btree_init_block(mp, bp, XFS_BTNUM_CNT, 0, 1, id->agno);
+	xfs_btree_init_block(mp, bp, XFS_BTNUM_CNT, 0, 0, id->agno);
 	xfs_freesp_init_recs(mp, bp, id);
 }
 

[PATCH 3/4] xfs: flush dirty data and drain directios before scrubbing cow fork

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

When we're scrubbing the COW fork, we need to take MMAPLOCK_EXCL to
prevent page_mkwrite from modifying any inode state.  The ILOCK should
suffice to avoid confusing online fsck, but let's take the same locks
that we do everywhere else.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/scrub/bmap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index 87ab9f95a487..69bc89d0fc68 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -42,12 +42,12 @@ xchk_setup_inode_bmap(
 	xfs_ilock(sc->ip, XFS_IOLOCK_EXCL);
 
 	/*
-	 * We don't want any ephemeral data fork updates sitting around
+	 * We don't want any ephemeral data/cow fork updates sitting around
 	 * while we inspect block mappings, so wait for directio to finish
 	 * and flush dirty data if we have delalloc reservations.
 	 */
 	if (S_ISREG(VFS_I(sc->ip)->i_mode) &&
-	    sc->sm->sm_type == XFS_SCRUB_TYPE_BMBTD) {
+	    sc->sm->sm_type != XFS_SCRUB_TYPE_BMBTA) {
 		struct address_space	*mapping = VFS_I(sc->ip)->i_mapping;
 
 		sc->ilock_flags |= XFS_MMAPLOCK_EXCL;

[PATCH 4/4] xfs: don't allocate into the data fork for an unshare request

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

For an unshare request, we only have to take action if the data fork has
a shared mapping.  We don't care if someone else set up a cow operation.
If we find nothing in the data fork, return a hole to avoid allocating
space.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_iomap.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 285885c308bd..18c8f168b153 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1006,8 +1006,9 @@ xfs_buffered_write_iomap_begin(
 	if (eof)
 		imap.br_startoff = end_fsb; /* fake hole until the end */
 
-	/* We never need to allocate blocks for zeroing a hole. */
-	if ((flags & IOMAP_ZERO) && imap.br_startoff > offset_fsb) {
+	/* We never need to allocate blocks for zeroing or unsharing a hole. */
+	if ((flags & (IOMAP_UNSHARE | IOMAP_ZERO)) &&
+	    imap.br_startoff > offset_fsb) {
 		xfs_hole_to_iomap(ip, iomap, offset_fsb, imap.br_startoff);
 		goto out_unlock;
 	}

Re: [PATCHSET 0/4] xfs: bug fixes for 6.4-rc1

[Darrick J. Wong]

Disregard this, I got Dave's email wrong, copy-pasted it around the
automation, and then forgot to make sure that /all/ of them were
corrected. :(

Will resend....

--D

On Thu, Apr 27, 2023 at 03:48:18PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> Here are some assorted bug fixes for 6.4:
> 
>  * A regression fix for the allocator refactoring that we did in 6.3.
>  * Fix a bug that occurs when formatting an internal log with a stripe
>    alignment such that there's free space before the start of the log
>    but not after.
>  * Make scrub actually take the MMAPLOCK (to lock out page faults) when
>    scrubbing the COW fork
>  * If we call FUNSHARE on a hole in the data fork, don't create a
>    delalloc reservation in the cow fork for that hole.
> 
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> 
> kernel git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=kernel-fixes-6.4
> ---
>  fs/xfs/libxfs/xfs_ag.c   |   10 +++++-----
>  fs/xfs/libxfs/xfs_bmap.c |    5 +++--
>  fs/xfs/scrub/bmap.c      |    4 ++--
>  fs/xfs/xfs_iomap.c       |    5 +++--
>  4 files changed, 13 insertions(+), 11 deletions(-)
> 

Re: [PATCH 1/4] xfs: don't unconditionally null args->pag in xfs_bmap_btalloc_at_eof

[Dave Chinner]

On Thu, Apr 27, 2023 at 03:48:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xfs/170 on a filesystem with su=128k,sw=4 produces this splat:
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000010
> #PF: supervisor write access in kernel mode
> #PF: error_code(0x0002) - not-present page
> PGD 0 P4D 0
> Oops: 0002 [#1] PREEMPT SMP
> CPU: 1 PID: 4022907 Comm: dd Tainted: G        W          6.3.0-xfsx #2 6ebeeffbe9577d32
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-bu
> RIP: 0010:xfs_perag_rele+0x10/0x70 [xfs]
> RSP: 0018:ffffc90001e43858 EFLAGS: 00010217
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000100
> RDX: ffffffffa054e717 RSI: 0000000000000005 RDI: 0000000000000000
> RBP: ffff888194eea000 R08: 0000000000000000 R09: 0000000000000037
> R10: ffff888100ac1cb0 R11: 0000000000000018 R12: 0000000000000000
> R13: ffffc90001e43a38 R14: ffff888194eea000 R15: ffff888194eea000
> FS:  00007f93d1a0e740(0000) GS:ffff88843fc80000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000010 CR3: 000000018a34f000 CR4: 00000000003506e0
> Call Trace:
>  <TASK>
>  xfs_bmap_btalloc+0x1a7/0x5d0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
>  xfs_bmapi_allocate+0xee/0x470 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
>  xfs_bmapi_write+0x539/0x9e0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
>  xfs_iomap_write_direct+0x1bb/0x2b0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
>  xfs_direct_write_iomap_begin+0x51c/0x710 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
>  iomap_iter+0x132/0x2f0
>  __iomap_dio_rw+0x2f8/0x840
>  iomap_dio_rw+0xe/0x30
>  xfs_file_dio_write_aligned+0xad/0x180 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
>  xfs_file_write_iter+0xfb/0x190 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
>  vfs_write+0x2eb/0x410
>  ksys_write+0x65/0xe0
>  do_syscall_64+0x2b/0x80
> 
> This crash occurs under the "out_low_space" label.  We grabbed a perag
> reference, passed it via args->pag into xfs_bmap_btalloc_at_eof, and
> afterwards args->pag is NULL.  Fix the second function not to clobber
> args->pag if the caller had passed one in.
> 
> Fixes: 85843327094f ("xfs: factor xfs_bmap_btalloc()")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_bmap.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index b512de0540d5..cd8870a16fd1 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3494,8 +3494,10 @@ xfs_bmap_btalloc_at_eof(
>  		if (!caller_pag)
>  			args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno));
>  		error = xfs_alloc_vextent_exact_bno(args, ap->blkno);
> -		if (!caller_pag)
> +		if (!caller_pag) {
>  			xfs_perag_put(args->pag);
> +			args->pag = NULL;
> +		}
>  		if (error)
>  			return error;
>  
> @@ -3505,7 +3507,6 @@ xfs_bmap_btalloc_at_eof(
>  		 * Exact allocation failed. Reset to try an aligned allocation
>  		 * according to the original allocation specification.
>  		 */
> -		args->pag = NULL;
>  		args->alignment = stripe_align;
>  		args->minlen = nextminlen;
>  		args->minalignslop = 0;

Yup, that'll fix the problem.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

FWIW, I'm working on some patches to take this further by always
providing a caller perag to this function.  That gets rid of all the
conditional code here and it gets rid of the only case where we call
xfs_alloc_vextent_near_bno() without args->pag set so the
conditional caller_pag code goes from there, too. This makes
xfs_alloc_vextent_{near,exact}_bno() identical except for one line
so they can be collapsed. And now that we always specify args->pag
for these functions, we can ignore the agno part of the target block
meaning we can select the perag to allocate from at a much higher
level via setting up args->pag when we initially scan the AGs to
set up args->minlen/maxlen based on the the nearest AG with 
contiguous free space large enough for the whole allocation. This
avoids attempting allocations that are guaranteed to fail before
we fall back to iterating from the target block agno and eventually
finding the same AG we originally found that had enough contiguous
free space in it for a maxlen allocation....

I think I can take it further an make both filestreams and the
normal allocator use the same "this ag" algorithm for EOF and
aligned allocation and then have them both fall back to the same
unaligned allocation attempts, which will then allow a bunch of code
to be collapsed in the xfs_bmap_btalloc() path...

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

Re: [PATCH 2/4] xfs: set bnobt/cntbt numrecs correctly when formatting new AGs

[Dave Chinner]

On Thu, Apr 27, 2023 at 03:49:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Through generic/300, I discovered that mkfs.xfs creates corrupt
> filesystems when given these parameters:
> 
> # mkfs.xfs -d size=512M /dev/sda -f -d su=128k,sw=4 --unsupported
> Filesystems formatted with --unsupported are not supported!!
> meta-data=/dev/sda               isize=512    agcount=8, agsize=16352 blks
>          =                       sectsz=512   attr=2, projid32bit=1
>          =                       crc=1        finobt=1, sparse=1, rmapbt=1
>          =                       reflink=1    bigtime=1 inobtcount=1 nrext64=1
> data     =                       bsize=4096   blocks=130816, imaxpct=25
>          =                       sunit=32     swidth=128 blks
> naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
> log      =internal log           bsize=4096   blocks=8192, version=2
>          =                       sectsz=512   sunit=32 blks, lazy-count=1
> realtime =none                   extsz=4096   blocks=0, rtextents=0
>          =                       rgcount=0    rgsize=0 blks
> Discarding blocks...Done.
> # xfs_repair -n /dev/sda
> Phase 1 - find and verify superblock...
>         - reporting progress in intervals of 15 minutes
> Phase 2 - using internal log
>         - zero log...
>         - 16:30:50: zeroing log - 16320 of 16320 blocks done
>         - scan filesystem freespace and inode maps...
> agf_freeblks 25, counted 0 in ag 4
> sb_fdblocks 8823, counted 8798
> 
> The root cause of this problem is the numrecs handling in
> xfs_freesp_init_recs, which is used to initialize a new AG.  Prior to
> calling the function, we set up the new bnobt block with numrecs == 1
> and rely on _freesp_init_recs to format that new record.  If the last
> record created has a blockcount of zero, then it sets numrecs = 0.
> 
> That last bit isn't correct if the AG contains the log, the start of the
> log is not immediately after the initial blocks due to stripe alignment,
> and the end of the log is perfectly aligned with the end of the AG.  For
> this case, we actually formatted a single bnobt record to handle the
> free space before the start of the (stripe aligned) log, and incremented
> arec to try to format a second record.  That second record turned out to
> be unnecessary, so what we really want is to leave numrecs at 1.
> 
> The numrecs handling itself is overly complicated because a different
> function sets numrecs == 1.  Change the bnobt creation code to start
> with numrecs set to zero and only increment it after successfully
> formatting a free space extent into the btree block.
> 
> Fixes: f327a00745ff ("xfs: account for log space when formatting new AGs")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_ag.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 1b078bbbf225..4481ce8ead9d 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -499,6 +499,7 @@ xfs_freesp_init_recs(
>  			 */
>  			arec->ar_blockcount = cpu_to_be32(start -
>  						mp->m_ag_prealloc_blocks);
> +			be16_add_cpu(&block->bb_numrecs, 1);
>  			nrec = arec + 1;
>  
>  			/*
> @@ -509,7 +510,6 @@ xfs_freesp_init_recs(
>  					be32_to_cpu(arec->ar_startblock) +
>  					be32_to_cpu(arec->ar_blockcount));
>  			arec = nrec;
> -			be16_add_cpu(&block->bb_numrecs, 1);
>  		}
>  		/*
>  		 * Change record start to after the internal log
> @@ -525,8 +525,8 @@ xfs_freesp_init_recs(
>  	 */
>  	arec->ar_blockcount = cpu_to_be32(id->agsize -
>  					  be32_to_cpu(arec->ar_startblock));
> -	if (!arec->ar_blockcount)
> -		block->bb_numrecs = 0;
> +	if (arec->ar_blockcount)
> +		be16_add_cpu(&block->bb_numrecs, 1);

Ok, but I think the comment above this about resetting the count
back to zero needs to be updated as we aren't resetting anything
back to zero anymore.

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

Re: [PATCH 3/4] xfs: flush dirty data and drain directios before scrubbing cow fork

[Dave Chinner]

On Thu, Apr 27, 2023 at 03:49:11PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When we're scrubbing the COW fork, we need to take MMAPLOCK_EXCL to
> prevent page_mkwrite from modifying any inode state.  The ILOCK should
> suffice to avoid confusing online fsck, but let's take the same locks
> that we do everywhere else.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Makes sense.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/4] xfs: don't allocate into the data fork for an unshare request

[Dave Chinner]

On Thu, Apr 27, 2023 at 03:49:16PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> For an unshare request, we only have to take action if the data fork has
> a shared mapping.  We don't care if someone else set up a cow operation.
> If we find nothing in the data fork, return a hole to avoid allocating
> space.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_iomap.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Looks ok, but I'm unsure of what bad behaviour this might be fixing.
Did you just notice this, or does it fix some kind of test failure?

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

Re: [PATCH 4/4] xfs: don't allocate into the data fork for an unshare request

[Darrick J. Wong]

On Fri, Apr 28, 2023 at 12:13:46PM +1000, Dave Chinner wrote:
> On Thu, Apr 27, 2023 at 03:49:16PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > For an unshare request, we only have to take action if the data fork has
> > a shared mapping.  We don't care if someone else set up a cow operation.
> > If we find nothing in the data fork, return a hole to avoid allocating
> > space.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_iomap.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Looks ok, but I'm unsure of what bad behaviour this might be fixing.
> Did you just notice this, or does it fix some kind of test failure?

I noticed it while I was running the freespace defrag code in djwong-dev
with tracepoints turned on.  THere was a math bug that I was trying to
sort out that resulted in FUNSHARE being called on a hole, and I was
surprised that it would create a delalloc reservation and then convert
it to an unwritten extent instead of going straight to an unwritten
extent.

AFAICT it has no user visible effect other than not wasting cycles on
pointless work.

--D

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

Re: [PATCH 4/4] xfs: don't allocate into the data fork for an unshare request

[Dave Chinner]

On Thu, Apr 27, 2023 at 07:20:55PM -0700, Darrick J. Wong wrote:
> On Fri, Apr 28, 2023 at 12:13:46PM +1000, Dave Chinner wrote:
> > On Thu, Apr 27, 2023 at 03:49:16PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > For an unshare request, we only have to take action if the data fork has
> > > a shared mapping.  We don't care if someone else set up a cow operation.
> > > If we find nothing in the data fork, return a hole to avoid allocating
> > > space.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/xfs_iomap.c |    5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > Looks ok, but I'm unsure of what bad behaviour this might be fixing.
> > Did you just notice this, or does it fix some kind of test failure?
> 
> I noticed it while I was running the freespace defrag code in djwong-dev
> with tracepoints turned on.  THere was a math bug that I was trying to
> sort out that resulted in FUNSHARE being called on a hole, and I was
> surprised that it would create a delalloc reservation and then convert
> it to an unwritten extent instead of going straight to an unwritten
> extent.
> 
> AFAICT it has no user visible effect other than not wasting cycles on
> pointless work.

Ok. If you add that to the commit message, then consider it

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4] xfs: set bnobt/cntbt numrecs correctly when formatting new AGs

[Darrick J. Wong]

On Fri, Apr 28, 2023 at 12:11:25PM +1000, Dave Chinner wrote:
> On Thu, Apr 27, 2023 at 03:49:05PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Through generic/300, I discovered that mkfs.xfs creates corrupt
> > filesystems when given these parameters:
> > 
> > # mkfs.xfs -d size=512M /dev/sda -f -d su=128k,sw=4 --unsupported
> > Filesystems formatted with --unsupported are not supported!!
> > meta-data=/dev/sda               isize=512    agcount=8, agsize=16352 blks
> >          =                       sectsz=512   attr=2, projid32bit=1
> >          =                       crc=1        finobt=1, sparse=1, rmapbt=1
> >          =                       reflink=1    bigtime=1 inobtcount=1 nrext64=1
> > data     =                       bsize=4096   blocks=130816, imaxpct=25
> >          =                       sunit=32     swidth=128 blks
> > naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
> > log      =internal log           bsize=4096   blocks=8192, version=2
> >          =                       sectsz=512   sunit=32 blks, lazy-count=1
> > realtime =none                   extsz=4096   blocks=0, rtextents=0
> >          =                       rgcount=0    rgsize=0 blks
> > Discarding blocks...Done.
> > # xfs_repair -n /dev/sda
> > Phase 1 - find and verify superblock...
> >         - reporting progress in intervals of 15 minutes
> > Phase 2 - using internal log
> >         - zero log...
> >         - 16:30:50: zeroing log - 16320 of 16320 blocks done
> >         - scan filesystem freespace and inode maps...
> > agf_freeblks 25, counted 0 in ag 4
> > sb_fdblocks 8823, counted 8798
> > 
> > The root cause of this problem is the numrecs handling in
> > xfs_freesp_init_recs, which is used to initialize a new AG.  Prior to
> > calling the function, we set up the new bnobt block with numrecs == 1
> > and rely on _freesp_init_recs to format that new record.  If the last
> > record created has a blockcount of zero, then it sets numrecs = 0.
> > 
> > That last bit isn't correct if the AG contains the log, the start of the
> > log is not immediately after the initial blocks due to stripe alignment,
> > and the end of the log is perfectly aligned with the end of the AG.  For
> > this case, we actually formatted a single bnobt record to handle the
> > free space before the start of the (stripe aligned) log, and incremented
> > arec to try to format a second record.  That second record turned out to
> > be unnecessary, so what we really want is to leave numrecs at 1.
> > 
> > The numrecs handling itself is overly complicated because a different
> > function sets numrecs == 1.  Change the bnobt creation code to start
> > with numrecs set to zero and only increment it after successfully
> > formatting a free space extent into the btree block.
> > 
> > Fixes: f327a00745ff ("xfs: account for log space when formatting new AGs")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_ag.c |   10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index 1b078bbbf225..4481ce8ead9d 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -499,6 +499,7 @@ xfs_freesp_init_recs(
> >  			 */
> >  			arec->ar_blockcount = cpu_to_be32(start -
> >  						mp->m_ag_prealloc_blocks);
> > +			be16_add_cpu(&block->bb_numrecs, 1);
> >  			nrec = arec + 1;
> >  
> >  			/*
> > @@ -509,7 +510,6 @@ xfs_freesp_init_recs(
> >  					be32_to_cpu(arec->ar_startblock) +
> >  					be32_to_cpu(arec->ar_blockcount));
> >  			arec = nrec;
> > -			be16_add_cpu(&block->bb_numrecs, 1);
> >  		}
> >  		/*
> >  		 * Change record start to after the internal log
> > @@ -525,8 +525,8 @@ xfs_freesp_init_recs(
> >  	 */
> >  	arec->ar_blockcount = cpu_to_be32(id->agsize -
> >  					  be32_to_cpu(arec->ar_startblock));
> > -	if (!arec->ar_blockcount)
> > -		block->bb_numrecs = 0;
> > +	if (arec->ar_blockcount)
> > +		be16_add_cpu(&block->bb_numrecs, 1);
> 
> Ok, but I think the comment above this about resetting the count
> back to zero needs to be updated as we aren't resetting anything
> back to zero anymore.

Done:

	/*
	 * Calculate the block count of this record; if it is nonzero,
	 * increment the record count.
	 */

--D

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

Re: [PATCH 1/4] xfs: don't unconditionally null args->pag in xfs_bmap_btalloc_at_eof

[Darrick J. Wong]

On Fri, Apr 28, 2023 at 11:49:28AM +1000, Dave Chinner wrote:
> On Thu, Apr 27, 2023 at 03:48:59PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > xfs/170 on a filesystem with su=128k,sw=4 produces this splat:
> > 
> > BUG: kernel NULL pointer dereference, address: 0000000000000010
> > #PF: supervisor write access in kernel mode
> > #PF: error_code(0x0002) - not-present page
> > PGD 0 P4D 0
> > Oops: 0002 [#1] PREEMPT SMP
> > CPU: 1 PID: 4022907 Comm: dd Tainted: G        W          6.3.0-xfsx #2 6ebeeffbe9577d32
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-bu
> > RIP: 0010:xfs_perag_rele+0x10/0x70 [xfs]
> > RSP: 0018:ffffc90001e43858 EFLAGS: 00010217
> > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000100
> > RDX: ffffffffa054e717 RSI: 0000000000000005 RDI: 0000000000000000
> > RBP: ffff888194eea000 R08: 0000000000000000 R09: 0000000000000037
> > R10: ffff888100ac1cb0 R11: 0000000000000018 R12: 0000000000000000
> > R13: ffffc90001e43a38 R14: ffff888194eea000 R15: ffff888194eea000
> > FS:  00007f93d1a0e740(0000) GS:ffff88843fc80000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000000000010 CR3: 000000018a34f000 CR4: 00000000003506e0
> > Call Trace:
> >  <TASK>
> >  xfs_bmap_btalloc+0x1a7/0x5d0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
> >  xfs_bmapi_allocate+0xee/0x470 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
> >  xfs_bmapi_write+0x539/0x9e0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
> >  xfs_iomap_write_direct+0x1bb/0x2b0 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
> >  xfs_direct_write_iomap_begin+0x51c/0x710 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
> >  iomap_iter+0x132/0x2f0
> >  __iomap_dio_rw+0x2f8/0x840
> >  iomap_dio_rw+0xe/0x30
> >  xfs_file_dio_write_aligned+0xad/0x180 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
> >  xfs_file_write_iter+0xfb/0x190 [xfs f85291d6841cbb3dc740083f1f331c0327394518]
> >  vfs_write+0x2eb/0x410
> >  ksys_write+0x65/0xe0
> >  do_syscall_64+0x2b/0x80
> > 
> > This crash occurs under the "out_low_space" label.  We grabbed a perag
> > reference, passed it via args->pag into xfs_bmap_btalloc_at_eof, and
> > afterwards args->pag is NULL.  Fix the second function not to clobber
> > args->pag if the caller had passed one in.
> > 
> > Fixes: 85843327094f ("xfs: factor xfs_bmap_btalloc()")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index b512de0540d5..cd8870a16fd1 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3494,8 +3494,10 @@ xfs_bmap_btalloc_at_eof(
> >  		if (!caller_pag)
> >  			args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno));
> >  		error = xfs_alloc_vextent_exact_bno(args, ap->blkno);
> > -		if (!caller_pag)
> > +		if (!caller_pag) {
> >  			xfs_perag_put(args->pag);
> > +			args->pag = NULL;
> > +		}
> >  		if (error)
> >  			return error;
> >  
> > @@ -3505,7 +3507,6 @@ xfs_bmap_btalloc_at_eof(
> >  		 * Exact allocation failed. Reset to try an aligned allocation
> >  		 * according to the original allocation specification.
> >  		 */
> > -		args->pag = NULL;
> >  		args->alignment = stripe_align;
> >  		args->minlen = nextminlen;
> >  		args->minalignslop = 0;
> 
> Yup, that'll fix the problem.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> FWIW, I'm working on some patches to take this further by always
> providing a caller perag to this function.  That gets rid of all the
> conditional code here and it gets rid of the only case where we call
> xfs_alloc_vextent_near_bno() without args->pag set so the
> conditional caller_pag code goes from there, too. This makes
> xfs_alloc_vextent_{near,exact}_bno() identical except for one line
> so they can be collapsed. And now that we always specify args->pag
> for these functions, we can ignore the agno part of the target block
> meaning we can select the perag to allocate from at a much higher
> level via setting up args->pag when we initially scan the AGs to
> set up args->minlen/maxlen based on the the nearest AG with 
> contiguous free space large enough for the whole allocation. This
> avoids attempting allocations that are guaranteed to fail before
> we fall back to iterating from the target block agno and eventually
> finding the same AG we originally found that had enough contiguous
> free space in it for a maxlen allocation....
> 
> I think I can take it further an make both filestreams and the
> normal allocator use the same "this ag" algorithm for EOF and
> aligned allocation and then have them both fall back to the same
> unaligned allocation attempts, which will then allow a bunch of code
> to be collapsed in the xfs_bmap_btalloc() path...

<nod> I was thinking along those lines too when I was writing this
patch, but for now I'll just patch everything back together. :)

That /would/ make the interfaces to those functions a lot more
consistent.

--D

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