[PATCH 0/2] Bypass sb geometry if custom alignment is supplied on mount

[PATCH 0/2] Bypass sb geometry if custom alignment is supplied on mount

[Carlos Maiolino]

Hi,

this is just a small series where the main goal is achieved by patch 2. Patch one is
just a small fix on mp->m_dalign and mp->m_swidth I spotted when working on
patch 2.

Carlos Maiolino (2):
  xfs: Fix xfs_mount sunit and swidth types
  xfs: Bypass sb alignment checks when custom values are used

 fs/xfs/libxfs/xfs_bmap.c   |  2 +-
 fs/xfs/libxfs/xfs_ialloc.c |  2 +-
 fs/xfs/libxfs/xfs_ialloc.h |  2 +-
 fs/xfs/libxfs/xfs_sb.c     | 30 +++++++++++++++++++-----------
 fs/xfs/xfs_mount.c         |  4 ++--
 fs/xfs/xfs_mount.h         |  6 ++++--
 fs/xfs/xfs_super.c         | 11 ++++++-----
 fs/xfs/xfs_trace.h         |  7 ++++---
 8 files changed, 38 insertions(+), 26 deletions(-)

-- 
2.26.2

[PATCH 1/2] xfs: Fix xfs_mount sunit and swidth types

[Carlos Maiolino]

The sunit (mp->m_dalign) and swidth (mp->m_swidth) are currently encoded
as int in the xfs_mount structure, while it's treated as unsigned
everywhere where it is used (saving some places also changed by this
patch).

Change their type to uint32_t and fix some code using them as int.

