Skip to contentSkip to conversation

fs/xfs: remove duplicated defines

PATCH In review Nicolas Kaiser 5 participants 14 messages Nov 11, 2007 – Nov 16, 2007

Subsystems (2)
Kernel janitors XFS filesystem
lore
Share & tools
lore

Nicolas Kaiser

author
#0 lore raw

PATCH 1/1 fs/xfs: remove duplicated defines

Remove duplicated defines.

Signed-off-by Nicolas Kaiser <[email protected]>

---

 fs/xfs/linux-2.6/xfs_linux.h |    4 ----
 1 file changed, 4 deletions(-)
fs/xfs/linux-2.6/xfs_linux.h +0 -4
@@ -167,12 +167,8 @@
 
 /* clicks to bytes */
 #define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
 #define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
 
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
 #define ENOATTR		ENODATA		/* Attribute not found */
 #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
 #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
#1 lore raw
Quoted 4 lines · Nicolas Kaiser

Remove duplicated defines.

Signed-off-by Nicolas Kaiser <[email protected]>

---

Heh, each defined twice, but used 0 times in the kernel. Could probably just remove them altogether (though I guess btoc is used in xfstests/ltp/doio.c in userspace/xfstests, but that's the *only* place)

-Eric

Quoted 20 lines
 fs/xfs/linux-2.6/xfs_linux.h |    4 ----
 1 file changed, 4 deletions(-)
fs/xfs/linux-2.6/xfs_linux.h +0 -4
@@ -167,12 +167,8 @@
 
 /* clicks to bytes */
 #define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
 #define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
 
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
 #define ENOATTR		ENODATA		/* Attribute not found */
 #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
 #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
#2 lore raw
Quoted 10 lines · Eric Sandeen
Quoted 4 lines · Nicolas Kaiser

Remove duplicated defines.

Signed-off-by Nicolas Kaiser <[email protected]>

---

Heh, each defined twice, but used 0 times in the kernel. Could probably just remove them altogether (though I guess btoc is used in xfstests/ltp/doio.c in userspace/xfstests, but that's the *only* place)

Yes, that's what I was just noticing - how they are not used anyway.

Quoted 1 line

grep -Ir btoc . | egrep -v 'tag|anot'

./linux-2.6/xfs_linux.h:#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoc64(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct64(x)      ((__uint64_t)(x)>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)

./linux-2.6/xfs_buf.c: page_count = xfs_buf_btoc(end) - xfs_buf_btoct(bp->b_file_offset);
./linux-2.6/xfs_buf.c: page = bp->b_pages[xfs_buf_btoct(boff + bp->b_offset)];
./linux-2.6/xfs_buf.h:#define xfs_buf_btoc(dd) (((dd) + PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT) ./linux-2.6/xfs_buf.h:#define xfs_buf_btoct(dd) ((dd) >> PAGE_CACHE_SHIFT)

--Tim

Quoted 23 lines

-Eric

Quoted 20 lines
 fs/xfs/linux-2.6/xfs_linux.h |    4 ----
 1 file changed, 4 deletions(-)
fs/xfs/linux-2.6/xfs_linux.h +0 -4
@@ -167,12 +167,8 @@
 
 /* clicks to bytes */
 #define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
 #define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
 
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
 #define ENOATTR		ENODATA		/* Attribute not found */
 #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
 #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
#3 lore raw
In reply to Nicolas Kaiser
Quoted 20 lines · Timothy Shimmin
Quoted 10 lines · Eric Sandeen
Quoted 4 lines · Nicolas Kaiser

Remove duplicated defines.

Signed-off-by Nicolas Kaiser <[email protected]>

---

Heh, each defined twice, but used 0 times in the kernel. Could probably just remove them altogether (though I guess btoc is used in xfstests/ltp/doio.c in userspace/xfstests, but that's the *only* place)

Yes, that's what I was just noticing - how they are not used anyway.

Quoted 1 line

grep -Ir btoc . | egrep -v 'tag|anot'

./linux-2.6/xfs_linux.h:#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoc64(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct64(x)      ((__uint64_t)(x)>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
Quoted 34 lines

./linux-2.6/xfs_buf.c: page_count = xfs_buf_btoc(end) - xfs_buf_btoct(bp->b_file_offset);
./linux-2.6/xfs_buf.c: page = bp->b_pages[xfs_buf_btoct(boff + bp->b_offset)];
./linux-2.6/xfs_buf.h:#define xfs_buf_btoc(dd) (((dd) + PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT) ./linux-2.6/xfs_buf.h:#define xfs_buf_btoct(dd) ((dd) >> PAGE_CACHE_SHIFT)

--Tim

Quoted 26 lines

-Eric

Quoted 23 lines
 fs/xfs/linux-2.6/xfs_linux.h |    4 ----
 1 file changed, 4 deletions(-)

--- a/fs/xfs/linux-2.6/xfs_linux.h 2007-11-07 11:26:20.000000000
+0100
+++ b/fs/xfs/linux-2.6/xfs_linux.h 2007-11-11 13:07:11.000000000
+0100

(quoted diff) +0 -4
@@ -167,12 +167,8 @@
 
 /* clicks to bytes */
 #define    ctob(x)        ((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
 #define    ctob64(x)    ((__uint64_t)(x)<<BPCSHIFT)
 
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
 #define ENOATTR        ENODATA        /* Attribute not found */
 #define EWRONGFS    EINVAL        /* Mount with wrong filesystem

type */
#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */

So I'm thinking of "seeing" your patch and raising you by my attached patch :-)

--Tim

===========================================================================
Index: fs/xfs/linux-2.6/xfs_linux.h
===========================================================================

fs/xfs/linux-2.6/xfs_linux.h +0 -14
@@ -159,12 +159,6 @@
 /* number of BB's per block device block */
 #define	BLKDEV_BB		BTOBB(BLKDEV_IOSIZE)
 
-/* bytes to clicks */
-#define	btoc(x)		(((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct(x)	((__psunsigned_t)(x)>>BPCSHIFT)
-#define	btoc64(x)	(((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct64(x)	((__uint64_t)(x)>>BPCSHIFT)
-
 /* off_t bytes to clicks */
 #define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
 #define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)

@@ -172,14 +166,6 @@
 /* clicks to off_t bytes */
 #define	ctooff(x)	((xfs_off_t)(x)<<BPCSHIFT)
 
-/* clicks to bytes */
-#define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
-#define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
-
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
 #define ENOATTR		ENODATA		/* Attribute not found */
 #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
 #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
#4 lore raw
In reply to Nicolas Kaiser
Quoted 62 lines · Timothy Shimmin
Quoted 50 lines · Timothy Shimmin
Quoted 9 lines · Eric Sandeen
Quoted 4 lines · Nicolas Kaiser

Remove duplicated defines.

Signed-off-by Nicolas Kaiser <[email protected]>

---

Heh, each defined twice, but used 0 times in the kernel. Could probably just remove them altogether (though I guess btoc is used in xfstests/ltp/doio.c in userspace/xfstests, but that's the *only* place)

Yes, that's what I was just noticing - how they are not used anyway.

Quoted 1 line

grep -Ir btoc . | egrep -v 'tag|anot'

./linux-2.6/xfs_linux.h:#define btoc(x) (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT) ./linux-2.6/xfs_linux.h:#define btoct(x) ((__psunsigned_t)(x)>>BPCSHIFT) ./linux-2.6/xfs_linux.h:#define btoc64(x) (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct64(x) ((__uint64_t)(x)>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define btoct(x) ((__psunsigned_t)(x)>>BPCSHIFT) ./linux-2.6/xfs_linux.h:#define btoc(x) (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT) ./linux-2.6/xfs_buf.c: page_count = xfs_buf_btoc(end) - xfs_buf_btoct(bp->b_file_offset);
./linux-2.6/xfs_buf.c: page = bp->b_pages[xfs_buf_btoct(boff +
bp->b_offset)];
./linux-2.6/xfs_buf.h:#define xfs_buf_btoc(dd) (((dd) + PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT) ./linux-2.6/xfs_buf.h:#define xfs_buf_btoct(dd) ((dd) >> PAGE_CACHE_SHIFT)

--Tim

Quoted 16 lines

-Eric

Quoted 14 lines
 fs/xfs/linux-2.6/xfs_linux.h |    4 ----
 1 file changed, 4 deletions(-)

--- a/fs/xfs/linux-2.6/xfs_linux.h 2007-11-07 11:26:20.000000000
+0100
+++ b/fs/xfs/linux-2.6/xfs_linux.h 2007-11-11 13:07:11.000000000
+0100

(quoted diff) +0 -1
@@ -167,12 +167,8 @@
 
 /* clicks to bytes */
 #define    ctob(x)        ((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
 #define    ctob64(x)    ((__uint64_t)(x)<<BPCSHIFT)
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
(quoted diff) +0 -3
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
 #define ENOATTR        ENODATA        /* Attribute not found */
 #define EWRONGFS    EINVAL        /* Mount with wrong filesystem

type */
#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */

So I'm thinking of "seeing" your patch and raising you by my attached patch :-)

Quoted 9 lines

--Tim

-- Binary/unsupported file stripped by Ecartis --
-- Type: text/x-patch
-- File: btoc.patch

Where the attachment was supposed to look like...

=====================================Index: fs/xfs/linux-2.6/xfs_linux.h
=====================================

fs/xfs/linux-2.6/xfs_linux.h +0 -14
@@ -159,12 +159,6 @@
  /* number of BB's per block device block */
  #define	BLKDEV_BB		BTOBB(BLKDEV_IOSIZE)
 
-/* bytes to clicks */
-#define	btoc(x)		(((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct(x)	((__psunsigned_t)(x)>>BPCSHIFT)
-#define	btoc64(x)	(((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct64(x)	((__uint64_t)(x)>>BPCSHIFT)
-
  /* off_t bytes to clicks */
  #define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
  #define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)

@@ -172,14 +166,6 @@
  /* clicks to off_t bytes */
  #define	ctooff(x)	((xfs_off_t)(x)<<BPCSHIFT)
 
-/* clicks to bytes */
-#define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
-#define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
-
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
  #define ENOATTR		ENODATA		/* Attribute not found */
  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
  #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
#5 lore raw
In reply to Nicolas Kaiser
Quoted 24 lines · Timothy Shimmin

Where the attachment was supposed to look like...

=====================================> Index: fs/xfs/linux-2.6/xfs_linux.h =====================================>

fs/xfs/linux-2.6/xfs_linux.h +0 -7
@@ -159,12 +159,6 @@
  /* number of BB's per block device block */
  #define	BLKDEV_BB		BTOBB(BLKDEV_IOSIZE)
 
-/* bytes to clicks */
-#define	btoc(x)		(((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct(x)	((__psunsigned_t)(x)>>BPCSHIFT)
-#define	btoc64(x)	(((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct64(x)	((__uint64_t)(x)>>BPCSHIFT)
-
  /* off_t bytes to clicks */
  #define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
  #define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)

@@ -172,14 +166,6 @@
  /* clicks to off_t bytes */
  #define	ctooff(x)	((xfs_off_t)(x)<<BPCSHIFT)
 
-/* clicks to bytes */

fs/xfs/linux-2.6/xfs_linux.h line 171

Quoted 10 lines
fs/xfs/linux-2.6/xfs_linux.h +0 -7
-#define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
-#define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
-
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
  #define ENOATTR		ENODATA		/* Attribute not found */
  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
  #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */

Perhaps we should look at cleaning up the cusers of offtoc, offtoct, etc and killing BPCSHIFT altogether....

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#6 lore raw
In reply to Nicolas Kaiser
Quoted 24 lines · David Chinner
Quoted 23 lines · Timothy Shimmin

Where the attachment was supposed to look like...

=====================================>> Index: fs/xfs/linux-2.6/xfs_linux.h =====================================>>

fs/xfs/linux-2.6/xfs_linux.h +0 -6
@@ -159,12 +159,6 @@
  /* number of BB's per block device block */
  #define	BLKDEV_BB		BTOBB(BLKDEV_IOSIZE)
 
-/* bytes to clicks */
-#define	btoc(x)		(((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct(x)	((__psunsigned_t)(x)>>BPCSHIFT)
-#define	btoc64(x)	(((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct64(x)	((__uint64_t)(x)>>BPCSHIFT)
-
  /* off_t bytes to clicks */
  #define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
  #define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)

@@ -172,14 +166,6 @@
  /* clicks to off_t bytes */
  #define	ctooff(x)	((xfs_off_t)(x)<<BPCSHIFT)
Quoted 15 lines

fs/xfs/linux-2.6/xfs_linux.h line 174

Quoted 11 lines
fs/xfs/linux-2.6/xfs_linux.h +0 -8
-/* clicks to bytes */
-#define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
-#define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
-
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
  #define ENOATTR		ENODATA		/* Attribute not found */
  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
  #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */

Perhaps we should look at cleaning up the cusers of offtoc, offtoct, etc and killing BPCSHIFT altogether....

Yeah, I had a quick look before, but I will look closer again ;-)

Quoted 1 line

egrep -Ir 'offtoc|ctoooff' . | egrep -v "anot|tag"

./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(*offset)),
./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(pos)), -1);
./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(pos)),
./linux-2.6/xfs_linux.h:#define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)
./xfs_vnodeops.c:                               ctooff(offtoct(ioffset)), -1);
./xfs_vnodeops.c:                               ctooff(offtoct(ioffset)),

