[PATCH 0/4] xfs: random fixes

[PATCH 0/4] xfs: random fixes

[Darrick J. Wong]

Hi all,

Here's a few more random fixes for 5.7.  The first patch solves some
conflicts between users of empty transactions and fs freeze that were
causing long delays and ASSERT failures during freeze.

The second patch adds some missing realtime device geometry checks to
the superblock verifier.

The last two patches teach the directory and xattr scrubbers to release
buffers sooner than later, which reduces memory pressure on clients
with large directories and not a lot of RAM.

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=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes

[PATCH 1/4] xfs: prohibit fs freezing when using empty transactions

[Darrick J. Wong]

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

I noticed that fsfreeze can take a very long time to freeze an XFS if
there happens to be a GETFSMAP caller running in the background.  I also
happened to notice the following in dmesg:

------------[ cut here ]------------
WARNING: CPU: 2 PID: 43492 at fs/xfs/xfs_super.c:853 xfs_quiesce_attr+0x83/0x90 [xfs]
Modules linked in: xfs libcrc32c ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 ip_set_hash_ip ip_set_hash_net xt_tcpudp xt_set ip_set_hash_mac ip_set nfnetlink ip6table_filter ip6_tables bfq iptable_filter sch_fq_codel ip_tables x_tables nfsv4 af_packet [last unloaded: xfs]
CPU: 2 PID: 43492 Comm: xfs_io Not tainted 5.6.0-rc4-djw #rc4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
RIP: 0010:xfs_quiesce_attr+0x83/0x90 [xfs]
Code: 7c 07 00 00 85 c0 75 22 48 89 df 5b e9 96 c1 00 00 48 c7 c6 b0 2d 38 a0 48 89 df e8 57 64 ff ff 8b 83 7c 07 00 00 85 c0 74 de <0f> 0b 48 89 df 5b e9 72 c1 00 00 66 90 0f 1f 44 00 00 41 55 41 54
RSP: 0018:ffffc900030f3e28 EFLAGS: 00010202
RAX: 0000000000000001 RBX: ffff88802ac54000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81e4a6f0 RDI: 00000000ffffffff
RBP: ffff88807859f070 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000010 R12: 0000000000000000
R13: ffff88807859f388 R14: ffff88807859f4b8 R15: ffff88807859f5e8
FS:  00007fad1c6c0fc0(0000) GS:ffff88807e000000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f0c7d237000 CR3: 0000000077f01003 CR4: 00000000001606a0
Call Trace:
 xfs_fs_freeze+0x25/0x40 [xfs]
 freeze_super+0xc8/0x180
 do_vfs_ioctl+0x70b/0x750
 ? __fget_files+0x135/0x210
 ksys_ioctl+0x3a/0xb0
 __x64_sys_ioctl+0x16/0x20
 do_syscall_64+0x50/0x1a0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

These two things appear to be related.  The assertion trips when another
thread initiates a fsmap request (which uses an empty transaction) after
the freezer waited for m_active_trans to hit zero but before the the
freezer executes the WARN_ON just prior to calling xfs_log_quiesce.

The lengthy delays in freezing happen because the freezer calls
xfs_wait_buftarg to clean out the buffer lru list.  Meanwhile, the
GETFSMAP caller is continuing to grab and release buffers, which means
that it can take a very long time for the buffer lru list to empty out.

We fix both of these races by calling sb_start_write to obtain freeze
protection while using empty transactions for GETFSMAP and for metadata
scrubbing.  The other two users occur during mount, during which time we
cannot fs freeze.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/scrub.c |    4 ++++
 fs/xfs/xfs_fsmap.c   |    4 ++++
 fs/xfs/xfs_trans.c   |    5 +++++
 3 files changed, 13 insertions(+)


diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index f1775bb19313..a42bb66b335d 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -168,6 +168,7 @@ xchk_teardown(
 			xfs_irele(sc->ip);
 		sc->ip = NULL;
 	}
