[PATCH 1/2] dm-stripe: move sector translation to a function

[PATCH 1/2] dm-stripe: move sector translation to a function

[Mikulas Patocka]

Abstract sector translation in dm-stripe

Abstract sector -> stripe, sector translation to a function.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-stripe.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Index: linux-2.6.35-rc6-fast/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.35-rc6-fast.orig/drivers/md/dm-stripe.c	2010-07-27 22:43:40.000000000 +0200
+++ linux-2.6.35-rc6-fast/drivers/md/dm-stripe.c	2010-07-27 23:08:28.000000000 +0200
@@ -207,11 +207,19 @@ static void stripe_dtr(struct dm_target 
 	kfree(sc);
 }
 
+static void stripe_map_sector(struct stripe_c *sc, sector_t sector,
+			      uint32_t *stripe, sector_t *result)
+{
+	sector_t offset = sector - sc->ti->begin;
+	sector_t chunk = offset >> sc->chunk_shift;
+	*stripe = sector_div(chunk, sc->stripes);
+	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
+}
+
 static int stripe_map(struct dm_target *ti, struct bio *bio,
 		      union map_info *map_context)
 {
 	struct stripe_c *sc = (struct stripe_c *) ti->private;
-	sector_t offset, chunk;
 	uint32_t stripe;
 
 	if (unlikely(bio_empty_barrier(bio))) {
@@ -220,13 +228,11 @@ static int stripe_map(struct dm_target *
 		return DM_MAPIO_REMAPPED;
 	}
 
-	offset = bio->bi_sector - ti->begin;
-	chunk = offset >> sc->chunk_shift;
-	stripe = sector_div(chunk, sc->stripes);
+	stripe_map_sector(sc, bio->bi_sector, &stripe, &bio->bi_sector);
 
+	bio->bi_sector += sc->stripe[stripe].physical_start;
 	bio->bi_bdev = sc->stripe[stripe].dev->bdev;
-	bio->bi_sector = sc->stripe[stripe].physical_start +
-	    (chunk << sc->chunk_shift) + (offset & sc->chunk_mask);
+
 	return DM_MAPIO_REMAPPED;
 }
 

[PATCH 2/2] dm-stripe: discard support

[Mikulas Patocka]

A simplified discard support.

Note --- it is untested (I tested only non-discard requests). Mike, please 
test.

Mikulas

---

Support discard request on stripe target

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-stripe.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Index: linux-2.6.35-rc6-fast/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.35-rc6-fast.orig/drivers/md/dm-stripe.c	2010-07-27 23:32:57.000000000 +0200
+++ linux-2.6.35-rc6-fast/drivers/md/dm-stripe.c	2010-07-27 23:39:32.000000000 +0200
@@ -167,6 +167,7 @@ static int stripe_ctr(struct dm_target *
 	sc->stripe_width = width;
 	ti->split_io = chunk_size;
 	ti->num_flush_requests = stripes;
+	ti->num_discard_requests = stripes;
 
 	sc->chunk_mask = ((sector_t) chunk_size) - 1;
 	for (sc->chunk_shift = 0; chunk_size; sc->chunk_shift++)
@@ -216,6 +217,37 @@ static void stripe_map_sector(struct str
 	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
 }
 
+static void stripe_map_range_sector(struct stripe_c *sc, sector_t sector,
+				    uint32_t target_stripe, sector_t *result)
+{
+	uint32_t stripe;
+	stripe_map_sector(sc, sector, &stripe, result);
+	if (stripe == target_stripe)
+		return;
+	*result &= ~sc->chunk_mask;			/* round down */
+	if (target_stripe < stripe)
+		*result += sc->chunk_mask + 1;		/* next chunk */
+}
+
+static int stripe_map_discard(struct stripe_c *sc, struct bio *bio,
+			      uint32_t target_stripe)
+{
+	sector_t begin, end;
+	stripe_map_range_sector(sc, bio->bi_sector, target_stripe, &begin);
+	stripe_map_range_sector(sc, bio->bi_sector + bio_sectors(bio),
+				  target_stripe, &end);
+	if (begin < end) {
+		bio->bi_bdev = sc->stripe[target_stripe].dev->bdev;
+		bio->bi_sector = begin + sc->stripe[target_stripe].physical_start;
+		bio->bi_size = to_bytes(end - begin);
+		return DM_MAPIO_REMAPPED;
+	} else {
+		/* The range doesn't map to the target stripe */
+		bio_endio(bio, 0);
+		return DM_MAPIO_SUBMITTED;
+	}
+}
+
 static int stripe_map(struct dm_target *ti, struct bio *bio,
 		      union map_info *map_context)
 {
@@ -227,6 +259,10 @@ static int stripe_map(struct dm_target *
 		bio->bi_bdev = sc->stripe[map_context->request_nr].dev->bdev;
 		return DM_MAPIO_REMAPPED;
 	}
+	if (unlikely(bio_rw_flagged(bio, BIO_RW_DISCARD))) {
+		BUG_ON(map_context->request_nr >= sc->stripes);
+		return stripe_map_discard(sc, bio, map_context->request_nr);
+	}
 
 	stripe_map_sector(sc, bio->bi_sector, &stripe, &bio->bi_sector);
 

[PATCH 1.5/2] dm-stripe: optimize sector division

[Mikulas Patocka]

Optimize sector division

Optimize sector division --- if the number of stripes is a power of two,
we don't have to do expensive division and can do shift and mask instead.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-stripe.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: linux-2.6.35-rc6-fast/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.35-rc6-fast.orig/drivers/md/dm-stripe.c	2010-07-28 00:01:04.000000000 +0200
+++ linux-2.6.35-rc6-fast/drivers/md/dm-stripe.c	2010-07-28 00:06:19.000000000 +0200
@@ -25,6 +25,7 @@ struct stripe {
 
 struct stripe_c {
 	uint32_t stripes;
+	int stripes_shift;
 
 	/* The size of this target / num. stripes */
 	sector_t stripe_width;
@@ -164,6 +165,10 @@ static int stripe_ctr(struct dm_target *
 	sc->ti = ti;
 
 	sc->stripes = stripes;
+	if (!(stripes & (stripes - 1)))
+		sc->stripes_shift = ffs(stripes) - 1;
+	else
+		sc->stripes_shift = -1;
 	sc->stripe_width = width;
 	ti->split_io = chunk_size;
 	ti->num_flush_requests = stripes;
@@ -212,7 +217,11 @@ static void stripe_map_sector(struct str
 {
 	sector_t offset = sector - sc->ti->begin;
 	sector_t chunk = offset >> sc->chunk_shift;
-	*stripe = sector_div(chunk, sc->stripes);
+	if (likely(sc->stripes_shift >= 0))
+		*stripe = chunk & ((1 << sc->stripes_shift) - 1),
+		chunk >>= sc->stripes_shift;
+	else
+		*stripe = sector_div(chunk, sc->stripes);
 	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
 }
 

Re: [PATCH 1.5/2] dm-stripe: optimize sector division

[Mike Snitzer]

On Tue, Jul 27 2010 at  6:12pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Optimize sector division
> 
> Optimize sector division --- if the number of stripes is a power of two,
> we don't have to do expensive division and can do shift and mask instead.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

(Tested and)

Reviewed-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 1.5/2] dm-stripe: optimize sector division

[Mike Snitzer]

Looking closer I'm seeing something odd:

On Tue, Jul 27 2010 at  6:12pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> @@ -212,7 +217,11 @@ static void stripe_map_sector(struct str
>  {
>  	sector_t offset = sector - sc->ti->begin;
>  	sector_t chunk = offset >> sc->chunk_shift;
> -	*stripe = sector_div(chunk, sc->stripes);
> +	if (likely(sc->stripes_shift >= 0))
> +		*stripe = chunk & ((1 << sc->stripes_shift) - 1),
> +		chunk >>= sc->stripes_shift;

What's going on here with the comma?  Shouldn't it be a semi-colon?  And
if so we need curly-braces around the if (likely...).

> +	else
> +		*stripe = sector_div(chunk, sc->stripes);
>  	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
>  }

Re: [PATCH 1.5/2] dm-stripe: optimize sector division

[Mikulas Patocka]

On Wed, 28 Jul 2010, Mike Snitzer wrote:

> Looking closer I'm seeing something odd:
> 
> On Tue, Jul 27 2010 at  6:12pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > @@ -212,7 +217,11 @@ static void stripe_map_sector(struct str
> >  {
> >  	sector_t offset = sector - sc->ti->begin;
> >  	sector_t chunk = offset >> sc->chunk_shift;
> > -	*stripe = sector_div(chunk, sc->stripes);
> > +	if (likely(sc->stripes_shift >= 0))
> > +		*stripe = chunk & ((1 << sc->stripes_shift) - 1),
> > +		chunk >>= sc->stripes_shift;
> 
> What's going on here with the comma?  Shouldn't it be a semi-colon?  And
> if so we need curly-braces around the if (likely...).

I'ts just a syntactic trick to avoid the braces. If you think it is weird, 
you can change it to:
if (likely(sc->stripes_shift >= 0)) {
	*stripe = chunk & ((1 << sc->stripes_shift) - 1);
	chunk >>= sc->stripes_shift;
} else

The comma operator in C (if not used to denote function arguments) means 
"evaluate the expression before the comma and discard the result, then 
evaluate the expression after the comma and return the result".

Comma has the lowest priority of all operators. Even lower than "=".

So "a = (1, 2, 3);" means "a = 3;"
"a = 1, 2, 3" is parsed as "(a = 1), 2, 3" and it assigns 1 to a, discards 
2 and returns 3 (if used as part of another expression; if used 
standalone, it discards that 3).

"if (c) a = 1, b = 2;" assigns 1 to a and 2 to b if c is non-zero. It is 
equivalent to "if (c) { a = 1; b = 2; }"

Another common use for comma is to stuff arbitrary function calls into the 
expression --- i.e. for example
#define read_register(x) (printk("reading register %x\n", x), inb(BASE + (x)))
which could then be used as
int r3 = read_register(3);

Mikulas

> > +	else
> > +		*stripe = sector_div(chunk, sc->stripes);
> >  	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
> >  }
> 

[PATCH 1.5/2 v2] dm stripe: optimize sector division

[Mike Snitzer]

From: Mikulas Patocka <mpatocka@redhat.com>

Optimize sector division --- if the number of stripes is a power of two,
we don't have to do expensive division and can do shift and mask instead.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-stripe.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

v2: minor code cleanup in stripe_map_sector

Index: linux-2.6-block/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6-block.orig/drivers/md/dm-stripe.c
+++ linux-2.6-block/drivers/md/dm-stripe.c
@@ -25,6 +25,7 @@ struct stripe {
 
 struct stripe_c {
 	uint32_t stripes;
+	int stripes_shift;
 
 	/* The size of this target / num. stripes */
 	sector_t stripe_width;
@@ -164,6 +165,10 @@ static int stripe_ctr(struct dm_target *
 	sc->ti = ti;
 
 	sc->stripes = stripes;
+	if (!(stripes & (stripes - 1)))
+		sc->stripes_shift = ffs(stripes) - 1;
+	else
+		sc->stripes_shift = -1;
 	sc->stripe_width = width;
 	ti->split_io = chunk_size;
 	ti->num_flush_requests = stripes;
@@ -213,7 +218,12 @@ static void stripe_map_sector(struct str
 {
 	sector_t offset = sector - sc->ti->begin;
 	sector_t chunk = offset >> sc->chunk_shift;
-	*stripe = sector_div(chunk, sc->stripes);
+
+	if (likely(sc->stripes_shift >= 0)) {
+		*stripe = chunk & ((1 << sc->stripes_shift) - 1);
+		chunk >>= sc->stripes_shift;
+	} else
+		*stripe = sector_div(chunk, sc->stripes);
 	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
 }
 

Re: [PATCH 2/2] dm-stripe: discard support

[Mike Snitzer]

On Tue, Jul 27 2010 at  5:42pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> A simplified discard support.
> 
> Note --- it is untested (I tested only non-discard requests). Mike, please 
> test.

Looks good.  My test cases work as expected.  I added some DMDEBUG to
help debugging (will send as a separate patch in case we'd prefer to
drop it).

I've rebased it to work with a 'for-2.6.36' block change and the latest
discard patches that Alasdair recently queued.

I'll post v2 of your 2nd patch shortly.

Thanks,
Mike

[PATCH 2/2 v2] dm stripe: enable discard support

[Mike Snitzer]

The DM core will submit a discard bio to the stripe target for each
stripe in a striped DM device.  The stripe target will determine
stripe-specific portions of the supplied bio to be remapped into
individual (at most 'num_discard_requests' extents).  If a given
stripe-specific discard bio doesn't touch a particular stripe the bio
will be dropped.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-stripe.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Index: linux-2.6-block/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6-block.orig/drivers/md/dm-stripe.c
+++ linux-2.6-block/drivers/md/dm-stripe.c
@@ -167,6 +167,7 @@ static int stripe_ctr(struct dm_target *
 	sc->stripe_width = width;
 	ti->split_io = chunk_size;
 	ti->num_flush_requests = stripes;
+	ti->num_discard_requests = stripes;
 
 	sc->chunk_mask = ((sector_t) chunk_size) - 1;
 	for (sc->chunk_shift = 0; chunk_size; sc->chunk_shift++)
@@ -216,6 +217,39 @@ static void stripe_map_sector(struct str
 	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
 }
 
+static void stripe_map_range_sector(struct stripe_c *sc, sector_t sector,
+				    uint32_t target_stripe, sector_t *result)
+{
+	uint32_t stripe;
+
+	stripe_map_sector(sc, sector, &stripe, result);
+	if (stripe == target_stripe)
+		return;
+	*result &= ~sc->chunk_mask;			/* round down */
+	if (target_stripe < stripe)
+		*result += sc->chunk_mask + 1;		/* next chunk */
+}
+
+static int stripe_map_discard(struct stripe_c *sc, struct bio *bio,
+			      uint32_t target_stripe)
+{
+	sector_t begin, end;
+
+	stripe_map_range_sector(sc, bio->bi_sector, target_stripe, &begin);
+	stripe_map_range_sector(sc, bio->bi_sector + bio_sectors(bio),
+				target_stripe, &end);
+	if (begin < end) {
+		bio->bi_bdev = sc->stripe[target_stripe].dev->bdev;
+		bio->bi_sector = begin + sc->stripe[target_stripe].physical_start;
+		bio->bi_size = to_bytes(end - begin);
+		return DM_MAPIO_REMAPPED;
+	} else {
+		/* The range doesn't map to the target stripe */
+		bio_endio(bio, 0);
+		return DM_MAPIO_SUBMITTED;
+	}
+}
+
 static int stripe_map(struct dm_target *ti, struct bio *bio,
 		      union map_info *map_context)
 {
@@ -229,6 +263,11 @@ static int stripe_map(struct dm_target *
 		bio->bi_bdev = sc->stripe[target_request_nr].dev->bdev;
 		return DM_MAPIO_REMAPPED;
 	}
+	if (unlikely(bio->bi_rw & REQ_DISCARD)) {
+		target_request_nr = map_context->target_request_nr;
+		BUG_ON(target_request_nr >= sc->stripes);
+		return stripe_map_discard(sc, bio, target_request_nr);
+	}
 
 	stripe_map_sector(sc, bio->bi_sector, &stripe, &bio->bi_sector);
 

Re: [PATCH 2/2 v2] dm stripe: enable discard support

[Mike Snitzer]

Alasdair,

I forgot to use the following in the patch header:

From: Mikulas Patocka <mpatocka@redhat.com>

please adjust, thanks!

[PATCH 1/2 v2] dm stripe: move sector translation to a function

[Mike Snitzer]

From: Mikulas Patocka <mpatocka@redhat.com>

Abstract sector -> stripe translation to a function.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-stripe.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

v2: fix conflict with newly staged dm_target_offset changes

Index: linux-2.6/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-stripe.c
+++ linux-2.6/drivers/md/dm-stripe.c
@@ -207,11 +207,19 @@ static void stripe_dtr(struct dm_target 
 	kfree(sc);
 }
 
+static void stripe_map_sector(struct stripe_c *sc, sector_t sector,
+			      uint32_t *stripe, sector_t *result)
+{
+	sector_t offset = dm_target_offset(sc->ti, sector);
+	sector_t chunk = offset >> sc->chunk_shift;
+	*stripe = sector_div(chunk, sc->stripes);
+	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
+}
+
 static int stripe_map(struct dm_target *ti, struct bio *bio,
 		      union map_info *map_context)
 {
 	struct stripe_c *sc = (struct stripe_c *) ti->private;
-	sector_t offset, chunk;
 	uint32_t stripe;
 	unsigned target_request_nr;
 
@@ -222,13 +230,11 @@ static int stripe_map(struct dm_target *
 		return DM_MAPIO_REMAPPED;
 	}
 
-	offset = dm_target_offset(ti, bio->bi_sector);
-	chunk = offset >> sc->chunk_shift;
-	stripe = sector_div(chunk, sc->stripes);
+	stripe_map_sector(sc, bio->bi_sector, &stripe, &bio->bi_sector);
 
+	bio->bi_sector += sc->stripe[stripe].physical_start;
 	bio->bi_bdev = sc->stripe[stripe].dev->bdev;
-	bio->bi_sector = sc->stripe[stripe].physical_start +
-	    (chunk << sc->chunk_shift) + (offset & sc->chunk_mask);
+
 	return DM_MAPIO_REMAPPED;
 }