[Cluster-devel] [PATCH] libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

[Cluster-devel] [PATCH] libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

[Andrew Price]

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 <anprice@redhat.com>
---
 gfs2/libgfs2/rgrp.c   | 7 +++----
 gfs2/mkfs/main_grow.c | 8 +++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/gfs2/libgfs2/rgrp.c b/gfs2/libgfs2/rgrp.c
index cf4385a..b14d210 100644
--- a/gfs2/libgfs2/rgrp.c
+++ b/gfs2/libgfs2/rgrp.c
@@ -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;
 }
 
 /**
diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
index 860c319..173466c 100644
--- a/gfs2/mkfs/main_grow.c
+++ b/gfs2/mkfs/main_grow.c
@@ -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;
-- 
2.4.3

[Cluster-devel] [PATCH] libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

[Bob Peterson]

----- Original Message -----
> 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 <anprice@redhat.com>
> ---
>  gfs2/libgfs2/rgrp.c   | 7 +++----
>  gfs2/mkfs/main_grow.c | 8 +++-----
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/gfs2/libgfs2/rgrp.c b/gfs2/libgfs2/rgrp.c
> index cf4385a..b14d210 100644
> --- a/gfs2/libgfs2/rgrp.c
> +++ b/gfs2/libgfs2/rgrp.c
> @@ -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;

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

[Cluster-devel] [PATCH] libgfs2: Return the rgrp count in lgfs2_rgrps_plan()

[Andrew Price]

Hi Bob,

On 12/10/15 16:47, Bob Peterson wrote:
> ----- Original Message -----
>> 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.
>>
> 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