[PATCH] Change lvextend to round up for stripped volumes

[PATCH] Change lvextend to round up for stripped volumes

[Iustin Pop]

Currently lvresize, when growing stripped volumes, rounds down if the
extents delta doesn't match exactly the full stripe size. This is
counterintuitive (we're requesting growth by X amount, and instead we
could get less than X, even though the operation succeded), and also
doesn't match the current behaviour in lvcreate (rounds up) and lvextend
for non-stripped volumes (which also rounds up).

Signed-off-by: Iustin Pop <iustin@google.com>
---
 Note: I've tested this in a two-stripe scenario, and it seems to work as
 intended; however, I'm not familiar with the code so I might have
 misunderstood things. Thanks in advance! iustin

 tools/lvresize.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/lvresize.c b/tools/lvresize.c
index 1c0f570..69239dc 100644
--- a/tools/lvresize.c
+++ b/tools/lvresize.c
@@ -283,6 +283,7 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg,
 	uint32_t seg_mirrors = 0;
 	uint32_t extents_used = 0;
 	uint32_t size_rest;
+        uint32_t full_stripe_size;
 	uint32_t pv_extent_count = 0;
 	alloc_policy_t alloc;
 	struct logical_volume *lock_lv;
@@ -517,12 +518,13 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg,
 	if ((lp->stripes > 1)) {
 		if (!(stripesize_extents = lp->stripe_size / vg->extent_size))
 			stripesize_extents = 1;
+                full_stripe_size = lp->stripes * stripesize_extents;
 
-		if ((size_rest = seg_size % (lp->stripes * stripesize_extents))) {
-			log_print("Rounding size (%d extents) down to stripe "
+		if ((size_rest = seg_size % full_stripe_size)) {
+			log_print("Rounding size (%d extents) up to stripe "
 				  "boundary size for segment (%d extents)",
-				  lp->extents, lp->extents - size_rest);
-			lp->extents = lp->extents - size_rest;
+				  lp->extents, lp->extents - size_rest + full_stripe_size);
+			lp->extents = lp->extents - size_rest + full_stripe_size;
 		}
 
 		if (lp->stripe_size < STRIPE_SIZE_MIN) {
-- 
1.6.3.3

[PATCH] Change lvextend to round up for stripped volumes

[Alasdair G Kergon]

On Wed, Jul 01, 2009 at 10:45:35PM +0200, Iustin Pop wrote:
> Currently lvresize, when growing stripped volumes, rounds down if the
> extents delta doesn't match exactly the full stripe size. This is
> counterintuitive (we're requesting growth by X amount, and instead we
> could get less than X, even though the operation succeded), and also
> doesn't match the current behaviour in lvcreate (rounds up) and lvextend
> for non-stripped volumes (which also rounds up).
 
The code is shared between lvextend and lvreduce, and accepts relative
as well as absolute sizes, so the logic is not simple.

The intent was for it to be cautious (consider a filesystem present
requiring the precise size specified) by rounding up the amount of
change when extending and rounding down the amount of change when
reducing.

Alasdair

[PATCH] Change lvextend to round up for stripped volumes

[Iustin Pop]

On Tue, Jul 07, 2009 at 11:17:54PM +0100, Alasdair G Kergon wrote:
> On Wed, Jul 01, 2009 at 10:45:35PM +0200, Iustin Pop wrote:
> > Currently lvresize, when growing stripped volumes, rounds down if the
> > extents delta doesn't match exactly the full stripe size. This is
> > counterintuitive (we're requesting growth by X amount, and instead we
> > could get less than X, even though the operation succeded), and also
> > doesn't match the current behaviour in lvcreate (rounds up) and lvextend
> > for non-stripped volumes (which also rounds up).
>  
> The code is shared between lvextend and lvreduce, and accepts relative
> as well as absolute sizes, so the logic is not simple.

Ack. Thanks for pointing it out.

> The intent was for it to be cautious (consider a filesystem present
> requiring the precise size specified) by rounding up the amount of
> change when extending and rounding down the amount of change when
> reducing.

But the code doesn't do this. It always rounds down for grows of stripped
volumes, because the code that does adjust this doesn't care about which
direction (+ or -) we change the size).

For example, the initial rounding code (for non-stripped) does take into
account whether lp-> == SIGN_MINUS or not. But the strip rounding code doesn't
and always choses one direction of rounding.

I'll look again at the code and send another patch. I was under the wrong
impression that lp->extents is the final volume size, and not the delta, so
rounding lp->extents up is always safe.

thanks!
iustin

[PATCH] Change lvextend to round up for stripped volumes

[Iustin Pop]

On Wed, Jul 08, 2009 at 10:50:03AM +0200, Iustin Pop wrote:
> On Tue, Jul 07, 2009 at 11:17:54PM +0100, Alasdair G Kergon wrote:
> > On Wed, Jul 01, 2009 at 10:45:35PM +0200, Iustin Pop wrote:
> > > Currently lvresize, when growing stripped volumes, rounds down if the
> > > extents delta doesn't match exactly the full stripe size. This is
> > > counterintuitive (we're requesting growth by X amount, and instead we
> > > could get less than X, even though the operation succeded), and also
> > > doesn't match the current behaviour in lvcreate (rounds up) and lvextend
> > > for non-stripped volumes (which also rounds up).
> >  
> > The code is shared between lvextend and lvreduce, and accepts relative
> > as well as absolute sizes, so the logic is not simple.
> 
> Ack. Thanks for pointing it out.
> 
> > The intent was for it to be cautious (consider a filesystem present
> > requiring the precise size specified) by rounding up the amount of
> > change when extending and rounding down the amount of change when
> > reducing.
> 
> But the code doesn't do this. It always rounds down for grows of stripped
> volumes, because the code that does adjust this doesn't care about which
> direction (+ or -) we change the size).
> 
> For example, the initial rounding code (for non-stripped) does take into
> account whether lp-> == SIGN_MINUS or not. But the strip rounding code doesn't
> and always choses one direction of rounding.
> 
> I'll look again at the code and send another patch. I was under the wrong
> impression that lp->extents is the final volume size, and not the delta, so
> rounding lp->extents up is always safe.

So I looked at the code some more and it seems that lp->extents is the intended
final size of the logical volume being resized.

Given this, I think that always rounding up lp->extents is safe; it means that
on grow we'll have a bigger LV than requested, and the same on reduce.

Did I got it right?

thanks,
iustin

[PATCH] Change lvextend to round up for stripped volumes

[Iustin Pop]

Currently lvresize, when growing stripped volumes, rounds down if the
extents delta doesn't match exactly the full stripe size. This is
counterintuitive (we're requesting growth by X amount, and instead we
could get less than X, even though the operation succeded), and also
doesn't match the current behaviour in lvcreate (rounds up) and lvextend
for non-stripped volumes (which also rounds up).

Signed-off-by: Iustin Pop <iustin@google.com>
---
 Note: I've tested this in a two-stripe scenario, and it seems to work as
 intended; however, I'm not familiar with the code so I might have
 misunderstood things. Thanks in advance! iustin

 Also this is a resend, it seems the first time it didn't go through.

 tools/lvresize.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/lvresize.c b/tools/lvresize.c
index 1c0f570..69239dc 100644
--- a/tools/lvresize.c
+++ b/tools/lvresize.c
@@ -283,6 +283,7 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg,
 	uint32_t seg_mirrors = 0;
 	uint32_t extents_used = 0;
 	uint32_t size_rest;
+        uint32_t full_stripe_size;
 	uint32_t pv_extent_count = 0;
 	alloc_policy_t alloc;
 	struct logical_volume *lock_lv;
@@ -517,12 +518,13 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg,
 	if ((lp->stripes > 1)) {
 		if (!(stripesize_extents = lp->stripe_size / vg->extent_size))
 			stripesize_extents = 1;
+                full_stripe_size = lp->stripes * stripesize_extents;
 
-		if ((size_rest = seg_size % (lp->stripes * stripesize_extents))) {
-			log_print("Rounding size (%d extents) down to stripe "
+		if ((size_rest = seg_size % full_stripe_size)) {
+			log_print("Rounding size (%d extents) up to stripe "
 				  "boundary size for segment (%d extents)",
-				  lp->extents, lp->extents - size_rest);
-			lp->extents = lp->extents - size_rest;
+				  lp->extents, lp->extents - size_rest + full_stripe_size);
+			lp->extents = lp->extents - size_rest + full_stripe_size;
 		}
 
 		if (lp->stripe_size < STRIPE_SIZE_MIN) {
-- 
1.6.3.3