[PATCH] dm snapshot: use unsigned integer chunk size

[PATCH] dm snapshot: use unsigned integer chunk size

[Mike Snitzer]

Use unsigned integer chunk size.

Maximum chunk size is 512kB, there won't ever be need to use 4GB chunk size,
so the number can be 32-bit. This fixes compiler failure on 32-bit systems
with large block devices.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Reviewed-by: Jonathan Brassow <jbrassow@redhat.com>

---
NOTE: this patch was made to go _before_ the snapshot exception
patchset; I'll be sending [v2] of 2 patches from that series.  They
needed small adjustments now that this chunk_size patch comes before
them.
---
 drivers/md/dm-exception-store.c |   23 +++++++++++------------
 drivers/md/dm-exception-store.h |    8 ++++----
 drivers/md/dm-snap-persistent.c |   16 ++++++++--------
 drivers/md/dm-snap.c            |    4 ++--
 4 files changed, 25 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/md/dm-snap.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-snap.c
+++ linux-2.6/drivers/md/dm-snap.c
@@ -527,7 +527,7 @@ static int dm_add_exception(void *contex
 static sector_t __minimum_chunk_size(struct origin *o)
 {
 	struct dm_snapshot *snap;
-	sector_t chunk_size = 0;
+	unsigned chunk_size = 0;
 
 	if (o)
 		list_for_each_entry(snap, &o->snapshots, list)
@@ -975,7 +975,7 @@ static void start_copy(struct dm_snap_pe
 
 	src.bdev = bdev;
 	src.sector = chunk_to_sector(s->store, pe->e.old_chunk);
-	src.count = min(s->store->chunk_size, dev_size - src.sector);
+	src.count = min((sector_t)s->store->chunk_size, dev_size - src.sector);
 
 	dest.bdev = s->store->cow->bdev;
 	dest.sector = chunk_to_sector(s->store, pe->e.new_chunk);
Index: linux-2.6/drivers/md/dm-exception-store.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-exception-store.c
+++ linux-2.6/drivers/md/dm-exception-store.c
@@ -141,48 +141,47 @@ EXPORT_SYMBOL(dm_exception_store_type_un
 static int set_chunk_size(struct dm_exception_store *store,
 			  const char *chunk_size_arg, char **error)
 {
-	unsigned long chunk_size_ulong;
+	unsigned chunk_size;
 	char *value;
 
-	chunk_size_ulong = simple_strtoul(chunk_size_arg, &value, 10);
+	chunk_size = simple_strtoul(chunk_size_arg, &value, 10);
 	if (*chunk_size_arg == '\0' || *value != '\0') {
 		*error = "Invalid chunk size";
 		return -EINVAL;
 	}
 
-	if (!chunk_size_ulong) {
+	if (!chunk_size) {
 		store->chunk_size = store->chunk_mask = store->chunk_shift = 0;
 		return 0;
 	}
 
-	return dm_exception_store_set_chunk_size(store, chunk_size_ulong,
-						 error);
+	return dm_exception_store_set_chunk_size(store, chunk_size, error);
 }
 
 int dm_exception_store_set_chunk_size(struct dm_exception_store *store,
-				      unsigned long chunk_size_ulong,
+				      unsigned chunk_size,
 				      char **error)
 {
 	/* Check chunk_size is a power of 2 */
-	if (!is_power_of_2(chunk_size_ulong)) {
+	if (!is_power_of_2(chunk_size)) {
 		*error = "Chunk size is not a power of 2";
 		return -EINVAL;
 	}
 
 	/* Validate the chunk size against the device block size */
-	if (chunk_size_ulong % (bdev_logical_block_size(store->cow->bdev) >> 9)) {
+	if (chunk_size % (bdev_logical_block_size(store->cow->bdev) >> 9)) {
 		*error = "Chunk size is not a multiple of device blocksize";
 		return -EINVAL;
 	}
 
-	if (chunk_size_ulong > INT_MAX >> SECTOR_SHIFT) {
+	if (chunk_size > INT_MAX >> SECTOR_SHIFT) {
 		*error = "Chunk size is too high";
 		return -EINVAL;
 	}
 
-	store->chunk_size = chunk_size_ulong;
-	store->chunk_mask = chunk_size_ulong - 1;
-	store->chunk_shift = ffs(chunk_size_ulong) - 1;
+	store->chunk_size = chunk_size;
+	store->chunk_mask = chunk_size - 1;
+	store->chunk_shift = ffs(chunk_size) - 1;
 
 	return 0;
 }
Index: linux-2.6/drivers/md/dm-exception-store.h
===================================================================
--- linux-2.6.orig/drivers/md/dm-exception-store.h
+++ linux-2.6/drivers/md/dm-exception-store.h
@@ -101,9 +101,9 @@ struct dm_exception_store {
 	struct dm_dev *cow;
 
 	/* Size of data blocks saved - must be a power of 2 */
-	chunk_t chunk_size;
-	chunk_t chunk_mask;
-	chunk_t chunk_shift;
+	unsigned chunk_size;
+	unsigned chunk_mask;
+	unsigned chunk_shift;
 
 	void *context;
 };
@@ -169,7 +169,7 @@ int dm_exception_store_type_register(str
 int dm_exception_store_type_unregister(struct dm_exception_store_type *type);
 
 int dm_exception_store_set_chunk_size(struct dm_exception_store *store,
-				      unsigned long chunk_size_ulong,
+				      unsigned chunk_size,
 				      char **error);
 
 int dm_exception_store_create(struct dm_target *ti, int argc, char **argv,
Index: linux-2.6/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-snap-persistent.c
+++ linux-2.6/drivers/md/dm-snap-persistent.c
@@ -284,12 +284,13 @@ static int read_header(struct pstore *ps
 {
 	int r;
 	struct disk_header *dh;
-	chunk_t chunk_size;
+	unsigned chunk_size;
 	int chunk_size_supplied = 1;
 	char *chunk_err;
 
 	/*
-	 * Use default chunk size (or hardsect_size, if larger) if none supplied
+	 * Use default chunk size (or logical_block_size, if larger)
+	 * if none supplied
 	 */
 	if (!ps->store->chunk_size) {
 		ps->store->chunk_size = max(DM_CHUNK_SIZE_DEFAULT_SECTORS,
@@ -334,10 +335,9 @@ static int read_header(struct pstore *ps
 		return 0;
 
 	if (chunk_size_supplied)
-		DMWARN("chunk size %llu in device metadata overrides "
-		       "table chunk size of %llu.",
-		       (unsigned long long)chunk_size,
-		       (unsigned long long)ps->store->chunk_size);
+		DMWARN("chunk size %u in device metadata overrides "
+		       "table chunk size of %u.",
+		       chunk_size, ps->store->chunk_size);
 
 	/* We had a bogus chunk_size. Fix stuff up. */
 	free_area(ps);
@@ -345,8 +345,8 @@ static int read_header(struct pstore *ps
 	r = dm_exception_store_set_chunk_size(ps->store, chunk_size,
 					      &chunk_err);
 	if (r) {
-		DMERR("invalid on-disk chunk size %llu: %s.",
-		      (unsigned long long)chunk_size, chunk_err);
+		DMERR("invalid on-disk chunk size %u: %s.",
+		      chunk_size, chunk_err);
 		return r;
 	}
 

Re: [PATCH] dm snapshot: use unsigned integer chunk size

[Alasdair G Kergon]

On Wed, Sep 30, 2009 at 10:41:10AM -0400, Mike Snitzer wrote:
> Maximum chunk size is 512kB, 

Can you confirm that a supplied chunk size of 4294971392, for example, still
gets rejected?

> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Reviewed-by: Jonathan Brassow <jbrassow@redhat.com>

> -	unsigned long chunk_size_ulong;
> +	unsigned chunk_size;

> -	chunk_size_ulong = simple_strtoul(chunk_size_arg, &value, 10);
> +	chunk_size = simple_strtoul(chunk_size_arg, &value, 10);

Alasdair

[PATCH] fix bug on invalid chunksize

[Mikulas Patocka]

On Fri, 2 Oct 2009, Alasdair G Kergon wrote:

> On Wed, Sep 30, 2009 at 10:41:10AM -0400, Mike Snitzer wrote:
> > Maximum chunk size is 512kB, 
> 
> Can you confirm that a supplied chunk size of 4294971392, for example, still
> gets rejected?

It gets rejected but there's a bug that device is left open after a 
failure. See this patch:

Properly close the device if failing because of an invalid chunk size.

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

---
 drivers/md/dm-exception-store.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.31-fast-new/drivers/md/dm-exception-store.c
===================================================================
--- linux-2.6.31-fast-new.orig/drivers/md/dm-exception-store.c	2009-10-07 00:05:28.000000000 +0200
+++ linux-2.6.31-fast-new/drivers/md/dm-exception-store.c	2009-10-07 00:05:35.000000000 +0200
@@ -237,7 +237,7 @@ int dm_exception_store_create(struct dm_
 
 	r = set_chunk_size(tmp_store, argv[2], &ti->error);
 	if (r)
-		goto bad_cow;
+		goto bad_ctr;
 
 	r = type->ctr(tmp_store, 0, NULL);
 	if (r) {

Re: [PATCH] fix bug on invalid chunksize

[Jonathan Brassow]

good catch.

Reviewed-by: Jonathan Brassow <jbrassow@redhat.com>

  brassow

On Oct 6, 2009, at 5:59 PM, Mikulas Patocka wrote:

>
>
> On Fri, 2 Oct 2009, Alasdair G Kergon wrote:
>
>> On Wed, Sep 30, 2009 at 10:41:10AM -0400, Mike Snitzer wrote:
>>> Maximum chunk size is 512kB,
>>
>> Can you confirm that a supplied chunk size of 4294971392, for  
>> example, still
>> gets rejected?
>
> It gets rejected but there's a bug that device is left open after a
> failure. See this patch:
>
> Properly close the device if failing because of an invalid chunk size.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>
> ---
> drivers/md/dm-exception-store.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux-2.6.31-fast-new/drivers/md/dm-exception-store.c
> ===================================================================
> --- linux-2.6.31-fast-new.orig/drivers/md/dm-exception-store.c	 
> 2009-10-07 00:05:28.000000000 +0200
> +++ linux-2.6.31-fast-new/drivers/md/dm-exception-store.c	2009-10-07  
> 00:05:35.000000000 +0200
> @@ -237,7 +237,7 @@ int dm_exception_store_create(struct dm_
>
> 	r = set_chunk_size(tmp_store, argv[2], &ti->error);
> 	if (r)
> -		goto bad_cow;
> +		goto bad_ctr;
>
> 	r = type->ctr(tmp_store, 0, NULL);
> 	if (r) {

[PATCH] dm snapshot: don't use zero chunksize

[Mikulas Patocka]

When playing with invalid chunksizes, I have found another bug: under 
certain conditions (too small devices or too big chunksize) the calculated 
hash table size is zero. This triggers invalid C operation (a shift by -1) 
and causes "out of memory" messages --- when there's really nothing out of 
memory.

This patch changes it to use minimum 64 hashsize.

Mikulas

---

Under some special conditions (too big chunk size or zero-sized device),
the resulting hash_size is calculated as zero.

rounddown_pow_of_two(0) is undefined operation (it expands to shift by -1).
And init_exception_table with zero argument would fail with -ENOMEM.

This patch makes minimum chunk size 64, just like for pending exception table.

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

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

Index: linux-2.6.31-fast-new/drivers/md/dm-snap.c
===================================================================
--- linux-2.6.31-fast-new.orig/drivers/md/dm-snap.c	2009-10-07 00:32:09.000000000 +0200
+++ linux-2.6.31-fast-new/drivers/md/dm-snap.c	2009-10-07 00:33:04.000000000 +0200
@@ -570,6 +570,8 @@ static int init_hash_tables(struct dm_sn
 	hash_size = min(origin_dev_size, cow_dev_size) >> s->store->chunk_shift;
 	hash_size = min(hash_size, max_buckets);
 
+	if (hash_size < 64)
+		hash_size = 64;
 	hash_size = rounddown_pow_of_two(hash_size);
 	if (init_exception_table(&s->complete, hash_size,
 				 DM_CHUNK_CONSECUTIVE_BITS))

Re: [PATCH] dm snapshot: don't use zero chunksize

[Alasdair G Kergon]

On Tue, Oct 06, 2009 at 07:01:37PM -0400, Mikulas Patocka wrote:
> Under some special conditions (too big chunk size or zero-sized device),
> the resulting hash_size is calculated as zero.
> 
> rounddown_pow_of_two(0) is undefined operation (it expands to shift by -1).
> And init_exception_table with zero argument would fail with -ENOMEM.
> 
> This patch makes minimum chunk size 64, just like for pending exception table.
 
Could we have some more specific information in this patch header?

How does a "too big" chunk size happen?  And what is "too big"?

How does a zero-sized device happen and how can this code do anything
sensible if it encounters one - shouldn't it fail with an error instead?
Or do we still have a problem with the sequence in which dm ioctls are being
issued?

Alasdairs

Re: [PATCH] dm snapshot: don't use zero chunksize

[Mikulas Patocka]

On Thu, 8 Oct 2009, Alasdair G Kergon wrote:

> On Tue, Oct 06, 2009 at 07:01:37PM -0400, Mikulas Patocka wrote:
> > Under some special conditions (too big chunk size or zero-sized device),
> > the resulting hash_size is calculated as zero.
> > 
> > rounddown_pow_of_two(0) is undefined operation (it expands to shift by -1).
> > And init_exception_table with zero argument would fail with -ENOMEM.
> > 
> > This patch makes minimum chunk size 64, just like for pending exception table.
>  
> Could we have some more specific information in this patch header?
> 
> How does a "too big" chunk size happen?  And what is "too big"?

The problem is if you have bigger chunk size than the origin device (this 
is valid, but useless). Then, hash_size is set to zero and false 
allocation failure is reported.

I think bug 502965 is caused by this --- Milan tried to create a 10k 
volume with 16k chunksize.

> How does a zero-sized device happen and how can this code do anything
> sensible if it encounters one - shouldn't it fail with an error instead?
> Or do we still have a problem with the sequence in which dm ioctls are being
> issued?

I don't know if zero-sized device can happen with lvm. I think not. It 
could be only created manually with dmsetup. Snapshot of a zero-sized 
device may be still considered allowed.

> Alasdairs

Mikulas