[PATCH] dm thin: relax hard limit on the maximum size of a metadata device

[PATCH] dm thin: relax hard limit on the maximum size of a metadata device

[Mike Snitzer]

The thin metadata format can only make use of a device that is <=
METADATA_DEV_MAX_SECTORS (currently 15.9375 GB).  Therefore, there is no
practical benefit to using a larger device.

However, it may be that other factors impose a certain granularity for
the space that is allocated to a device (E.g. lvm2 can impose a coarse
granularity through the use of large, >= 1 GB, physical extents).

Rather than reject a larger metadata device, during thin-pool device
construction, switch to allowing it but issue a warning if a device
larger than METADATA_DEV_MAX_SECTORS_NEAREST_POWER_OF_2 (16 GB) is
provided.  Any space over 15.9375 GB will not be used.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 Documentation/device-mapper/thin-provisioning.txt |    8 +++++---
 drivers/md/dm-thin-metadata.c                     |    5 ++++-
 drivers/md/dm-thin-metadata.h                     |   15 +++++++++++++++
 drivers/md/dm-thin.c                              |   18 ++++--------------
 4 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/Documentation/device-mapper/thin-provisioning.txt b/Documentation/device-mapper/thin-provisioning.txt
index 57076dd..ca54bac 100644
--- a/Documentation/device-mapper/thin-provisioning.txt
+++ b/Documentation/device-mapper/thin-provisioning.txt
@@ -75,10 +75,12 @@ less sharing than average you'll need a larger-than-average metadata device.
 
 As a guide, we suggest you calculate the number of bytes to use in the
 metadata device as 48 * $data_dev_size / $data_block_size but round it up
-to 2MB if the answer is smaller.  The largest size supported is 16GB.
+to 2MB if the answer is smaller.  If you're creating large numbers of
+snapshots which are recording large amounts of change, you may find you
+need to increase this.
 
-If you're creating large numbers of snapshots which are recording large
-amounts of change, you may need find you need to increase this.
+The largest size supported, without a warning about space being wasted,
+is 16GB.
 
 Reloading a pool table
 ----------------------
diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
index f3ba61d..1783741 100644
--- a/drivers/md/dm-thin-metadata.c
+++ b/drivers/md/dm-thin-metadata.c
@@ -718,7 +718,10 @@ struct dm_pool_metadata *dm_pool_metadata_open(struct block_device *bdev,
 	disk_super->version = cpu_to_le32(THIN_VERSION);
 	disk_super->time = 0;
 	disk_super->metadata_block_size = cpu_to_le32(THIN_METADATA_BLOCK_SIZE >> SECTOR_SHIFT);
-	disk_super->metadata_nr_blocks = cpu_to_le64(bdev_size >> SECTOR_TO_BLOCK_SHIFT);
+	if (bdev_size > METADATA_DEV_MAX_SECTORS)
+		disk_super->metadata_nr_blocks = cpu_to_le64(METADATA_DEV_MAX_SECTORS >> SECTOR_TO_BLOCK_SHIFT);	
+	else
+		disk_super->metadata_nr_blocks = cpu_to_le64(bdev_size >> SECTOR_TO_BLOCK_SHIFT);
 	disk_super->data_block_size = cpu_to_le32(data_block_size);
 
 	r = dm_bm_unlock(sblock);
diff --git a/drivers/md/dm-thin-metadata.h b/drivers/md/dm-thin-metadata.h
index cfc7d0b..df290fc 100644
--- a/drivers/md/dm-thin-metadata.h
+++ b/drivers/md/dm-thin-metadata.h
@@ -11,6 +11,21 @@
 
 #define THIN_METADATA_BLOCK_SIZE 4096
 
+/*
+ * The metadata device is currently limited in size.
+ *
+ * We have one block of index, which can hold 255 index entries.  Each
+ * index entry contains allocation info about 16k metadata blocks.
+ */
+#define METADATA_DEV_MAX_SECTORS (255 * (1 << 14) * (THIN_METADATA_BLOCK_SIZE / (1 << SECTOR_SHIFT)))
+
+/*
+ * Userspace may need to provide a metadata device that is larger than
+ * METADATA_DEV_MAX_SECTORS.  Anything larger than the power of 2 nearest
+ * to METADATA_DEV_MAX_SECTORS will trigger a warning.
+ */
+#define METADATA_DEV_MAX_SECTORS_NEAREST_POWER_OF_2 (16 * (1024 * 1024 * 1024 >> SECTOR_SHIFT))
+
 /*----------------------------------------------------------------*/
 
 struct dm_pool_metadata;
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 2b1d5bd..d9ea643 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -33,16 +33,6 @@
 #define DATA_DEV_BLOCK_SIZE_MAX_SECTORS (1024 * 1024 * 1024 >> SECTOR_SHIFT)
 
 /*
- * The metadata device is currently limited in size.  The limitation is
- * checked lower down in dm-space-map-metadata, but we also check it here
- * so we can fail early.
- *
- * We have one block of index, which can hold 255 index entries.  Each
- * index entry contains allocation info about 16k metadata blocks.
- */
-#define METADATA_DEV_MAX_SECTORS (255 * (1 << 14) * (THIN_METADATA_BLOCK_SIZE / (1 << SECTOR_SHIFT)))
-
-/*
  * Device id is restricted to 24 bits.
  */
 #define MAX_DEV_ID ((1 << 24) - 1)
@@ -1927,10 +1917,10 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	}
 
 	metadata_dev_size = i_size_read(metadata_dev->bdev->bd_inode) >> SECTOR_SHIFT;