So we basically just use:

ctooff(offtoct(pos))

where
#define ctooff(x) ((xfs_off_t)(x)<<BPCSHIFT)
#define offtoct(x) ((xfs_off_t)(x)>>BPCSHIFT)
#define BPCSHIFT PAGE_SHIFT /* LOG2(NBPC) if exact */

seems basically to be a:

#define round_down_page(x) ((x) & ~(PAGE_SIZE - 1))

or just use a
round_down(x, PAGE_SIZE)
and
define the round_down for size which is power of 2.

Like in asm-x86_64/proto.h
#define round_up(x,y) (((x) + (y) - 1) & ~((y)-1)) #define round_down(x,y) ((x) & ~((y)-1))

What way do you reckon?

--Tim

#7 lore raw
In reply to Nicolas Kaiser
Quoted 33 lines · Timothy Shimmin
Quoted 3 lines · David Chinner

Perhaps we should look at cleaning up the cusers of offtoc, offtoct, etc and killing BPCSHIFT altogether....

Yeah, I had a quick look before, but I will look closer again ;-)

Quoted 1 line

egrep -Ir 'offtoc|ctoooff' . | egrep -v "anot|tag"

./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(*offset)),
./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(pos)), -1);
./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(pos)),
./linux-2.6/xfs_linux.h:#define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)
./xfs_vnodeops.c:                               ctooff(offtoct(ioffset)), -1);
./xfs_vnodeops.c:                               ctooff(offtoct(ioffset)),

