2009-06-24 16:30:57

by Eric Sandeen

Subject: [PATCH] naturally align struct ext4_allocation_request

As Ted noted, the ext4_allocation_request isn't well aligned.

Looking at it with pahole we're wasting space on 64-bit
arches:

struct ext4_allocation_request {
struct inode * inode; /* 0 8 */
ext4_lblk_t logical; /* 8 4 */

/* XXX 4 bytes hole, try to pack */

ext4_fsblk_t goal; /* 16 8 */
ext4_lblk_t lleft; /* 24 4 */

/* XXX 4 bytes hole, try to pack */

ext4_fsblk_t pleft; /* 32 8 */
ext4_lblk_t lright; /* 40 4 */

/* XXX 4 bytes hole, try to pack */

ext4_fsblk_t pright; /* 48 8 */
unsigned int len; /* 56 4 */
unsigned int flags; /* 60 4 */
/* --- cacheline 1 boundary (64 bytes) --- */

/* size: 64, cachelines: 1, members: 9 */
/* sum members: 52, holes: 3, sum holes: 12 */
};

grouping 32-bit members together closes these holes and shrinks
the structure by 12 bytes.

Signed-off-by: Eric Sandeen <[email protected]>
---


diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index cc7d5ed..9a438cf 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -93,20 +93,20 @@ typedef unsigned int ext4_group_t;
struct ext4_allocation_request {
/* target inode for block we're allocating */
struct inode *inode;
+ /* how many blocks we want to allocate */
+ unsigned int len;
/* logical block in target inode */
ext4_lblk_t logical;
- /* phys. target (a hint) */
- ext4_fsblk_t goal;
/* the closest logical allocated block to the left */
ext4_lblk_t lleft;
- /* phys. block for ^^^ */
- ext4_fsblk_t pleft;
/* the closest logical allocated block to the right */
ext4_lblk_t lright;
+ /* phys. target (a hint) */
+ ext4_fsblk_t goal;
+ /* phys. block for ^^^ */
+ ext4_fsblk_t pleft;
/* phys. block for ^^^ */
ext4_fsblk_t pright;
- /* how many blocks we want to allocate */
- unsigned int len;
/* flags. see above EXT4_MB_HINT_* */
unsigned int flags;
};

2009-06-26 00:50:14

by Mike Snitzer

Subject: Re: [PATCH] naturally align struct ext4_allocation_request

