On Thu, May 12, 2016 at 06:03:15PM -0500, Eric Sandeen wrote:
> On 5/12/16 5:35 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Currently we can't write corrupt structures with valid CRCs on v5
> > filesystems via xfs_db. TO emulate certain types of corruption
> > result from software bugs in the kernel code, we need this
> > capability to set up the corrupted state. i.e. corrupt state with a
> > valid CRC needs to appear on disk.
> >
> > This requires us to avoid running the verifier that would otherwise
> > prevent writing corrupt state to disk. To enable this, add the CRC
> > offset to the type table for different buffers and add a new flag to
> > the write command to trigger running a CRC calculation base don this
> > type table. We can then insert the calculated value into the correct
> > location in the buffer...
> >
> > Because some objects are not directly buffer based, we can't easily
> > do this CRC trick. Those object types will be marked as
> > TYP_NO_CRC_OFF, and as a result will emit an error such as:
>
> Using "TYP_NO_CRC_OFF" seems a little weird from a naming perspective;
> it's not really a TYP_* is it? Its opposite is things like
> XFS_AGI_CRC_OFF; NO_FIXED_CRC_OFF might be better to not confuse it
> with the TYP_ on-disk types? Just a thought.
I just preficed it like that because it's something specific to the
type table. From that perspecitive, TYP_NO_CRC_RECALC might make
more sense. i.e. "this type cannot recalculate CRCs".
[...]
> > argc -= optind;
> > argv += optind;
> >
> > - if (iocur_top->bp->b_ops && corrupt) {
> > - /* Temporarily remove write verifier to write bad data */
> > - stashed_ops = iocur_top->bp->b_ops;
> > - nowrite_ops.verify_read = stashed_ops->verify_read;
> > + /* If we don't have to juggle verifiers, then just issue the write */
>
> This is a little confusing - we know what juggling verifiers means but
> future readers may not have that fresh in mind. ;)
>
> /* No verifier, or standard verifier paths; just write it out and return */
Sure.
> > + if (!iocur_top->bp->b_ops ||
> > + !(corrupt || invalid_data)) {
> > + (*pf)(DB_WRITE, cur_typ->fields, argc, argv);
> > + return 0;
> > + }
> > +
> > +
> > + /* Temporarily remove write verifier to write bad data */
> > + stashed_ops = iocur_top->bp->b_ops;
> > + nowrite_ops.verify_read = stashed_ops->verify_read;
> > + iocur_top->bp->b_ops = &nowrite_ops;
>
> I'm regretting my name choice of "nowrite_ops" ...
I can rename it to "local_ops"...
> > +
> > + if (corrupt) {
> > nowrite_ops.verify_write = xfs_dummy_verify;
> > - iocur_top->bp->b_ops = &nowrite_ops;
> > - dbprintf(_("Allowing write of corrupted data\n"));
> > + dbprintf(_("Allowing write of corrupted data and bad CRC\n"));
> > + } else {
>
> Maybe a helpful/redundant comment about /* invalid_data */ alongside } else {
> ?
I though the dbprintf() documented it well enough? maybe move that
to the top of each branch?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|