So we basically just use:

ctooff(offtoct(pos))

where
#define ctooff(x) ((xfs_off_t)(x)<<BPCSHIFT)
#define offtoct(x) ((xfs_off_t)(x)>>BPCSHIFT)
#define BPCSHIFT PAGE_SHIFT /* LOG2(NBPC) if exact */

seems basically to be a:

#define round_down_page(x) ((x) & ~(PAGE_SIZE - 1))

or just use a
round_down(x, PAGE_SIZE)
and
define the round_down for size which is power of 2.

Quoted 5 lines

Like in asm-x86_64/proto.h
#define round_up(x,y) (((x) + (y) - 1) & ~((y)-1)) #define round_down(x,y) ((x) & ~((y)-1))

What way do you reckon?

Neither ;)

Just replace them with (val & PAGE_CACHE_MASK)

Actually, the code in xfs_vnodeops.c culd probably just be removed; the ioffset variable is already rounded to a multiple of page size....

Cheers,

dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#8 lore raw
In reply to Nicolas Kaiser
Quoted 34 lines · David Chinner
Quoted 33 lines · Timothy Shimmin
Quoted 3 lines · David Chinner

Perhaps we should look at cleaning up the cusers of offtoc, offtoct, etc and killing BPCSHIFT altogether....

Yeah, I had a quick look before, but I will look closer again ;-)

Quoted 1 line

egrep -Ir 'offtoc|ctoooff' . | egrep -v "anot|tag"

./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(*offset)),
./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(pos)), -1);
./linux-2.6/xfs_lrw.c:                                  ctooff(offtoct(pos)),
./linux-2.6/xfs_linux.h:#define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
./linux-2.6/xfs_linux.h:#define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)
./xfs_vnodeops.c:                               ctooff(offtoct(ioffset)), -1);
./xfs_vnodeops.c:                               ctooff(offtoct(ioffset)),

So we basically just use:

ctooff(offtoct(pos))

where
#define ctooff(x) ((xfs_off_t)(x)<<BPCSHIFT)
#define offtoct(x) ((xfs_off_t)(x)>>BPCSHIFT)
#define BPCSHIFT PAGE_SHIFT /* LOG2(NBPC) if exact */

seems basically to be a:

#define round_down_page(x) ((x) & ~(PAGE_SIZE - 1))

or just use a
round_down(x, PAGE_SIZE)
and
define the round_down for size which is power of 2.

Quoted 7 lines
Quoted 5 lines

Like in asm-x86_64/proto.h
#define round_up(x,y) (((x) + (y) - 1) & ~((y)-1)) #define round_down(x,y) ((x) & ~((y)-1))

What way do you reckon?

Neither ;)

:)

Quoted 3 lines

Just replace them with (val & PAGE_CACHE_MASK)

Ah, there is already a macro for ~(PAGE_SIZE - 1) that would be good. (You can tell I look at a lot of this page code:)

Quoted 3 lines

Actually, the code in xfs_vnodeops.c culd probably just be removed; the ioffset variable is already rounded to a multiple of page size....

Yep...

         rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, NBPP);
         ioffset = offset & ~(rounding - 1);

