On Fri, Mar 20, 2015 at 09:26:12PM -0500, Eric Sandeen wrote:
> On 3/19/15 10:07 AM, Brian Foster wrote:
> > On Tue, Mar 17, 2015 at 03:33:04PM -0500, Eric Sandeen wrote:
> >> Currently, the "ino_crc_ok" field on the io cursor reflects
> >> overall inode validity, not CRC correctness. Because it is
> >> only used when printing CRC validity, change it to reflect
> >> only that state - and update it whenever we re-write the
> >> inode (thus updating the CRC).
> >>
> >> In addition, when reading an inode, warn if the CRC is bad.
> >>
> >> Note, when specifying an inode which doesn't actually exist,
> >> this will claim corruption; I'm not sure if that's good or
> >> bad. Today, it already issues corruption errors on the way;
> >> this adds a new message as well:
> >>
> >> xfs_db> inode 129
> >> Metadata corruption detected at block 0x80/0x2000
> >> Metadata corruption detected at block 0x80/0x2000
> >> ...
> >> Metadata CRC error detected for ino 129
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> >> ---
> >
> > So is the objective here to simply give the field an explicit meaning?
> > E.g., indicate whether the crc is valid, irrespective of whether
> > something else might be wrong with the inode?
>
> Yeah, ino_crc_ok kinda indicates that the inode... crc ... is ok? :)
>
> Prior to this, it was only used in iocur_crc_valid(), which is called
> from fp_crc() to print unchecked/bad/correct/unknown for the crc.
>
> That's only used to print crc fields in the table-driven db stuff:
>
> { FLDT_CRC, "crc", fp_crc, "%#x (%s)", SI(bitsz(__uint32_t)),
> 0, NULL, NULL },
>
> so it seems to have a very specific meaning, and wrapping it up
> w/ the verifier didn't make sense.
>
> I had something more specific when I first sent the patch but that
> was yonks ago. :)
>
> >> db/inode.c | 7 ++++++-
> >> db/io.c | 4 +++-
> >> include/libxfs.h | 2 ++
> >> 3 files changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/db/inode.c b/db/inode.c
> >> index 24170ba..982acb7 100644
> >> --- a/db/inode.c
> >> +++ b/db/inode.c
> >> @@ -684,13 +684,18 @@ set_cur_inode(
> >> numblks, DB_RING_IGN, NULL);
> >> off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);
> >> dip = iocur_top->data;
> >> - iocur_top->ino_crc_ok = libxfs_dinode_verify(mp, ino, dip);
> >> + iocur_top->ino_crc_ok = libxfs_verify_cksum((char *)dip,
> >> + mp->m_sb.sb_inodesize,
> >> + XFS_DINODE_CRC_OFF);
> >
> > With this replaced, it doesn't look like anybody else will call
> > libxfs_dinode_verify (analogous to xfs_iread() in kernel). Is that
> > intentional? I guess the magic and version should be checked in the read
> > verifier, but there are a couple other checks in that helper as well.
>
> xfs_iread still calls xfs_dinode_verify, right?
>
Yeah, but I didn't see that being called in this path. My question was
basically if we would notice whether the uuid or i_ino value got
scribbled. The verifier looks like it only checks the magic and version
number. Thinking more about it, it probably doesn't matter. If some
field is wrong, chances are the crc is busted. I think the uuid is
actually a v5 only field as well. Finally, the actual ino check in
xfs_verify_dinode() only occurs for v5, so there's no change there
either. Nothing to see here, move along... ;)
> >> iocur_top->ino_buf = 1;
> >> iocur_top->ino = ino;
> >> iocur_top->mode = be16_to_cpu(dip->di_mode);
> >> if ((iocur_top->mode & S_IFMT) == S_IFDIR)
> >> iocur_top->dirino = ino;
> >>
> >> + if (xfs_sb_version_hascrc(&mp->m_sb) && !iocur_top->ino_crc_ok)
> >> + dbprintf(_("Metadata CRC error detected for ino %lld\n"), ino);
> >> +
> >
> > Hmm.. if we keep this, could we combine with the hunk above? I ask
> > simply because I'd rather see the _hascrc() check around the verify_cksum()
> > check as well, rather than verifying a cksum and ignoring it.
>
> ok, sure.
>
> > It's also a little confusing how this field is handled without crc
> > enabled. write_cur() looks like it sets it and calls
> > libxfs_dinode_calc_crc() blindly, which asserts that crc is enabled. I
> > guess we just don't print anything if crc=0, but it would be nice if the
> > flag was consistent.
>
> Hm, I can see that, but I don't know how we can test for the presence of
> crcs in write_cur. We don't have that info, AFAICT.
>
mp is a global. That's how it's resolved in set_cur_inode() as well.
That could also be a separate patch, btw. ;) This seems Ok to me with
the minor cleanup above, and we should probably get these fixes in at
some point. :)
Brian
> -Eric
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
