Re: [PATCH] xfs: don't commit sunit/swidth updates to disk if that would cause repair failures

[Brian Foster <bfoster@redhat.com>]

On Thu, Dec 05, 2019 at 01:42:22PM -0800, Darrick J. Wong wrote:
> On Thu, Dec 05, 2019 at 09:36:18AM -0500, Brian Foster wrote:
> > On Wed, Dec 04, 2019 at 09:03:40AM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Alex Lyakas reported[1] that mounting an xfs filesystem with new sunit
> > > and swidth values could cause xfs_repair to fail loudly.  The problem
> > > here is that repair calculates the where mkfs should have allocated the
> > > root inode, based on the superblock geometry.  The allocation decisions
> > > depend on sunit, which means that we really can't go updating sunit if
> > > it would lead to a subsequent repair failure on an otherwise correct
> > > filesystem.
> > > 
> > > Port the computation code from xfs_repair and teach mount to avoid the
> > > ondisk update if it would cause problems for repair.  We allow the mount
> > > to proceed (and new allocations will reflect this new geometry) because
> > > we've never screened this kind of thing before.
> > > 
> > > [1] https://lore.kernel.org/linux-xfs/20191125130744.GA44777@bfoster/T/#m00f9594b511e076e2fcdd489d78bc30216d72a7d
> > > 
> > > Reported-by: Alex Lyakas <alex@zadara.com>
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > v2: compute the root inode location directly
> > > ---
> > >  fs/xfs/libxfs/xfs_ialloc.c |   81 ++++++++++++++++++++++++++++++++++++++++++++
> > >  fs/xfs/libxfs/xfs_ialloc.h |    1 +
> > >  fs/xfs/xfs_mount.c         |   51 ++++++++++++++++++----------
> > >  3 files changed, 115 insertions(+), 18 deletions(-)
> > > 
...
> > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > index fca65109cf24..a4eb3ae34a84 100644
> > > --- a/fs/xfs/xfs_mount.c
> > > +++ b/fs/xfs/xfs_mount.c
...
> > > @@ -398,28 +399,42 @@ xfs_update_alignment(xfs_mount_t *mp)
> > >  			}
> > >  		}
> > >  
> > > -		/*
> > > -		 * Update superblock with new values
> > > -		 * and log changes
> > > -		 */
> > > -		if (xfs_sb_version_hasdalign(sbp)) {
> > > -			if (sbp->sb_unit != mp->m_dalign) {
> > > -				sbp->sb_unit = mp->m_dalign;
> > > -				mp->m_update_sb = true;
> > > -			}
> > > -			if (sbp->sb_width != mp->m_swidth) {
> > > -				sbp->sb_width = mp->m_swidth;
> > > -				mp->m_update_sb = true;
> > > -			}
> > > -		} else {
> > > +		/* Update superblock with new values and log changes. */
> > > +		if (!xfs_sb_version_hasdalign(sbp)) {
> > >  			xfs_warn(mp,
> > >  	"cannot change alignment: superblock does not support data alignment");
> > >  			return -EINVAL;
> > >  		}
> > > +
> > > +		if (sbp->sb_unit == mp->m_dalign &&
> > > +		    sbp->sb_width == mp->m_swidth)
> > > +			return 0;
> > > +
> > > +		/*
> > > +		 * If the sunit/swidth change would move the precomputed root
> > > +		 * inode value, we must reject the ondisk change because repair
> > > +		 * will stumble over that.  However, we allow the mount to
> > > +		 * proceed because we never rejected this combination before.
> > > +		 */
> > > +		if (sbp->sb_rootino !=
> > > +		    xfs_ialloc_calc_rootino(mp, mp->m_dalign)) {
> > > +			xfs_warn(mp,
> > > +	"cannot change stripe alignment: would require moving root inode");
> > > +
> > 
> > FWIW, I read this error message as the mount option was ignored. I don't
> > much care whether we ignore the mount option or simply the on-disk
> > update, but the error could be a bit more clear in the latter case.
> 
> Ok, I'll add a message about how we're skipping the sb update.
> 
> > Also, what is the expected behavior for xfs_info in the latter
> > situation?
> 
> A previous revision of the patch had the ioctl feeding xfs_info using
> the incore values, but Dave objected so I dropped it.
> 

Ok, could you document the expected behavior for this new state in the
commit log so it's clear when looking back at it? I.e., xfs_info should
return superblock values, xfs_growfs should update based on superblock
values, etc.

Brian

> --D
> 
> > Brian
> > 
> > > +			/*
> > > +			 * XXX: Next time we add a new incompat feature, this
> > > +			 * should start returning -EINVAL.
> > > +			 */
> > > +			return 0;
> > > +		}
> > > +
> > > +		sbp->sb_unit = mp->m_dalign;
> > > +		sbp->sb_width = mp->m_swidth;
> > > +		mp->m_update_sb = true;
> > >  	} else if ((mp->m_flags & XFS_MOUNT_NOALIGN) != XFS_MOUNT_NOALIGN &&
> > >  		    xfs_sb_version_hasdalign(&mp->m_sb)) {
> > > -			mp->m_dalign = sbp->sb_unit;
> > > -			mp->m_swidth = sbp->sb_width;
> > > +		mp->m_dalign = sbp->sb_unit;
> > > +		mp->m_swidth = sbp->sb_width;
> > >  	}
> > >  
> > >  	return 0;
> > > 
> > 
>