On Fri, Dec 06, 2019 at 05:57:51AM -0500, Brian Foster wrote: > 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@xxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxx> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > --- > > > > 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. Yeah, I'll do that. --D > 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; > > > > > > > > > >
