Re: [patch 05/11] dm-snap: remove SECTOR_SHIFT define

Re: [patch 05/11] dm-snap: remove SECTOR_SHIFT define

[Alasdair G Kergon]

On Fri, Aug 10, 2007 at 10:02:09PM +0200, Jan Blunck wrote:
> Sector size on Linux is always 512 bytes. Don't even try to give the
> impression this is changeable.

If that's what worries you, add a comment next to the definition,
perhaps?

It's there so you can easily locate all the places within dm that
perform these conversions by using a simple search.  Searching for '9'
wouldn't be as easy.  (I don't know about other people, but I find the
code easier to read the way it is.)

BTW When changing one thing across the whole of dm, you may combine
the sub-component patches into a single patch.

Alasdair
-- 
agk@redhat.com

Re: [patch 05/11] dm-snap: remove SECTOR_SHIFT define

[Jan Blunck]

On Fri, Aug 10, Alasdair G Kergon wrote:

> On Fri, Aug 10, 2007 at 10:02:09PM +0200, Jan Blunck wrote:
> > Sector size on Linux is always 512 bytes. Don't even try to give the
> > impression this is changeable.
> 
> If that's what worries you, add a comment next to the definition,
> perhaps?

That and that the code just looks more like the rest of the block layer
code. Only a few users (UFS, MSDOS, HFS, IDE) use a predefined
SECTOR_{SHIFT,SIZE}.

> It's there so you can easily locate all the places within dm that
> perform these conversions by using a simple search.  Searching for '9'
> wouldn't be as easy.  (I don't know about other people, but I find the
> code easier to read the way it is.)

Hmm, so I guess this is more about dis/like of how the code looks. Maybe we
should define a global SECTOR_{SIZE,SHIFT} into blkdev.h.

Regards,
	Jan

-- 
Jan Blunck <jblunck@suse.de>

Re: [patch 03/11] dm-snap: Remove dead queued_bios code

[Alasdair G Kergon]

On Fri, Aug 10, 2007 at 10:02:07PM +0200, Jan Blunck wrote:
> This patch removes the unused queued_bios handling code.
 
Well I'm going to leave this in for now (as one existing unfinished
patch series does build upon it).  We can include its removal as part of
your following patch series that no doubt replaces the functionality
with something better.

Alasdair
-- 
agk@redhat.com

Re: [patch 03/11] dm-snap: Remove dead queued_bios code

[Jan Blunck]

On Fri, Aug 10, Alasdair G Kergon wrote:

> On Fri, Aug 10, 2007 at 10:02:07PM +0200, Jan Blunck wrote:
> > This patch removes the unused queued_bios handling code.
>  
> Well I'm going to leave this in for now (as one existing unfinished
> patch series does build upon it).  We can include its removal as part of
> your following patch series that no doubt replaces the functionality
> with something better.

You mean the snapshot read tracking. Yes, I think I can fix this problem too.

Regards,
	Jan

-- 
Jan Blunck <jblunck@suse.de>

Re: [patch 01/11] dm-snap: Replace special round_down()

[Jan Blunck]

On Fri, 10 Aug 2007 22:02:05 +0200, Jan Blunck wrote:

> This patch removes the special round_down() to next power of 2 implementation
> used only at one place in the snapshot target. It is replaced by an equivalent
> 1 << fls() which might use an architecture specific implementation.
> 
> Signed-off-by: Jan Blunck <jblunck@suse.de>
> ---
>  drivers/md/dm-snap.c |   12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> --- a/drivers/md/dm-snap.c
> +++ b/drivers/md/dm-snap.c
> @@ -333,16 +333,6 @@ static int calc_max_buckets(void)
>  }
>  
>  /*
> - * Rounds a number down to a power of 2.
> - */
> -static uint32_t round_down(uint32_t n)
> -{
> -	while (n & (n - 1))
> -		n &= (n - 1);
> -	return n;
> -}
> -
> -/*
>   * Allocate room for a suitable hash table.
>   */
>  static int init_hash_tables(struct dm_snapshot *s)
> @@ -361,7 +351,7 @@ static int init_hash_tables(struct dm_sn
>  	hash_size = min(hash_size, max_buckets);
>  
>  	/* Round it down to a power of 2 */
> -	hash_size = round_down(hash_size);
> +	hash_size = 1 << fls(hash_size);
>  	if (init_exception_table(&s->complete, hash_size))
>  		return -ENOMEM;
>  
>

Err, this is sooo stupid. First, 1 << fls(n) isn't round down but
roundup_pow_of_2(). Second, hash_size is of type sector_t which could be
u64 with CONFIG_LBD.

-- 
Subject: dm-snap: Replace special round_down()

This patch removes the special round_down() to next power of 2 implementation
used only at one place in the snapshot target. It is replaced by an equivalent
1 << fls() which might use an architecture specific implementation.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/md/dm-snap.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -333,16 +333,6 @@ static int calc_max_buckets(void)
 }
 
 /*
- * Rounds a number down to a power of 2.
- */
-static uint32_t round_down(uint32_t n)
-{
-	while (n & (n - 1))
-		n &= (n - 1);
-	return n;
-}
-
-/*
  * Allocate room for a suitable hash table.
  */
 static int init_hash_tables(struct dm_snapshot *s)
@@ -361,7 +351,7 @@ static int init_hash_tables(struct dm_sn
 	hash_size = min(hash_size, max_buckets);
 
 	/* Round it down to a power of 2 */
-	hash_size = round_down(hash_size);
+	hash_size = 1 << (fls_long(hash_size) - 1);
 	if (init_exception_table(&s->complete, hash_size))
 		return -ENOMEM;

Re: [dm-devel] [patch 01/11] dm-snap: Replace special round_down()

[Jan Blunck]

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

On Fri, Aug 10, Jan Blunck wrote:

> This patch removes the special round_down() to next power of 2 implementation
> used only at one place in the snapshot target. It is replaced by an equivalent
> 1 << fls() which might use an architecture specific implementation.

Err, ok this is the second fix for this patch. I hope it is the last. It
doesn't make any sense to have hash_size be defined as sector_t. Same goes for
max_buckets.

Regards,
	Jan

-- 
Jan Blunck <jblunck@suse.de>

[-- Attachment #2: dm-snap-remove-round_down.diff --]
[-- Type: text/x-patch, Size: 1301 bytes --]

Subject: dm-snap: Replace special round_down()

This patch removes the special round_down() to next power of 2 implementation
used only at one place in the snapshot target. It is replaced by an equivalent
1 << fls() which might use an architecture specific implementation.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 drivers/md/dm-snap.c |   15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -333,21 +333,12 @@ static int calc_max_buckets(void)
 }
 
 /*
- * Rounds a number down to a power of 2.
- */
-static uint32_t round_down(uint32_t n)
-{
-	while (n & (n - 1))
-		n &= (n - 1);
-	return n;
-}
-
-/*
  * Allocate room for a suitable hash table.
  */
 static int init_hash_tables(struct dm_snapshot *s)
 {
-	sector_t hash_size, cow_dev_size, origin_dev_size, max_buckets;
+	unsigned int hash_size, max_buckets;
+	sector_t cow_dev_size, origin_dev_size;
 
 	/*
 	 * Calculate based on the size of the original volume or
@@ -361,7 +352,7 @@ static int init_hash_tables(struct dm_sn
 	hash_size = min(hash_size, max_buckets);
 
 	/* Round it down to a power of 2 */
-	hash_size = round_down(hash_size);
+	hash_size = 1 << (fls(hash_size) - 1);
 	if (init_exception_table(&s->complete, hash_size))
 		return -ENOMEM;