-	if (metadata_dev_size > METADATA_DEV_MAX_SECTORS) {
-		ti->error = "Metadata device is too large";
-		r = -EINVAL;
-		goto out_metadata;
+	if (metadata_dev_size > METADATA_DEV_MAX_SECTORS_NEAREST_POWER_OF_2) {
+		char b[BDEVNAME_SIZE];
+		DMWARN("Metadata device %s is larger than %u sectors, excess space will not be used",
+		       bdevname(metadata_dev->bdev, b), METADATA_DEV_MAX_SECTORS);
 	}
 
 	r = dm_get_device(ti, argv[1], FMODE_READ | FMODE_WRITE, &data_dev);
-- 
1.7.1

Re: [PATCH] dm thin: relax hard limit on the maximum size of a metadata device

[Joe Thornber]

Hi Mike,

My concerns are:

i) The current behaviour is upstream; by changing this aren't you
   making the tools writers life more complicated rather than less by
   making them support both interfaces?

ii) 16G is a ludicrous amount of space to allocate for metadata (16M
    would be much better).  Why are the tools trying to allocate this
    much?  LVM2's unit of _allocation_ may be the extent, but this is
    separate from activation.  If your extent size is 16G you can
    probably squeeze 1000 metadata areas into there.

    As a side issue it's not clear to me why anyone would want to use
    16G extents?  (eg, 16M extents lets them address 2^56 bytes of
    data in the VG).  Maybe the sys admins mistakenly think they're
    getting performance benefits through having more contiguous data?
    [LVM2's allocation policies try and allocate contiguous extents
    anyway].

If you can reassure me about (i) and that your patch isn't encouraging
poor tool code (ii), then I'll ack this patch.

- Joe


On Fri, Mar 02, 2012 at 04:32:33PM -0500, Mike Snitzer wrote:
> The thin metadata format can only make use of a device that is <=
> METADATA_DEV_MAX_SECTORS (currently 15.9375 GB).  Therefore, there is no
> practical benefit to using a larger device.
> 
> However, it may be that other factors impose a certain granularity for
> the space that is allocated to a device (E.g. lvm2 can impose a coarse
> granularity through the use of large, >= 1 GB, physical extents).
> 
> Rather than reject a larger metadata device, during thin-pool device
> construction, switch to allowing it but issue a warning if a device
> larger than METADATA_DEV_MAX_SECTORS_NEAREST_POWER_OF_2 (16 GB) is
> provided.  Any space over 15.9375 GB will not be used.
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

Re: dm thin: relax hard limit on the maximum size of a metadata device

[Mike Snitzer]

On Mon, Mar 05 2012 at  5:21am -0500,
Joe Thornber <thornber@redhat.com> wrote:

> Hi Mike,
> 
> My concerns are:
> 
> i) The current behaviour is upstream; by changing this aren't you
>    making the tools writers life more complicated rather than less by
>    making them support both interfaces?

It is an incremental improvement.  Allows the kernel to be forgiving.
How does this impact some tool that took the special care to limit the
size of the device to METADATA_DEV_MAX_SECTORS (which is < 16G)?

