[Cluster-devel] [PATCH 3/3] gfs2: Add a crc field to resource group headers

[Andrew Price]

On 07/12/17 12:02, Steven Whitehouse wrote:
> On 07/12/17 11:52, Andrew Price wrote:
>> Add the rg_crc field to store a crc32 of the gfs2_rgrp structure. This
>> allows us to check resource group headers' integrity and removes the
>> requirement to check them against the rindex entries in fsck. If this
>> field is found to be zero, it should be ignored (or updated with an
>> accurate value).
>>
>> Signed-off-by: Andrew Price <anprice at redhat.com>
>> ---
>>   fs/gfs2/rgrp.c                   | 5 +++++
>>   include/uapi/linux/gfs2_ondisk.h | 3 ++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
>> index 9a6fa21a2f5a..80e020fc117f 100644
>> --- a/fs/gfs2/rgrp.c
>> +++ b/fs/gfs2/rgrp.c
>> @@ -34,6 +34,7 @@
>>   #include "log.h"
>>   #include "inode.h"
>>   #include "trace_gfs2.h"
>> +#include "dir.h"
>>   #define BFITNOENT ((u32)~0)
>>   #define NO_BLOCK ((u64)~0)
>> @@ -1047,6 +1048,7 @@ static void gfs2_rgrp_out(struct gfs2_rgrpd 
>> *rgd, void *buf)
>>   {
>>       struct gfs2_rgrpd *next = gfs2_rgrpd_get_next(rgd);
>>       struct gfs2_rgrp *str = buf;
>> +    u32 crc;
>>       str->rg_flags = cpu_to_be32(rgd->rd_flags & ~GFS2_RDF_MASK);
>>       str->rg_free = cpu_to_be32(rgd->rd_free);
>> @@ -1057,6 +1059,9 @@ static void gfs2_rgrp_out(struct gfs2_rgrpd 
>> *rgd, void *buf)
>>       str->rg_data0 = cpu_to_be64(rgd->rd_data0);
>>       str->rg_data = cpu_to_be64(rgd->rd_data);
>>       str->rg_bitbytes = cpu_to_be64(rgd->rd_bitbytes);
>> +    str->rg_crc = 0;
>> +    crc = gfs2_disk_hash(buf, sizeof(struct gfs2_rgrp));
>> +    str->rg_crc = cpu_to_be32(crc);
>>       memset(&str->rg_reserved, 0, sizeof(str->rg_reserved));
>>   }
>> diff --git a/include/uapi/linux/gfs2_ondisk.h 
>> b/include/uapi/linux/gfs2_ondisk.h
>> index 648e0cbca574..09f0920f07e9 100644
>> --- a/include/uapi/linux/gfs2_ondisk.h
>> +++ b/include/uapi/linux/gfs2_ondisk.h
>> @@ -197,8 +197,9 @@ struct gfs2_rgrp {
>>       __be64 rg_data0;     /* First data location */
>>       __be32 rg_data;      /* Number of data blocks in rgrp */
>>       __be32 rg_bitbytes;  /* Number of bytes in data bitmaps */
>> +    __be32 rg_crc;       /* crc32 of the structure with this field 0 */
> In this case because rg_crc is followed by another structure element, if 
> that element is 64 bit aligned, then there might be a gap here. Worth 
> checking that the overall structure size has not changed, and might be 
> worth adding an explicit 32 bit pad field to pair up with the crc too, 
> and reducing the reserved field by another 4 bytes.

sizeof(struct gfs2_rgrp) is 128 before and after the change, which I 
would expect as rg_reserved doesn't require 64-bit alignment. I can add 
a padding field as documentation though.

>> -    __u8 rg_reserved[64]; /* Several fields from gfs1 now reserved */
>> +    __u8 rg_reserved[60]; /* Several fields from gfs1 now reserved */
>>   };
>>   /*
> 
> Otherwise the patches look good, and this should be a major step forward 
> for fsck in due course,

Thanks for the quick review!

Andy