Skip to contentSkip to conversation

dm-stripe: move sector translation to a function

PATCH In review Mikulas Patocka 2 participants 11 messages Jul 27, 2010 – Jul 28, 2010 Device mapper

lore
Share & tools
lore

Mikulas Patocka

author maintainer
#0 lore raw

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

Abstract sector translation in dm-stripe

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

Signed-off-by Mikulas Patocka <[email protected]>
---
 drivers/md/dm-stripe.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/md/dm-stripe.c +12 -6
Index: linux-2.6.35-rc6-fast/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 = 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;
 }
author maintainer
#1 lore raw

PATCH 2/2 dm-stripe: discard support

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 <[email protected]>
---
 drivers/md/dm-stripe.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
drivers/md/dm-stripe.c +36 -0
Index: linux-2.6.35-rc6-fast/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,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);
author maintainer
#2 lore raw

PATCH 1/1 dm-stripe: optimize sector division

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 <[email protected]>
---
 drivers/md/dm-stripe.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/md/dm-stripe.c +10 -1
Index: linux-2.6.35-rc6-fast/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;

@@ -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);
 }
#4 lore raw

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

In reply to Mikulas Patocka

Looking closer I'm seeing something odd:

drivers/md/dm-stripe.c static void stripe_map_sector(struct str · line 222

Quoted 8 lines · Mikulas Patocka
drivers/md/dm-stripe.c +3 -1
@@ -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...).

Quoted 4 lines
drivers/md/dm-stripe.c +2 -0
+	else
+		*stripe = sector_div(chunk, sc->stripes);
 	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
 }
author maintainer
#5 lore raw

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

Quoted 5 lines · Mike Snitzer

Looking closer I'm seeing something odd:

On Tue, Jul 27 2010 at 6:12pm -0400,
Mikulas Patocka <[email protected]> wrote:

Quoted 11 lines

drivers/md/dm-stripe.c static void stripe_map_sector(struct str · line 222

Quoted 8 lines
drivers/md/dm-stripe.c +3 -1
@@ -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

Quoted 5 lines
Quoted 4 lines
drivers/md/dm-stripe.c +2 -0
+	else
+		*stripe = sector_div(chunk, sc->stripes);
 	*result = (chunk << sc->chunk_shift) | (offset & sc->chunk_mask);
 }
#6 lore raw

PATCH v2 1/1 dm stripe: optimize sector division

From: Mikulas Patocka <[email protected]>

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 <[email protected]>
Signed-off-by Mike Snitzer <[email protected]>
---
 drivers/md/dm-stripe.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

v2: minor code cleanup in stripe_map_sector

drivers/md/dm-stripe.c +11 -1
Index: 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);
 }
#7 lore raw

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

In reply to Mikulas Patocka
Quoted 4 lines · Mikulas Patocka

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

#8 lore raw

PATCH v2 2/2 dm stripe: enable discard support

In reply to Mikulas Patocka

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 <[email protected]>
Signed-off-by Mike Snitzer <[email protected]>
---
 drivers/md/dm-stripe.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
drivers/md/dm-stripe.c +39 -0
Index: 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);
#9 lore raw

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

Alasdair,

I forgot to use the following in the patch header:

From: Mikulas Patocka <[email protected]>

please adjust, thanks!

#10 lore raw

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

In reply to Mikulas Patocka

From: Mikulas Patocka <[email protected]>

Abstract sector -> stripe translation to a function.

Signed-off-by Mikulas Patocka <[email protected]>
Signed-off-by Mike Snitzer <[email protected]>
---
 drivers/md/dm-stripe.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

v2: fix conflict with newly staged dm_target_offset changes

drivers/md/dm-stripe.c +12 -6
Index: 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;
 }