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

2005-07-26 Thread 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2005-07-26 Thread 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2005-07-26 Thread 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 [EMAIL PROTECTED]
---

 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;
 }
 
 /*
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2005-07-25 Thread 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 [EMAIL PROTECTED]
---

 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)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2005-07-24 Thread 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 [EMAIL PROTECTED]
---

 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)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2005-07-24 Thread 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2005-07-24 Thread 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/