Looks like we used to use ioffset directly in a call to xfs_inval_cached_pages() and when hch moved to VOP_FLUSH_INVALPAGES we used the ctoff(offtoct(...

Quoted 3 lines

Cheers,

dave.

Remove the BPCSHIFT based macros from XFS.

The BPCSHIFT based macros, btoc*, ctob*, offtoc* and ctooff are either not used or don't need to be used. Initial patch and motivation from Nicolas Kaiser.

Signed-off-by Tim Shimmin <[email protected]>
---
  b/fs/xfs/linux-2.6/xfs_linux.h |   21 ---------------------
  b/fs/xfs/linux-2.6/xfs_lrw.c   |    9 ++++-----
  b/fs/xfs/xfs_vnodeops.c        |    7 ++-----
  3 files changed, 6 insertions(+), 31 deletions(-)

=====================================Index: fs/xfs/linux-2.6/xfs_linux.h
=====================================

fs/xfs/linux-2.6/xfs_linux.h +0 -21
@@ -159,27 +159,6 @@
  /* number of BB's per block device block */
  #define	BLKDEV_BB		BTOBB(BLKDEV_IOSIZE)
 
-/* bytes to clicks */
-#define	btoc(x)		(((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct(x)	((__psunsigned_t)(x)>>BPCSHIFT)
-#define	btoc64(x)	(((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct64(x)	((__uint64_t)(x)>>BPCSHIFT)
-
-/* off_t bytes to clicks */
-#define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)
-
-/* clicks to off_t bytes */
-#define	ctooff(x)	((xfs_off_t)(x)<<BPCSHIFT)
-
-/* clicks to bytes */
-#define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
-#define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
-
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
  #define ENOATTR		ENODATA		/* Attribute not found */
  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
  #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */

=====================================Index: fs/xfs/linux-2.6/xfs_lrw.c
=====================================

fs/xfs/linux-2.6/xfs_lrw.c +4 -5
@@ -254,9 +254,8 @@ xfs_read(
 
  	if (unlikely(ioflags & IO_ISDIRECT)) {
  		if (VN_CACHED(vp))
-			ret = xfs_flushinval_pages(ip,
-					ctooff(offtoct(*offset)),
-					-1, FI_REMAPF_LOCKED);
+			ret = xfs_flushinval_pages(ip, (*offset & PAGE_MASK),
+						    -1, FI_REMAPF_LOCKED);
  		mutex_unlock(&inode->i_mutex);
  		if (ret) {
  			xfs_iunlock(ip, XFS_IOLOCK_SHARED);

@@ -742,9 +741,9 @@ retry:
  		if (VN_CACHED(vp)) {
  			WARN_ON(need_i_mutex = 0);
  			xfs_inval_cached_trace(xip, pos, -1,
-					ctooff(offtoct(pos)), -1);
+					(pos & PAGE_MASK), -1);
  			error = xfs_flushinval_pages(xip,
-					ctooff(offtoct(pos)),
+					(pos & PAGE_MASK),
  					-1, FI_REMAPF_LOCKED);
  			if (error)
  				goto out_unlock_internal;

=====================================Index: fs/xfs/xfs_vnodeops.c
=====================================

fs/xfs/xfs_vnodeops.c +2 -5
@@ -4168,11 +4168,8 @@ xfs_free_file_space(
  	ioffset = offset & ~(rounding - 1);
 
  	if (VN_CACHED(vp) != 0) {
-		xfs_inval_cached_trace(ip, ioffset, -1,
-				ctooff(offtoct(ioffset)), -1);
-		error = xfs_flushinval_pages(ip,
-				ctooff(offtoct(ioffset)),
-				-1, FI_REMAPF_LOCKED);
+		xfs_inval_cached_trace(ip, ioffset, -1, ioffset, -1);
+		error = xfs_flushinval_pages(ip, ioffset, -1, FI_REMAPF_LOCKED);
  		if (error)
  			goto out_unlock_iolock;
  	}
#9 lore raw
In reply to Nicolas Kaiser
Quoted 2 lines · Timothy Shimmin
Quoted 1 line · David Chinner

Just replace them with (val & PAGE_CACHE_MASK)

                                  ^^^^^^^^^^^^^^^

......

fs/xfs/linux-2.6/xfs_lrw.c xfs_read( · line 257

Quoted 10 lines
fs/xfs/linux-2.6/xfs_lrw.c +1 -3
@@ -254,9 +254,8 @@ xfs_read(
 
  	if (unlikely(ioflags & IO_ISDIRECT)) {
  		if (VN_CACHED(vp))
-			ret = xfs_flushinval_pages(ip,
-					ctooff(offtoct(*offset)),
-					-1, FI_REMAPF_LOCKED);
+			ret = xfs_flushinval_pages(ip, (*offset & PAGE_MASK),

                                                                  ^^^^^^^^^

s/PAGE_MASK/PAGE_CACHE_MASK/g

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#10 lore raw
In reply to Nicolas Kaiser
Quoted 17 lines · David Chinner
Quoted 2 lines · Timothy Shimmin
Quoted 1 line · David Chinner

Just replace them with (val & PAGE_CACHE_MASK)

                                  ^^^^^^^^^^^^^^^

......

fs/xfs/linux-2.6/xfs_lrw.c xfs_read( · line 257

Quoted 10 lines
fs/xfs/linux-2.6/xfs_lrw.c +1 -3
@@ -254,9 +254,8 @@ xfs_read(
 
  	if (unlikely(ioflags & IO_ISDIRECT)) {
  		if (VN_CACHED(vp))
-			ret = xfs_flushinval_pages(ip,
-					ctooff(offtoct(*offset)),
-					-1, FI_REMAPF_LOCKED);
+			ret = xfs_flushinval_pages(ip, (*offset & PAGE_MASK),

                                                                  ^^^^^^^^^
Quoted 2 lines

s/PAGE_MASK/PAGE_CACHE_MASK/g

Okay.

While here, looking at a few others...

I'll get rid of BPCSHIFT.

And then...

#define NBPP            PAGE_SIZE
#define NDPP            (1 << (PAGE_SHIFT - 9)) <--- not used - another to nuke
#define NBPC            PAGE_SIZE <----- used once

grep -Ir 'NBPC' . | egrep -v 'tag|anot|diff'
./linux-2.6/xfs_linux.h:#define NBPC PAGE_SIZE /* Number of bytes per click */
./xfs_itable.c: irbuf = kmem_zalloc_greedy(&irbsize, NBPC, NBPC * 4,

Quoted 1 line

grep -Ir 'NBPP' . | egrep -v 'tag|anot|diff|NBPPR'

./linux-2.6/xfs_linux.h:#define NBPP PAGE_SIZE
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_LOW (NBPP / sizeof(xfs_dqhash_t))
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_HIGH ((NBPP * 4) / sizeof(xfs_dqhash_t))
./xfs_bmap.c: } else if (mp->m_sb.sb_blocksize >= NBPP) {
./xfs_bmap.c: args.prod = NBPP >> mp->m_sb.sb_blocklog;
./xfs_itable.c: bcount = MIN(left, (int)(NBPP / sizeof(*buffer)));
./xfs_log.c: kmem_free(tic, NBPP);
./xfs_log.c: uint i = (NBPP / sizeof(xlog_ticket_t)) - 2;
./xfs_log.c: buf = (xfs_caddr_t) kmem_zalloc(NBPP, KM_SLEEP);
./xfs_vnodeops.c: rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, NBPP);

Might as well get rid of NBPC and replace by NBPP.

Is it just worth s/NBPC/PAGE_SIZE/g ?

Okay, the xfs_vnodeops.c one should be PAGE_CACHE_SIZE, then right? How about the bmap ones?

I don't know if I want to keep asking questions ... ;-)

--Tim

#11 lore raw
In reply to Nicolas Kaiser
Quoted 33 lines · Timothy Shimmin
Quoted 17 lines · David Chinner
Quoted 2 lines · Timothy Shimmin
Quoted 1 line · David Chinner

Just replace them with (val & PAGE_CACHE_MASK)

                                  ^^^^^^^^^^^^^^^

......

fs/xfs/linux-2.6/xfs_lrw.c xfs_read( · line 257

Quoted 10 lines
fs/xfs/linux-2.6/xfs_lrw.c +1 -3
@@ -254,9 +254,8 @@ xfs_read(
 
  	if (unlikely(ioflags & IO_ISDIRECT)) {
  		if (VN_CACHED(vp))
-			ret = xfs_flushinval_pages(ip,
-					ctooff(offtoct(*offset)),
-					-1, FI_REMAPF_LOCKED);
+			ret = xfs_flushinval_pages(ip, (*offset & PAGE_MASK),

                                                                  ^^^^^^^^^
Quoted 2 lines

s/PAGE_MASK/PAGE_CACHE_MASK/g

Okay.

While here, looking at a few others...

I'll get rid of BPCSHIFT.

And then...

#define NBPP            PAGE_SIZE
#define NDPP            (1 << (PAGE_SHIFT - 9)) <--- not used - another to nuke
#define NBPC            PAGE_SIZE <----- used once
Quoted 8 lines

grep -Ir 'NBPC' . | egrep -v 'tag|anot|diff'
./linux-2.6/xfs_linux.h:#define NBPC PAGE_SIZE /* Number of bytes per click */
./xfs_itable.c: irbuf = kmem_zalloc_greedy(&irbsize, NBPC, NBPC * 4,

Quoted 1 line

grep -Ir 'NBPP' . | egrep -v 'tag|anot|diff|NBPPR'

./linux-2.6/xfs_linux.h:#define NBPP PAGE_SIZE
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_LOW (NBPP / sizeof(xfs_dqhash_t))
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_HIGH ((NBPP * 4) / sizeof(xfs_dqhash_t))

PAGE_SIZE

Quoted 2 lines

./xfs_bmap.c: } else if (mp->m_sb.sb_blocksize >= NBPP) {
./xfs_bmap.c: args.prod = NBPP >> mp->m_sb.sb_blocklog;

PAGE_CACHE_SIZE

Quoted 1 line

./xfs_itable.c: bcount = MIN(left, (int)(NBPP / sizeof(*buffer)));

PAGE_SIZE

Quoted 3 lines

./xfs_log.c: kmem_free(tic, NBPP);
./xfs_log.c: uint i = (NBPP / sizeof(xlog_ticket_t)) - 2;
./xfs_log.c: buf = (xfs_caddr_t) kmem_zalloc(NBPP, KM_SLEEP);

We should replace that hand rolled ticket allocator with a slab cache.

Quoted 1 line

./xfs_vnodeops.c: rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, NBPP);

PAGE_CACHE_SIZE. (as suggested ;)

Quoted 3 lines

Might as well get rid of NBPC and replace by NBPP.

Is it just worth s/NBPC/PAGE_SIZE/g ?

yup.

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#12 lore raw

PATCH 1/1 remove BPCSHIFT and NB?P? macros - was fs/xfs: remove duplicated

Quoted 23 lines · David Chinner
Quoted 5 lines

While here, looking at a few others...

#define NBPP            PAGE_SIZE
#define NDPP            (1 << (PAGE_SHIFT - 9)) <--- not used - another to nuke
#define NBPC            PAGE_SIZE <----- used once
Quoted 8 lines

grep -Ir 'NBPC' . | egrep -v 'tag|anot|diff'
./linux-2.6/xfs_linux.h:#define NBPC PAGE_SIZE /* Number of bytes per click */
./xfs_itable.c: irbuf = kmem_zalloc_greedy(&irbsize, NBPC, NBPC * 4,

Quoted 1 line

grep -Ir 'NBPP' . | egrep -v 'tag|anot|diff|NBPPR'

./linux-2.6/xfs_linux.h:#define NBPP PAGE_SIZE
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_LOW (NBPP / sizeof(xfs_dqhash_t))
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_HIGH ((NBPP * 4) / sizeof(xfs_dqhash_t))

PAGE_SIZE

Quoted 2 lines

./xfs_bmap.c: } else if (mp->m_sb.sb_blocksize >= NBPP) {
./xfs_bmap.c: args.prod = NBPP >> mp->m_sb.sb_blocklog;

PAGE_CACHE_SIZE

Quoted 1 line

./xfs_itable.c: bcount = MIN(left, (int)(NBPP / sizeof(*buffer)));

Quoted 8 lines

PAGE_SIZE

Quoted 3 lines

./xfs_log.c: kmem_free(tic, NBPP);
./xfs_log.c: uint i = (NBPP / sizeof(xlog_ticket_t)) - 2;
./xfs_log.c: buf = (xfs_caddr_t) kmem_zalloc(NBPP, KM_SLEEP);

We should replace that hand rolled ticket allocator with a slab cache.

In another patch though.

Quoted 10 lines
Quoted 1 line

./xfs_vnodeops.c: rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, NBPP);

PAGE_CACHE_SIZE. (as suggested ;)

Quoted 3 lines

Might as well get rid of NBPC and replace by NBPP.

Is it just worth s/NBPC/PAGE_SIZE/g ?

yup.

Thanks.

Patch:
-------------

Remove the BPCSHIFT and NB* based macros from XFS.

The BPCSHIFT based macros, btoc*, ctob*, offtoc* and ctooff are either not used or don't need to be used.
The NDPP, NDPP, NBBY macros don't need to be used but instead are replaced directly by PAGE_SIZE and PAGE_CACHE_SIZE where appropriate.
Initial patch and motivation from Nicolas Kaiser.

Signed-off-by Tim Shimmin <[email protected]>
---
  b/fs/xfs/linux-2.6/xfs_linux.h |   28 +---------------------------
  b/fs/xfs/linux-2.6/xfs_lrw.c   |    9 ++++-----
  b/fs/xfs/quota/xfs_qm.h        |    4 ++--
  b/fs/xfs/xfs_bmap.c            |    4 ++--
  b/fs/xfs/xfs_itable.c          |    4 ++--
  b/fs/xfs/xfs_log.c             |    6 +++---
  b/fs/xfs/xfs_vnodeops.c        |    9 +++------
  7 files changed, 17 insertions(+), 47 deletions(-)

=====================================Index: fs/xfs/linux-2.6/xfs_linux.h
=====================================

fs/xfs/linux-2.6/xfs_linux.h +1 -27
@@ -143,43 +143,17 @@
 
  #define spinlock_destroy(lock)
 
-#define NBPP		PAGE_SIZE
-#define NDPP		(1 << (PAGE_SHIFT - 9))
-
  #define NBBY		8		/* number of bits per byte */
-#define	NBPC		PAGE_SIZE	/* Number of bytes per click */
-#define	BPCSHIFT	PAGE_SHIFT	/* LOG2(NBPC) if exact */
 
  /*
   * Size of block device i/o is parameterized here.
   * Currently the system supports page-sized i/o.
   */
-#define	BLKDEV_IOSHIFT		BPCSHIFT
+#define	BLKDEV_IOSHIFT		PAGE_CACHE_SHIFT
  #define	BLKDEV_IOSIZE		(1<<BLKDEV_IOSHIFT)
  /* number of BB's per block device block */
  #define	BLKDEV_BB		BTOBB(BLKDEV_IOSIZE)
 
-/* bytes to clicks */
-#define	btoc(x)		(((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct(x)	((__psunsigned_t)(x)>>BPCSHIFT)
-#define	btoc64(x)	(((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct64(x)	((__uint64_t)(x)>>BPCSHIFT)
-
-/* off_t bytes to clicks */
-#define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)
-
-/* clicks to off_t bytes */
-#define	ctooff(x)	((xfs_off_t)(x)<<BPCSHIFT)
-
-/* clicks to bytes */
-#define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
-#define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
-
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
  #define ENOATTR		ENODATA		/* Attribute not found */
  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
  #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */

=====================================Index: fs/xfs/linux-2.6/xfs_lrw.c
=====================================

fs/xfs/linux-2.6/xfs_lrw.c +4 -5
@@ -254,9 +254,8 @@ xfs_read(
 
  	if (unlikely(ioflags & IO_ISDIRECT)) {
  		if (VN_CACHED(vp))
-			ret = xfs_flushinval_pages(ip,
-					ctooff(offtoct(*offset)),
-					-1, FI_REMAPF_LOCKED);
+			ret = xfs_flushinval_pages(ip, (*offset & PAGE_CACHE_MASK),
+						    -1, FI_REMAPF_LOCKED);
  		mutex_unlock(&inode->i_mutex);
  		if (ret) {
  			xfs_iunlock(ip, XFS_IOLOCK_SHARED);

@@ -742,9 +741,9 @@ retry:
  		if (VN_CACHED(vp)) {
  			WARN_ON(need_i_mutex = 0);
  			xfs_inval_cached_trace(xip, pos, -1,
-					ctooff(offtoct(pos)), -1);
+					(pos & PAGE_CACHE_MASK), -1);
  			error = xfs_flushinval_pages(xip,
-					ctooff(offtoct(pos)),
+					(pos & PAGE_CACHE_MASK),
  					-1, FI_REMAPF_LOCKED);
  			if (error)
  				goto out_unlock_internal;

=====================================Index: fs/xfs/quota/xfs_qm.h
=====================================

fs/xfs/quota/xfs_qm.h +2 -2
@@ -52,8 +52,8 @@ extern kmem_zone_t	*qm_dqtrxzone;
  /*
   * Dquot hashtable constants/threshold values.
   */
-#define XFS_QM_HASHSIZE_LOW		(NBPP / sizeof(xfs_dqhash_t))
-#define XFS_QM_HASHSIZE_HIGH		((NBPP * 4) / sizeof(xfs_dqhash_t))
+#define XFS_QM_HASHSIZE_LOW		(PAGE_SIZE / sizeof(xfs_dqhash_t))
+#define XFS_QM_HASHSIZE_HIGH		((PAGE_SIZE * 4) / sizeof(xfs_dqhash_t))
 
  /*
   * This defines the unit of allocation of dquots.

=====================================Index: fs/xfs/xfs_bmap.c
=====================================

fs/xfs/xfs_bmap.c +2 -2
@@ -2830,11 +2830,11 @@ xfs_bmap_btalloc(
  		args.prod = align;
  		if ((args.mod = (xfs_extlen_t)do_mod(ap->off, args.prod)))
  			args.mod = (xfs_extlen_t)(args.prod - args.mod);
-	} else if (mp->m_sb.sb_blocksize >= NBPP) {
+	} else if (mp->m_sb.sb_blocksize >= PAGE_CACHE_SIZE) {
  		args.prod = 1;
  		args.mod = 0;
  	} else {
-		args.prod = NBPP >> mp->m_sb.sb_blocklog;
+		args.prod = PAGE_CACHE_SIZE >> mp->m_sb.sb_blocklog;
  		if ((args.mod = (xfs_extlen_t)(do_mod(ap->off, args.prod))))
  			args.mod = (xfs_extlen_t)(args.prod - args.mod);
  	}

=====================================Index: fs/xfs/xfs_itable.c
=====================================

fs/xfs/xfs_itable.c +2 -2
@@ -393,7 +393,7 @@ xfs_bulkstat(
  		(XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog);
  	nimask = ~(nicluster - 1);
  	nbcluster = nicluster >> mp->m_sb.sb_inopblog;
-	irbuf = kmem_zalloc_greedy(&irbsize, NBPC, NBPC * 4,
+	irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4,
  				   KM_SLEEP | KM_MAYFAIL | KM_LARGE);
  	nirbuf = irbsize / sizeof(*irbuf);
 

@@ -815,7 +815,7 @@ xfs_inumbers(
  	agino = XFS_INO_TO_AGINO(mp, ino);
  	left = *count;
  	*count = 0;
-	bcount = MIN(left, (int)(NBPP / sizeof(*buffer)));
+	bcount = MIN(left, (int)(PAGE_SIZE / sizeof(*buffer)));
  	buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP);
  	error = bufidx = 0;
  	cur = NULL;

=====================================Index: fs/xfs/xfs_log.c
=====================================

fs/xfs/xfs_log.c +3 -3
@@ -1552,7 +1552,7 @@ xlog_dealloc_log(xlog_t *log)
  		tic = log->l_unmount_free;
  		while (tic) {
  			next_tic = tic->t_next;
-			kmem_free(tic, NBPP);
+			kmem_free(tic, PAGE_SIZE);
  			tic = next_tic;
  		}
  	}

@@ -3161,13 +3161,13 @@ xlog_state_ticket_alloc(xlog_t *log)
  	xlog_ticket_t	*t_list;
  	xlog_ticket_t	*next;
  	xfs_caddr_t	buf;
-	uint		i = (NBPP / sizeof(xlog_ticket_t)) - 2;
+	uint		i = (PAGE_SIZE / sizeof(xlog_ticket_t)) - 2;
 
  	/*
  	 * The kmem_zalloc may sleep, so we shouldn't be holding the
  	 * global lock.  XXXmiken: may want to use zone allocator.
  	 */
-	buf = (xfs_caddr_t) kmem_zalloc(NBPP, KM_SLEEP);
+	buf = (xfs_caddr_t) kmem_zalloc(PAGE_SIZE, KM_SLEEP);
 
  	spin_lock(&log->l_icloglock);

=====================================Index: fs/xfs/xfs_vnodeops.c
=====================================

fs/xfs/xfs_vnodeops.c +3 -6
@@ -4164,15 +4164,12 @@ xfs_free_file_space(
  		vn_iowait(ip);	/* wait for the completion of any pending DIOs */
  	}
 
-	rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, NBPP);
+	rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
  	ioffset = offset & ~(rounding - 1);
 
  	if (VN_CACHED(vp) != 0) {
-		xfs_inval_cached_trace(ip, ioffset, -1,
-				ctooff(offtoct(ioffset)), -1);
-		error = xfs_flushinval_pages(ip,
-				ctooff(offtoct(ioffset)),
-				-1, FI_REMAPF_LOCKED);
+		xfs_inval_cached_trace(ip, ioffset, -1, ioffset, -1);
+		error = xfs_flushinval_pages(ip, ioffset, -1, FI_REMAPF_LOCKED);
  		if (error)
  			goto out_unlock_iolock;
  	}
#13 lore raw

Re: [xfs-masters] [PATCH] remove BPCSHIFT and NB?P? macros - was

Looks good Tim. Nice work.

Quoted 257 lines · Timothy Shimmin
Quoted 19 lines · David Chinner
Quoted 5 lines

While here, looking at a few others...

#define NBPP            PAGE_SIZE
#define NDPP            (1 << (PAGE_SHIFT - 9)) <--- not used - another to nuke
#define NBPC            PAGE_SIZE <----- used once
Quoted 8 lines

grep -Ir 'NBPC' . | egrep -v 'tag|anot|diff'
./linux-2.6/xfs_linux.h:#define NBPC PAGE_SIZE /* Number of bytes per click */
./xfs_itable.c: irbuf = kmem_zalloc_greedy(&irbsize, NBPC, NBPC * 4,

Quoted 1 line

grep -Ir 'NBPP' . | egrep -v 'tag|anot|diff|NBPPR'

./linux-2.6/xfs_linux.h:#define NBPP PAGE_SIZE
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_LOW (NBPP / sizeof(xfs_dqhash_t))
./quota/xfs_qm.h:#define XFS_QM_HASHSIZE_HIGH ((NBPP * 4) / sizeof(xfs_dqhash_t))

PAGE_SIZE

Quoted 2 lines

./xfs_bmap.c: } else if (mp->m_sb.sb_blocksize >= NBPP) {
./xfs_bmap.c: args.prod = NBPP >> mp->m_sb.sb_blocklog;

PAGE_CACHE_SIZE

Quoted 8 lines
Quoted 1 line

./xfs_itable.c: bcount = MIN(left, (int)(NBPP / sizeof(*buffer)));

PAGE_SIZE

Quoted 3 lines

./xfs_log.c: kmem_free(tic, NBPP);
./xfs_log.c: uint i = (NBPP / sizeof(xlog_ticket_t)) - 2;
./xfs_log.c: buf = (xfs_caddr_t) kmem_zalloc(NBPP, KM_SLEEP);

We should replace that hand rolled ticket allocator with a slab cache.

In another patch though.

Quoted 8 lines
Quoted 1 line

./xfs_vnodeops.c: rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, NBPP);

PAGE_CACHE_SIZE. (as suggested ;)

Quoted 3 lines

Might as well get rid of NBPC and replace by NBPP.

Is it just worth s/NBPC/PAGE_SIZE/g ?

yup.

Thanks.

Patch:
-------------

Remove the BPCSHIFT and NB* based macros from XFS.

The BPCSHIFT based macros, btoc*, ctob*, offtoc* and ctooff are either not used or don't need to be used.
The NDPP, NDPP, NBBY macros don't need to be used but instead are replaced directly by PAGE_SIZE and PAGE_CACHE_SIZE where appropriate.
Initial patch and motivation from Nicolas Kaiser.

Signed-off-by Tim Shimmin <[email protected]>
---
  b/fs/xfs/linux-2.6/xfs_linux.h |   28 +---------------------------
  b/fs/xfs/linux-2.6/xfs_lrw.c   |    9 ++++-----
  b/fs/xfs/quota/xfs_qm.h        |    4 ++--
  b/fs/xfs/xfs_bmap.c            |    4 ++--
  b/fs/xfs/xfs_itable.c          |    4 ++--
  b/fs/xfs/xfs_log.c             |    6 +++---
  b/fs/xfs/xfs_vnodeops.c        |    9 +++------
  7 files changed, 17 insertions(+), 47 deletions(-)

=====================================> Index: fs/xfs/linux-2.6/xfs_linux.h =====================================>

fs/xfs/linux-2.6/xfs_linux.h +1 -27
@@ -143,43 +143,17 @@
 
  #define spinlock_destroy(lock)
 
-#define NBPP		PAGE_SIZE
-#define NDPP		(1 << (PAGE_SHIFT - 9))
-
  #define NBBY		8		/* number of bits per byte */
-#define	NBPC		PAGE_SIZE	/* Number of bytes per click */
-#define	BPCSHIFT	PAGE_SHIFT	/* LOG2(NBPC) if exact */
 
  /*
   * Size of block device i/o is parameterized here.
   * Currently the system supports page-sized i/o.
   */
-#define	BLKDEV_IOSHIFT		BPCSHIFT
+#define	BLKDEV_IOSHIFT		PAGE_CACHE_SHIFT
  #define	BLKDEV_IOSIZE		(1<<BLKDEV_IOSHIFT)
  /* number of BB's per block device block */
  #define	BLKDEV_BB		BTOBB(BLKDEV_IOSIZE)
 
-/* bytes to clicks */
-#define	btoc(x)		(((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct(x)	((__psunsigned_t)(x)>>BPCSHIFT)
-#define	btoc64(x)	(((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define	btoct64(x)	((__uint64_t)(x)>>BPCSHIFT)
-
-/* off_t bytes to clicks */
-#define offtoc(x)       (((__uint64_t)(x)+(NBPC-1))>>BPCSHIFT)
-#define offtoct(x)      ((xfs_off_t)(x)>>BPCSHIFT)
-
-/* clicks to off_t bytes */
-#define	ctooff(x)	((xfs_off_t)(x)<<BPCSHIFT)
-
-/* clicks to bytes */
-#define	ctob(x)		((__psunsigned_t)(x)<<BPCSHIFT)
-#define btoct(x)        ((__psunsigned_t)(x)>>BPCSHIFT)
-#define	ctob64(x)	((__uint64_t)(x)<<BPCSHIFT)
-
-/* bytes to clicks */
-#define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
-
  #define ENOATTR		ENODATA		/* Attribute not found */
  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
  #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */

=====================================> Index: fs/xfs/linux-2.6/xfs_lrw.c =====================================>

fs/xfs/linux-2.6/xfs_lrw.c +4 -5
@@ -254,9 +254,8 @@ xfs_read(
 
  	if (unlikely(ioflags & IO_ISDIRECT)) {
  		if (VN_CACHED(vp))
-			ret = xfs_flushinval_pages(ip,
-					ctooff(offtoct(*offset)),
-					-1, FI_REMAPF_LOCKED);
+			ret = xfs_flushinval_pages(ip, (*offset & PAGE_CACHE_MASK),
+						    -1, FI_REMAPF_LOCKED);
  		mutex_unlock(&inode->i_mutex);
  		if (ret) {
  			xfs_iunlock(ip, XFS_IOLOCK_SHARED);

@@ -742,9 +741,9 @@ retry:
  		if (VN_CACHED(vp)) {
  			WARN_ON(need_i_mutex = 0);
  			xfs_inval_cached_trace(xip, pos, -1,
-					ctooff(offtoct(pos)), -1);
+					(pos & PAGE_CACHE_MASK), -1);
  			error = xfs_flushinval_pages(xip,
-					ctooff(offtoct(pos)),
+					(pos & PAGE_CACHE_MASK),
  					-1, FI_REMAPF_LOCKED);
  			if (error)
  				goto out_unlock_internal;

=====================================> Index: fs/xfs/quota/xfs_qm.h =====================================>

fs/xfs/quota/xfs_qm.h +2 -2
@@ -52,8 +52,8 @@ extern kmem_zone_t	*qm_dqtrxzone;
  /*
   * Dquot hashtable constants/threshold values.
   */
-#define XFS_QM_HASHSIZE_LOW		(NBPP / sizeof(xfs_dqhash_t))
-#define XFS_QM_HASHSIZE_HIGH		((NBPP * 4) / sizeof(xfs_dqhash_t))
+#define XFS_QM_HASHSIZE_LOW		(PAGE_SIZE / sizeof(xfs_dqhash_t))
+#define XFS_QM_HASHSIZE_HIGH		((PAGE_SIZE * 4) / sizeof(xfs_dqhash_t))
 
  /*
   * This defines the unit of allocation of dquots.

=====================================> Index: fs/xfs/xfs_bmap.c =====================================>

fs/xfs/xfs_bmap.c +2 -2
@@ -2830,11 +2830,11 @@ xfs_bmap_btalloc(
  		args.prod = align;
  		if ((args.mod = (xfs_extlen_t)do_mod(ap->off, args.prod)))
  			args.mod = (xfs_extlen_t)(args.prod - args.mod);
-	} else if (mp->m_sb.sb_blocksize >= NBPP) {
+	} else if (mp->m_sb.sb_blocksize >= PAGE_CACHE_SIZE) {
  		args.prod = 1;
  		args.mod = 0;
  	} else {
-		args.prod = NBPP >> mp->m_sb.sb_blocklog;
+		args.prod = PAGE_CACHE_SIZE >> mp->m_sb.sb_blocklog;
  		if ((args.mod = (xfs_extlen_t)(do_mod(ap->off, args.prod))))
  			args.mod = (xfs_extlen_t)(args.prod - args.mod);
  	}

=====================================> Index: fs/xfs/xfs_itable.c =====================================>

fs/xfs/xfs_itable.c +2 -2
@@ -393,7 +393,7 @@ xfs_bulkstat(
  		(XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog);
  	nimask = ~(nicluster - 1);
  	nbcluster = nicluster >> mp->m_sb.sb_inopblog;
-	irbuf = kmem_zalloc_greedy(&irbsize, NBPC, NBPC * 4,
+	irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4,
  				   KM_SLEEP | KM_MAYFAIL | KM_LARGE);
  	nirbuf = irbsize / sizeof(*irbuf);
 

@@ -815,7 +815,7 @@ xfs_inumbers(
  	agino = XFS_INO_TO_AGINO(mp, ino);
  	left = *count;
  	*count = 0;
-	bcount = MIN(left, (int)(NBPP / sizeof(*buffer)));
+	bcount = MIN(left, (int)(PAGE_SIZE / sizeof(*buffer)));
  	buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP);
  	error = bufidx = 0;
  	cur = NULL;

=====================================> Index: fs/xfs/xfs_log.c =====================================>

fs/xfs/xfs_log.c +3 -3
@@ -1552,7 +1552,7 @@ xlog_dealloc_log(xlog_t *log)
  		tic = log->l_unmount_free;
  		while (tic) {
  			next_tic = tic->t_next;
-			kmem_free(tic, NBPP);
+			kmem_free(tic, PAGE_SIZE);
  			tic = next_tic;
  		}
  	}

@@ -3161,13 +3161,13 @@ xlog_state_ticket_alloc(xlog_t *log)
  	xlog_ticket_t	*t_list;
  	xlog_ticket_t	*next;
  	xfs_caddr_t	buf;
-	uint		i = (NBPP / sizeof(xlog_ticket_t)) - 2;
+	uint		i = (PAGE_SIZE / sizeof(xlog_ticket_t)) - 2;
 
  	/*
  	 * The kmem_zalloc may sleep, so we shouldn't be holding the
  	 * global lock.  XXXmiken: may want to use zone allocator.
  	 */
-	buf = (xfs_caddr_t) kmem_zalloc(NBPP, KM_SLEEP);
+	buf = (xfs_caddr_t) kmem_zalloc(PAGE_SIZE, KM_SLEEP);
 
  	spin_lock(&log->l_icloglock);

=====================================> Index: fs/xfs/xfs_vnodeops.c =====================================>

fs/xfs/xfs_vnodeops.c +3 -6
@@ -4164,15 +4164,12 @@ xfs_free_file_space(
  		vn_iowait(ip);	/* wait for the completion of any pending DIOs */
  	}
 
-	rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, NBPP);
+	rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
  	ioffset = offset & ~(rounding - 1);
 
  	if (VN_CACHED(vp) != 0) {
-		xfs_inval_cached_trace(ip, ioffset, -1,
-				ctooff(offtoct(ioffset)), -1);
-		error = xfs_flushinval_pages(ip,
-				ctooff(offtoct(ioffset)),
-				-1, FI_REMAPF_LOCKED);
+		xfs_inval_cached_trace(ip, ioffset, -1, ioffset, -1);
+		error = xfs_flushinval_pages(ip, ioffset, -1, FI_REMAPF_LOCKED);
  		if (error)
  			goto out_unlock_iolock;
  	}