| Message ID | 20200601140153.200864-3-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Bypass sb geometry if custom alignment is supplied on mount | expand |
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.
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
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.
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. >
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
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.
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
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.
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;
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(-)