libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

PATCH New Andrew Price 2 participants 3 messages Oct 12, 2015

Share & tools

Andrew Price

author

2015-10-12 15:22 UTC #0 lore raw

PATCH 1/1 libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

lgfs2_rgrps_plan() previously returned one of the rgrp sizes specifically so that gfs2_grow could check it. This check was also done inside the function so the return value would always be 0 when the rgrp size was too small anyway (really this means that there's not enough space in which to place sensibly sized rgrps). As returning 0 is sufficient to pass back the same information, we can return the resource group count from lgfs2_rgrps_plan() instead, as that is a more useful value in general.

Signed-off-by Andrew Price <[email protected]>

---
 gfs2/libgfs2/rgrp.c   | 7 +++----
 gfs2/mkfs/main_grow.c | 8 +++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

gfs2/libgfs2/rgrp.c +3 -4

index cf4385a..b14d210 100644
@@ -302,8 +302,8 @@ uint32_t lgfs2_rgrp_align_len(const lgfs2_rgrps_t rgs, uint32_t len)
  * rgs: The resource groups descriptor
  * space: The number of remaining blocks to be allocated
  * tgtsize: The target resource group size in blocks
- * Returns the larger of the calculated resource group sizes, in blocks, or 0
- * if the smaller would be less than GFS2_MIN_RGSIZE.
+ * Returns the number of resource groups planned to fit in the given space, or
+ * 0 if the smallest resource group would be smaller than GFS2_MIN_RGSIZE.
  */
 uint32_t lgfs2_rgrps_plan(const lgfs2_rgrps_t rgs, uint64_t space, uint32_t tgtsize)
 {

@@ -352,11 +352,10 @@ uint32_t lgfs2_rgrps_plan(const lgfs2_rgrps_t rgs, uint64_t space, uint32_t tgts
 	/* Once we've reached this point,
 	   (plan[0].num * plan[0].len) + (plan[1].num * plan[1].len)
 	   will be less than one adjustment smaller than 'space'.  */
-
 	if (rgs->plan[0].len < minlen)
 		return 0;
 
-	return rgs->plan[0].len;
+	return rgs->plan[0].num + rgs->plan[1].num;
 }
 
 /**

gfs2/mkfs/main_grow.c +3 -5

index 860c319..173466c 100644
@@ -32,7 +32,6 @@
 static uint64_t override_device_size = 0;
 static int test = 0;
 static uint64_t fssize = 0, fsgrowth;
-static unsigned int rgsize = 0;
 int print_level = MSG_NOTICE;
 
 extern int create_new_inode(struct gfs2_sbd *sdp);

@@ -315,7 +314,6 @@ static void print_info(struct gfs2_sbd *sdp, char *device, char *mnt_path)
 	log_notice("FS: %-25s%s\n", _("Device:"), device);
 	log_notice("FS: %-25s%llu (0x%llx)\n", _("Size:"),
 		   (unsigned long long)fssize, (unsigned long long)fssize);
-	log_notice("FS: %-25s%u (0x%x)\n", _("New resource group size:"), rgsize, rgsize);
 	log_notice("DEV: %-24s%llu (0x%llx)\n", _("Length:"),
 		   (unsigned long long)sdp->device.length,
 		   (unsigned long long)sdp->device.length);

@@ -359,7 +357,7 @@ int main(int argc, char *argv[])
 	sdp->qcsize = GFS2_DEFAULT_QCSIZE;
 	sdp->md.journals = 1;
 	decode_arguments(argc, argv, sdp);
-	
+
 	for(; (argc - optind) > 0; optind++) {
 		struct metafs mfs = {0};
 		struct mntent *mnt;

@@ -444,8 +442,8 @@ int main(int argc, char *argv[])
 			goto out;
 		}
 		fsgrowth = (sdp->device.length - fssize);
-		rgsize = lgfs2_rgrps_plan(rgs, fsgrowth, ((GFS2_MAX_RGSIZE << 20) / sdp->bsize));
-		if (rgsize < ((GFS2_MIN_RGSIZE << 20) / sdp->bsize)) {
+		rgcount = lgfs2_rgrps_plan(rgs, fsgrowth, ((GFS2_MAX_RGSIZE << 20) / sdp->bsize));
+		if (rgcount == 0) {
 			log_err( _("The calculated resource group size is too small.\n"));
 			log_err( _("%s has not grown.\n"), argv[optind]);
 			error = -1;

Signature · 1 line

2.4.3

Bob Peterson

2015-10-12 15:47 UTC #1 lore raw

PATCH 1/1 libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

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

Quoted 36 lines

lgfs2_rgrps_plan() previously returned one of the rgrp sizes specifically so that gfs2_grow could check it. This check was also done inside the function so the return value would always be 0 when the rgrp size was too small anyway (really this means that there's not enough space in which to place sensibly sized rgrps). As returning 0 is sufficient to pass back the same information, we can return the resource group count from lgfs2_rgrps_plan() instead, as that is a more useful value in general.
Signed-off-by Andrew Price <[email protected]>
---
 gfs2/libgfs2/rgrp.c   | 7 +++----
 gfs2/mkfs/main_grow.c | 8 +++-----
 2 files changed, 6 insertions(+), 9 deletions(-)
gfs2/libgfs2/rgrp.c +0 -1
index cf4385a..b14d210 100644
@@ -302,8 +302,8 @@ uint32_t lgfs2_rgrp_align_len(const lgfs2_rgrps_t rgs,
 uint32_t len)
  * rgs: The resource groups descriptor
  * space: The number of remaining blocks to be allocated
  * tgtsize: The target resource group size in blocks
- * Returns the larger of the calculated resource group sizes, in blocks, or
0
- * if the smaller would be less than GFS2_MIN_RGSIZE.
+ * Returns the number of resource groups planned to fit in the given space, or
+ * 0 if the smallest resource group would be smaller than GFS2_MIN_RGSIZE. */
uint32_t lgfs2_rgrps_plan(const lgfs2_rgrps_t rgs, uint64_t space, uint32_t tgtsize)
{
gfs2/libgfs2/rgrp.c +0 -0
@@ -352,11 +352,10 @@ uint32_t lgfs2_rgrps_plan(const lgfs2_rgrps_t rgs,
uint64_t space, uint32_t tgts

gfs2/libgfs2/rgrp.c uint32_t lgfs2_rgrps_plan(const lgfs2_rgrps_t rgs, uint64_t space, uint32_t tgts · line 358

Quoted 9 lines

gfs2/libgfs2/rgrp.c +1 -2

 	/* Once we've reached this point,
 	   (plan[0].num * plan[0].len) + (plan[1].num * plan[1].len)
 	   will be less than one adjustment smaller than 'space'.  */
-
 	if (rgs->plan[0].len < minlen)
 		return 0;
 
-	return rgs->plan[0].len;
+	return rgs->plan[0].num + rgs->plan[1].num;

Hi Andy,

Does this all work with a tiny file system that only has one resource group? I remember trying to code similar things and Abhi kept having to fix it because of problems with single-rgrp file systems. Bear in mind, I haven't looked at the rest of this function in detail, so I'm speaking somewhat out of ignorance.

Regards,

Bob Peterson
Red Hat File Systems

Andrew Price

author

2015-10-12 17:14 UTC #2 lore raw

PATCH 1/1 libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

Hi Bob,

Quoted 10 lines · Bob Peterson

----- Original Message -----
Quoted 9 lines
lgfs2_rgrps_plan() previously returned one of the rgrp sizes specifically so that gfs2_grow could check it. This check was also done inside the function so the return value would always be 0 when the rgrp size was too small anyway (really this means that there's not enough space in which to place sensibly sized rgrps). As returning 0 is sufficient to pass back the same information, we can return the resource group count from lgfs2_rgrps_plan() instead, as that is a more useful value in general.

Quoted 7 lines

Hi Andy,
Does this all work with a tiny file system that only has one resource group? I remember trying to code similar things and Abhi kept having to fix it because of problems with single-rgrp file systems. Bear in mind, I haven't looked at the rest of this function in detail, so I'm speaking somewhat out of ignorance.

I'm not sure which problems you're referring to to test for them but this patch is essentially cosmetic so it doesn't change the behaviour of mkfs.gfs2 nor gfs2_grow. I have done various tests with small file systems since the lgfs2_rgrp* functions were introduced though and they've held up well (or have been fixed), and those functions are well-covered by the test suite.

That said, to double-check I've created a 9M fs (minimum journal size is 8M) and ran a successful mount-fill-umount-fsck-mount-grow-umount-fsck test on it. The initial resource groups were obviously tiny (ri_length =
1) but the ones gfs2_grow added were much larger, which is expected as lgfs2_rgrps_plan() doesn't use the existing rgrps' sizes in its calculations.

Cheers,
Andy