Re: [PATCH] xfs_metadump: tag metadump image with attribute flags

[Dave Chinner <david@fromorbit.com>]

On Tue, May 23, 2017 at 10:29:33PM -0500, Eric Sandeen wrote:
> On 5/23/17 10:26 PM, Dave Chinner wrote:
> > On Tue, May 23, 2017 at 10:47:50AM -0500, Eric Sandeen wrote:
> >> On 5/23/17 10:46 AM, Darrick J. Wong wrote:
> >>> On Mon, May 22, 2017 at 07:33:35PM -0500, Eric Sandeen wrote:
> >>>> After the long discussion about warning the user and/or consumer
> >>>> of xfs_metadumps about dirty logs, it crossed my mind that we
> >>>> could use the reserved slot in the metadump header to tag the
> >>>> file with attributes, so the consumer of the metadump knows how
> >>>> it was created.
> >>>>
> >>>> This patch adds 3 flags to describe the metadump: dirty log,
> >>>> obfuscated, and full blocks (unused portions of metadata blocks
> >>>> are not zeroed out).
> >>>>
> >>>> It then adds a new option to xfs_mdrestore, "-i" to show info,
> >>>> which can be used with or without a target file:
> >>
> >>
> >>>> +/* mb_flags */
> >>>> +#define XFS_METADUMP_OBFUSCATED	(1 << 0)
> >>>> +#define XFS_METADUMP_FULLBLOCKS	(1 << 1)
> >>>> +#define XFS_METADUMP_DIRTYLOG	(1 << 2)
> >>>
> >>> If we always wrote zero for mb_reserved previously, how do we
> >>> distinguish between a non-obfuscated partial-block clean-log metadump
> >>> and an old metadump?  Do we care?
> >>
> >> Oh right.  ;)
> >>
> >>> I imagine metadumps are fairly transitory in nature, so it might not be
> >>> a big deal, and probably not worth burning 1/8 of our flag-space over
> >>> since AFAICT we don't use the(se) flags for any serious behavioral
> >>> changes.
> >>
> >> OTOH, how many flags would we possibly have?  I'm ok with adding a
> >> "we have flags" flag, too.
> > 
> > We can't extend the metadump header in a backwards compatible way
> > unless mdrestore already rejects metadumps with non-zero mb_reserved
> > fields....
> 
> There's no action taken based on the contents; it's information only,
> and essentially optional.  It doesn't affect the primary functionality of
> mdrestore in any way.
> 
> The reserved field has always been zeroed before; if newer mdrestore finds
> all zeros, it can say "no info available."
> 
> If newer mdrestore finds a "we have flags" flag, it can report information
> contained there.
> 
> What hole am I missing?

That I read "we have flags flag" as a "we have another flags field"
flag. Which can't be done because the block index is packed hard
against the struct xfs_metablock header.

typedef struct xfs_metablock {
        __be32          mb_magic;
        __be16          mb_count;
        __uint8_t       mb_blocklog;
        __uint8_t       mb_reserved;
        /* followed by an array of xfs_daddr_t */      <<<<<<<<<
} xfs_metablock_t;

And:

block_index = (__be64 *)((char *)metablock + sizeof(xfs_metablock_t));

So, if you change the size of the struct xfs_metablock - which would
need to be done to version the structure and add a specific flags
field - then it changes the location of the first block index in the
metadump header. Older mdrestore binaries will not understand this
and do the wrong thing....

As such, even if we don't change the size of the header, the flags
can only be informational. They can't indicate a change in metadata
format or behaviour such feature flags requires older mdrestorei
binaries to reject flags they don't understand. In that case,
calling the field "mb_info" might be more appropriate....

Cheers,

Dave.
> 
> Thanks,
> -Eric
> 

-- 
Dave Chinner
david@fromorbit.com