From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: xfs <linux-xfs@vger.kernel.org>, Alex Lyakas <alex@zadara.com>,
Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH] xfs: don't commit sunit/swidth updates to disk if that would cause repair failures
Date: Fri, 6 Dec 2019 05:57:51 -0500 [thread overview]
Message-ID: <20191206105751.GA55746@bfoster> (raw)
In-Reply-To: <20191205214222.GE13260@magnolia>
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;
> > >
> >
>
next prev parent reply other threads:[~2019-12-06 10:57 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-04 17:03 [PATCH] xfs: don't commit sunit/swidth updates to disk if that would cause repair failures Darrick J. Wong
2019-12-05 14:36 ` Brian Foster
2019-12-05 21:42 ` Darrick J. Wong
2019-12-06 10:57 ` Brian Foster [this message]
2019-12-13 16:10 ` Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191206105751.GA55746@bfoster \
--to=bfoster@redhat.com \
--cc=alex@zadara.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).