[PATCH v2] dm: Fix alignment stacking on partitioned devices

[PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

From: Martin K. Petersen <martin.petersen@oracle.com> 

The DM device limits function passes the start offset within the block
device to the block layer stacking function.  The stacking function
expects the provided start offset to be relative to start of the disk
(request_queue).  Until now DM was passing a start offset that was
relative to beginning of the partition (block_device), resulting in
incorrect alignment stacking.

Add the partition offset to the values passed to blk_stack_limits().

Also clarify, in the DMWARN message, that the device which caused
blk_stack_limits() to return failure isn't necessarily misaligned
itself.  It caused the top-level (DM) device to have inconsistent
alignment when taken in combination with all previously stacked
device(s).

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index be62547..44527c1 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -495,6 +495,7 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
 	struct queue_limits *limits = data;
 	struct block_device *bdev = dev->bdev;
 	struct request_queue *q = bdev_get_queue(bdev);
+	sector_t offset = (get_start_sect(bdev) + start) << 9;
 	char b[BDEVNAME_SIZE];
 
 	if (unlikely(!q)) {
@@ -503,15 +504,15 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
 		return 0;
 	}
 
-	if (blk_stack_limits(limits, &q->limits, start << 9) < 0)
-		DMWARN("%s: target device %s is misaligned: "
+	if (blk_stack_limits(limits, &q->limits, offset) < 0)
+		DMWARN("%s: adding target device %s caused an alignment inconsistency: "
 		       "physical_block_size=%u, logical_block_size=%u, "
 		       "alignment_offset=%u, start=%llu",
 		       dm_device_name(ti->table->md), bdevname(bdev, b),
 		       q->limits.physical_block_size,
 		       q->limits.logical_block_size,
 		       q->limits.alignment_offset,
-		       (unsigned long long) start << 9);
+		       (unsigned long long) offset);
 
 
 	/*

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Wed, Dec 23 2009 at  1:07pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> From: Martin K. Petersen <martin.petersen@oracle.com> 
> 
> The DM device limits function passes the start offset within the block
> device to the block layer stacking function.  The stacking function
> expects the provided start offset to be relative to start of the disk
> (request_queue).  Until now DM was passing a start offset that was
> relative to beginning of the partition (block_device), resulting in
> incorrect alignment stacking.
> 
> Add the partition offset to the values passed to blk_stack_limits().
> 
> Also clarify, in the DMWARN message, that the device which caused
> blk_stack_limits() to return failure isn't necessarily misaligned
> itself.  It caused the top-level (DM) device to have inconsistent
> alignment when taken in combination with all previously stacked
> device(s).

Alasdair,

This should be pushed as a fix for 2.6.33.  It should probably go to
stable (2.6.31.y and 2.6.32.y) too.

Mike

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Alasdair G Kergon]

On Wed, Dec 23, 2009 at 01:07:20PM -0500, Mike Snitzer wrote:
> +	sector_t offset = (get_start_sect(bdev) + start) << 9;

Please can you fix the types here?

'offset' is defined as sector_t i.e. number of sectors, so 
shouldn't have '<< 9' which would make it bytes i.e. *not* sector_t.

Alasdair

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Alasdair G Kergon]

On Wed, Dec 23, 2009 at 01:07:20PM -0500, Mike Snitzer wrote:
> +	if (blk_stack_limits(limits, &q->limits, offset) < 0)

extern int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
                            sector_t offset);

This function is asking for the offset to be supplied as sector_t i.e.
in units of sectors, but this patch uses bytes.  Please either change
that to sectors as per the prototype, or if it really does want bytes,
fix the prototype to make that clear.

Alasdair

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Martin K. Petersen]

>>>>> "Alasdair" == Alasdair G Kergon <agk@redhat.com> writes:

Alasdair> extern int blk_stack_limits(struct queue_limits *t, struct
Alasdair> queue_limits *b,
Alasdair>                             sector_t offset);

Alasdair> This function is asking for the offset to be supplied as
Alasdair> sector_t i.e.  in units of sectors, but this patch uses bytes.
Alasdair> Please either change that to sectors as per the prototype, or
Alasdair> if it really does want bytes, fix the prototype to make that
Alasdair> clear.

It is sector_t because we don't have an existing type that fits the bill
(i.e. >= sector_t and dependent on whether LBD is on or not).  We're
trying to move away from counting in sectors because the notion is
confusing in the light of the logical vs. physical block size, device
alignment reporting, etc.

So maybe something like this?


block: Introduce blk_off_t

There are several places we want to communicate alignment and offsets in
bytes to avoid confusion with regards to underlying physical and logical
block sizes.  Introduce blk_off_t for block device byte offsets.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/block/blk-settings.c b/block/blk-settings.c
index d52d4ad..9404666 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -525,9 +525,9 @@ static unsigned int lcm(unsigned int a, unsigned int b)
  *    the alignment_offset is undefined.
  */
 int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
-		     sector_t offset)
+		     blk_off_t offset)
 {
-	sector_t alignment;
+	blk_off_t alignment;
 	unsigned int top, bottom;
 
 	t->max_sectors = min_not_zero(t->max_sectors, b->max_sectors);
@@ -642,7 +642,7 @@ EXPORT_SYMBOL(blk_stack_limits);
  *    misalignment.
  */
 void disk_stack_limits(struct gendisk *disk, struct block_device *bdev,
-		       sector_t offset)
+		       blk_off_t offset)
 {
 	struct request_queue *t = disk->queue;
 	struct request_queue *b = bdev_get_queue(bdev);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9b98173..4b4436a 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -937,9 +937,9 @@ extern void blk_limits_io_opt(struct queue_limits *limits, unsigned int opt);
 extern void blk_queue_io_opt(struct request_queue *q, unsigned int opt);
 extern void blk_set_default_limits(struct queue_limits *lim);
 extern int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
-			    sector_t offset);
+			    blk_off_t offset);
 extern void disk_stack_limits(struct gendisk *disk, struct block_device *bdev,
-			      sector_t offset);
+			      blk_off_t offset);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);
 extern void blk_queue_dma_pad(struct request_queue *, unsigned int);
 extern void blk_queue_update_dma_pad(struct request_queue *, unsigned int);
