gfs2: Add new resource group header fields

Hi,

----- Original Message -----

| Add a new rg_skip field to struct gfs2_rgrp, replacing __pad. The
| -	str->__pad = cpu_to_be32(0);
| +	if (next != NULL && next->rd_addr > rgd->rd_addr)
| +		str->rg_skip = cpu_to_be32(next->rd_addr - rgd->rd_addr);

Without having looked at the original:

Shouldn't we still initialize it to 0 or put in an else clause? I'm concerned that we might end up with an uninitialized random value at the last rgrp (which may not be a big concern until gfs2_grow or fsck time).

Hi,

----- Original Message -----

| Add rg_data0, rg_data and rg_bitbytes to struct gfs2_rgrp. The fields
| are identical to their counterparts in struct gfs2_rindex and are
| intended to reduce the use of the rindex. For now the fields are only
| written back as the in-memory equivalents in struct gfs2_rgrpd are set
| using values from the rindex. However, they are needed at this point so
| that userspace can make use of them, allowing a migration away from the
| rindex over time.
|
| The new fields take up previously reserved space which was explicitly
| zeroed on write so, in clusters with mixed kernels, these fields could
| get zeroed after being set and this should not be treated as an error.
|
| Signed-off-by: Andrew Price <[email protected]>
(snip)
| @@ -1053,6 +1054,10 @@ static void gfs2_rgrp_out(struct gfs2_rgrpd *rgd, void
| *buf)
|  	if (next != NULL && next->rd_addr > rgd->rd_addr)
|  		str->rg_skip = cpu_to_be32(next->rd_addr - rgd->rd_addr);
|  	str->rg_igeneration = cpu_to_be64(rgd->rd_igeneration);
| +	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);

These last two should be cpu_to_be32() not 64()

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 <[email protected]>

---
  fs/gfs2/rgrp.c                   | 5 +++++
  include/uapi/linux/gfs2_ondisk.h | 3 ++-
  2 files changed, 7 insertions(+), 1 deletion(-)

fs/gfs2/rgrp.c +5 -0

index 9a6fa21a2f5a..80e020fc117f 100644
@@ -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));
  }

include/uapi/linux/gfs2_ondisk.h +0 -0

index 648e0cbca574..09f0920f07e9 100644

Quoted 48 lines · Andrew Price

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 <[email protected]>

---
? fs/gfs2/rgrp.c?????????????????? | 5 +++++
? include/uapi/linux/gfs2_ondisk.h | 3 ++-
? 2 files changed, 7 insertions(+), 1 deletion(-)

fs/gfs2/rgrp.c +0 -0

index 9a6fa21a2f5a..80e020fc117f 100644
@@ -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)

fs/gfs2/rgrp.c +4 -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));
 ? }

include/uapi/linux/gfs2_ondisk.h +0 -0

index 648e0cbca574..09f0920f07e9 100644
@@ -197,8 +197,9 @@ struct gfs2_rgrp {

????? __be64 rg_data0;???? /* First data location */

Quoted 3 lines

????? __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.

Quoted 4 lines

-??? __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,

Quoted 4 lines · Steven Whitehouse

Quoted 4 lines

include/uapi/linux/gfs2_ondisk.h +0 -0

@@ -197,8 +197,9 @@ struct gfs2_rgrp {

????? __be64 rg_data0;???? /* First data location */

Quoted 8 lines

Quoted 3 lines

????? __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.