linux-next: block tree build failure

linux-next: block tree build failure

[Stephen Rothwell]

Hi Jens,

Today's linux-next build (powerpc ppc64_defconfig) failed like this:

drivers/md/dm.c: In function 'dm_merge_bvec':
drivers/md/dm.c:976: error: 'struct request_queue' has no member named 'max_hw_sectors'

Caused by commit 025146e13b63483add912706c101fb0fb6f015cc ("block: Move
queue limits to an embedded struct") from the block tree interacting with
commit c8f4b88baca86491a6125cf9373a75e04b22ff8b
("dm-avoid-unsupported-spanning-of-md-stripe-boundaries") from the
device-mapper tree.

Another sudden API change ...

The correct way:
	add new accessor implemented in terms of old API that is a noop
	push that to Linus
	fixup all the references to the old API (telling the users)
	wait for other users (if any) to use new accessor
	remove old API

We have done this several times ...  Linus is will to take the "add new
accessor" patches basically any time.

So I added this patch:

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7b939fa..ea9a9e6 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -973,7 +973,7 @@ static int dm_merge_bvec(struct request_queue *q,
 	 * entries.  So always set max_size to 0, and the code below allows
 	 * just one page.
 	 */
-	else if (q->max_hw_sectors <= PAGE_SIZE >> 9)
+	else if (queue_max_hw_sectors(q) <= PAGE_SIZE >> 9)
 		max_size = 0;
 
 out_table:

But now I get:

drivers/md/dm-table.c: In function 'device_area_is_valid':
drivers/md/dm-table.c:390: error: 'struct io_restrictions' has no member named 'hardsect_size'
drivers/md/dm-table.c:414: error: 'struct io_restrictions' has no member named 'hardsect_size'
drivers/md/dm-table.c: In function 'validate_hardsect_alignment':
drivers/md/dm-table.c:738: error: 'struct io_restrictions' has no member named 'hardsect_size'
drivers/md/dm-table.c:765: error: 'struct io_restrictions' has no member named 'hardsect_size'
drivers/md/dm-table.c:777: error: 'struct io_restrictions' has no member named 'hardsect_size'

This is caused by commit e1defc4ff0cf57aca6c5e3ff99fa503f5943c1f1
("block: Do away with the notion of hardsect_size") from the block tree
interacting with commits cc1018ae8a413b595a1f0f822928dd9e81a75e59
("dm-table-ensure-targets-are-aligned-to-hardsect_size") and
64b184139ca6cd3d53dc45d7782c8be50b3e0331
("dm-table-validate-device-hardsect_size") from the device-mapper tree.

So I added the following patch as well.

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index d38b911..8f734ad 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -387,7 +387,7 @@ static int device_area_is_valid(struct dm_target *ti, struct block_device *bdev,
 			     sector_t start, sector_t len)
 {
 	sector_t dev_size = bdev->bd_inode->i_size >> SECTOR_SHIFT;
-	unsigned short hardsect_size_sectors = ti->limits.hardsect_size >>
+	unsigned short hardsect_size_sectors = ti->limits.logical_block_size >>
 					       SECTOR_SHIFT;
 	char b[BDEVNAME_SIZE];
 
@@ -414,7 +414,7 @@ static int device_area_is_valid(struct dm_target *ti, struct block_device *bdev,
 		DMWARN("%s: len=%llu not aligned to h/w sector size %hu of %s",
 		       dm_device_name(ti->table->md),
 		       (unsigned long long)len,
-		       ti->limits.hardsect_size, bdevname(bdev, b));
+		       ti->limits.logical_block_size, bdevname(bdev, b));
 		return 0;
 	}
 
@@ -735,7 +735,7 @@ static int validate_hardsect_alignment(struct dm_table *table)
 	 * (in units of 512-byte sectors).
 	 */
 	unsigned short device_hardsect_size_sects =
-			    table->limits.hardsect_size >> SECTOR_SHIFT;
+			    table->limits.logical_block_size >> SECTOR_SHIFT;
 
 	/*
 	 * Offset of the start of the next table entry, mod hardsect_size.
@@ -762,7 +762,7 @@ static int validate_hardsect_alignment(struct dm_table *table)
 		 * table entry are they compatible with its hardsect_size?
 		 */
 		if (remaining < ti->len &&
-		    remaining & ((ti->limits.hardsect_size >>
+		    remaining & ((ti->limits.logical_block_size >>
 				  SECTOR_SHIFT) - 1))
 			break;	/* Error */
 
@@ -779,7 +779,7 @@ static int validate_hardsect_alignment(struct dm_table *table)
 		       dm_device_name(table->md), i,
 		       (unsigned long long) ti->begin,
 		       (unsigned long long) ti->len,
-		       table->limits.hardsect_size);
+		       table->limits.logical_block_size);
 		return -EINVAL;
 	}
 

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Re: linux-next: block tree build failure

[Martin K. Petersen]

>>>>> "Stephen" == Stephen Rothwell <sfr@canb.auug.org.au> writes:

Stephen> Caused by commit 025146e13b63483add912706c101fb0fb6f015cc
Stephen> ("block: Move queue limits to an embedded struct") from the
Stephen> block tree interacting with commit
Stephen> c8f4b88baca86491a6125cf9373a75e04b22ff8b
Stephen> ("dm-avoid-unsupported-spanning-of-md-stripe-boundaries") from
Stephen> the device-mapper tree.

Stephen> This is caused by commit
Stephen> e1defc4ff0cf57aca6c5e3ff99fa503f5943c1f1 ("block: Do away with
Stephen> the notion of hardsect_size") from the block tree interacting
Stephen> with commits cc1018ae8a413b595a1f0f822928dd9e81a75e59
Stephen> ("dm-table-ensure-targets-are-aligned-to-hardsect_size") and
Stephen> 64b184139ca6cd3d53dc45d7782c8be50b3e0331
Stephen> ("dm-table-validate-device-hardsect_size") from the
Stephen> device-mapper tree.

Bummer. I wasn't aware Alasdair had taken those patches into his DM tree
yet.  I thought he was waiting for my block layer patches to land.

The accessor function patch in my patch series was explicitly put in
place to enable changing the API without affecting users.  And we've
tried to be careful about staging these patches in the right order
throughout all the involved trees.

Sorry about the slip-up :|

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: linux-next: block tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

Hi Martin,

On Mon, 25 May 2009 01:38:26 -0400 "Martin K. Petersen" <martin.petersen@oracle.com> wrote:
>
> The accessor function patch in my patch series was explicitly put in
> place to enable changing the API without affecting users.  And we've
> tried to be careful about staging these patches in the right order
> throughout all the involved trees.

What you really need is to have the patches that introduce the accessors
in a tree common to all the possible users of them.  In practise this
often means Linus' tree - in which case the patches should introduce noop
versions of the accessors.  If that is possible, Linus is quite happy to
take those patches after which all the other users can take the
conversions into their own tree and everyone is happy.  :-)

Currently the patches that introduce the accessors only exist in the
block tree ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[RFC] updated DM table alignment validation patches [Was: Re: linux-next: block tree build failure]

[Mike Snitzer]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On Mon, May 25 2009 at  1:54am -0400,
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Martin,
> 
> On Mon, 25 May 2009 01:38:26 -0400 "Martin K. Petersen" <martin.petersen@oracle.com> wrote:
> >
> > The accessor function patch in my patch series was explicitly put in
> > place to enable changing the API without affecting users.  And we've
> > tried to be careful about staging these patches in the right order
> > throughout all the involved trees.
> 
> What you really need is to have the patches that introduce the accessors
> in a tree common to all the possible users of them.  In practise this
> often means Linus' tree - in which case the patches should introduce noop
> versions of the accessors.  If that is possible, Linus is quite happy to
> take those patches after which all the other users can take the
> conversions into their own tree and everyone is happy.  :-)
> 
> Currently the patches that introduce the accessors only exist in the
> block tree ...

