[PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[Rene Scharfe]

[PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

Function nfs_block_bits() an open-coded version of (the non-existing)
rounddown_pow_of_two().  That means that for non-power-of-two target
sizes it returns half the size needed for a block to fully contain
the target.  I guess this is wrong. :-)  The patch uses the built-in
roundup_pow_of_two() instead.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

 fs/nfs/inode.c |   22 +++-------------------
 1 files changed, 3 insertions(+), 19 deletions(-)

4130722d1eeb5eb22c38df9f09dfa6be554bc72c
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -185,22 +185,6 @@ nfs_umount_begin(struct super_block *sb)
 		rpc_killall_tasks(rpc);
 }
 
-
-static inline unsigned long
-nfs_block_bits(unsigned long bsize)
-{
-	/* make sure blocksize is a power of two */
-	if (bsize & (bsize - 1)) {
-		unsigned char	nrbits;
-
-		for (nrbits = 31; nrbits && !(bsize & (1 << nrbits)); nrbits--)
-			;
-		bsize = 1 << nrbits;
-	}
-
-	return bsize;
-}
-
 /*
  * Calculate the number of 512byte blocks used.
  */
@@ -222,7 +206,7 @@ nfs_block_size(unsigned long bsize)
 	else if (bsize >= NFS_MAX_FILE_IO_BUFFER_SIZE)
 		bsize = NFS_MAX_FILE_IO_BUFFER_SIZE;
 
