| Message ID | 157982507752.2765631.16955377241063712365.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs_repair: do not trash valid root dirs | expand |
On 1/23/20 6:17 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > If sb_rootino doesn't point to where we think mkfs should have allocated > the root directory, check to see if the alleged root directory actually > looks like a root directory. If so, we'll let it live because someone > could have changed sunit since formatting time, and that changes the > root directory inode estimate. I forget, is there an fstest for this? > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> ... > @@ -438,6 +469,20 @@ calc_mkfs( > > rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit); > > + /* > + * If the root inode isn't where we think it is, check its plausibility > + * as a root directory. It's possible that somebody changed sunit > + * since the filesystem was created, which can change the value of the > + * above computation. Don't blow up the root directory if this is the > + * case. > + */ > + if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) { > + do_warn( > +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"), > + mp->m_sb.sb_rootino, rootino); what would a user do with this warning? Is there any value in emitting it? Otherwise this looks good. > + rootino = mp->m_sb.sb_rootino; > + } > + > ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino, > _("root")); > ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1, >
On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote: > On 1/23/20 6:17 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > If sb_rootino doesn't point to where we think mkfs should have allocated > > the root directory, check to see if the alleged root directory actually > > looks like a root directory. If so, we'll let it live because someone > > could have changed sunit since formatting time, and that changes the > > root directory inode estimate. > > I forget, is there an fstest for this? https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/ > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > ... > > > @@ -438,6 +469,20 @@ calc_mkfs( > > > > rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit); > > > > + /* > > + * If the root inode isn't where we think it is, check its plausibility > > + * as a root directory. It's possible that somebody changed sunit > > + * since the filesystem was created, which can change the value of the > > + * above computation. Don't blow up the root directory if this is the > > + * case. > > + */ > > + if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) { > > + do_warn( > > +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"), > > + mp->m_sb.sb_rootino, rootino); > > what would a user do with this warning? Is there any value in emitting it? > > Otherwise this looks good. I dunno -- on the one hand, I understand that nobody wants to deal with the support calls that will erupt from that message. On the other hand, it's an indication that this filesystem isn't /quite/ the way we expected it to be, and that would be a helpful hint if you were debugging some other weird problem with an xfs filesystem. What if this were a do_log()? --D > > > > + rootino = mp->m_sb.sb_rootino; > > + } > > + > > ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino, > > _("root")); > > ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1, > >
On 1/30/20 2:34 PM, Darrick J. Wong wrote: > On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote: >> On 1/23/20 6:17 PM, Darrick J. Wong wrote: >>> From: Darrick J. Wong <darrick.wong@oracle.com> >>> >>> If sb_rootino doesn't point to where we think mkfs should have allocated >>> the root directory, check to see if the alleged root directory actually >>> looks like a root directory. If so, we'll let it live because someone >>> could have changed sunit since formatting time, and that changes the >>> root directory inode estimate. >> >> I forget, is there an fstest for this? > > https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/ of course :) ... >>> + if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) { >>> + do_warn( >>> +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"), >>> + mp->m_sb.sb_rootino, rootino); >> >> what would a user do with this warning? Is there any value in emitting it? >> >> Otherwise this looks good. > > I dunno -- on the one hand, I understand that nobody wants to deal with > the support calls that will erupt from that message. On the other hand, > it's an indication that this filesystem isn't /quite/ the way we > expected it to be, and that would be a helpful hint if you were > debugging some other weird problem with an xfs filesystem. > > What if this were a do_log()? how about something that's less indicative of a problem and more informational, "sb root inode validated in unaligned location possibly due to sunit change" -Eric
On Thu, Jan 30, 2020 at 02:41:29PM -0600, Eric Sandeen wrote: > On 1/30/20 2:34 PM, Darrick J. Wong wrote: > > On Thu, Jan 30, 2020 at 02:18:52PM -0600, Eric Sandeen wrote: > >> On 1/23/20 6:17 PM, Darrick J. Wong wrote: > >>> From: Darrick J. Wong <darrick.wong@oracle.com> > >>> > >>> If sb_rootino doesn't point to where we think mkfs should have allocated > >>> the root directory, check to see if the alleged root directory actually > >>> looks like a root directory. If so, we'll let it live because someone > >>> could have changed sunit since formatting time, and that changes the > >>> root directory inode estimate. > >> > >> I forget, is there an fstest for this? > > > > https://lore.kernel.org/linux-xfs/20191218041831.GK12765@magnolia/ > > of course :) :D > ... > > >>> + if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) { > >>> + do_warn( > >>> +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"), > >>> + mp->m_sb.sb_rootino, rootino); > >> > >> what would a user do with this warning? Is there any value in emitting it? > >> > >> Otherwise this looks good. > > > > I dunno -- on the one hand, I understand that nobody wants to deal with > > the support calls that will erupt from that message. On the other hand, > > it's an indication that this filesystem isn't /quite/ the way we > > expected it to be, and that would be a helpful hint if you were > > debugging some other weird problem with an xfs filesystem. > > > > What if this were a do_log()? > > how about something that's less indicative of a problem and more informational, > > "sb root inode validated in unaligned location possibly due to sunit change" Sounds good to me. --D > -Eric
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c index 53b04dae..372616c4 100644 --- a/repair/xfs_repair.c +++ b/repair/xfs_repair.c @@ -426,6 +426,37 @@ _("would reset superblock %s inode pointer to %"PRIu64"\n"), *ino = expected_ino; } +/* Does the root directory inode look like a plausible root directory? */ +static bool +has_plausible_rootdir( + struct xfs_mount *mp) +{ + struct xfs_inode *ip; + xfs_ino_t ino; + int error; + bool ret = false; + + error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip, + &xfs_default_ifork_ops); + if (error) + goto out; + if (!S_ISDIR(VFS_I(ip)->i_mode)) + goto out_rele; + + error = -libxfs_dir_lookup(NULL, ip, &xfs_name_dotdot, &ino, NULL); + if (error) + goto out_rele; + + /* The root directory '..' entry points to the directory. */ + if (ino == mp->m_sb.sb_rootino) + ret = true; + +out_rele: + libxfs_irele(ip); +out: + return ret; +} + /* * Make sure that the first 3 inodes in the filesystem are the root directory, * the realtime bitmap, and the realtime summary, in that order. @@ -438,6 +469,20 @@ calc_mkfs( rootino = libxfs_ialloc_calc_rootino(mp, mp->m_sb.sb_unit); + /* + * If the root inode isn't where we think it is, check its plausibility + * as a root directory. It's possible that somebody changed sunit + * since the filesystem was created, which can change the value of the + * above computation. Don't blow up the root directory if this is the + * case. + */ + if (mp->m_sb.sb_rootino != rootino && has_plausible_rootdir(mp)) { + do_warn( +_("sb root inode value %" PRIu64 " inconsistent with alignment (expected %"PRIu64")\n"), + mp->m_sb.sb_rootino, rootino); + rootino = mp->m_sb.sb_rootino; + } + ensure_fixed_ino(&mp->m_sb.sb_rootino, rootino, _("root")); ensure_fixed_ino(&mp->m_sb.sb_rbmino, rootino + 1,