I'm not imposing new or conflicting restrictions that would trip up any
existing/hypothetical tools.

> ii) 16G is a ludicrous amount of space to allocate for metadata (16M
>     would be much better).  Why are the tools trying to allocate this
>     much?  LVM2's unit of _allocation_ may be the extent, but this is
>     separate from activation.  If your extent size is 16G you can
>     probably squeeze 1000 metadata areas into there.
> 
>     As a side issue it's not clear to me why anyone would want to use
>     16G extents?  (eg, 16M extents lets them address 2^56 bytes of
>     data in the VG).  Maybe the sys admins mistakenly think they're
>     getting performance benefits through having more contiguous data?
>     [LVM2's allocation policies try and allocate contiguous extents
>     anyway].

Whatever the tools may be doing is not my concern.  Ideally the users
and tool authors understand that 16G is insane for thinp metadata.  But
in the event that they use 16G would you rather we reject them?
I do think so, especially given that we've already documented that 16G
is the max.
 
> If you can reassure me about (i) and that your patch isn't encouraging
> poor tool code (ii), then I'll ack this patch.

OK... let me know if I passed ;)

Mike

Re: dm thin: relax hard limit on the maximum size of a metadata device

[Mike Snitzer]

On Mon, Mar 05 2012 at  9:04am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Mon, Mar 05 2012 at  5:21am -0500,
> Joe Thornber <thornber@redhat.com> wrote:
> 
> > Hi Mike,
> > 
> > My concerns are:
> > 
> > i) The current behaviour is upstream; by changing this aren't you
> >    making the tools writers life more complicated rather than less by
> >    making them support both interfaces?
> 
> It is an incremental improvement.  Allows the kernel to be forgiving.
> How does this impact some tool that took the special care to limit the
> size of the device to METADATA_DEV_MAX_SECTORS (which is < 16G)?
> 
> I'm not imposing new or conflicting restrictions that would trip up any
> existing/hypothetical tools.
> 
> > ii) 16G is a ludicrous amount of space to allocate for metadata (16M
> >     would be much better).  Why are the tools trying to allocate this
> >     much?  LVM2's unit of _allocation_ may be the extent, but this is
> >     separate from activation.  If your extent size is 16G you can
> >     probably squeeze 1000 metadata areas into there.
> > 
> >     As a side issue it's not clear to me why anyone would want to use
> >     16G extents?  (eg, 16M extents lets them address 2^56 bytes of
> >     data in the VG).  Maybe the sys admins mistakenly think they're
> >     getting performance benefits through having more contiguous data?
> >     [LVM2's allocation policies try and allocate contiguous extents
> >     anyway].
> 
> Whatever the tools may be doing is not my concern.  Ideally the users
> and tool authors understand that 16G is insane for thinp metadata.  But
> in the event that they use 16G would you rather we reject them?
> I do think so, especially given that we've already documented that 16G
> is the max.

I clearly meant "I do _not_ think so" ;)

Re: dm thin: relax hard limit on the maximum size of a metadata device

[Joe Thornber]

On Mon, Mar 05, 2012 at 09:04:21AM -0500, Mike Snitzer wrote:
> On Mon, Mar 05 2012 at  5:21am -0500,
> Joe Thornber <thornber@redhat.com> wrote:
> 
> > Hi Mike,
> > 
> > My concerns are:
> > 
> > i) The current behaviour is upstream; by changing this aren't you
> >    making the tools writers life more complicated rather than less by
> >    making them support both interfaces?
> 
> It is an incremental improvement.  Allows the kernel to be forgiving.
> How does this impact some tool that took the special care to limit the
> size of the device to METADATA_DEV_MAX_SECTORS (which is < 16G)?

You're making this change to make life easier for tool writers, yet
tool writers still have to support the existing 3.2 kernel and deal
with the 16G limit.

> Whatever the tools may be doing is not my concern.  Ideally the users
> and tool authors understand that 16G is insane for thinp metadata.  But
> in the event that they use 16G would you rather we reject them?

Yes, I would rather reject, than let people think they had 32G of
metadata.  It also forces the tool writers to do something sane.

I don't feel strongly enough about this to keep arguing.  So consider
this an ACK and see if you can get it past Alasdair.

- Joe