[Cluster-devel] [GFS2 PATCH] GFS2: Introduce new gfs2_log_header_v2

[Steven Whitehouse]

Hi,


On 04/12/17 15:59, Bob Peterson wrote:
> Hi,
>
> ----- Original Message -----
> | On 01/12/17 19:34, Bob Peterson wrote:
> | > Hi,
> | >
> | > This patch adds a new structure called gfs2_log_header_v2 which is
> | > used to store expanded fields into previously unused areas of the
> | > log headers. Some of these are used for debug purposes so we can
> | > backtrack when problems occur. Others are reserved for future
> | > expansion.
> | >
> | > This is based on a prototype patch from Steve Whitehouse.
> (snip)
> | Where has setting the CRC gone?  That appears to be missing from this patch.
>
> Well, I'm not convinced we really need another crc in this first patch.
> The original part of the log header already has a crc32 field.
> The new fields are all just for debugging until we get to the patches
> that take advantage of the local statfs and quota fields (rather than
> separate inodes), in which case keeping a crc may be a good idea to
> ensure their integrity. But if (or when) we do that, we should also add
> code to check its integrity during recovery. Setting the crc just seemed
> out of the scope of this patch, but I guess it's not that big of a deal;
> I have no problem adding it back in on the assumption that we'll
> eventually get to that point.
We can't just leave those fields not set. lh_addr and lh_crc both need 
setting, otherwise how is fsck supposed to detect these blocks? The 
whole reason that it has taken so long to get this in, was down to the 
performance testing to ensure that the checksum doesn't make a big 
difference here. I assume that this is the case?
>
> (snip)
> | > +	__be32 lh_pad;
> | Why is there padding here? There is nothing following it.
>
> Yeah, I fully expected to expand the structure with some more
> fields. So I was going to use it, but didn't quite get there. So it was
> kind of just a reminder to myself: I wanted to preempt potential
> compiler-padding during future expansion.
>
> Bob
Thats ok, we can expand it without any issue since there is nothing that 
follows this structure. We don't need any padding for that.

We do need to make sure that we write the new header from recovery too. 
We also could perhaps find a way of writing out the actual location of 
the journal extent into which we are writing, i.e. physical block start 
& length. That would be very handy for fsck, but would have to be added 
at the time we write the header, as per lh_addr. In that case we'd have 
to get gfs2_log_bmap() to return the extent info, not just the block for 
the header,

Steve.