On Wed, Jun 24, 2009 at 12:30 PM, Eric Sandeen<[email protected]> wrote:
> As Ted noted, the ext4_allocation_request isn't well aligned.
>
> Looking at it with pahole we're wasting space on 64-bit
> arches:
>
> struct ext4_allocation_request {
> ? ? ? ?struct inode * ? ? ? ? ? ? inode; ? ? ? ? ? ? ?/* ? ? 0 ? ? 8 */
> ? ? ? ?ext4_lblk_t ? ? ? ? ? ? ? ?logical; ? ? ? ? ? ?/* ? ? 8 ? ? 4 */
>
> ? ? ? ?/* XXX 4 bytes hole, try to pack */
>
> ? ? ? ?ext4_fsblk_t ? ? ? ? ? ? ? goal; ? ? ? ? ? ? ? /* ? ?16 ? ? 8 */
> ? ? ? ?ext4_lblk_t ? ? ? ? ? ? ? ?lleft; ? ? ? ? ? ? ?/* ? ?24 ? ? 4 */
>
> ? ? ? ?/* XXX 4 bytes hole, try to pack */
>
> ? ? ? ?ext4_fsblk_t ? ? ? ? ? ? ? pleft; ? ? ? ? ? ? ?/* ? ?32 ? ? 8 */
> ? ? ? ?ext4_lblk_t ? ? ? ? ? ? ? ?lright; ? ? ? ? ? ? /* ? ?40 ? ? 4 */
>
> ? ? ? ?/* XXX 4 bytes hole, try to pack */
>
> ? ? ? ?ext4_fsblk_t ? ? ? ? ? ? ? pright; ? ? ? ? ? ? /* ? ?48 ? ? 8 */
> ? ? ? ?unsigned int ? ? ? ? ? ? ? len; ? ? ? ? ? ? ? ?/* ? ?56 ? ? 4 */
> ? ? ? ?unsigned int ? ? ? ? ? ? ? flags; ? ? ? ? ? ? ?/* ? ?60 ? ? 4 */
> ? ? ? ?/* --- cacheline 1 boundary (64 bytes) --- */
>
> ? ? ? ?/* size: 64, cachelines: 1, members: 9 */
> ? ? ? ?/* sum members: 52, holes: 3, sum holes: 12 */
> };
>
> grouping 32-bit members together closes these holes and shrinks
> the structure by 12 bytes.
>
> Signed-off-by: Eric Sandeen <[email protected]>
> ---
>
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index cc7d5ed..9a438cf 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -93,20 +93,20 @@ typedef unsigned int ext4_group_t;
> ?struct ext4_allocation_request {
> ? ? ? ?/* target inode for block we're allocating */
> ? ? ? ?struct inode *inode;
> + ? ? ? /* how many blocks we want to allocate */
> + ? ? ? unsigned int len;
> ? ? ? ?/* logical block in target inode */
> ? ? ? ?ext4_lblk_t logical;
> - ? ? ? /* phys. target (a hint) */
> - ? ? ? ext4_fsblk_t goal;
> ? ? ? ?/* the closest logical allocated block to the left */
> ? ? ? ?ext4_lblk_t lleft;
> - ? ? ? /* phys. block for ^^^ */
> - ? ? ? ext4_fsblk_t pleft;
> ? ? ? ?/* the closest logical allocated block to the right */
> ? ? ? ?ext4_lblk_t lright;
> + ? ? ? /* phys. target (a hint) */
> + ? ? ? ext4_fsblk_t goal;
> + ? ? ? /* phys. block for ^^^ */
> + ? ? ? ext4_fsblk_t pleft;
> ? ? ? ?/* phys. block for ^^^ */
> ? ? ? ?ext4_fsblk_t pright;

Hey Eric,

Looks like the comments for pleft and pright should be updated to
reference lleft and lright respectively (rather than ^^^).

Mike

2009-06-26 01:09:40

by Eric Sandeen

Subject: [PATCH V2] naturally align struct ext4_allocation_request

As Ted noted, the ext4_allocation_request isn't well aligned.

Looking at it with pahole we're wasting space on 64-bit
arches:

struct ext4_allocation_request {
struct inode * inode; /* 0 8 */
ext4_lblk_t logical; /* 8 4 */

/* XXX 4 bytes hole, try to pack */

ext4_fsblk_t goal; /* 16 8 */
ext4_lblk_t lleft; /* 24 4 */

/* XXX 4 bytes hole, try to pack */

ext4_fsblk_t pleft; /* 32 8 */
ext4_lblk_t lright; /* 40 4 */

/* XXX 4 bytes hole, try to pack */

ext4_fsblk_t pright; /* 48 8 */
unsigned int len; /* 56 4 */
unsigned int flags; /* 60 4 */
/* --- cacheline 1 boundary (64 bytes) --- */

/* size: 64, cachelines: 1, members: 9 */
/* sum members: 52, holes: 3, sum holes: 12 */
};

grouping 32-bit members together closes these holes and shrinks
the structure by 12 bytes.

Signed-off-by: Eric Sandeen <[email protected]>
---

V2: fix comments, thanks to Mike Snitzer

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index cc7d5ed..cefd94a 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -93,20 +93,20 @@ typedef unsigned int ext4_group_t;
struct ext4_allocation_request {
/* target inode for block we're allocating */
struct inode *inode;
+ /* how many blocks we want to allocate */
+ unsigned int len;
/* logical block in target inode */
ext4_lblk_t logical;
- /* phys. target (a hint) */
- ext4_fsblk_t goal;
/* the closest logical allocated block to the left */
ext4_lblk_t lleft;
- /* phys. block for ^^^ */
- ext4_fsblk_t pleft;
/* the closest logical allocated block to the right */
ext4_lblk_t lright;
- /* phys. block for ^^^ */
+ /* phys. target (a hint) */
+ ext4_fsblk_t goal;
+ /* phys. block for the closest logical allocated block to the left */
+ ext4_fsblk_t pleft;
+ /* phys. block for the closest logical allocated block to the right */
ext4_fsblk_t pright;
- /* how many blocks we want to allocate */
- unsigned int len;
/* flags. see above EXT4_MB_HINT_* */
unsigned int flags;
};

2009-07-06 02:24:11

by Theodore Ts'o

Subject: Re: [PATCH V2] naturally align struct ext4_allocation_request

On Thu, Jun 25, 2009 at 08:09:40PM -0500, Eric Sandeen wrote:
> As Ted noted, the ext4_allocation_request isn't well aligned.
>
> Looking at it with pahole we're wasting space on 64-bit
> arches...
> grouping 32-bit members together closes these holes and shrinks
> the structure by 12 bytes.
>
> Signed-off-by: Eric Sandeen <[email protected]>


Thanks, added to the ext4 patch queue.

- Ted