This has been spotted by code inspection.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c   |  2 +-
 fs/xfs/libxfs/xfs_ialloc.c |  2 +-
 fs/xfs/libxfs/xfs_ialloc.h |  2 +-
 fs/xfs/xfs_mount.c         |  4 ++--
 fs/xfs/xfs_mount.h         |  4 ++--
 fs/xfs/xfs_super.c         | 10 +++++-----
 fs/xfs/xfs_trace.h         |  7 ++++---
 7 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 667cdd0dfdf4..8b46f15cc414 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3477,7 +3477,7 @@ xfs_bmap_btalloc(
 	int		isaligned;
 	int		tryagain;
 	int		error;
-	int		stripe_align;
+	uint32_t	stripe_align;
 
 	ASSERT(ap->length);
 	orig_offset = ap->offset;
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 7fcf62b324b0..75cb2dd78a7a 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -2913,7 +2913,7 @@ xfs_ialloc_setup_geometry(
 xfs_ino_t
 xfs_ialloc_calc_rootino(
 	struct xfs_mount	*mp,
-	int			sunit)
+	uint32_t		sunit)
 {
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
 	xfs_agblock_t		first_bno;
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 72b3468b97b1..a992bcae5358 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -152,6 +152,6 @@ int xfs_inobt_insert_rec(struct xfs_btree_cur *cur, uint16_t holemask,
 
 int xfs_ialloc_cluster_alignment(struct xfs_mount *mp);
 void xfs_ialloc_setup_geometry(struct xfs_mount *mp);
-xfs_ino_t xfs_ialloc_calc_rootino(struct xfs_mount *mp, int sunit);
+xfs_ino_t xfs_ialloc_calc_rootino(struct xfs_mount *mp, uint32_t sunit);
 
 #endif	/* __XFS_IALLOC_H__ */
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index d5dcf9869860..1b1861376854 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -368,7 +368,7 @@ xfs_readsb(
 static inline int
 xfs_check_new_dalign(
 	struct xfs_mount	*mp,
-	int			new_dalign,
+	uint32_t		new_dalign,
 	bool			*update_sb)
 {
 	struct xfs_sb		*sbp = &mp->m_sb;
@@ -432,7 +432,7 @@ xfs_validate_new_dalign(
 			mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
 		} else {
 			xfs_warn(mp,
-		"alignment check failed: sunit(%d) less than bsize(%d)",
+		"alignment check failed: sunit(%u) less than bsize(%d)",
 				 mp->m_dalign, mp->m_sb.sb_blocksize);
 			return -EINVAL;
 		}
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 3725d25ad97e..6552473ab117 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -119,8 +119,8 @@ typedef struct xfs_mount {
 	xfs_extlen_t		m_ag_prealloc_blocks; /* reserved ag blocks */
 	uint			m_alloc_set_aside; /* space we can't use */
 	uint			m_ag_max_usable; /* max space per AG */
-	int			m_dalign;	/* stripe unit */
-	int			m_swidth;	/* stripe width */
+	uint32_t		m_dalign;	/* stripe unit */
+	uint32_t		m_swidth;	/* stripe width */
 	xfs_agnumber_t		m_maxagi;	/* highest inode alloc group */
 	uint			m_allocsize_log;/* min write size log bytes */
 	uint			m_allocsize_blocks; /* min write size blocks */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index fa58cb07c8fd..580072b19e8a 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -193,11 +193,11 @@ xfs_fs_show_options(
 		seq_show_option(m, "rtdev", mp->m_rtname);
 
 	if (mp->m_dalign > 0)
-		seq_printf(m, ",sunit=%d",
-				(int)XFS_FSB_TO_BB(mp, mp->m_dalign));
+		seq_printf(m, ",sunit=%u",
+				XFS_FSB_TO_BB(mp, mp->m_dalign));
 	if (mp->m_swidth > 0)
-		seq_printf(m, ",swidth=%d",
-				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
+		seq_printf(m, ",swidth=%u",
+				XFS_FSB_TO_BB(mp, mp->m_swidth));
 
 	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
 		seq_puts(m, ",usrquota");
@@ -1338,7 +1338,7 @@ xfs_fc_validate_params(
 
 	if (mp->m_dalign && (mp->m_swidth % mp->m_dalign != 0)) {
 		xfs_warn(mp,
-	"stripe width (%d) must be a multiple of the stripe unit (%d)",
+	"stripe width (%u) must be a multiple of the stripe unit (%u)",
 			mp->m_swidth, mp->m_dalign);
 		return -EINVAL;
 	}
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 460136628a79..ad0c10e01a73 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3585,11 +3585,12 @@ DEFINE_KMEM_EVENT(kmem_realloc);
 DEFINE_KMEM_EVENT(kmem_zone_alloc);
 
 TRACE_EVENT(xfs_check_new_dalign,
-	TP_PROTO(struct xfs_mount *mp, int new_dalign, xfs_ino_t calc_rootino),
+	TP_PROTO(struct xfs_mount *mp, uint32_t new_dalign,
+		 xfs_ino_t calc_rootino),
 	TP_ARGS(mp, new_dalign, calc_rootino),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
-		__field(int, new_dalign)
+		__field(uint32_t, new_dalign)
 		__field(xfs_ino_t, sb_rootino)
 		__field(xfs_ino_t, calc_rootino)
 	),
@@ -3599,7 +3600,7 @@ TRACE_EVENT(xfs_check_new_dalign,
 		__entry->sb_rootino = mp->m_sb.sb_rootino;
 		__entry->calc_rootino = calc_rootino;
 	),
-	TP_printk("dev %d:%d new_dalign %d sb_rootino %llu calc_rootino %llu",
+	TP_printk("dev %d:%d new_dalign %u sb_rootino %llu calc_rootino %llu",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->new_dalign, __entry->sb_rootino,
 		  __entry->calc_rootino)
-- 
2.26.2

[PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Carlos Maiolino]

This patch introduces a new mount flag: XFS_MOUNT_ALIGN that is set when
custom alignment values are set, making easier to identify when user
passes custom alignment options via mount command line.

Then use this flag to bypass on-disk alignment checks.

This is useful specifically for users which had filesystems created with
wrong alignment provided by buggy storage, which, after commit
fa4ca9c5574605, these filesystems won't be mountable anymore. But, using
custom alignment settings, there is no need to check those values, once
the alignment used will be the one provided during mount time, avoiding
the issues in the allocator caused by bad alignment values anyway. This
at least give a chance for users to remount their filesystems on newer
kernels, without needing to reformat it.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/libxfs/xfs_sb.c | 30 +++++++++++++++++++-----------
 fs/xfs/xfs_mount.h     |  2 ++
 fs/xfs/xfs_super.c     |  1 +
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 4df87546bd40..72dae95a5e4a 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -360,19 +360,27 @@ xfs_validate_sb_common(
 		}
 	}
 
-	if (sbp->sb_unit) {
-		if (!xfs_sb_version_hasdalign(sbp) ||
-		    sbp->sb_unit > sbp->sb_width ||
-		    (sbp->sb_width % sbp->sb_unit) != 0) {
-			xfs_notice(mp, "SB stripe unit sanity check failed");
+	/*
+	 * Ignore superblock alignment checks if sunit/swidth mount options
+	 * were used or alignment turned off.
+	 * The custom alignment validation will happen later on xfs_mountfs()
+	 */
+	if (!(mp->m_flags & XFS_MOUNT_ALIGN) &&
+	    !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
+		if (sbp->sb_unit) {
+			if (!xfs_sb_version_hasdalign(sbp) ||
+			    sbp->sb_unit > sbp->sb_width ||
+			    (sbp->sb_width % sbp->sb_unit) != 0) {
+				xfs_notice(mp, "SB stripe unit sanity check failed");
+				return -EFSCORRUPTED;
+			}
+		} else if (xfs_sb_version_hasdalign(sbp)) {
+			xfs_notice(mp, "SB stripe alignment sanity check failed");
+			return -EFSCORRUPTED;
+		} else if (sbp->sb_width) {
+			xfs_notice(mp, "SB stripe width sanity check failed");
 			return -EFSCORRUPTED;
 		}
-	} else if (xfs_sb_version_hasdalign(sbp)) {
-		xfs_notice(mp, "SB stripe alignment sanity check failed");
-		return -EFSCORRUPTED;
-	} else if (sbp->sb_width) {
-		xfs_notice(mp, "SB stripe width sanity check failed");
-		return -EFSCORRUPTED;
 	}
 
 
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 6552473ab117..3b650795fbc3 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -233,6 +233,8 @@ typedef struct xfs_mount {
 						   operations, typically for
 						   disk errors in metadata */
 #define XFS_MOUNT_DISCARD	(1ULL << 5)	/* discard unused blocks */
+#define XFS_MOUNT_ALIGN		(1ULL << 6)	/* Custom alignment set via
+						   mount */
 #define XFS_MOUNT_NOALIGN	(1ULL << 7)	/* turn off stripe alignment
 						   allocations */
 #define XFS_MOUNT_ATTR2		(1ULL << 8)	/* allow use of attr2 format */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 580072b19e8a..981e69845620 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1214,6 +1214,7 @@ xfs_fc_parse_param(
 		return 0;
 	case Opt_sunit:
 		mp->m_dalign = result.uint_32;
+		mp->m_flags |= XFS_MOUNT_ALIGN;
 		return 0;
 	case Opt_swidth:
 		mp->m_swidth = result.uint_32;
-- 
2.26.2

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Dave Chinner]

On Mon, Jun 01, 2020 at 04:01:53PM +0200, Carlos Maiolino wrote:
> This patch introduces a new mount flag: XFS_MOUNT_ALIGN that is set when
> custom alignment values are set, making easier to identify when user
> passes custom alignment options via mount command line.
> 
> Then use this flag to bypass on-disk alignment checks.
> 
> This is useful specifically for users which had filesystems created with
> wrong alignment provided by buggy storage, which, after commit
> fa4ca9c5574605, these filesystems won't be mountable anymore. But, using
> custom alignment settings, there is no need to check those values, once
> the alignment used will be the one provided during mount time, avoiding
> the issues in the allocator caused by bad alignment values anyway. This
> at least give a chance for users to remount their filesystems on newer
> kernels, without needing to reformat it.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c | 30 +++++++++++++++++++-----------
>  fs/xfs/xfs_mount.h     |  2 ++
>  fs/xfs/xfs_super.c     |  1 +
>  3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 4df87546bd40..72dae95a5e4a 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -360,19 +360,27 @@ xfs_validate_sb_common(
>  		}
>  	}
>  
> -	if (sbp->sb_unit) {
> -		if (!xfs_sb_version_hasdalign(sbp) ||
> -		    sbp->sb_unit > sbp->sb_width ||
> -		    (sbp->sb_width % sbp->sb_unit) != 0) {
> -			xfs_notice(mp, "SB stripe unit sanity check failed");
> +	/*
> +	 * Ignore superblock alignment checks if sunit/swidth mount options
> +	 * were used or alignment turned off.
> +	 * The custom alignment validation will happen later on xfs_mountfs()
> +	 */
> +	if (!(mp->m_flags & XFS_MOUNT_ALIGN) &&
> +	    !(mp->m_flags & XFS_MOUNT_NOALIGN)) {

mp->m_dalign tells us at this point if a user specified sunit as a
mount option.  That's how xfs_fc_validate_params() determines the user
specified a custom sunit, so there is no need for a new mount flag
here to indicate that mp->m_dalign was set by the user....

Also, I think if the user specifies "NOALIGN" then we should still
check the sunit/swidth and issue a warning that they are
bad/invalid, or at least indicate in some way that the superblock is
unhealthy and needs attention. Using mount options to sweep issues
that need fixing under the carpet is less than ideal...

Also, I see nothing that turns off XFS_MOUNT_ALIGN when the custom
alignment is written to the superblock and becomes the new on-disk
values. Once we have those values in the in-core superblock, the
write of the superblock should run the verifier to validate them.
i.e. leaving this XFS_MOUNT_ALIGN set allows fields of the
superblock we just modified to be written to disk without verifier
validation.

From that last perspective, I _really_ don't like the idea of
having user controlled conditional validation like this in the
common verifier.

From a user perspective, I think this "use mount options to override
bad values" approach is really nasty. How do you fix a system that
won't boot because the root filesystem has bad sunit/swidth values?
Telling the data center admin that they have to go boot every
machine in their data center into a rescue distro after an automated
upgrade triggered widespread boot failures is really not very user
or admin friendly.

IMO, this bad sunit/swidth condition should be:

	a) detected automatically at mount time,
	b) corrected automatically at mount time, and
	c) always verified to be valid at superblock write time.

IOWs, instead of failing to mount because sunit/swidth is invalid,
we issue a warning and automatically correct it to something valid.
There is precedence for this - we've done it with the AGFL free list
format screwups and for journal structures that are different shapes
on different platforms.

Hence we need to move this verification out of the common sb
verifier and move it into the write verifier (i.e. write is always
verified).  Then in the mount path where we set user specified mount
options, we should extent that to validate the existing on-disk
values and then modify them if they are invalid. Rules for fixing
are simple:

	1. if !hasdalign(sb), set both sunit/swidth to zero.
	2. If sunit is zero, zero swidth.
	1. If swidth is not valid, round it up it to the nearest
	   integer multiple of sunit.

The user was not responsible for this mess (combination of missing
validation in XFS code and bad storage firmware providing garbage)
so we should not put them on the hook for fixing it. We can do it
easily and without needing user intervention and so that's what we
should do.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Eric Sandeen]

On 6/1/20 4:21 PM, Dave Chinner wrote:
> The user was not responsible for this mess (combination of missing
> validation in XFS code and bad storage firmware providing garbage)
> so we should not put them on the hook for fixing it. We can do it
> easily and without needing user intervention and so that's what we
> should do.

FWIW, I have a working xfs_repair that fixes bad geometry as well,
I ... guess that's probably still useful?

It was doing similar things to what you suggested, though I wasn't
rounding swidth up, I was setting it equal to sunit.  *shrug* rounding
up is maybe better; the problematic devices usually have width < unit
so rounding up will be the same as setting equal  :)

phase1()

+       /*
+        * Now see if there's a problem with the stripe width; if it's bad,
+        * we just set it equal to the stripe unit as a harmless alternative.
+        */
+        if (xfs_sb_version_hasdalign(sb)) {
+                if ((sb->sb_unit && !sb->sb_width) ||
+                    (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit)) {
+                       sb->sb_width = sb->sb_unit;
+                       primary_sb_modified = 1;
+                       geometry_modified = 1;
+                       do_warn(
+_("superblock has a bad stripe width, resetting to %d\n"),
+                               sb->sb_width);
+               }
+       }

I also had to more or less ignore bad swidths throughout repair (and in
xfs_validate_sb_common IIRC) to be able to get this far.  If repair thinks
a superblock is bad, it goes looking for otheres to replace it and if the
bad geometry came from the device, they are all equally "bad..."

-Eric

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Dave Chinner]

On Mon, Jun 01, 2020 at 05:06:36PM -0500, Eric Sandeen wrote:
> On 6/1/20 4:21 PM, Dave Chinner wrote:
> > The user was not responsible for this mess (combination of missing
> > validation in XFS code and bad storage firmware providing garbage)
> > so we should not put them on the hook for fixing it. We can do it
> > easily and without needing user intervention and so that's what we
> > should do.
> 
> FWIW, I have a working xfs_repair that fixes bad geometry as well,
> I ... guess that's probably still useful?

Yes, repair should definitely be proactive on this.

> It was doing similar things to what you suggested, though I wasn't
> rounding swidth up, I was setting it equal to sunit.  *shrug* rounding
> up is maybe better; the problematic devices usually have width < unit
> so rounding up will be the same as setting equal  :)

Yup, that's exactly why I suggested rounding up :)

> phase1()
> 
> +       /*
> +        * Now see if there's a problem with the stripe width; if it's bad,
> +        * we just set it equal to the stripe unit as a harmless alternative.
> +        */
> +        if (xfs_sb_version_hasdalign(sb)) {
> +                if ((sb->sb_unit && !sb->sb_width) ||
> +                    (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit)) {
> +                       sb->sb_width = sb->sb_unit;
> +                       primary_sb_modified = 1;
> +                       geometry_modified = 1;
> +                       do_warn(
> +_("superblock has a bad stripe width, resetting to %d\n"),
> +                               sb->sb_width);
> +               }
> +       }
> 
> I also had to more or less ignore bad swidths throughout repair (and in
> xfs_validate_sb_common IIRC) to be able to get this far.  If repair thinks
> a superblock is bad, it goes looking for otheres to replace it and if the
> bad geometry came from the device, they are all equally "bad..."

Yeah. Which leads me to ask: should the kernel be updating the
secondary superblocks when it finds bad geometry in the primary, or
overwrites the geometry with user supplied info?

(I have a vague recollection of being asked something about this on
IRC and me muttering something about CXFS only trashing the primary
superblock...)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Eric Sandeen]

On 6/1/20 6:35 PM, Dave Chinner wrote:
> On Mon, Jun 01, 2020 at 05:06:36PM -0500, Eric Sandeen wrote:
>> On 6/1/20 4:21 PM, Dave Chinner wrote:
>>> The user was not responsible for this mess (combination of missing
>>> validation in XFS code and bad storage firmware providing garbage)
>>> so we should not put them on the hook for fixing it. We can do it
>>> easily and without needing user intervention and so that's what we
>>> should do.
>>
>> FWIW, I have a working xfs_repair that fixes bad geometry as well,
>> I ... guess that's probably still useful?
> 
> Yes, repair should definitely be proactive on this.
> 
>> It was doing similar things to what you suggested, though I wasn't
>> rounding swidth up, I was setting it equal to sunit.  *shrug* rounding
>> up is maybe better; the problematic devices usually have width < unit
>> so rounding up will be the same as setting equal  :)
> 
> Yup, that's exactly why I suggested rounding up :)
> 
>> phase1()
>>
>> +       /*
>> +        * Now see if there's a problem with the stripe width; if it's bad,
>> +        * we just set it equal to the stripe unit as a harmless alternative.
>> +        */
>> +        if (xfs_sb_version_hasdalign(sb)) {
>> +                if ((sb->sb_unit && !sb->sb_width) ||
>> +                    (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit)) {
>> +                       sb->sb_width = sb->sb_unit;
>> +                       primary_sb_modified = 1;
>> +                       geometry_modified = 1;
>> +                       do_warn(
>> +_("superblock has a bad stripe width, resetting to %d\n"),
>> +                               sb->sb_width);
>> +               }
>> +       }
>>
>> I also had to more or less ignore bad swidths throughout repair (and in
>> xfs_validate_sb_common IIRC) to be able to get this far.  If repair thinks
>> a superblock is bad, it goes looking for otheres to replace it and if the
>> bad geometry came from the device, they are all equally "bad..."
> 
> Yeah. Which leads me to ask: should the kernel be updating the
> secondary superblocks when it finds bad geometry in the primary, or
> overwrites the geometry with user supplied info?

since repair depends on it .... probably so.

(hm I haven't seen what happens if we update the primary under normal
circumstances, and then primary & secondaries have valid but different
geometries...?)

> (I have a vague recollection of being asked something about this on
> IRC and me muttering something about CXFS only trashing the primary
> superblock...)

Yeah I blathered about it, we have a function to call to do this for growfs,
so it'd be trivial and seems wise.

-Eric

> Cheers,
> 
> Dave.
> 

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Carlos Maiolino]

Hi Dave.

On Tue, Jun 02, 2020 at 07:21:15AM +1000, Dave Chinner wrote:
> On Mon, Jun 01, 2020 at 04:01:53PM +0200, Carlos Maiolino wrote:
> > index 4df87546bd40..72dae95a5e4a 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -360,19 +360,27 @@ xfs_validate_sb_common(
> >  		}
> >  	}
> >  
> > -	if (sbp->sb_unit) {
> > -		if (!xfs_sb_version_hasdalign(sbp) ||
> > -		    sbp->sb_unit > sbp->sb_width ||
> > -		    (sbp->sb_width % sbp->sb_unit) != 0) {
> > -			xfs_notice(mp, "SB stripe unit sanity check failed");
> > +	/*
> > +	 * Ignore superblock alignment checks if sunit/swidth mount options
> > +	 * were used or alignment turned off.
> > +	 * The custom alignment validation will happen later on xfs_mountfs()
> > +	 */
> > +	if (!(mp->m_flags & XFS_MOUNT_ALIGN) &&
> > +	    !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> 
> mp->m_dalign tells us at this point if a user specified sunit as a
> mount option.  That's how xfs_fc_validate_params() determines the user
> specified a custom sunit, so there is no need for a new mount flag
> here to indicate that mp->m_dalign was set by the user....

At a first glance, I thought about it too, but, there is nothing preventing an
user to mount a filesystem passing sunit=0,swidth=0. So, this means we can't
really rely on the m_dalign/m_swidth values to check an user passed in (or not)
alignment values. Unless we first deny users to pass 0 values into it.

> 
> Also, I think if the user specifies "NOALIGN" then we should still
> check the sunit/swidth and issue a warning that they are
> bad/invalid, or at least indicate in some way that the superblock is
> unhealthy and needs attention. Using mount options to sweep issues
> that need fixing under the carpet is less than ideal...
> 
> Also, I see nothing that turns off XFS_MOUNT_ALIGN when the custom
> alignment is written to the superblock and becomes the new on-disk
> values. Once we have those values in the in-core superblock, the
> write of the superblock should run the verifier to validate them.
> i.e. leaving this XFS_MOUNT_ALIGN set allows fields of the
> superblock we just modified to be written to disk without verifier
> validation.

I didn't think about it, thanks for the heads up.

> 
> From that last perspective, I _really_ don't like the idea of
> having user controlled conditional validation like this in the
> common verifier.
> 
> From a user perspective, I think this "use mount options to override
> bad values" approach is really nasty. How do you fix a system that
> won't boot because the root filesystem has bad sunit/swidth values?
> Telling the data center admin that they have to go boot every
> machine in their data center into a rescue distro after an automated
> upgrade triggered widespread boot failures is really not very user
> or admin friendly.
> 
> IMO, this bad sunit/swidth condition should be:
> 
> 	a) detected automatically at mount time,
> 	b) corrected automatically at mount time, and
> 	c) always verified to be valid at superblock write time.
> 
> IOWs, instead of failing to mount because sunit/swidth is invalid,
> we issue a warning and automatically correct it to something valid.
> There is precedence for this - we've done it with the AGFL free list
> format screwups and for journal structures that are different shapes
> on different platforms.

Eh, that was one of the options I considered, and also pointed by Eric when we
talked about it previously. At the end, I thought automatically modifying it
under the hoods was too invasive in regards of changing geometry configuration
without user interaction. Foolish me :P

> 
> Hence we need to move this verification out of the common sb
> verifier and move it into the write verifier (i.e. write is always
> verified).  Then in the mount path where we set user specified mount
> options, we should extent that to validate the existing on-disk
> values and then modify them if they are invalid. Rules for fixing
> are simple:
> 
> 	1. if !hasdalign(sb), set both sunit/swidth to zero.
> 	2. If sunit is zero, zero swidth.
> 	1. If swidth is not valid, round it up it to the nearest
> 	   integer multiple of sunit.
> 
> The user was not responsible for this mess (combination of missing
> validation in XFS code and bad storage firmware providing garbage)
> so we should not put them on the hook for fixing it. We can do it
> easily and without needing user intervention and so that's what we
> should do.
> 

Thanks for the insights, I'll work on that direction.

Cheers

-- 
Carlos

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Dave Chinner]

On Tue, Jun 02, 2020 at 11:18:44AM +0200, Carlos Maiolino wrote:
> Hi Dave.
> 
> On Tue, Jun 02, 2020 at 07:21:15AM +1000, Dave Chinner wrote:
> > On Mon, Jun 01, 2020 at 04:01:53PM +0200, Carlos Maiolino wrote:
> > > index 4df87546bd40..72dae95a5e4a 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -360,19 +360,27 @@ xfs_validate_sb_common(
> > >  		}
> > >  	}
> > >  
> > > -	if (sbp->sb_unit) {
> > > -		if (!xfs_sb_version_hasdalign(sbp) ||
> > > -		    sbp->sb_unit > sbp->sb_width ||
> > > -		    (sbp->sb_width % sbp->sb_unit) != 0) {
> > > -			xfs_notice(mp, "SB stripe unit sanity check failed");
> > > +	/*
> > > +	 * Ignore superblock alignment checks if sunit/swidth mount options
> > > +	 * were used or alignment turned off.
> > > +	 * The custom alignment validation will happen later on xfs_mountfs()
> > > +	 */
> > > +	if (!(mp->m_flags & XFS_MOUNT_ALIGN) &&
> > > +	    !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > 
> > mp->m_dalign tells us at this point if a user specified sunit as a
> > mount option.  That's how xfs_fc_validate_params() determines the user
> > specified a custom sunit, so there is no need for a new mount flag
> > here to indicate that mp->m_dalign was set by the user....
> 
> At a first glance, I thought about it too, but, there is nothing preventing an
> user to mount a filesystem passing sunit=0,swidth=0. So, this means we can't
> really rely on the m_dalign/m_swidth values to check an user passed in (or not)
> alignment values. Unless we first deny users to pass 0 values into it.

Sure we can. We do this sort of "was the mount option set" detection
with m_logbufs and m_logbsize by initialising them to -1. Hence if
they are set by mount options, they'll have a valid, in-range value
instead of "-1".

That said, if you want users passing in sunit=0,swidth=0 to
correctly override existing on-disk values (i.e. effectively mean -o
noalign), then you are going to need to modify
xfs_update_alignment() and xfs_validate_new_dalign() to handle
mp->m_dalign == 0 as a valid value instead of "sunit/swidth mount
option not set, use existing superblock values".....

IOWs, there are deeper changes needed here than just setting a new
flag to say "mount option was set" for it to function correctly and
consistently as you intend. This is why I think we should just fix
this situation automatically, and not require the user to manually
override the bad values.

Thinking bigger picture, I'd like to see the mount options
deprecated and new xfs_admin options added to change the values on a
live, mounted filesystem. That way users who have issues like this
don't need to unmount the filesystem to fix it, not to mention users
who reshape online raid arrays and grow the filesystem can also
change the filesystem geometry without needing to take the
filesystem offline...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Darrick J. Wong]

On Wed, Jun 03, 2020 at 09:12:35AM +1000, Dave Chinner wrote:
> On Tue, Jun 02, 2020 at 11:18:44AM +0200, Carlos Maiolino wrote:
> > Hi Dave.
> > 
> > On Tue, Jun 02, 2020 at 07:21:15AM +1000, Dave Chinner wrote:
> > > On Mon, Jun 01, 2020 at 04:01:53PM +0200, Carlos Maiolino wrote:
> > > > index 4df87546bd40..72dae95a5e4a 100644
> > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > @@ -360,19 +360,27 @@ xfs_validate_sb_common(
> > > >  		}
> > > >  	}
> > > >  
> > > > -	if (sbp->sb_unit) {
> > > > -		if (!xfs_sb_version_hasdalign(sbp) ||
> > > > -		    sbp->sb_unit > sbp->sb_width ||
> > > > -		    (sbp->sb_width % sbp->sb_unit) != 0) {
> > > > -			xfs_notice(mp, "SB stripe unit sanity check failed");
> > > > +	/*
> > > > +	 * Ignore superblock alignment checks if sunit/swidth mount options
> > > > +	 * were used or alignment turned off.
> > > > +	 * The custom alignment validation will happen later on xfs_mountfs()
> > > > +	 */
> > > > +	if (!(mp->m_flags & XFS_MOUNT_ALIGN) &&
> > > > +	    !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > > 
> > > mp->m_dalign tells us at this point if a user specified sunit as a
> > > mount option.  That's how xfs_fc_validate_params() determines the user
> > > specified a custom sunit, so there is no need for a new mount flag
> > > here to indicate that mp->m_dalign was set by the user....
> > 
> > At a first glance, I thought about it too, but, there is nothing preventing an
> > user to mount a filesystem passing sunit=0,swidth=0. So, this means we can't
> > really rely on the m_dalign/m_swidth values to check an user passed in (or not)
> > alignment values. Unless we first deny users to pass 0 values into it.
> 
> Sure we can. We do this sort of "was the mount option set" detection
> with m_logbufs and m_logbsize by initialising them to -1. Hence if
> they are set by mount options, they'll have a valid, in-range value
> instead of "-1".
> 
> That said, if you want users passing in sunit=0,swidth=0 to
> correctly override existing on-disk values (i.e. effectively mean -o
> noalign), then you are going to need to modify
> xfs_update_alignment() and xfs_validate_new_dalign() to handle
> mp->m_dalign == 0 as a valid value instead of "sunit/swidth mount
> option not set, use existing superblock values".....
> 
> IOWs, there are deeper changes needed here than just setting a new
> flag to say "mount option was set" for it to function correctly and
> consistently as you intend. This is why I think we should just fix
> this situation automatically, and not require the user to manually
> override the bad values.
> 
> Thinking bigger picture, I'd like to see the mount options
> deprecated and new xfs_admin options added to change the values on a
> live, mounted filesystem. That way users who have issues like this
> don't need to unmount the filesystem to fix it, not to mention users
> who reshape online raid arrays and grow the filesystem can also
> change the filesystem geometry without needing to take the
> filesystem offline...

TBH I've always wondered, why not let root obtain the fs geometry,
modify whatever features it wants, and then push it back to the fs to
validate and update as necessary?  In theory you could even use this for
online filesystem upgrades, should we ever again allow users to do
that...

--D

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

Re: [PATCH 2/2] xfs: Bypass sb alignment checks when custom values are used

[Carlos Maiolino]

On Wed, Jun 03, 2020 at 09:12:35AM +1000, Dave Chinner wrote:
> On Tue, Jun 02, 2020 at 11:18:44AM +0200, Carlos Maiolino wrote:
> > Hi Dave.
> > 
> > On Tue, Jun 02, 2020 at 07:21:15AM +1000, Dave Chinner wrote:
> > > On Mon, Jun 01, 2020 at 04:01:53PM +0200, Carlos Maiolino wrote:
> > > > index 4df87546bd40..72dae95a5e4a 100644
> > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > @@ -360,19 +360,27 @@ xfs_validate_sb_common(
> > > >  		}
> > > >  	}
> > > >  
> > > > -	if (sbp->sb_unit) {
> > > > -		if (!xfs_sb_version_hasdalign(sbp) ||
> > > > -		    sbp->sb_unit > sbp->sb_width ||
> > > > -		    (sbp->sb_width % sbp->sb_unit) != 0) {
> > > > -			xfs_notice(mp, "SB stripe unit sanity check failed");
> > > > +	/*
> > > > +	 * Ignore superblock alignment checks if sunit/swidth mount options
> > > > +	 * were used or alignment turned off.
> > > > +	 * The custom alignment validation will happen later on xfs_mountfs()
> > > > +	 */
> > > > +	if (!(mp->m_flags & XFS_MOUNT_ALIGN) &&
> > > > +	    !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > > 
> > > mp->m_dalign tells us at this point if a user specified sunit as a
> > > mount option.  That's how xfs_fc_validate_params() determines the user
> > > specified a custom sunit, so there is no need for a new mount flag
> > > here to indicate that mp->m_dalign was set by the user....
> > 
> > At a first glance, I thought about it too, but, there is nothing preventing an
> > user to mount a filesystem passing sunit=0,swidth=0. So, this means we can't
> > really rely on the m_dalign/m_swidth values to check an user passed in (or not)
> > alignment values. Unless we first deny users to pass 0 values into it.
> 
> Sure we can. We do this sort of "was the mount option set" detection
> with m_logbufs and m_logbsize by initialising them to -1. Hence if
> they are set by mount options, they'll have a valid, in-range value
> instead of "-1".

Funny thing is I thought about "let's initialize to -1" and gave up because it
seemed ugly :)

> 
> That said, if you want users passing in sunit=0,swidth=0 to
> correctly override existing on-disk values (i.e. effectively mean -o
> noalign), then you are going to need to modify
> xfs_update_alignment() and xfs_validate_new_dalign() to handle
> mp->m_dalign == 0 as a valid value instead of "sunit/swidth mount
> option not set, use existing superblock values".....
> 
> IOWs, there are deeper changes needed here than just setting a new
> flag to say "mount option was set" for it to function correctly and
> consistently as you intend. This is why I think we should just fix
> this situation automatically, and not require the user to manually
> override the bad values.

Sure, I'm not opposed to fix things automatically, I just thought it wasn't an
acceptable solution, but looks like I'm wrong.

> 
> Thinking bigger picture, I'd like to see the mount options
> deprecated and new xfs_admin options added to change the values on a
> live, mounted filesystem.
I haven't been following this development. So, you meant geometry mount options
or mount options as a whole?

> That way users who have issues like this
> don't need to unmount the filesystem to fix it,

Sure, but it doesn't fix those filesystems which were already unmounted. But,
fixing it automatically during mount seem to cover this part.

cheers.

-- 
Carlos

Re: [PATCH 1/2] xfs: Fix xfs_mount sunit and swidth types

[Darrick J. Wong]

On Mon, Jun 01, 2020 at 04:01:52PM +0200, Carlos Maiolino wrote:
> The sunit (mp->m_dalign) and swidth (mp->m_swidth) are currently encoded
> as int in the xfs_mount structure, while it's treated as unsigned
> everywhere where it is used (saving some places also changed by this
> patch).
> 
> Change their type to uint32_t and fix some code using them as int.
> 
> This has been spotted by code inspection.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c   |  2 +-
>  fs/xfs/libxfs/xfs_ialloc.c |  2 +-
>  fs/xfs/libxfs/xfs_ialloc.h |  2 +-
>  fs/xfs/xfs_mount.c         |  4 ++--
>  fs/xfs/xfs_mount.h         |  4 ++--
>  fs/xfs/xfs_super.c         | 10 +++++-----
>  fs/xfs/xfs_trace.h         |  7 ++++---
>  7 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 667cdd0dfdf4..8b46f15cc414 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3477,7 +3477,7 @@ xfs_bmap_btalloc(
>  	int		isaligned;
>  	int		tryagain;
>  	int		error;
> -	int		stripe_align;
> +	uint32_t	stripe_align;

I might've used unsigned int for this, but that's mostly academic.

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

--D

>  
>  	ASSERT(ap->length);
>  	orig_offset = ap->offset;
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 7fcf62b324b0..75cb2dd78a7a 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -2913,7 +2913,7 @@ xfs_ialloc_setup_geometry(
>  xfs_ino_t
>  xfs_ialloc_calc_rootino(
>  	struct xfs_mount	*mp,
> -	int			sunit)
> +	uint32_t		sunit)
>  {
>  	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
>  	xfs_agblock_t		first_bno;
> diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
> index 72b3468b97b1..a992bcae5358 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.h
> +++ b/fs/xfs/libxfs/xfs_ialloc.h
> @@ -152,6 +152,6 @@ int xfs_inobt_insert_rec(struct xfs_btree_cur *cur, uint16_t holemask,
>  
>  int xfs_ialloc_cluster_alignment(struct xfs_mount *mp);
>  void xfs_ialloc_setup_geometry(struct xfs_mount *mp);
> -xfs_ino_t xfs_ialloc_calc_rootino(struct xfs_mount *mp, int sunit);
> +xfs_ino_t xfs_ialloc_calc_rootino(struct xfs_mount *mp, uint32_t sunit);
>  
>  #endif	/* __XFS_IALLOC_H__ */
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index d5dcf9869860..1b1861376854 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -368,7 +368,7 @@ xfs_readsb(
>  static inline int
>  xfs_check_new_dalign(
>  	struct xfs_mount	*mp,
> -	int			new_dalign,
> +	uint32_t		new_dalign,
>  	bool			*update_sb)
>  {
>  	struct xfs_sb		*sbp = &mp->m_sb;
> @@ -432,7 +432,7 @@ xfs_validate_new_dalign(
>  			mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
>  		} else {
>  			xfs_warn(mp,
> -		"alignment check failed: sunit(%d) less than bsize(%d)",
> +		"alignment check failed: sunit(%u) less than bsize(%d)",
>  				 mp->m_dalign, mp->m_sb.sb_blocksize);
>  			return -EINVAL;
>  		}
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 3725d25ad97e..6552473ab117 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -119,8 +119,8 @@ typedef struct xfs_mount {
>  	xfs_extlen_t		m_ag_prealloc_blocks; /* reserved ag blocks */
>  	uint			m_alloc_set_aside; /* space we can't use */
>  	uint			m_ag_max_usable; /* max space per AG */
> -	int			m_dalign;	/* stripe unit */
> -	int			m_swidth;	/* stripe width */
> +	uint32_t		m_dalign;	/* stripe unit */
> +	uint32_t		m_swidth;	/* stripe width */
>  	xfs_agnumber_t		m_maxagi;	/* highest inode alloc group */
>  	uint			m_allocsize_log;/* min write size log bytes */
>  	uint			m_allocsize_blocks; /* min write size blocks */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index fa58cb07c8fd..580072b19e8a 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -193,11 +193,11 @@ xfs_fs_show_options(
>  		seq_show_option(m, "rtdev", mp->m_rtname);
>  
>  	if (mp->m_dalign > 0)
> -		seq_printf(m, ",sunit=%d",
> -				(int)XFS_FSB_TO_BB(mp, mp->m_dalign));
> +		seq_printf(m, ",sunit=%u",
> +				XFS_FSB_TO_BB(mp, mp->m_dalign));
>  	if (mp->m_swidth > 0)
> -		seq_printf(m, ",swidth=%d",
> -				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
> +		seq_printf(m, ",swidth=%u",
> +				XFS_FSB_TO_BB(mp, mp->m_swidth));
>  
>  	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
>  		seq_puts(m, ",usrquota");
> @@ -1338,7 +1338,7 @@ xfs_fc_validate_params(
>  
>  	if (mp->m_dalign && (mp->m_swidth % mp->m_dalign != 0)) {
>  		xfs_warn(mp,
> -	"stripe width (%d) must be a multiple of the stripe unit (%d)",
> +	"stripe width (%u) must be a multiple of the stripe unit (%u)",
>  			mp->m_swidth, mp->m_dalign);
>  		return -EINVAL;
>  	}
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 460136628a79..ad0c10e01a73 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3585,11 +3585,12 @@ DEFINE_KMEM_EVENT(kmem_realloc);
>  DEFINE_KMEM_EVENT(kmem_zone_alloc);
>  
>  TRACE_EVENT(xfs_check_new_dalign,
> -	TP_PROTO(struct xfs_mount *mp, int new_dalign, xfs_ino_t calc_rootino),
> +	TP_PROTO(struct xfs_mount *mp, uint32_t new_dalign,
> +		 xfs_ino_t calc_rootino),
>  	TP_ARGS(mp, new_dalign, calc_rootino),
>  	TP_STRUCT__entry(
>  		__field(dev_t, dev)
> -		__field(int, new_dalign)
> +		__field(uint32_t, new_dalign)
>  		__field(xfs_ino_t, sb_rootino)
>  		__field(xfs_ino_t, calc_rootino)
>  	),
> @@ -3599,7 +3600,7 @@ TRACE_EVENT(xfs_check_new_dalign,
>  		__entry->sb_rootino = mp->m_sb.sb_rootino;
>  		__entry->calc_rootino = calc_rootino;
>  	),
> -	TP_printk("dev %d:%d new_dalign %d sb_rootino %llu calc_rootino %llu",
> +	TP_printk("dev %d:%d new_dalign %u sb_rootino %llu calc_rootino %llu",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  __entry->new_dalign, __entry->sb_rootino,
>  		  __entry->calc_rootino)
> -- 
> 2.26.2
>