@@ -1110,7 +1110,7 @@ static inline int queue_alignment_offset(struct request_queue *q)
 	return q->limits.alignment_offset;
 }
 
-static inline int queue_limit_alignment_offset(struct queue_limits *lim, sector_t offset)
+static inline int queue_limit_alignment_offset(struct queue_limits *lim, blk_off_t offset)
 {
 	unsigned int granularity = max(lim->physical_block_size, lim->io_min);
 
diff --git a/include/linux/types.h b/include/linux/types.h
index c42724f..729f87a 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -134,9 +134,11 @@ typedef		__s64		int64_t;
 #ifdef CONFIG_LBDAF
 typedef u64 sector_t;
 typedef u64 blkcnt_t;
+typedef u64 blk_off_t;
 #else
 typedef unsigned long sector_t;
 typedef unsigned long blkcnt_t;
+typedef unsigned long blk_off_t;
 #endif
 
 /*

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Tue, Jan 05 2010 at  9:23pm -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Alasdair" == Alasdair G Kergon <agk@redhat.com> writes:
> 
> Alasdair> extern int blk_stack_limits(struct queue_limits *t, struct
> Alasdair> queue_limits *b,
> Alasdair>                             sector_t offset);
> 
> Alasdair> This function is asking for the offset to be supplied as
> Alasdair> sector_t i.e.  in units of sectors, but this patch uses bytes.
> Alasdair> Please either change that to sectors as per the prototype, or
> Alasdair> if it really does want bytes, fix the prototype to make that
> Alasdair> clear.
> 
> It is sector_t because we don't have an existing type that fits the bill
> (i.e. >= sector_t and dependent on whether LBD is on or not).  We're
> trying to move away from counting in sectors because the notion is
> confusing in the light of the logical vs. physical block size, device
> alignment reporting, etc.
> 
> So maybe something like this?
> 
> 
> block: Introduce blk_off_t
> 
> There are several places we want to communicate alignment and offsets in
> bytes to avoid confusion with regards to underlying physical and logical
> block sizes.  Introduce blk_off_t for block device byte offsets.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
...
> diff --git a/include/linux/types.h b/include/linux/types.h
> index c42724f..729f87a 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -134,9 +134,11 @@ typedef		__s64		int64_t;
>  #ifdef CONFIG_LBDAF
>  typedef u64 sector_t;
>  typedef u64 blkcnt_t;
> +typedef u64 blk_off_t;
>  #else
>  typedef unsigned long sector_t;
>  typedef unsigned long blkcnt_t;
> +typedef unsigned long blk_off_t;
>  #endif
>  
>  /*

After looking closer there seems to be various type inconsistencies in
the alignment_offset and discard_alignment related routines (returning
'int' in places, etc).

The following patch is what I found; I have no problem with switching
from 'unsigned long' to blk_off_t for LBD though.

Martin, would like to carry forward with this?  Have I gone overboard
with this patch?

---
 block/blk-settings.c   |    8 ++++----
 block/genhd.c          |    4 ++--
 include/linux/blkdev.h |   21 +++++++++++----------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index d52d4ad..446eeef 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -493,7 +493,7 @@ void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b)
 }
 EXPORT_SYMBOL(blk_queue_stack_limits);
 
-static unsigned int lcm(unsigned int a, unsigned int b)
+static unsigned long lcm(unsigned long a, unsigned long b)
 {
 	if (a && b)
 		return (a * b) / gcd(a, b);
@@ -525,10 +525,10 @@ static unsigned int lcm(unsigned int a, unsigned int b)
  *    the alignment_offset is undefined.
  */
 int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
-		     sector_t offset)
+		     unsigned long offset)
 {
-	sector_t alignment;
-	unsigned int top, bottom;
+	unsigned long alignment;
+	unsigned long top, bottom;
 
 	t->max_sectors = min_not_zero(t->max_sectors, b->max_sectors);
 	t->max_hw_sectors = min_not_zero(t->max_hw_sectors, b->max_hw_sectors);
diff --git a/block/genhd.c b/block/genhd.c
index b11a4ad..94fc2b0 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -858,7 +858,7 @@ static ssize_t disk_alignment_offset_show(struct device *dev,
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
-	return sprintf(buf, "%d\n", queue_alignment_offset(disk->queue));
+	return sprintf(buf, "%lu\n", queue_alignment_offset(disk->queue));
 }
 
 static ssize_t disk_discard_alignment_show(struct device *dev,
@@ -867,7 +867,7 @@ static ssize_t disk_discard_alignment_show(struct device *dev,
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
-	return sprintf(buf, "%u\n", queue_discard_alignment(disk->queue));
+	return sprintf(buf, "%lu\n", queue_discard_alignment(disk->queue));
 }
 
 static DEVICE_ATTR(range, S_IRUGO, disk_range_show, NULL);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9b98173..92f9eac 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -308,12 +308,12 @@ struct queue_limits {
 	unsigned int		max_sectors;
 	unsigned int		max_segment_size;
 	unsigned int		physical_block_size;
-	unsigned int		alignment_offset;
+	unsigned long		alignment_offset;
 	unsigned int		io_min;
 	unsigned int		io_opt;
 	unsigned int		max_discard_sectors;
 	unsigned int		discard_granularity;
-	unsigned int		discard_alignment;
+	unsigned long		discard_alignment;
 
 	unsigned short		logical_block_size;
 	unsigned short		max_hw_segments;
@@ -1102,7 +1102,7 @@ static inline int bdev_io_opt(struct block_device *bdev)
 	return queue_io_opt(bdev_get_queue(bdev));
 }
 
-static inline int queue_alignment_offset(struct request_queue *q)
+static inline unsigned long queue_alignment_offset(struct request_queue *q)
 {
 	if (q->limits.misaligned)
 		return -1;
@@ -1110,7 +1110,8 @@ static inline int queue_alignment_offset(struct request_queue *q)
 	return q->limits.alignment_offset;
 }
 
-static inline int queue_limit_alignment_offset(struct queue_limits *lim, sector_t offset)
+static inline unsigned long queue_limit_alignment_offset(struct queue_limits *lim,
+							 unsigned long offset)
 {
 	unsigned int granularity = max(lim->physical_block_size, lim->io_min);
 
@@ -1118,13 +1119,13 @@ static inline int queue_limit_alignment_offset(struct queue_limits *lim, sector_
 	return (granularity + lim->alignment_offset - offset) & (granularity - 1);
 }
 
-static inline int queue_sector_alignment_offset(struct request_queue *q,
-						sector_t sector)
+static inline unsigned long queue_sector_alignment_offset(struct request_queue *q,
+							  sector_t sector)
 {
 	return queue_limit_alignment_offset(&q->limits, sector << 9);
 }
 
-static inline int bdev_alignment_offset(struct block_device *bdev)
+static inline unsigned long bdev_alignment_offset(struct block_device *bdev)
 {
 	struct request_queue *q = bdev_get_queue(bdev);
 
@@ -1137,7 +1138,7 @@ static inline int bdev_alignment_offset(struct block_device *bdev)
 	return q->limits.alignment_offset;
 }
 
-static inline int queue_discard_alignment(struct request_queue *q)
+static inline unsigned long queue_discard_alignment(struct request_queue *q)
 {
 	if (q->limits.discard_misaligned)
 		return -1;
@@ -1145,8 +1146,8 @@ static inline int queue_discard_alignment(struct request_queue *q)
 	return q->limits.discard_alignment;
 }
 
-static inline int queue_sector_discard_alignment(struct request_queue *q,
-						 sector_t sector)
+static inline unsigned long queue_sector_discard_alignment(struct request_queue *q,
+							   sector_t sector)
 {
 	return ((sector << 9) - q->limits.discard_alignment)
 		& (q->limits.discard_granularity - 1);

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Tue, Jan 05 2010 at  9:57pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Tue, Jan 05 2010 at  9:23pm -0500,
> Martin K. Petersen <martin.petersen@oracle.com> wrote:
> 
> > >>>>> "Alasdair" == Alasdair G Kergon <agk@redhat.com> writes:
> > 
> > Alasdair> extern int blk_stack_limits(struct queue_limits *t, struct
> > Alasdair> queue_limits *b,
> > Alasdair>                             sector_t offset);
> > 
> > Alasdair> This function is asking for the offset to be supplied as
> > Alasdair> sector_t i.e.  in units of sectors, but this patch uses bytes.
> > Alasdair> Please either change that to sectors as per the prototype, or
> > Alasdair> if it really does want bytes, fix the prototype to make that
> > Alasdair> clear.
> > 
> > It is sector_t because we don't have an existing type that fits the bill
> > (i.e. >= sector_t and dependent on whether LBD is on or not).  We're
> > trying to move away from counting in sectors because the notion is
> > confusing in the light of the logical vs. physical block size, device
> > alignment reporting, etc.
> > 
> > So maybe something like this?
> > 
> > 
> > block: Introduce blk_off_t
> > 
> > There are several places we want to communicate alignment and offsets in
> > bytes to avoid confusion with regards to underlying physical and logical
> > block sizes.  Introduce blk_off_t for block device byte offsets.
> > 
> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ...
> > diff --git a/include/linux/types.h b/include/linux/types.h
> > index c42724f..729f87a 100644
> > --- a/include/linux/types.h
> > +++ b/include/linux/types.h
> > @@ -134,9 +134,11 @@ typedef		__s64		int64_t;
> >  #ifdef CONFIG_LBDAF
> >  typedef u64 sector_t;
> >  typedef u64 blkcnt_t;
> > +typedef u64 blk_off_t;
> >  #else
> >  typedef unsigned long sector_t;
> >  typedef unsigned long blkcnt_t;
> > +typedef unsigned long blk_off_t;
> >  #endif
> >  
> >  /*
> 
> After looking closer there seems to be various type inconsistencies in
> the alignment_offset and discard_alignment related routines (returning
> 'int' in places, etc).
> 
> The following patch is what I found; I have no problem with switching
> from 'unsigned long' to blk_off_t for LBD though.
> 
> Martin, would like to carry forward with this?  Have I gone overboard
> with this patch?

I missed fixing disk_stack_limits()'s 'offset' though...

Mike

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike,

Mike> After looking closer there seems to be various type
Mike> inconsistencies in the alignment_offset and discard_alignment
Mike> related routines (returning 'int' in places, etc).

Mike> The following patch is what I found; I have no problem with
Mike> switching from 'unsigned long' to blk_off_t for LBD though.

I only use blk_off_t in the places where we're dealing with absolute
offsets.

Blindly converting alignment_offset from int to unsigned long won't
work.  We depend on being able to return -1 in case of
misalignment. Hence int and not unsigned int. Furthermore, the returned
values are always modulo the granularity so int is plenty big.

So NAK from here.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Tue, Jan 05 2010 at 10:24pm -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> Mike,
> 
> Mike> After looking closer there seems to be various type
> Mike> inconsistencies in the alignment_offset and discard_alignment
> Mike> related routines (returning 'int' in places, etc).
> 
> Mike> The following patch is what I found; I have no problem with
> Mike> switching from 'unsigned long' to blk_off_t for LBD though.
> 
> I only use blk_off_t in the places where we're dealing with absolute
> offsets.
> 
> Blindly converting alignment_offset from int to unsigned long won't
> work.  We depend on being able to return -1 in case of
> misalignment. Hence int and not unsigned int.

Right, I realized/noticed that after I sent that patch.

But even with your blk_off_t patch (and prior to it with sector_t)
you're mixing int with blk_off_t in blk_stack_limits() by doing:

        alignment = queue_limit_alignment_offset(b, offset);

This helped motivate my "blind" conversion.

> Furthermore, the returned values are always modulo the granularity so
> int is plenty big.

OK.

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike> But even with your blk_off_t patch (and prior to it with sector_t)
Mike> you're mixing int with blk_off_t in blk_stack_limits() by doing:

Mike>         alignment = queue_limit_alignment_offset(b, offset);

Mike> This helped motivate my "blind" conversion.

Yeah.  However, that particular wart is fixed by an unrelated patch in
my queue wrt. discard alignment.  But I wanted something you guys could
look at that applied on top of upstream.

I'm tinkering with a few things on this end to see if I can make
the distinction between an offset and an alignment obvious.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Tue, Jan 05 2010 at 11:16pm -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> Mike> But even with your blk_off_t patch (and prior to it with sector_t)
> Mike> you're mixing int with blk_off_t in blk_stack_limits() by doing:
> 
> Mike>         alignment = queue_limit_alignment_offset(b, offset);
> 
> Mike> This helped motivate my "blind" conversion.
> 
> Yeah.  However, that particular wart is fixed by an unrelated patch in
> my queue wrt. discard alignment.  But I wanted something you guys could
> look at that applied on top of upstream.
> 
> I'm tinkering with a few things on this end to see if I can make
> the distinction between an offset and an alignment obvious.

Sounds good.  Ultimately we're looking to make sure the DM code is using
"obviously correct" types.  So a near-term minimalist type fix for
upstream (2.6.33) would be awesome.

BTW, the other related concern Alasdair had was that DM shouldn't need
to know to add the start of the partition to the 'offset' it passes to
blk_stack_limits() -- sees it as a layering violation.

I went over the fact that the 'struct queue_limits' was added to
abstract out the limit stacking in a way that DM could use too.  Without
passing asymmetric types for the 'top' and 'bottom' device to
blk_stack_limits(), e.g.:

int blk_stack_limits(struct queue_limits *t, struct block_device *bdev,
                     blk_off_t offset)

we're left with blk_stack_limits() callers needing to pass an offset
relative to the start of the disk.  Alasdair would rather that not be
the case but I'll defer to him on whether he'd still like to see
get_start_sect() be pushed out of DM into blk_stack_limits().

Mike

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike> BTW, the other related concern Alasdair had was that DM shouldn't
Mike> need to know to add the start of the partition to the 'offset' it
Mike> passes to blk_stack_limits() -- sees it as a layering violation.

Mike> I went over the fact that the 'struct queue_limits' was added to
Mike> abstract out the limit stacking in a way that DM could use too.
Mike> Without passing asymmetric types for the 'top' and 'bottom' device
Mike> to blk_stack_limits(), e.g.:

I don't particularly care about the symmetry.  The current set of
arguments were first and foremost there to meet DM's needs.  I don't
think you had access to the bdev at the right time when we originally
came up with this.

However, there are other subsystems that need to stack without an
associated block_device.  So instead of changing blk_stack_limits I
propose we use the wrapper below.

You'll notice that bdev_stack_limits takes a sector offset as argument.
I have a patch that I intend to push at a later date that will convert
the remaining stacking functions to take soft sector offsets instead of
bytes.  I deal with the appropriate conversion in the alignment
calculation.  That way we avoid the blk_off_t goo entirely and it makes
the difference between absolute offsets and alignment offsets crystal
clear.

Alasdair, if you are OK with the approach below I propose you give an
Acked-by: and then we can feed the patch through Jens instead of dealing
with a clunky two-stage merge this late in the .33 game.



block: bdev_stack_limits wrapper

DM does not want to know about partition offsets.  Add a partition-aware
wrapper that DM can use when stacking block devices.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/block/blk-settings.c b/block/blk-settings.c
index d52d4ad..4b43e14 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -631,6 +631,27 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 EXPORT_SYMBOL(blk_stack_limits);
 
 /**
+ * bdev_stack_limits - adjust queue limits for stacked drivers
+ * @t:	the stacking driver limits (top device)
+ * @bdev:  the component block_device (bottom)
+ * @start:  first data sector within component device
+ *
+ * Description:
+ *    Merges queue limits for a top device and a block_device.  Returns
+ *    0 if alignment didn't change.  Returns -1 if adding the bottom
+ *    device caused misalignment.
+ */
+int bdev_stack_limits(struct queue_limits *t, struct block_device *bdev,
+		      sector_t start)
+{
+	struct request_queue *bq = bdev_get_queue(bdev);
+
+	start += get_start_sect(bdev);
+
+	return blk_stack_limits(t, &bq->limits, start << 9);
+}
+
+/**
  * disk_stack_limits - adjust queue limits for stacked drivers
  * @disk:  MD/DM gendisk (top)
  * @bdev:  the underlying block device (bottom)
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index be62547..e0866a0 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -503,7 +503,7 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
 		return 0;
 	}
 
-	if (blk_stack_limits(limits, &q->limits, start << 9) < 0)
+	if (bdev_stack_limits(limits, bdev, start) < 0)
 		DMWARN("%s: target device %s is misaligned: "
 		       "physical_block_size=%u, logical_block_size=%u, "
 		       "alignment_offset=%u, start=%llu",
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9b98173..70b7ede 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -938,6 +938,8 @@ extern void blk_queue_io_opt(struct request_queue *q, unsigned int opt);
 extern void blk_set_default_limits(struct queue_limits *lim);
 extern int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 			    sector_t offset);
+extern int bdev_stack_limits(struct queue_limits *t, struct block_device *bdev,
+			    sector_t offset);
 extern void disk_stack_limits(struct gendisk *disk, struct block_device *bdev,
 			      sector_t offset);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Wed, Jan 06 2010 at  3:39am -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> Mike> BTW, the other related concern Alasdair had was that DM shouldn't
> Mike> need to know to add the start of the partition to the 'offset' it
> Mike> passes to blk_stack_limits() -- sees it as a layering violation.
> 
> Mike> I went over the fact that the 'struct queue_limits' was added to
> Mike> abstract out the limit stacking in a way that DM could use too.
> Mike> Without passing asymmetric types for the 'top' and 'bottom' device
> Mike> to blk_stack_limits(), e.g.:
> 
> I don't particularly care about the symmetry.  The current set of
> arguments were first and foremost there to meet DM's needs.  I don't
> think you had access to the bdev at the right time when we originally
> came up with this.

I think we always had it for the bottom device but not the top..
 
> However, there are other subsystems that need to stack without an
> associated block_device.  So instead of changing blk_stack_limits I
> propose we use the wrapper below.

Yes, looks like osdblk.c's use of blk_queue_stack_limits()
 
> You'll notice that bdev_stack_limits takes a sector offset as argument.
> I have a patch that I intend to push at a later date that will convert
> the remaining stacking functions to take soft sector offsets instead of
> bytes.  I deal with the appropriate conversion in the alignment
> calculation.  That way we avoid the blk_off_t goo entirely and it makes
> the difference between absolute offsets and alignment offsets crystal
> clear.

Looks good.  But you're missing the EXPORT_SYMBOL(bdev_stack_limits)

> Alasdair, if you are OK with the approach below I propose you give an
> Acked-by: and then we can feed the patch through Jens instead of dealing
> with a clunky two-stage merge this late in the .33 game.

Good point, you can have mine while we're at it.


> block: bdev_stack_limits wrapper
> 
> DM does not want to know about partition offsets.  Add a partition-aware
> wrapper that DM can use when stacking block devices.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

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

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

>> However, there are other subsystems that need to stack without an
>> associated block_device.  So instead of changing blk_stack_limits I
>> propose we use the wrapper below.

Mike> Yes, looks like osdblk.c's use of blk_queue_stack_limits()

DM also does a stacking pass without a bdev (dm_calculate_queue_limits),
although I think you can avoid that by getting rid of ti_limits
altogether and just stack limits.

I couldn't figure out why the alignment_offset for a misaligned DM
device was zero, despite being -1 after stacking had completed.  I
finally spotted this in dm_table_set_restrictions():

        limits->alignment_offset = 0;
        limits->misaligned = 0;

So after all the stacking function's hard work you set the alignment to
0 and clear the misaligned flag.  Why?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Thu, Jan 07 2010 at 12:15pm -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> >> However, there are other subsystems that need to stack without an
> >> associated block_device.  So instead of changing blk_stack_limits I
> >> propose we use the wrapper below.
> 
> Mike> Yes, looks like osdblk.c's use of blk_queue_stack_limits()
> 
> DM also does a stacking pass without a bdev (dm_calculate_queue_limits),
> although I think you can avoid that by getting rid of ti_limits
> altogether and just stack limits.

Yeah, I'm keeping the stacking of all devices within a target
self-contained; I also do validation of the target's limits:

                /*
                 * Check each device area is consistent with the target's
                 * overall queue limits.
                 */
                if (ti->type->iterate_devices(ti, device_area_is_invalid,
                                              &ti_limits))
                        return -EINVAL;

The validation, with device_area_is_invalid, is confined to validating
only this target's devices' limits.  I couldn't do the same target-local
validation by stacking a single flat 'limits'.

I then stack that target's limits on the final DM device's 'limits' (and
will spew a warning accordingly).

> I couldn't figure out why the alignment_offset for a misaligned DM
> device was zero, despite being -1 after stacking had completed.  I
> finally spotted this in dm_table_set_restrictions():
> 
>         limits->alignment_offset = 0;
>         limits->misaligned = 0;
> 
> So after all the stacking function's hard work you set the alignment to
> 0 and clear the misaligned flag.  Why?

Good question, it made my head hurt trying to recall why :)

A very critical piece to this is the assumption that alignment_offset is
consumed by LVM2 (it adjusts by shifting a target device's 'start' to
include alignment_offset).  DM doesn't rely on the block layer to
validate alignment_offset at all.

The comment above the lines you referenced offers:

        /*
         * Each target device in the table has a data area that should normally
         * be aligned such that the DM device's alignment_offset is 0.
         * FIXME: Propagate alignment_offsets up the stack and warn of
         *        sub-optimal or inconsistent settings.
         */

Seems I took the time to add a comment whose FIXME doesn't ring many
bells now!  But ignoring that, the comment before the FIXME is making a
veiled reference to userspace having consumed alignment_offset.

To recap: Userspace consumes alignment_offset so it shouldn't be a
concern in DM.  Each target device is validated (like I covered above)
such that any misalignment (purely logicical vs physical misalignment)
within a target device is not tolerated.  So by the time the DM table's
limits get copied to the DM device's limits (in
dm_table_set_restrictions) any misalignment percieved by the block layer
is not trusted/published.

Mike

p.s. Any feedback on fatal flaws in all of this is much appreciated.  I
thought I cleared most of this with you as it was developed but seems
not (obviously I didn't clear resetting alignment_offset and
misaligned).

But I'm now thinking that if all the above works according to plan we
shouldn't ever have a DM device's alignment_offset or misaligned be
anything but 0 (unless one isn't using a trained LVM2; which I'd imagine
you aren't).  Anyway, long story short, those lines _should_ likely be
removed... but it needs further testing that I can't do right now.

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike,

Mike> Seems I took the time to add a comment whose FIXME doesn't ring
Mike> many bells now!  But ignoring that, the comment before the FIXME
Mike> is making a veiled reference to userspace having consumed
Mike> alignment_offset.

I read and understood the comment.  But even then explicitly zeroing out
those two values didn't make sense (because if user space did in fact do
the right thing they'd always be zero).

I know that the DM utilities take care of business.  And that DM devices
are special because they are always set up by user space and not a
kernel discovery process.

But since the code *is* in place to validate things I'm not so keen on
you clearing fields that have been calculated and have a meaning.  For
me it masked a case where the DM utilities did the wrong thing (because
they were old).

With the Enterprise Linux hat on it is easy for us to specify that you
must use this version of the kernel and the device mapper utilities.
But reality is that lots of people are running upstream kernels on
distributions with older userland.  And some distributions get things
wrong, ship broken bits, etc.

It's great that new DM utils will transparently adjust the starting
offset.  That's the way it's supposed to work.  No arguments there.

My main concern is making sure that we never get into a case where we
run with misaligned components without indicating that there is a
problem.  Ever!  Regardless of which DM utils might be in place.

We have the power to get that right.  All the pieces are in place.  I'd
simply like us to stop making assumptions about user space always doing
the right thing.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Mike Snitzer]

On Fri, Jan 08 2010 at  1:41pm -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> Mike,
> 
> Mike> Seems I took the time to add a comment whose FIXME doesn't ring
> Mike> many bells now!  But ignoring that, the comment before the FIXME
> Mike> is making a veiled reference to userspace having consumed
> Mike> alignment_offset.
> 
> I read and understood the comment.  But even then explicitly zeroing out
> those two values didn't make sense (because if user space did in fact do
> the right thing they'd always be zero).
> 
> I know that the DM utilities take care of business.  And that DM devices
> are special because they are always set up by user space and not a
> kernel discovery process.
> 
> But since the code *is* in place to validate things I'm not so keen on
> you clearing fields that have been calculated and have a meaning.  For
> me it masked a case where the DM utilities did the wrong thing (because
> they were old).
> 
> With the Enterprise Linux hat on it is easy for us to specify that you
> must use this version of the kernel and the device mapper utilities.
> But reality is that lots of people are running upstream kernels on
> distributions with older userland.  And some distributions get things
> wrong, ship broken bits, etc.
> 
> It's great that new DM utils will transparently adjust the starting
> offset.  That's the way it's supposed to work.  No arguments there.
> 
> My main concern is making sure that we never get into a case where we
> run with misaligned components without indicating that there is a
> problem.  Ever!  Regardless of which DM utils might be in place.
> 
> We have the power to get that right.  All the pieces are in place.  I'd
> simply like us to stop making assumptions about user space always doing
> the right thing.

Yeap, I already agreed that those 2 lines should be removed (in the
p.s. of my previous mail).  They really aren't serving any purpose and
their justification was/is tenuous at best.

I'll get the fix, for 2.6.33, to Alasdair shortly.

Mike

[PATCH] block: bdev_stack_limits wrapper and DM topology fixes

[Mike Snitzer]

Martin,

On Wed, Jan 06 2010 at  8:25am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Wed, Jan 06 2010 at  3:39am -0500,
> Martin K. Petersen <martin.petersen@oracle.com> wrote:
>
> > You'll notice that bdev_stack_limits takes a sector offset as argument.
> > I have a patch that I intend to push at a later date that will convert
> > the remaining stacking functions to take soft sector offsets instead of
> > bytes.  I deal with the appropriate conversion in the alignment
> > calculation.  That way we avoid the blk_off_t goo entirely and it makes
> > the difference between absolute offsets and alignment offsets crystal
> > clear.
> 
> Looks good.  But you're missing the EXPORT_SYMBOL(bdev_stack_limits)

In that you're carrying the dm_set_device_limits() change as part of
this patch:
http://oss.oracle.com/git/?p=mkp/linux-2.6.git;a=commit;h=f3bc9c611895

Any chance you'd shepherd this revised patch to Jens?  It includes
various DM topology cleanups that we've discussed as well as the
EXPORT_SYMBOL(bdev_stack_limits).

The dm_set_device_limits DMWARN() cleanup is the line after your 
sector change so it'd require coordination.  I'm purposely displaying
the 'start' in bytes because all other topology limit values in that
DMWARN are in bytes.  [and yes it's > 80-char, linus is OK with that now
if it "makes sense"]

The 2nd and 3rd hunks of the dm-table.c changes respectively offer
another topology DMWARN cleanup and the removal of the alignment_offset
and misaligned resets that we discussed extensively.

If you'd rather not take those 2 hunks I can queue them with Alasdair
but I figured we could just add them to this patch.

Let me know, thanks.


From: Martin K. Petersen <martin.petersen@oracle.com>

block: bdev_stack_limits wrapper and DM topology fixes

DM does not want to know about partition offsets.  Add a partition-aware
wrapper that DM can use when stacking block devices.

Also remove the clearing of alignment_offset and misaligned in
dm_table_set_restrictions().  Adjust some misleading topology DMWARN
messages too.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Cc: Alasdair G. Kergon <agk@redhat.com>
---
 block/blk-settings.c   |   22 ++++++++++++++++++++++
 drivers/md/dm-table.c  |   20 +++++---------------
 include/linux/blkdev.h |    2 ++
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index d52d4ad..0d8de4e 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -631,6 +631,28 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 EXPORT_SYMBOL(blk_stack_limits);
 
 /**
+ * bdev_stack_limits - adjust queue limits for stacked drivers
+ * @t:	the stacking driver limits (top device)
+ * @bdev:  the component block_device (bottom)
+ * @start:  first data sector within component device
+ *
+ * Description:
+ *    Merges queue limits for a top device and a block_device.  Returns
+ *    0 if alignment didn't change.  Returns -1 if adding the bottom
+ *    device caused misalignment.
+ */
+int bdev_stack_limits(struct queue_limits *t, struct block_device *bdev,
+		      sector_t start)
+{
+	struct request_queue *bq = bdev_get_queue(bdev);
+
+	start += get_start_sect(bdev);
+
+	return blk_stack_limits(t, &bq->limits, start << 9);
+}
+EXPORT_SYMBOL(bdev_stack_limits);
+
+/**
  * disk_stack_limits - adjust queue limits for stacked drivers
  * @disk:  MD/DM gendisk (top)
  * @bdev:  the underlying block device (bottom)
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index be62547..4b22feb 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -503,16 +503,15 @@ int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
 		return 0;
 	}
 
-	if (blk_stack_limits(limits, &q->limits, start << 9) < 0)
-		DMWARN("%s: target device %s is misaligned: "
+	if (bdev_stack_limits(limits, bdev, start) < 0)
+		DMWARN("%s: adding target device %s caused an alignment inconsistency: "
 		       "physical_block_size=%u, logical_block_size=%u, "
 		       "alignment_offset=%u, start=%llu",
 		       dm_device_name(ti->table->md), bdevname(bdev, b),
 		       q->limits.physical_block_size,
 		       q->limits.logical_block_size,
 		       q->limits.alignment_offset,
-		       (unsigned long long) start << 9);
-
+		       (unsigned long long) start << SECTOR_SHIFT);
 
 	/*
 	 * Check if merge fn is supported.
@@ -1026,9 +1025,9 @@ combine_limits:
 		 * for the table.
 		 */
 		if (blk_stack_limits(limits, &ti_limits, 0) < 0)
-			DMWARN("%s: target device "
+			DMWARN("%s: adding target device "
 			       "(start sect %llu len %llu) "
-			       "is misaligned",
+			       "caused an alignment inconsistency",
 			       dm_device_name(table->md),
 			       (unsigned long long) ti->begin,
 			       (unsigned long long) ti->len);
@@ -1080,15 +1079,6 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			       struct queue_limits *limits)
 {
 	/*
-	 * Each target device in the table has a data area that should normally
-	 * be aligned such that the DM device's alignment_offset is 0.
-	 * FIXME: Propagate alignment_offsets up the stack and warn of
-	 *	  sub-optimal or inconsistent settings.
-	 */
-	limits->alignment_offset = 0;
-	limits->misaligned = 0;
-
-	/*
 	 * Copy table's limits to the DM device's request_queue
 	 */
 	q->limits = *limits;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9b98173..70b7ede 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -938,6 +938,8 @@ extern void blk_queue_io_opt(struct request_queue *q, unsigned int opt);
 extern void blk_set_default_limits(struct queue_limits *lim);
 extern int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 			    sector_t offset);
+extern int bdev_stack_limits(struct queue_limits *t, struct block_device *bdev,
+			    sector_t offset);
 extern void disk_stack_limits(struct gendisk *disk, struct block_device *bdev,
 			      sector_t offset);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);

Re: [PATCH v2] dm: Fix alignment stacking on partitioned devices

[Alasdair G Kergon]

On Wed, Jan 06, 2010 at 03:39:12AM -0500, Martin K. Petersen wrote:
> Alasdair, if you are OK with the approach below I propose you give an
> Acked-by: and then we can feed the patch through Jens instead of dealing
> with a clunky two-stage merge this late in the .33 game.
 
> DM does not want to know about partition offsets.  Add a partition-aware
> wrapper that DM can use when stacking block devices.
 
> +int bdev_stack_limits(struct queue_limits *t, struct block_device *bdev,
> +		      sector_t start)

Reviewed-by: Alasdair G Kergon <agk@redhat.com>

Alasdair