-	return nfs_block_bits(bsize);
+	return roundup_pow_of_two(bsize);
 }
 
 /*
@@ -319,10 +303,10 @@ nfs_sb_init(struct super_block *sb, rpc_
 	}
 
 	if (sb->s_blocksize == 0) {
-		sb->s_blocksize = nfs_block_bits(server->wsize);
+		sb->s_blocksize = roundup_pow_of_two(server->wsize);
 		sb->s_blocksize_bits = fls(sb->s_blocksize - 1);
 	}
-	server->wtmult = nfs_block_bits(fsinfo.wtmult);
+	server->wtmult = roundup_pow_of_two(fsinfo.wtmult);
 
 	server->dtsize = nfs_block_size(fsinfo.dtpref);
 	if (server->dtsize > PAGE_CACHE_SIZE)

Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[Trond Myklebust]

su den 24.07.2005 Klokka 16:36 (+0200) skreiv Rene Scharfe:
> [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()
> 
> Function nfs_block_bits() an open-coded version of (the non-existing)
> rounddown_pow_of_two().  That means that for non-power-of-two target
> sizes it returns half the size needed for a block to fully contain
> the target.  I guess this is wrong. :-)  The patch uses the built-in
> roundup_pow_of_two() instead.

What non-power-of-two target? Anything _not_ aligned to a power of two
boundary is a BUG!

Cheers,
  Trond

Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[Trond Myklebust]

su den 24.07.2005 Klokka 19:09 (-0400) skreiv Trond Myklebust:
> su den 24.07.2005 Klokka 16:36 (+0200) skreiv Rene Scharfe:
> > [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()
> > 
> > Function nfs_block_bits() an open-coded version of (the non-existing)
> > rounddown_pow_of_two().  That means that for non-power-of-two target
> > sizes it returns half the size needed for a block to fully contain
> > the target.  I guess this is wrong. :-)  The patch uses the built-in
> > roundup_pow_of_two() instead.
> 
> What non-power-of-two target? Anything _not_ aligned to a power of two
> boundary is a BUG!

Furthermore, rounding UP in order to "correct" this non-alignment would
definitely be a bug.

If users choose to override the default rsize/wsize, that is almost
always in order to limit the UDP fragmentation per read/write request on
lossy networks. By rounding up, you are doubling the number of fragments
that the user requested instead of respecting the limit.

Cheers,
  Trond

Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[Rene Scharfe]

On Sun, Jul 24, 2005 at 07:24:23PM -0400, Trond Myklebust wrote:
> su den 24.07.2005 Klokka 19:09 (-0400) skreiv Trond Myklebust:
> > What non-power-of-two target? Anything _not_ aligned to a power of two
> > boundary is a BUG!

So we could simply replace the loop in nfs_block_bits() with call to
BUG()? :->

> Furthermore, rounding UP in order to "correct" this non-alignment would
> definitely be a bug.
> 
> If users choose to override the default rsize/wsize, that is almost
> always in order to limit the UDP fragmentation per read/write request on
> lossy networks. By rounding up, you are doubling the number of fragments
> that the user requested instead of respecting the limit.

OK.  Either way, this function can be cleaned up.  Apparently the Plan 9
filesystem guys thought it rounds up.

What's your opinion of the following patch, which explicitly states the
intent of nfs_block_bits()?  It still needs patch 1 and 2.

Thanks,
Rene



[PATCH 3/4] Simplify and rename nfs_block_bits() to rounddown_pow_of_two()

nfs_block_bits() doesn't have a lot to do with bits anymore.  It can
also be implemented simpler and clearer with fls().

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

 fs/nfs/inode.c |   21 +++++----------------
 1 files changed, 5 insertions(+), 16 deletions(-)

1388b63224334877b1b154e38ad9ee17f1726bca
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -185,20 +185,9 @@ nfs_umount_begin(struct super_block *sb)
 		rpc_killall_tasks(rpc);
 }
 
-
-static inline unsigned long
-nfs_block_bits(unsigned long bsize)
+static inline unsigned long rounddown_pow_of_two(unsigned long x)
 {
-	/* make sure blocksize is a power of two */
-	if (bsize & (bsize - 1)) {
-		unsigned char	nrbits;
-
-		for (nrbits = 31; nrbits && !(bsize & (1 << nrbits)); nrbits--)
-			;
-		bsize = 1 << nrbits;
-	}
-
-	return bsize;
+	return x ? (1UL << (fls(x) - 1)) : 0;
 }
 
 /*
@@ -222,7 +211,7 @@ nfs_block_size(unsigned long bsize)
 	else if (bsize >= NFS_MAX_FILE_IO_BUFFER_SIZE)
 		bsize = NFS_MAX_FILE_IO_BUFFER_SIZE;
 
-	return nfs_block_bits(bsize);
+	return rounddown_pow_of_two(bsize);
 }
 
 /*
@@ -319,10 +308,10 @@ nfs_sb_init(struct super_block *sb, rpc_
 	}
 
 	if (sb->s_blocksize == 0) {
-		sb->s_blocksize = nfs_block_bits(server->wsize);
+		sb->s_blocksize = rounddown_pow_of_two(server->wsize);
 		sb->s_blocksize_bits = fls(sb->s_blocksize - 1);
 	}
-	server->wtmult = nfs_block_bits(fsinfo.wtmult);
+	server->wtmult = rounddown_pow_of_two(fsinfo.wtmult);
 
 	server->dtsize = nfs_block_size(fsinfo.dtpref);
 	if (server->dtsize > PAGE_CACHE_SIZE)

Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[Trond Myklebust]

må den 25.07.2005 Klokka 17:56 (+0200) skreiv Rene Scharfe:

> What's your opinion of the following patch, which explicitly states the
> intent of nfs_block_bits()?  It still needs patch 1 and 2.

I really don't like the choice of name. If you feel you must change the
name, then make it something like nfs_blocksize_align(). That describes
its function, instead of the implementation details.

Cheers,
  Trond

Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[Peter Staubach]

Trond Myklebust wrote:

>må den 25.07.2005 Klokka 17:56 (+0200) skreiv Rene Scharfe:
>
>  
>
>>What's your opinion of the following patch, which explicitly states the
>>intent of nfs_block_bits()?  It still needs patch 1 and 2.
>>    
>>
>
>I really don't like the choice of name. If you feel you must change the
>name, then make it something like nfs_blocksize_align(). That describes
>its function, instead of the implementation details.
>

I would agree.  Was there really a driving requirement to change the name?

    Thanx...

       ps

Re: [PATCH NFS 3/3] Replace nfs_block_bits() with roundup_pow_of_two()

[Rene Scharfe]

On Tue, Jul 26, 2005 at 01:48:46PM -0400, Trond Myklebust wrote:
> I really don't like the choice of name. If you feel you must change the
> name, then make it something like nfs_blocksize_align(). That describes
> its function, instead of the implementation details.

Yes, rounddown_pow_of_two() belongs in kernel.h next to
roundup_pow_of_two().  And maybe it should get a shorter name.

Anyway, I also don't like "nfs_blocksize_align".  So let's simply keep
the old name.  Renaming can be done later if really needed.

Rene


[PATCH 3/3] Simplify nfs_block_bits()

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

 fs/nfs/inode.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

ddad5eadf4c2907842bf9baa2610e0a35ea14137
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -189,16 +189,8 @@ nfs_umount_begin(struct super_block *sb)
 static inline unsigned long
 nfs_block_bits(unsigned long bsize)
 {
-	/* make sure blocksize is a power of two */
-	if (bsize & (bsize - 1)) {
-		unsigned char	nrbits;
-
-		for (nrbits = 31; nrbits && !(bsize & (1 << nrbits)); nrbits--)
-			;
-		bsize = 1 << nrbits;
-	}
-
-	return bsize;
+	/* round down to the nearest power of two */
+	return bsize ? (1UL << (fls(bsize) - 1)) : 0;
 }
 
 /*