diff mbox series

[4/4] xfs: rearrange xfs_da_args a bit to use less space

Message ID 171270968452.3631393.6758018766662309716.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded, archived
Headers show
Series [1/4] xfs: remove XFS_DA_OP_REMOVE | expand

Commit Message

Darrick J. Wong April 10, 2024, 12:50 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

A few notes about struct xfs_da_args:

The XFS_ATTR_* flags only go up as far as XFS_ATTR_INCOMPLETE, which
means that attr_filter could be a u8 field.

The XATTR_* flags only have two values, which means that xattr_flags
could be shrunk to a u8.

I've reduced the number of XFS_DA_OP_* flags down to the point where
op_flags would also fit into a u8.

filetype has 7 bytes of slack after it, which is wasteful.

namelen will never be greater than MAXNAMELEN, which is 256.  This field
could be reduced to a short.

Rearrange the fields in xfs_da_args to waste less space.  This reduces
the structure size from 136 bytes to 128.  Later when we add extra
fields to support parent pointer replacement, this will only bloat the
structure to 144 bytes, instead of 168.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_da_btree.h |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Christoph Hellwig April 10, 2024, 5:02 a.m. UTC | #1 
On Tue, Apr 09, 2024 at 05:50:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> A few notes about struct xfs_da_args:
> 
> The XFS_ATTR_* flags only go up as far as XFS_ATTR_INCOMPLETE, which
> means that attr_filter could be a u8 field.
> 
> The XATTR_* flags only have two values, which means that xattr_flags
> could be shrunk to a u8.
> 
> I've reduced the number of XFS_DA_OP_* flags down to the point where
> op_flags would also fit into a u8.
> 
> filetype has 7 bytes of slack after it, which is wasteful.
> 
> namelen will never be greater than MAXNAMELEN, which is 256.  This field
> could be reduced to a short.
> 
> Rearrange the fields in xfs_da_args to waste less space.  This reduces
> the structure size from 136 bytes to 128.  Later when we add extra
> fields to support parent pointer replacement, this will only bloat the
> structure to 144 bytes, instead of 168.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Eventually we should probaly split this up, at lot of fields are
used only by the attr set code, and a few less only by dir vs attr.
Darrick J. Wong April 10, 2024, 8:56 p.m. UTC | #2 
On Tue, Apr 09, 2024 at 10:02:45PM -0700, Christoph Hellwig wrote:
> On Tue, Apr 09, 2024 at 05:50:23PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > A few notes about struct xfs_da_args:
> > 
> > The XFS_ATTR_* flags only go up as far as XFS_ATTR_INCOMPLETE, which
> > means that attr_filter could be a u8 field.
> > 
> > The XATTR_* flags only have two values, which means that xattr_flags
> > could be shrunk to a u8.
> > 
> > I've reduced the number of XFS_DA_OP_* flags down to the point where
> > op_flags would also fit into a u8.
> > 
> > filetype has 7 bytes of slack after it, which is wasteful.
> > 
> > namelen will never be greater than MAXNAMELEN, which is 256.  This field
> > could be reduced to a short.
> > 
> > Rearrange the fields in xfs_da_args to waste less space.  This reduces
> > the structure size from 136 bytes to 128.  Later when we add extra
> > fields to support parent pointer replacement, this will only bloat the
> > structure to 144 bytes, instead of 168.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Eventually we should probaly split this up, at lot of fields are
> used only by the attr set code, and a few less only by dir vs attr.

Agreed, though we're veering dangerously close to object inheritance.

But it would be useful for code analysis if dir operations would pass in
an xfs_dir_op structure containing a much smaller xfs_da_args and
likewise for xattrs.

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
index e585d0fa9caea..47485f5edae86 100644
--- a/fs/xfs/libxfs/xfs_da_btree.h
+++ b/fs/xfs/libxfs/xfs_da_btree.h
@@ -54,17 +54,21 @@  enum xfs_dacmp {
  */
 typedef struct xfs_da_args {
 	struct xfs_da_geometry *geo;	/* da block geometry */
-	const uint8_t		*name;		/* string (maybe not NULL terminated) */
-	int		namelen;	/* length of string (maybe no NULL) */
-	uint8_t		filetype;	/* filetype of inode for directories */
+	const uint8_t	*name;		/* string (maybe not NULL terminated) */
 	void		*value;		/* set of bytes (maybe contain NULLs) */
-	int		valuelen;	/* length of value */
-	unsigned int	attr_filter;	/* XFS_ATTR_{ROOT,SECURE,INCOMPLETE} */
-	unsigned int	xattr_flags;	/* XATTR_{CREATE,REPLACE} */
-	xfs_dahash_t	hashval;	/* hash value of name */
-	xfs_ino_t	inumber;	/* input/output inode number */
 	struct xfs_inode *dp;		/* directory inode to manipulate */
 	struct xfs_trans *trans;	/* current trans (changes over time) */
+
+	xfs_ino_t	inumber;	/* input/output inode number */
+	xfs_ino_t	owner;		/* inode that owns the dir/attr data */
+
+	int		valuelen;	/* length of value */
+	uint8_t		filetype;	/* filetype of inode for directories */
+	uint8_t		op_flags;	/* operation flags */
+	uint8_t		attr_filter;	/* XFS_ATTR_{ROOT,SECURE,INCOMPLETE} */
+	uint8_t		xattr_flags;	/* XATTR_{CREATE,REPLACE} */
+	short		namelen;	/* length of string (maybe no NULL) */
+	xfs_dahash_t	hashval;	/* hash value of name */
 	xfs_extlen_t	total;		/* total blocks needed, for 1st bmap */
 	int		whichfork;	/* data or attribute fork */
 	xfs_dablk_t	blkno;		/* blkno of attr leaf of interest */
@@ -77,9 +81,7 @@  typedef struct xfs_da_args {
 	xfs_dablk_t	rmtblkno2;	/* remote attr value starting blkno */
 	int		rmtblkcnt2;	/* remote attr value block count */
 	int		rmtvaluelen2;	/* remote attr value length in bytes */
-	uint32_t	op_flags;	/* operation flags */
 	enum xfs_dacmp	cmpresult;	/* name compare result for lookups */
-	xfs_ino_t	owner;		/* inode that owns the dir/attr data */
 } xfs_da_args_t;
 
 /*