Re: [PATCH 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid

[Eric Sandeen <sandeen@sandeen.net>]

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@redhat.com>
>> ---
> 
> 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?

>>  	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.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs