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

[Andrew Price <anprice@redhat.com>]

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