+	sb_end_write(sc->mp->m_super);
 	if (sc->flags & XCHK_REAPING_DISABLED)
 		xchk_start_reaping(sc);
 	if (sc->flags & XCHK_HAS_QUOTAOFFLOCK) {
@@ -490,6 +491,9 @@ xfs_scrub_metadata(
 	sc.ops = &meta_scrub_ops[sm->sm_type];
 	sc.sick_mask = xchk_health_mask_for_scrub_type(sm->sm_type);
 retry_op:
+	/* Avoid conflicts with fs freeze. */
+	sb_start_write(mp->m_super);
+
 	/* Set up for the operation. */
 	error = sc.ops->setup(&sc, ip);
 	if (error)
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 918456ca29e1..2bb2cd1e63cf 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -896,6 +896,9 @@ xfs_getfsmap(
 	info.format_arg = arg;
 	info.head = head;
 
+	/* Avoid conflicts with fs freeze. */
+	sb_start_write(mp->m_super);
+
 	/* For each device we support... */
 	for (i = 0; i < XFS_GETFSMAP_DEVS; i++) {
 		/* Is this device within the range the user asked for? */
@@ -935,6 +938,7 @@ xfs_getfsmap(
 
 	if (tp)
 		xfs_trans_cancel(tp);
+	sb_end_write(mp->m_super);
 	head->fmh_oflags = FMH_OF_DEV_T;
 	return error;
 }
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 3b208f9a865c..a65dc227e40d 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -306,6 +306,11 @@ xfs_trans_alloc(
  *
  * Note the zero-length reservation; this transaction MUST be cancelled
  * without any dirty data.
+ *
+ * Callers should obtain freeze protection to avoid two conflicts with fs
+ * freezing: (1) having active transactions trip the m_active_trans ASSERTs;
+ * and (2) grabbing buffers at the same time that freeze is trying to drain
+ * the buffer LRU list.
  */
 int
 xfs_trans_alloc_empty(

Re: [PATCH 1/4] xfs: prohibit fs freezing when using empty transactions

[Dave Chinner]

On Tue, Mar 24, 2020 at 08:24:36PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I noticed that fsfreeze can take a very long time to freeze an XFS if
> there happens to be a GETFSMAP caller running in the background.  I also
> happened to notice the following in dmesg:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 43492 at fs/xfs/xfs_super.c:853 xfs_quiesce_attr+0x83/0x90 [xfs]
> Modules linked in: xfs libcrc32c ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 ip_set_hash_ip ip_set_hash_net xt_tcpudp xt_set ip_set_hash_mac ip_set nfnetlink ip6table_filter ip6_tables bfq iptable_filter sch_fq_codel ip_tables x_tables nfsv4 af_packet [last unloaded: xfs]
> CPU: 2 PID: 43492 Comm: xfs_io Not tainted 5.6.0-rc4-djw #rc4
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
> RIP: 0010:xfs_quiesce_attr+0x83/0x90 [xfs]
> Code: 7c 07 00 00 85 c0 75 22 48 89 df 5b e9 96 c1 00 00 48 c7 c6 b0 2d 38 a0 48 89 df e8 57 64 ff ff 8b 83 7c 07 00 00 85 c0 74 de <0f> 0b 48 89 df 5b e9 72 c1 00 00 66 90 0f 1f 44 00 00 41 55 41 54
> RSP: 0018:ffffc900030f3e28 EFLAGS: 00010202
> RAX: 0000000000000001 RBX: ffff88802ac54000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffffffff81e4a6f0 RDI: 00000000ffffffff
> RBP: ffff88807859f070 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000010 R12: 0000000000000000
> R13: ffff88807859f388 R14: ffff88807859f4b8 R15: ffff88807859f5e8
> FS:  00007fad1c6c0fc0(0000) GS:ffff88807e000000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f0c7d237000 CR3: 0000000077f01003 CR4: 00000000001606a0
> Call Trace:
>  xfs_fs_freeze+0x25/0x40 [xfs]
>  freeze_super+0xc8/0x180
>  do_vfs_ioctl+0x70b/0x750
>  ? __fget_files+0x135/0x210
>  ksys_ioctl+0x3a/0xb0
>  __x64_sys_ioctl+0x16/0x20
>  do_syscall_64+0x50/0x1a0
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> These two things appear to be related.  The assertion trips when another
> thread initiates a fsmap request (which uses an empty transaction) after
> the freezer waited for m_active_trans to hit zero but before the the
> freezer executes the WARN_ON just prior to calling xfs_log_quiesce.
> 
> The lengthy delays in freezing happen because the freezer calls
> xfs_wait_buftarg to clean out the buffer lru list.  Meanwhile, the
> GETFSMAP caller is continuing to grab and release buffers, which means
> that it can take a very long time for the buffer lru list to empty out.
> 
> We fix both of these races by calling sb_start_write to obtain freeze
> protection while using empty transactions for GETFSMAP and for metadata
> scrubbing.  The other two users occur during mount, during which time we
> cannot fs freeze.

Makes sense. Minor nits:

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/scrub.c |    4 ++++
>  fs/xfs/xfs_fsmap.c   |    4 ++++
>  fs/xfs/xfs_trans.c   |    5 +++++
>  3 files changed, 13 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index f1775bb19313..a42bb66b335d 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -168,6 +168,7 @@ xchk_teardown(
>  			xfs_irele(sc->ip);
>  		sc->ip = NULL;
>  	}
> +	sb_end_write(sc->mp->m_super);
>  	if (sc->flags & XCHK_REAPING_DISABLED)
>  		xchk_start_reaping(sc);
>  	if (sc->flags & XCHK_HAS_QUOTAOFFLOCK) {
> @@ -490,6 +491,9 @@ xfs_scrub_metadata(
>  	sc.ops = &meta_scrub_ops[sm->sm_type];
>  	sc.sick_mask = xchk_health_mask_for_scrub_type(sm->sm_type);
>  retry_op:
> +	/* Avoid conflicts with fs freeze. */
> +	sb_start_write(mp->m_super);
> +

Rather than saying something realtively meaningless like "avoid
conflicts with freeze", wouldn't it be better to say something like:

	/*
	 * If freeze runs concurrently with a scrub, the freeze can
	 * be delayed indefinitely as we walk the filesystem and
	 * iterate over metadata buffers. Freeze quiesces the log
	 * which waits for the buffer LRU to be emptied and that
	 * won't happen while checking runs.
	 */

>  	/* Set up for the operation. */
>  	error = sc.ops->setup(&sc, ip);
>  	if (error)
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 918456ca29e1..2bb2cd1e63cf 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -896,6 +896,9 @@ xfs_getfsmap(
>  	info.format_arg = arg;
>  	info.head = head;
>  
> +	/* Avoid conflicts with fs freeze. */
> +	sb_start_write(mp->m_super);
> +

And a similar comment here?

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

Re: [PATCH 1/4] xfs: prohibit fs freezing when using empty transactions

[Darrick J. Wong]

On Wed, Mar 25, 2020 at 03:53:53PM +1100, Dave Chinner wrote:
> On Tue, Mar 24, 2020 at 08:24:36PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > I noticed that fsfreeze can take a very long time to freeze an XFS if
> > there happens to be a GETFSMAP caller running in the background.  I also
> > happened to notice the following in dmesg:
> > 
> > ------------[ cut here ]------------
> > WARNING: CPU: 2 PID: 43492 at fs/xfs/xfs_super.c:853 xfs_quiesce_attr+0x83/0x90 [xfs]
> > Modules linked in: xfs libcrc32c ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 ip_set_hash_ip ip_set_hash_net xt_tcpudp xt_set ip_set_hash_mac ip_set nfnetlink ip6table_filter ip6_tables bfq iptable_filter sch_fq_codel ip_tables x_tables nfsv4 af_packet [last unloaded: xfs]
> > CPU: 2 PID: 43492 Comm: xfs_io Not tainted 5.6.0-rc4-djw #rc4
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
> > RIP: 0010:xfs_quiesce_attr+0x83/0x90 [xfs]
> > Code: 7c 07 00 00 85 c0 75 22 48 89 df 5b e9 96 c1 00 00 48 c7 c6 b0 2d 38 a0 48 89 df e8 57 64 ff ff 8b 83 7c 07 00 00 85 c0 74 de <0f> 0b 48 89 df 5b e9 72 c1 00 00 66 90 0f 1f 44 00 00 41 55 41 54
> > RSP: 0018:ffffc900030f3e28 EFLAGS: 00010202
> > RAX: 0000000000000001 RBX: ffff88802ac54000 RCX: 0000000000000000
> > RDX: 0000000000000000 RSI: ffffffff81e4a6f0 RDI: 00000000ffffffff
> > RBP: ffff88807859f070 R08: 0000000000000001 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000010 R12: 0000000000000000
> > R13: ffff88807859f388 R14: ffff88807859f4b8 R15: ffff88807859f5e8
> > FS:  00007fad1c6c0fc0(0000) GS:ffff88807e000000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007f0c7d237000 CR3: 0000000077f01003 CR4: 00000000001606a0
> > Call Trace:
> >  xfs_fs_freeze+0x25/0x40 [xfs]
> >  freeze_super+0xc8/0x180
> >  do_vfs_ioctl+0x70b/0x750
> >  ? __fget_files+0x135/0x210
> >  ksys_ioctl+0x3a/0xb0
> >  __x64_sys_ioctl+0x16/0x20
> >  do_syscall_64+0x50/0x1a0
> >  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > 
> > These two things appear to be related.  The assertion trips when another
> > thread initiates a fsmap request (which uses an empty transaction) after
> > the freezer waited for m_active_trans to hit zero but before the the
> > freezer executes the WARN_ON just prior to calling xfs_log_quiesce.
> > 
> > The lengthy delays in freezing happen because the freezer calls
> > xfs_wait_buftarg to clean out the buffer lru list.  Meanwhile, the
> > GETFSMAP caller is continuing to grab and release buffers, which means
> > that it can take a very long time for the buffer lru list to empty out.
> > 
> > We fix both of these races by calling sb_start_write to obtain freeze
> > protection while using empty transactions for GETFSMAP and for metadata
> > scrubbing.  The other two users occur during mount, during which time we
> > cannot fs freeze.
> 
> Makes sense. Minor nits:
> 
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/scrub.c |    4 ++++
> >  fs/xfs/xfs_fsmap.c   |    4 ++++
> >  fs/xfs/xfs_trans.c   |    5 +++++
> >  3 files changed, 13 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> > index f1775bb19313..a42bb66b335d 100644
> > --- a/fs/xfs/scrub/scrub.c
> > +++ b/fs/xfs/scrub/scrub.c
> > @@ -168,6 +168,7 @@ xchk_teardown(
> >  			xfs_irele(sc->ip);
> >  		sc->ip = NULL;
> >  	}
> > +	sb_end_write(sc->mp->m_super);
> >  	if (sc->flags & XCHK_REAPING_DISABLED)
> >  		xchk_start_reaping(sc);
> >  	if (sc->flags & XCHK_HAS_QUOTAOFFLOCK) {
> > @@ -490,6 +491,9 @@ xfs_scrub_metadata(
> >  	sc.ops = &meta_scrub_ops[sm->sm_type];
> >  	sc.sick_mask = xchk_health_mask_for_scrub_type(sm->sm_type);
> >  retry_op:
> > +	/* Avoid conflicts with fs freeze. */
> > +	sb_start_write(mp->m_super);
> > +
> 
> Rather than saying something realtively meaningless like "avoid
> conflicts with freeze", wouldn't it be better to say something like:
> 
> 	/*
> 	 * If freeze runs concurrently with a scrub, the freeze can
> 	 * be delayed indefinitely as we walk the filesystem and
> 	 * iterate over metadata buffers. Freeze quiesces the log
> 	 * which waits for the buffer LRU to be emptied and that
> 	 * won't happen while checking runs.
> 	 */
> 
> >  	/* Set up for the operation. */
> >  	error = sc.ops->setup(&sc, ip);
> >  	if (error)
> > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > index 918456ca29e1..2bb2cd1e63cf 100644
> > --- a/fs/xfs/xfs_fsmap.c
> > +++ b/fs/xfs/xfs_fsmap.c
> > @@ -896,6 +896,9 @@ xfs_getfsmap(
> >  	info.format_arg = arg;
> >  	info.head = head;
> >  
> > +	/* Avoid conflicts with fs freeze. */
> > +	sb_start_write(mp->m_super);
> > +
> 
> And a similar comment here?

Ok.  Will fix & resend.

--D

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

[PATCH v2 1/4] xfs: prohibit fs freezing when using empty transactions

[Darrick J. Wong]

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

I noticed that fsfreeze can take a very long time to freeze an XFS if
there happens to be a GETFSMAP caller running in the background.  I also
happened to notice the following in dmesg:

------------[ cut here ]------------
WARNING: CPU: 2 PID: 43492 at fs/xfs/xfs_super.c:853 xfs_quiesce_attr+0x83/0x90 [xfs]
Modules linked in: xfs libcrc32c ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 ip_set_hash_ip ip_set_hash_net xt_tcpudp xt_set ip_set_hash_mac ip_set nfnetlink ip6table_filter ip6_tables bfq iptable_filter sch_fq_codel ip_tables x_tables nfsv4 af_packet [last unloaded: xfs]
CPU: 2 PID: 43492 Comm: xfs_io Not tainted 5.6.0-rc4-djw #rc4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
RIP: 0010:xfs_quiesce_attr+0x83/0x90 [xfs]
Code: 7c 07 00 00 85 c0 75 22 48 89 df 5b e9 96 c1 00 00 48 c7 c6 b0 2d 38 a0 48 89 df e8 57 64 ff ff 8b 83 7c 07 00 00 85 c0 74 de <0f> 0b 48 89 df 5b e9 72 c1 00 00 66 90 0f 1f 44 00 00 41 55 41 54
RSP: 0018:ffffc900030f3e28 EFLAGS: 00010202
RAX: 0000000000000001 RBX: ffff88802ac54000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81e4a6f0 RDI: 00000000ffffffff
RBP: ffff88807859f070 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000010 R12: 0000000000000000
R13: ffff88807859f388 R14: ffff88807859f4b8 R15: ffff88807859f5e8
FS:  00007fad1c6c0fc0(0000) GS:ffff88807e000000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f0c7d237000 CR3: 0000000077f01003 CR4: 00000000001606a0
Call Trace:
 xfs_fs_freeze+0x25/0x40 [xfs]
 freeze_super+0xc8/0x180
 do_vfs_ioctl+0x70b/0x750
 ? __fget_files+0x135/0x210
 ksys_ioctl+0x3a/0xb0
 __x64_sys_ioctl+0x16/0x20
 do_syscall_64+0x50/0x1a0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

These two things appear to be related.  The assertion trips when another
thread initiates a fsmap request (which uses an empty transaction) after
the freezer waited for m_active_trans to hit zero but before the the
freezer executes the WARN_ON just prior to calling xfs_log_quiesce.

The lengthy delays in freezing happen because the freezer calls
xfs_wait_buftarg to clean out the buffer lru list.  Meanwhile, the
GETFSMAP caller is continuing to grab and release buffers, which means
that it can take a very long time for the buffer lru list to empty out.

We fix both of these races by calling sb_start_write to obtain freeze
protection while using empty transactions for GETFSMAP and for metadata
scrubbing.  The other two users occur during mount, during which time we
cannot fs freeze.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: improve comments
---
 fs/xfs/scrub/scrub.c |    9 +++++++++
 fs/xfs/xfs_fsmap.c   |    9 +++++++++
 fs/xfs/xfs_trans.c   |    5 +++++
 3 files changed, 23 insertions(+)

diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index f1775bb19313..8ebf35b115ce 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -168,6 +168,7 @@ xchk_teardown(
 			xfs_irele(sc->ip);
 		sc->ip = NULL;
 	}
+	sb_end_write(sc->mp->m_super);
 	if (sc->flags & XCHK_REAPING_DISABLED)
 		xchk_start_reaping(sc);
 	if (sc->flags & XCHK_HAS_QUOTAOFFLOCK) {
@@ -490,6 +491,14 @@ xfs_scrub_metadata(
 	sc.ops = &meta_scrub_ops[sm->sm_type];
 	sc.sick_mask = xchk_health_mask_for_scrub_type(sm->sm_type);
 retry_op:
+	/*
+	 * If freeze runs concurrently with a scrub, the freeze can be delayed
+	 * indefinitely as we walk the filesystem and iterate over metadata
+	 * buffers.  Freeze quiesces the log (which waits for the buffer LRU to
+	 * be emptied) and that won't happen while checking is running.
+	 */
+	sb_start_write(mp->m_super);
+
 	/* Set up for the operation. */
 	error = sc.ops->setup(&sc, ip);
 	if (error)
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 918456ca29e1..442fd4311f18 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -896,6 +896,14 @@ xfs_getfsmap(
 	info.format_arg = arg;
 	info.head = head;
 
+	/*
+	 * If fsmap runs concurrently with a scrub, the freeze can be delayed
+	 * indefinitely as we walk the rmapbt and iterate over metadata
+	 * buffers.  Freeze quiesces the log (which waits for the buffer LRU to
+	 * be emptied) and that won't happen while we're reading buffers.
+	 */
+	sb_start_write(mp->m_super);
+
 	/* For each device we support... */
 	for (i = 0; i < XFS_GETFSMAP_DEVS; i++) {
 		/* Is this device within the range the user asked for? */
@@ -935,6 +943,7 @@ xfs_getfsmap(
 
 	if (tp)
 		xfs_trans_cancel(tp);
+	sb_end_write(mp->m_super);
 	head->fmh_oflags = FMH_OF_DEV_T;
 	return error;
 }
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 3b208f9a865c..a65dc227e40d 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -306,6 +306,11 @@ xfs_trans_alloc(
  *
  * Note the zero-length reservation; this transaction MUST be cancelled
  * without any dirty data.
+ *
+ * Callers should obtain freeze protection to avoid two conflicts with fs
+ * freezing: (1) having active transactions trip the m_active_trans ASSERTs;
+ * and (2) grabbing buffers at the same time that freeze is trying to drain
+ * the buffer LRU list.
  */
 int
 xfs_trans_alloc_empty(

Re: [PATCH v2 1/4] xfs: prohibit fs freezing when using empty transactions

[Dave Chinner]

On Tue, Mar 24, 2020 at 11:06:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I noticed that fsfreeze can take a very long time to freeze an XFS if
> there happens to be a GETFSMAP caller running in the background.  I also
> happened to notice the following in dmesg:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 43492 at fs/xfs/xfs_super.c:853 xfs_quiesce_attr+0x83/0x90 [xfs]
> Modules linked in: xfs libcrc32c ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 ip_set_hash_ip ip_set_hash_net xt_tcpudp xt_set ip_set_hash_mac ip_set nfnetlink ip6table_filter ip6_tables bfq iptable_filter sch_fq_codel ip_tables x_tables nfsv4 af_packet [last unloaded: xfs]
> CPU: 2 PID: 43492 Comm: xfs_io Not tainted 5.6.0-rc4-djw #rc4
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
> RIP: 0010:xfs_quiesce_attr+0x83/0x90 [xfs]
> Code: 7c 07 00 00 85 c0 75 22 48 89 df 5b e9 96 c1 00 00 48 c7 c6 b0 2d 38 a0 48 89 df e8 57 64 ff ff 8b 83 7c 07 00 00 85 c0 74 de <0f> 0b 48 89 df 5b e9 72 c1 00 00 66 90 0f 1f 44 00 00 41 55 41 54
> RSP: 0018:ffffc900030f3e28 EFLAGS: 00010202
> RAX: 0000000000000001 RBX: ffff88802ac54000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffffffff81e4a6f0 RDI: 00000000ffffffff
> RBP: ffff88807859f070 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000010 R12: 0000000000000000
> R13: ffff88807859f388 R14: ffff88807859f4b8 R15: ffff88807859f5e8
> FS:  00007fad1c6c0fc0(0000) GS:ffff88807e000000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f0c7d237000 CR3: 0000000077f01003 CR4: 00000000001606a0
> Call Trace:
>  xfs_fs_freeze+0x25/0x40 [xfs]
>  freeze_super+0xc8/0x180
>  do_vfs_ioctl+0x70b/0x750
>  ? __fget_files+0x135/0x210
>  ksys_ioctl+0x3a/0xb0
>  __x64_sys_ioctl+0x16/0x20
>  do_syscall_64+0x50/0x1a0
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> These two things appear to be related.  The assertion trips when another
> thread initiates a fsmap request (which uses an empty transaction) after
> the freezer waited for m_active_trans to hit zero but before the the
> freezer executes the WARN_ON just prior to calling xfs_log_quiesce.
> 
> The lengthy delays in freezing happen because the freezer calls
> xfs_wait_buftarg to clean out the buffer lru list.  Meanwhile, the
> GETFSMAP caller is continuing to grab and release buffers, which means
> that it can take a very long time for the buffer lru list to empty out.
> 
> We fix both of these races by calling sb_start_write to obtain freeze
> protection while using empty transactions for GETFSMAP and for metadata
> scrubbing.  The other two users occur during mount, during which time we
> cannot fs freeze.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: improve comments

Looks good now.

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

Re: [PATCH 1/4] xfs: prohibit fs freezing when using empty transactions

[Chandan Rajendra]

On Wednesday, March 25, 2020 8:54 AM Darrick J. Wong wrote: 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I noticed that fsfreeze can take a very long time to freeze an XFS if
> there happens to be a GETFSMAP caller running in the background.  I also
> happened to notice the following in dmesg:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 43492 at fs/xfs/xfs_super.c:853 xfs_quiesce_attr+0x83/0x90 [xfs]
> Modules linked in: xfs libcrc32c ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 ip_set_hash_ip ip_set_hash_net xt_tcpudp xt_set ip_set_hash_mac ip_set nfnetlink ip6table_filter ip6_tables bfq iptable_filter sch_fq_codel ip_tables x_tables nfsv4 af_packet [last unloaded: xfs]
> CPU: 2 PID: 43492 Comm: xfs_io Not tainted 5.6.0-rc4-djw #rc4
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
> RIP: 0010:xfs_quiesce_attr+0x83/0x90 [xfs]
> Code: 7c 07 00 00 85 c0 75 22 48 89 df 5b e9 96 c1 00 00 48 c7 c6 b0 2d 38 a0 48 89 df e8 57 64 ff ff 8b 83 7c 07 00 00 85 c0 74 de <0f> 0b 48 89 df 5b e9 72 c1 00 00 66 90 0f 1f 44 00 00 41 55 41 54
> RSP: 0018:ffffc900030f3e28 EFLAGS: 00010202
> RAX: 0000000000000001 RBX: ffff88802ac54000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffffffff81e4a6f0 RDI: 00000000ffffffff
> RBP: ffff88807859f070 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000010 R12: 0000000000000000
> R13: ffff88807859f388 R14: ffff88807859f4b8 R15: ffff88807859f5e8
> FS:  00007fad1c6c0fc0(0000) GS:ffff88807e000000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f0c7d237000 CR3: 0000000077f01003 CR4: 00000000001606a0
> Call Trace:
>  xfs_fs_freeze+0x25/0x40 [xfs]
>  freeze_super+0xc8/0x180
>  do_vfs_ioctl+0x70b/0x750
>  ? __fget_files+0x135/0x210
>  ksys_ioctl+0x3a/0xb0
>  __x64_sys_ioctl+0x16/0x20
>  do_syscall_64+0x50/0x1a0
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> These two things appear to be related.  The assertion trips when another
> thread initiates a fsmap request (which uses an empty transaction) after
> the freezer waited for m_active_trans to hit zero but before the the
> freezer executes the WARN_ON just prior to calling xfs_log_quiesce.
> 
> The lengthy delays in freezing happen because the freezer calls
> xfs_wait_buftarg to clean out the buffer lru list.  Meanwhile, the
> GETFSMAP caller is continuing to grab and release buffers, which means
> that it can take a very long time for the buffer lru list to empty out.
> 
> We fix both of these races by calling sb_start_write to obtain freeze
> protection while using empty transactions for GETFSMAP and for metadata
> scrubbing.  The other two users occur during mount, during which time we
> cannot fs freeze.
>

sb_[start|end]_write() calls do indeed exclude the fsfreezer task from
executing when fsmap ioctl call is being serviced.  I do not understand the
code associated with xfs scrub. Apart from that, the changes look good to me.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/scrub.c |    4 ++++
>  fs/xfs/xfs_fsmap.c   |    4 ++++
>  fs/xfs/xfs_trans.c   |    5 +++++
>  3 files changed, 13 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index f1775bb19313..a42bb66b335d 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -168,6 +168,7 @@ xchk_teardown(
>  			xfs_irele(sc->ip);
>  		sc->ip = NULL;
>  	}
> +	sb_end_write(sc->mp->m_super);
>  	if (sc->flags & XCHK_REAPING_DISABLED)
>  		xchk_start_reaping(sc);
>  	if (sc->flags & XCHK_HAS_QUOTAOFFLOCK) {
> @@ -490,6 +491,9 @@ xfs_scrub_metadata(
>  	sc.ops = &meta_scrub_ops[sm->sm_type];
>  	sc.sick_mask = xchk_health_mask_for_scrub_type(sm->sm_type);
>  retry_op:
> +	/* Avoid conflicts with fs freeze. */
> +	sb_start_write(mp->m_super);
> +
>  	/* Set up for the operation. */
>  	error = sc.ops->setup(&sc, ip);
>  	if (error)
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 918456ca29e1..2bb2cd1e63cf 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -896,6 +896,9 @@ xfs_getfsmap(
>  	info.format_arg = arg;
>  	info.head = head;
>  
> +	/* Avoid conflicts with fs freeze. */
> +	sb_start_write(mp->m_super);
> +
>  	/* For each device we support... */
>  	for (i = 0; i < XFS_GETFSMAP_DEVS; i++) {
>  		/* Is this device within the range the user asked for? */
> @@ -935,6 +938,7 @@ xfs_getfsmap(
>  
>  	if (tp)
>  		xfs_trans_cancel(tp);
> +	sb_end_write(mp->m_super);
>  	head->fmh_oflags = FMH_OF_DEV_T;
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 3b208f9a865c..a65dc227e40d 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -306,6 +306,11 @@ xfs_trans_alloc(
>   *
>   * Note the zero-length reservation; this transaction MUST be cancelled
>   * without any dirty data.
> + *
> + * Callers should obtain freeze protection to avoid two conflicts with fs
> + * freezing: (1) having active transactions trip the m_active_trans ASSERTs;
> + * and (2) grabbing buffers at the same time that freeze is trying to drain
> + * the buffer LRU list.
>   */
>  int
>  xfs_trans_alloc_empty(
> 
> 


-- 
chandan

[PATCH 2/4] xfs: validate the realtime geometry in xfs_validate_sb_common

[Darrick J. Wong]

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

Validate the geometry of the realtime geometry when we mount the
filesystem, so that we don't abruptly shut down the filesystem later on.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_sb.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 2f60fc3c99a0..dee0a1a594dc 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -328,6 +328,41 @@ xfs_validate_sb_common(
 		return -EFSCORRUPTED;
 	}
 
+	/* Validate the realtime geometry; stolen from xfs_repair */
+	if (unlikely(
+	    sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE	||
+	    sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)) {
+		xfs_notice(mp,
+			"realtime extent sanity check failed");
+		return -EFSCORRUPTED;
+	}
+
+	if (sbp->sb_rblocks == 0) {
+		if (unlikely(
+		    sbp->sb_rextents != 0				||
+		    sbp->sb_rbmblocks != 0				||
+		    sbp->sb_rextslog != 0				||
+		    sbp->sb_frextents != 0)) {
+			xfs_notice(mp,
+				"realtime zeroed geometry sanity check failed");
+			return -EFSCORRUPTED;
+		}
+	} else {
+		xfs_rtblock_t	rexts;
+		uint32_t	temp;
+
+		rexts = div_u64_rem(sbp->sb_rblocks, sbp->sb_rextsize, &temp);
+		if (unlikely(
+		    rexts != sbp->sb_rextents				||
+		    sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents)	||
+		    sbp->sb_rbmblocks != howmany(sbp->sb_rextents,
+						NBBY * sbp->sb_blocksize))) {
+			xfs_notice(mp,
+				"realtime geometry sanity check failed");
+			return -EFSCORRUPTED;
+		}
+	}
+
 	if (sbp->sb_unit) {
 		if (!xfs_sb_version_hasdalign(sbp) ||
 		    sbp->sb_unit > sbp->sb_width ||

Re: [PATCH 2/4] xfs: validate the realtime geometry in xfs_validate_sb_common

[Dave Chinner]

On Tue, Mar 24, 2020 at 08:24:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Validate the geometry of the realtime geometry when we mount the
> filesystem, so that we don't abruptly shut down the filesystem later on.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 2f60fc3c99a0..dee0a1a594dc 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -328,6 +328,41 @@ xfs_validate_sb_common(
>  		return -EFSCORRUPTED;
>  	}
>  
> +	/* Validate the realtime geometry; stolen from xfs_repair */
> +	if (unlikely(
> +	    sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE	||

Whacky whitespace before the ||

> +	    sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)) {
> +		xfs_notice(mp,
> +			"realtime extent sanity check failed");
> +		return -EFSCORRUPTED;
> +	}

We really don't need unlikely() in code like this. the compiler
already considers code that returns inside an if statement as
"unlikely" because it's the typical error handling pattern, for
cases like this it really isn't necessary.


> +
> +	if (sbp->sb_rblocks == 0) {
> +		if (unlikely(
> +		    sbp->sb_rextents != 0				||
> +		    sbp->sb_rbmblocks != 0				||
> +		    sbp->sb_rextslog != 0				||
> +		    sbp->sb_frextents != 0)) {

Ditto on the unlikely and the whitespace. That code looks weird...

		if (sbp->sb_rextents || sbp->sb_rbmblocks ||
		    sbp->sb_rextslog || sbp->sb_frextents) {

> +			xfs_notice(mp,
> +				"realtime zeroed geometry sanity check failed");
> +			return -EFSCORRUPTED;
> +		}
> +	} else {
> +		xfs_rtblock_t	rexts;
> +		uint32_t	temp;
> +
> +		rexts = div_u64_rem(sbp->sb_rblocks, sbp->sb_rextsize, &temp);
> +		if (unlikely(
> +		    rexts != sbp->sb_rextents				||
> +		    sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents)	||

And again.

At least you're consistent, Darrick :)

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

Re: [PATCH 2/4] xfs: validate the realtime geometry in xfs_validate_sb_common

[Darrick J. Wong]

On Wed, Mar 25, 2020 at 04:00:28PM +1100, Dave Chinner wrote:
> On Tue, Mar 24, 2020 at 08:24:43PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Validate the geometry of the realtime geometry when we mount the
> > filesystem, so that we don't abruptly shut down the filesystem later on.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_sb.c |   35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 2f60fc3c99a0..dee0a1a594dc 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -328,6 +328,41 @@ xfs_validate_sb_common(
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> > +	/* Validate the realtime geometry; stolen from xfs_repair */
> > +	if (unlikely(
> > +	    sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE	||
> 
> Whacky whitespace before the ||
> 
> > +	    sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)) {
> > +		xfs_notice(mp,
> > +			"realtime extent sanity check failed");
> > +		return -EFSCORRUPTED;
> > +	}
> 
> We really don't need unlikely() in code like this. the compiler
> already considers code that returns inside an if statement as
> "unlikely" because it's the typical error handling pattern, for
> cases like this it really isn't necessary.
> 
> 
> > +
> > +	if (sbp->sb_rblocks == 0) {
> > +		if (unlikely(
> > +		    sbp->sb_rextents != 0				||
> > +		    sbp->sb_rbmblocks != 0				||
> > +		    sbp->sb_rextslog != 0				||
> > +		    sbp->sb_frextents != 0)) {
> 
> Ditto on the unlikely and the whitespace. That code looks weird...
> 
> 		if (sbp->sb_rextents || sbp->sb_rbmblocks ||
> 		    sbp->sb_rextslog || sbp->sb_frextents) {
> 
> > +			xfs_notice(mp,
> > +				"realtime zeroed geometry sanity check failed");
> > +			return -EFSCORRUPTED;
> > +		}
> > +	} else {
> > +		xfs_rtblock_t	rexts;
> > +		uint32_t	temp;
> > +
> > +		rexts = div_u64_rem(sbp->sb_rblocks, sbp->sb_rextsize, &temp);
> > +		if (unlikely(
> > +		    rexts != sbp->sb_rextents				||
> > +		    sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents)	||
> 
> And again.
> 
> At least you're consistent, Darrick :)

Copy-pastaing the weird style of the rest of that function. :)

I'll fix it & resend.

--D

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

[PATCH v2 2/4] xfs: validate the realtime geometry in xfs_validate_sb_common

[Darrick J. Wong]

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

Validate the geometry of the realtime geometry when we mount the
filesystem, so that we don't abruptly shut down the filesystem later on.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: remove the excess whitespace and unlikely() bits
---
 fs/xfs/libxfs/xfs_sb.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 2f60fc3c99a0..9a0cd25ec22a 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -328,6 +328,36 @@ xfs_validate_sb_common(
 		return -EFSCORRUPTED;
 	}
 
+	/* Validate the realtime geometry; stolen from xfs_repair */
+	if (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE ||
+	    sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE) {
+		xfs_notice(mp,
+			"realtime extent sanity check failed");
+		return -EFSCORRUPTED;
+	}
+
+	if (sbp->sb_rblocks == 0) {
+		if (sbp->sb_rextents != 0 || sbp->sb_rbmblocks != 0 ||
+		    sbp->sb_rextslog != 0 || sbp->sb_frextents != 0) {
+			xfs_notice(mp,
+				"realtime zeroed geometry sanity check failed");
+			return -EFSCORRUPTED;
+		}
+	} else {
+		xfs_rtblock_t	rexts;
+		uint32_t	temp;
+
+		rexts = div_u64_rem(sbp->sb_rblocks, sbp->sb_rextsize, &temp);
+		if (rexts != sbp->sb_rextents ||
+		    sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents) ||
+		    sbp->sb_rbmblocks != howmany(sbp->sb_rextents,
+						 NBBY * sbp->sb_blocksize)) {
+			xfs_notice(mp,
+				"realtime geometry sanity check failed");
+			return -EFSCORRUPTED;
+		}
+	}
+
 	if (sbp->sb_unit) {
 		if (!xfs_sb_version_hasdalign(sbp) ||
 		    sbp->sb_unit > sbp->sb_width ||

Re: [PATCH v2 2/4] xfs: validate the realtime geometry in xfs_validate_sb_common

[Dave Chinner]

On Tue, Mar 24, 2020 at 11:07:19PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Validate the geometry of the realtime geometry when we mount the
> filesystem, so that we don't abruptly shut down the filesystem later on.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: remove the excess whitespace and unlikely() bits

Much nicer!

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

Re: [PATCH 2/4] xfs: validate the realtime geometry in xfs_validate_sb_common

[Chandan Rajendra]

On Wednesday, March 25, 2020 8:54 AM Darrick J. Wong wrote: 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Validate the geometry of the realtime geometry when we mount the
> filesystem, so that we don't abruptly shut down the filesystem later on.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

The changes look logically correct.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> ---
>  fs/xfs/libxfs/xfs_sb.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 2f60fc3c99a0..dee0a1a594dc 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -328,6 +328,41 @@ xfs_validate_sb_common(
>  		return -EFSCORRUPTED;
>  	}
>  
> +	/* Validate the realtime geometry; stolen from xfs_repair */
> +	if (unlikely(
> +	    sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE	||
> +	    sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)) {
> +		xfs_notice(mp,
> +			"realtime extent sanity check failed");
> +		return -EFSCORRUPTED;
> +	}
> +
> +	if (sbp->sb_rblocks == 0) {
> +		if (unlikely(
> +		    sbp->sb_rextents != 0				||
> +		    sbp->sb_rbmblocks != 0				||
> +		    sbp->sb_rextslog != 0				||
> +		    sbp->sb_frextents != 0)) {
> +			xfs_notice(mp,
> +				"realtime zeroed geometry sanity check failed");
> +			return -EFSCORRUPTED;
> +		}
> +	} else {
> +		xfs_rtblock_t	rexts;
> +		uint32_t	temp;
> +
> +		rexts = div_u64_rem(sbp->sb_rblocks, sbp->sb_rextsize, &temp);
> +		if (unlikely(
> +		    rexts != sbp->sb_rextents				||
> +		    sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents)	||
> +		    sbp->sb_rbmblocks != howmany(sbp->sb_rextents,
> +						NBBY * sbp->sb_blocksize))) {
> +			xfs_notice(mp,
> +				"realtime geometry sanity check failed");
> +			return -EFSCORRUPTED;
> +		}
> +	}
> +
>  	if (sbp->sb_unit) {
>  		if (!xfs_sb_version_hasdalign(sbp) ||
>  		    sbp->sb_unit > sbp->sb_width ||
> 
> 


-- 
chandan

[PATCH 3/4] xfs: drop all altpath buffers at the end of the sibling check

[Darrick J. Wong]

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

The dirattr btree checking code uses the altpath substructure of the
dirattr state structure to check the sibling pointers of dir/attr tree
blocks.  At the end of sibling checks, xfs_da3_path_shift could have
changed multiple levels of buffer pointers in the altpath structure.
Although we release the leaf level buffer, this isn't enough -- we also
need to release the node buffers that are unique to the altpath.

Not releasing all of the altpath buffers leaves them locked to the
transaction.  This is suboptimal because we should release resources
when we don't need them anymore.  Fix the function to loop all levels of
the altpath, and fix the return logic so that we always run the loop.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dabtree.c |   42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)


diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
index 97a15b6f2865..9a2e27ac1300 100644
--- a/fs/xfs/scrub/dabtree.c
+++ b/fs/xfs/scrub/dabtree.c
@@ -219,19 +219,21 @@ xchk_da_btree_block_check_sibling(
 	int			direction,
 	xfs_dablk_t		sibling)
 {
+	struct xfs_da_state_path *path = &ds->state->path;
+	struct xfs_da_state_path *altpath = &ds->state->altpath;
 	int			retval;
+	int			plevel;
 	int			error;
 
-	memcpy(&ds->state->altpath, &ds->state->path,
-			sizeof(ds->state->altpath));
+	memcpy(altpath, path, sizeof(ds->state->altpath));
 
 	/*
 	 * If the pointer is null, we shouldn't be able to move the upper
 	 * level pointer anywhere.
 	 */
 	if (sibling == 0) {
-		error = xfs_da3_path_shift(ds->state, &ds->state->altpath,
-				direction, false, &retval);
+		error = xfs_da3_path_shift(ds->state, altpath, direction,
+				false, &retval);
 		if (error == 0 && retval == 0)
 			xchk_da_set_corrupt(ds, level);
 		error = 0;
@@ -239,27 +241,33 @@ xchk_da_btree_block_check_sibling(
 	}
 
 	/* Move the alternate cursor one block in the direction given. */
-	error = xfs_da3_path_shift(ds->state, &ds->state->altpath,
-			direction, false, &retval);
+	error = xfs_da3_path_shift(ds->state, altpath, direction, false,
+			&retval);
 	if (!xchk_da_process_error(ds, level, &error))
-		return error;
+		goto out;
 	if (retval) {
 		xchk_da_set_corrupt(ds, level);
-		return error;
+		goto out;
 	}
-	if (ds->state->altpath.blk[level].bp)
-		xchk_buffer_recheck(ds->sc,
-				ds->state->altpath.blk[level].bp);
+	if (altpath->blk[level].bp)
+		xchk_buffer_recheck(ds->sc, altpath->blk[level].bp);
 
 	/* Compare upper level pointer to sibling pointer. */
-	if (ds->state->altpath.blk[level].blkno != sibling)
+	if (altpath->blk[level].blkno != sibling)
 		xchk_da_set_corrupt(ds, level);
-	if (ds->state->altpath.blk[level].bp) {
-		xfs_trans_brelse(ds->dargs.trans,
-				ds->state->altpath.blk[level].bp);
-		ds->state->altpath.blk[level].bp = NULL;
-	}
+
 out:
+	/* Free all buffers in the altpath that aren't referenced from path. */
+	for (plevel = 0; plevel < altpath->active; plevel++) {
+		if (altpath->blk[plevel].bp == NULL ||
+		    (plevel < path->active &&
+		     altpath->blk[plevel].bp == path->blk[plevel].bp))
+			continue;
+
+		xfs_trans_brelse(ds->dargs.trans, altpath->blk[plevel].bp);
+		altpath->blk[plevel].bp = NULL;
+	}
+
 	return error;
 }
 

Re: [PATCH 3/4] xfs: drop all altpath buffers at the end of the sibling check

[Dave Chinner]

On Tue, Mar 24, 2020 at 08:24:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The dirattr btree checking code uses the altpath substructure of the
> dirattr state structure to check the sibling pointers of dir/attr tree
> blocks.  At the end of sibling checks, xfs_da3_path_shift could have
> changed multiple levels of buffer pointers in the altpath structure.
> Although we release the leaf level buffer, this isn't enough -- we also
> need to release the node buffers that are unique to the altpath.
> 
> Not releasing all of the altpath buffers leaves them locked to the
> transaction.  This is suboptimal because we should release resources
> when we don't need them anymore.  Fix the function to loop all levels of
> the altpath, and fix the return logic so that we always run the loop.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/dabtree.c |   42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)

looks reasonable.

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

Re: [PATCH 3/4] xfs: drop all altpath buffers at the end of the sibling check

[Chandan Rajendra]

On Wednesday, March 25, 2020 8:54 AM Darrick J. Wong wrote: 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The dirattr btree checking code uses the altpath substructure of the
> dirattr state structure to check the sibling pointers of dir/attr tree
> blocks.  At the end of sibling checks, xfs_da3_path_shift could have
> changed multiple levels of buffer pointers in the altpath structure.
> Although we release the leaf level buffer, this isn't enough -- we also
> need to release the node buffers that are unique to the altpath.
> 
> Not releasing all of the altpath buffers leaves them locked to the
> transaction.  This is suboptimal because we should release resources
> when we don't need them anymore.  Fix the function to loop all levels of
> the altpath, and fix the return logic so that we always run the loop.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

The patch indeed releases 'altpath' block buffers that are not referenced by
by block buffers in 'path' after the sibling block is checked for
inconsistencies.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> ---
>  fs/xfs/scrub/dabtree.c |   42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
> index 97a15b6f2865..9a2e27ac1300 100644
> --- a/fs/xfs/scrub/dabtree.c
> +++ b/fs/xfs/scrub/dabtree.c
> @@ -219,19 +219,21 @@ xchk_da_btree_block_check_sibling(
>  	int			direction,
>  	xfs_dablk_t		sibling)
>  {
> +	struct xfs_da_state_path *path = &ds->state->path;
> +	struct xfs_da_state_path *altpath = &ds->state->altpath;
>  	int			retval;
> +	int			plevel;
>  	int			error;
>  
> -	memcpy(&ds->state->altpath, &ds->state->path,
> -			sizeof(ds->state->altpath));
> +	memcpy(altpath, path, sizeof(ds->state->altpath));
>  
>  	/*
>  	 * If the pointer is null, we shouldn't be able to move the upper
>  	 * level pointer anywhere.
>  	 */
>  	if (sibling == 0) {
> -		error = xfs_da3_path_shift(ds->state, &ds->state->altpath,
> -				direction, false, &retval);
> +		error = xfs_da3_path_shift(ds->state, altpath, direction,
> +				false, &retval);
>  		if (error == 0 && retval == 0)
>  			xchk_da_set_corrupt(ds, level);
>  		error = 0;
> @@ -239,27 +241,33 @@ xchk_da_btree_block_check_sibling(
>  	}
>  
>  	/* Move the alternate cursor one block in the direction given. */
> -	error = xfs_da3_path_shift(ds->state, &ds->state->altpath,
> -			direction, false, &retval);
> +	error = xfs_da3_path_shift(ds->state, altpath, direction, false,
> +			&retval);
>  	if (!xchk_da_process_error(ds, level, &error))
> -		return error;
> +		goto out;
>  	if (retval) {
>  		xchk_da_set_corrupt(ds, level);
> -		return error;
> +		goto out;
>  	}
> -	if (ds->state->altpath.blk[level].bp)
> -		xchk_buffer_recheck(ds->sc,
> -				ds->state->altpath.blk[level].bp);
> +	if (altpath->blk[level].bp)
> +		xchk_buffer_recheck(ds->sc, altpath->blk[level].bp);
>  
>  	/* Compare upper level pointer to sibling pointer. */
> -	if (ds->state->altpath.blk[level].blkno != sibling)
> +	if (altpath->blk[level].blkno != sibling)
>  		xchk_da_set_corrupt(ds, level);
> -	if (ds->state->altpath.blk[level].bp) {
> -		xfs_trans_brelse(ds->dargs.trans,
> -				ds->state->altpath.blk[level].bp);
> -		ds->state->altpath.blk[level].bp = NULL;
> -	}
> +
>  out:
> +	/* Free all buffers in the altpath that aren't referenced from path. */
> +	for (plevel = 0; plevel < altpath->active; plevel++) {
> +		if (altpath->blk[plevel].bp == NULL ||
> +		    (plevel < path->active &&
> +		     altpath->blk[plevel].bp == path->blk[plevel].bp))
> +			continue;
> +
> +		xfs_trans_brelse(ds->dargs.trans, altpath->blk[plevel].bp);
> +		altpath->blk[plevel].bp = NULL;
> +	}
> +
>  	return error;
>  }
>  
> 
> 


-- 
chandan

[PATCH 4/4] xfs: directory bestfree check should release buffers

[Darrick J. Wong]

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

When we're checking bestfree information in directory blocks, always
drop the block buffer at the end of the function.  We should always
release resources when we're done using them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dir.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index c186c83544ac..f4655453f605 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -508,7 +508,7 @@ xchk_directory_leaf1_bestfree(
 	/* Read the free space block. */
 	error = xfs_dir3_leaf_read(sc->tp, sc->ip, lblk, &bp);
 	if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk, &error))
-		goto out;
+		return error;
 	xchk_buffer_recheck(sc, bp);
 
 	leaf = bp->b_addr;
@@ -573,9 +573,10 @@ xchk_directory_leaf1_bestfree(
 		xchk_directory_check_freesp(sc, lblk, dbp, best);
 		xfs_trans_brelse(sc->tp, dbp);
 		if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
-			goto out;
+			break;
 	}
 out:
+	xfs_trans_brelse(sc->tp, bp);
 	return error;
 }
 
@@ -597,7 +598,7 @@ xchk_directory_free_bestfree(
 	/* Read the free space block */
 	error = xfs_dir2_free_read(sc->tp, sc->ip, lblk, &bp);
 	if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk, &error))
-		goto out;
+		return error;
 	xchk_buffer_recheck(sc, bp);
 
 	if (xfs_sb_version_hascrc(&sc->mp->m_sb)) {
@@ -620,7 +621,7 @@ xchk_directory_free_bestfree(
 				0, &dbp);
 		if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk,
 				&error))
-			break;
+			goto out;
 		xchk_directory_check_freesp(sc, lblk, dbp, best);
 		xfs_trans_brelse(sc->tp, dbp);
 	}
@@ -628,6 +629,7 @@ xchk_directory_free_bestfree(
 	if (freehdr.nused + stale != freehdr.nvalid)
 		xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
 out:
+	xfs_trans_brelse(sc->tp, bp);
 	return error;
 }
 

Re: [PATCH 4/4] xfs: directory bestfree check should release buffers

[Dave Chinner]

On Tue, Mar 24, 2020 at 08:24:55PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're checking bestfree information in directory blocks, always
> drop the block buffer at the end of the function.  We should always
> release resources when we're done using them.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good.

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