I've refreshed the DM linux-next patches that were off Martin's radar;
they are available here (and are also attached):

http://people.redhat.com/msnitzer/patches/dm-topology/dm-table-ensure-targets-are-aligned-to-logical_block_size.patch
http://people.redhat.com/msnitzer/patches/dm-topology/dm-table-validate-device-logical_block_size.patch

These updated patches still seem useful (as negative checks independent
of the topology code).

There is more work that is needed in DM and LVM2 userspace in order to
properly support and utilize the new topology infrastructure.  So I'm
also taking a much closer look at the needed changes now and should have
it sorted out in the coming days.

Mike



[-- Attachment #2: dm-table-ensure-targets-are-aligned-to-logical_block_size.patch --]
[-- Type: text/plain, Size: 2872 bytes --]

From: Mike Snitzer <snitzer@redhat.com>

Ensure I/O is aligned to the logical block size of target devices.

Rename check_device_area() to device_area_is_valid() for clarity and
establish the device limits including the logical block size prior to
calling it.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>

---
 drivers/md/dm-table.c |   54 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 13 deletions(-)

Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c
+++ linux-2.6/drivers/md/dm-table.c
@@ -385,15 +385,42 @@ static void close_dev(struct dm_dev_inte
 /*
  * If possible, this checks an area of a destination device is valid.
  */
-static int check_device_area(struct dm_dev_internal *dd, sector_t start,
-			     sector_t len)
+static int device_area_is_valid(struct dm_target *ti, struct block_device *bdev,
+			     sector_t start, sector_t len)
 {
-	sector_t dev_size = dd->dm_dev.bdev->bd_inode->i_size >> SECTOR_SHIFT;
+	sector_t dev_size = bdev->bd_inode->i_size >> SECTOR_SHIFT;
+	unsigned short logical_block_size_sectors =
+		ti->limits.logical_block_size >> SECTOR_SHIFT;
+	char b[BDEVNAME_SIZE];
 
 	if (!dev_size)
 		return 1;
 
-	return ((start < dev_size) && (len <= (dev_size - start)));
+	if ((start >= dev_size) || (start + len > dev_size)) {
+		DMWARN("%s: %s too small for target",
+		       dm_device_name(ti->table->md), bdevname(bdev, b));
+		return 0;
+	}
+
+	if (logical_block_size_sectors <= 1)
+		return 1;
+
+	if (start & (logical_block_size_sectors - 1)) {
+		DMWARN("%s: start=%llu not aligned to h/w sector of %s",
+		       dm_device_name(ti->table->md),
+		       (unsigned long long)start, bdevname(bdev, b));
+		return 0;
+	}
+
+	if (len & (logical_block_size_sectors - 1)) {
+		DMWARN("%s: len=%llu not aligned to h/w sector size %hu of %s",
+		       dm_device_name(ti->table->md),
+		       (unsigned long long)len,
+		       ti->limits.logical_block_size, bdevname(bdev, b));
+		return 0;
+	}
+
+	return 1;
 }
 
 /*
@@ -479,14 +506,7 @@ static int __table_get_device(struct dm_
 	}
 	atomic_inc(&dd->count);
 
-	if (!check_device_area(dd, start, len)) {
-		DMWARN("device %s too small for target", path);
-		dm_put_device(ti, &dd->dm_dev);
-		return -EINVAL;
-	}
-
 	*result = &dd->dm_dev;
-
 	return 0;
 }
 
@@ -555,8 +575,16 @@ int dm_get_device(struct dm_target *ti, 
 	int r = __table_get_device(ti->table, ti, path,
 				   start, len, mode, result);
 
-	if (!r)
-		dm_set_device_limits(ti, (*result)->bdev);
+	if (r)
+		return r;
+
+	dm_set_device_limits(ti, (*result)->bdev);
+
+	if (!device_area_is_valid(ti, (*result)->bdev, start, len)) {
+		dm_put_device(ti, *result);
+		*result = NULL;
+		return -EINVAL;
+	}
 
 	return r;
 }

[-- Attachment #3: dm-table-validate-device-logical_block_size.patch --]
[-- Type: text/plain, Size: 3175 bytes --]

From: Mike Snitzer <snitzer@redhat.com>

Impose necessary and sufficient conditions on a devices's table such
that any incoming bio which respects its logical_block_size can be
processed successfully.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>

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

Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c
+++ linux-2.6/drivers/md/dm-table.c
@@ -723,6 +723,71 @@ static void check_for_valid_limits(struc
 		rs->bounce_pfn = -1;
 }
 
+/*
+ * Impose necessary and sufficient conditions on a devices's table such
+ * that any incoming bio which respects its logical_block_size can be
+ * processed successfully.  If it falls across the boundary between
+ * two or more targets, the size of each piece it gets split into must
+ * be compatible with the logical_block_size of the target processing it.
+ */
+static int validate_hardware_logical_block_alignment(struct dm_table *table)
+{
+	/*
+	 * This function uses arithmetic modulo the logical_block_size
+	 * (in units of 512-byte sectors).
+	 */
+	unsigned short device_logical_block_size_sects =
+		table->limits.logical_block_size >> SECTOR_SHIFT;
+
+	/*
+	 * Offset of the start of the next table entry, mod logical_block_size.
+	 */
+	unsigned short next_target_start = 0;
+
+	/*
+	 * Given an aligned bio that extends beyond the end of a
+	 * target, how many sectors must the next target handle?
+	 */
+	unsigned short remaining = 0;
+
+	struct dm_target *uninitialized_var(ti);
+	unsigned i = 0;
+
+	/*
+	 * Check each entry in the table in turn.
+	 */
+	while (i < dm_table_get_num_targets(table)) {
+		ti = dm_table_get_target(table, i++);
+
+		/*
+		 * If the remaining sectors fall entirely within this
+		 * table entry are they compatible with its logical_block_size?
+		 */
+		if (remaining < ti->len &&
+		    remaining & ((ti->limits.logical_block_size >>
+				  SECTOR_SHIFT) - 1))
+			break;	/* Error */
+
+		next_target_start =
+		    (unsigned short) ((next_target_start + ti->len) &
+				      (device_logical_block_size_sects - 1));
+		remaining = next_target_start ?
+		    device_logical_block_size_sects - next_target_start : 0;
+	}
+
+	if (remaining) {
+		DMWARN("%s: table line %u (start sect %llu len %llu) "
+		       "not aligned to hardware logical block size %hu",
+		       dm_device_name(table->md), i,
+		       (unsigned long long) ti->begin,
+		       (unsigned long long) ti->len,
+		       table->limits.logical_block_size);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int dm_table_add_target(struct dm_table *t, const char *type,
 			sector_t start, sector_t len, char *params)
 {
@@ -822,6 +887,10 @@ int dm_table_complete(struct dm_table *t
 
 	check_for_valid_limits(&t->limits);
 
+	r = validate_hardware_logical_block_alignment(t);
+	if (r)
+		return r;
+
 	/* how many indexes will the btree have ? */
 	leaf_nodes = dm_div_up(t->num_targets, KEYS_PER_NODE);
 	t->depth = 1 + int_log(leaf_nodes, CHILDREN_PER_NODE);

[-- Attachment #4: Type: text/plain, Size: 0 bytes --]