sanitize xlog_in_core_t definition

PATCH In review Christoph Hellwig 2 participants 4 messages Oct 27, 2008 – Oct 28, 2008 XFS filesystem

Share & tools

Christoph Hellwig

author

2008-10-27 13:39 UTC #0 lore raw

PATCH 3/7 sanitize xlog_in_core_t definition

Move all fields from xlog_iclog_fields_t into xlog_in_core_t instead of having them in a substructure and the using #defines to make it look like they were directly in xlog_in_core_t. Also document that xlog_in_core_2_t is grossly misnamed, and make all references to it typesafe.

(First sent on Semptember 15th)

Signed-off-by Christoph Hellwig <[email protected]>

fs/xfs/xfs_log.c +3 -9

Index: linux-2.6-xfs/fs/xfs/xfs_log.c
===================================================================
@@ -1024,12 +1024,6 @@ xlog_iodone(xfs_buf_t *bp)
 	ASSERT(XFS_BUF_FSPRIVATE2(bp, unsigned long) == (unsigned long) 2);
 	XFS_BUF_SET_FSPRIVATE2(bp, (unsigned long)1);
 	aborted = 0;
-
-	/*
-	 * Some versions of cpp barf on the recursive definition of
-	 * ic_log -> hic_fields.ic_log and expand ic_log twice when
-	 * it is passed through two macros.  Workaround broken cpp.
-	 */
 	l = iclog->ic_log;
 
 	/*

@@ -1287,7 +1281,7 @@ xlog_alloc_log(xfs_mount_t	*mp,
 		XFS_BUF_SET_BDSTRAT_FUNC(bp, xlog_bdstrat_cb);
 		XFS_BUF_SET_FSPRIVATE2(bp, (unsigned long)1);
 		iclog->ic_bp = bp;
-		iclog->hic_data = bp->b_addr;
+		iclog->ic_data = bp->b_addr;
 #ifdef DEBUG
 		log->l_iclog_bak[i] = (xfs_caddr_t)&(iclog->ic_header);
 #endif

@@ -1307,7 +1301,7 @@ xlog_alloc_log(xfs_mount_t	*mp,
 		atomic_set(&iclog->ic_refcnt, 0);
 		spin_lock_init(&iclog->ic_callback_lock);
 		iclog->ic_callback_tail = &(iclog->ic_callback);
-		iclog->ic_datap = (char *)iclog->hic_data + log->l_iclog_hsize;
+		iclog->ic_datap = (char *)iclog->ic_data + log->l_iclog_hsize;
 
 		ASSERT(XFS_BUF_ISBUSY(iclog->ic_bp));
 		ASSERT(XFS_BUF_VALUSEMA(iclog->ic_bp) <= 0);

@@ -3418,7 +3412,7 @@ xlog_verify_iclog(xlog_t	 *log,
 	ptr = iclog->ic_datap;
 	base_ptr = ptr;
 	ophead = (xlog_op_header_t *)ptr;
-	xhdr = (xlog_in_core_2_t *)&iclog->ic_header;
+	xhdr = iclog->ic_data;
 	for (i = 0; i < len; i++) {
 		ophead = (xlog_op_header_t *)ptr;

fs/xfs/xfs_log_priv.h +13 ~~-33~~

67 lines not on this page · raw message

fs/xfs/xfs_log_recover.c +3 -4

Index: linux-2.6-xfs/fs/xfs/xfs_log_recover.c
===================================================================
@@ -3359,7 +3359,6 @@ xlog_pack_data(
 	int			size = iclog->ic_offset + roundoff;
 	__be32			cycle_lsn;
 	xfs_caddr_t		dp;
-	xlog_in_core_2_t	*xhdr;
 
 	xlog_pack_data_checksum(log, iclog, size);

@@ -3374,7 +3373,8 @@ xlog_pack_data(
 	}
 
 	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
-		xhdr = (xlog_in_core_2_t *)&iclog->ic_header;
+		xlog_in_core_2_t *xhdr = iclog->ic_data;
+
 		for ( ; i < BTOBB(size); i++) {
 			j = i / (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
 			k = i % (XLOG_HEADER_CYCLE_SIZE / BBSIZE);

@@ -3432,7 +3432,6 @@ xlog_unpack_data(
 	xlog_t			*log)
 {
 	int			i, j, k;
-	xlog_in_core_2_t	*xhdr;
 
 	for (i = 0; i < BTOBB(be32_to_cpu(rhead->h_len)) &&
 		  i < (XLOG_HEADER_CYCLE_SIZE / BBSIZE); i++) {

@@ -3441,7 +3440,7 @@ xlog_unpack_data(
 	}
 
 	if (xfs_sb_version_haslogv2(&log->l_mp->m_sb)) {
-		xhdr = (xlog_in_core_2_t *)rhead;
+		xlog_in_core_2_t *xhdr = (xlog_in_core_2_t *)rhead;
 		for ( ; i < BTOBB(be32_to_cpu(rhead->h_len)); i++) {
 			j = i / (XLOG_HEADER_CYCLE_SIZE / BBSIZE);
 			k = i % (XLOG_HEADER_CYCLE_SIZE / BBSIZE);

fs/xfs/xfsidbg.c +1 -1

Index: linux-2.6-xfs/fs/xfs/xfsidbg.c
===================================================================
@@ -5763,7 +5763,7 @@ xfsidbg_xiclog(xlog_in_core_t *iclog)
 	};
 
 	kdb_printf("xlog_in_core/header at 0x%p/0x%p\n",
-		iclog, iclog->hic_data);
+		iclog, iclog->ic_data);
 	kdb_printf("magicno: %x  cycle: %d  version: %d  lsn: 0x%Lx\n",
 		be32_to_cpu(iclog->ic_header.h_magicno),
 		be32_to_cpu(iclog->ic_header.h_cycle),

Dave Chinner

2008-10-28 05:17 UTC #1 lore raw

Quoted 6 lines · Christoph Hellwig

Move all fields from xlog_iclog_fields_t into xlog_in_core_t instead of having them in a substructure and the using #defines to make it look like they were directly in xlog_in_core_t. Also document that xlog_in_core_2_t is grossly misnamed, and make all references to it typesafe.
(First sent on Semptember 15th)

Couple of things.

Quoted 8 lines

fs/xfs/xfs_log_priv.h +0 -4

@@ -361,41 +371,11 @@ typedef struct xlog_iclog_fields {
 
 	/* reference counts need their own cacheline */
 	atomic_t		ic_refcnt ____cacheline_aligned_in_smp;
-} xlog_iclog_fields_t;
-
-typedef union xlog_in_core2 {
-	xlog_rec_header_t	hic_header;

fs/xfs/xfs_log_priv.h typedef struct xlog_iclog_fields { · line 376

Quoted 10 lines

fs/xfs/xfs_log_priv.h +2 -7

-	xlog_rec_ext_header_t	hic_xheader;
-	char			hic_sector[XLOG_HEADER_SIZE];
-} xlog_in_core_2_t;
-
-typedef struct xlog_in_core {
-	xlog_iclog_fields_t	hic_fields;
-	xlog_in_core_2_t	*hic_data;
+	xlog_in_core_2_t	*ic_data;
+#define ic_header	ic_data->hic_header
 } xlog_in_core_t;

The ic_data pointer should not be on the same cacheline as the reference count seeing as it is read-only field.

Otherwise seems ok.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Christoph Hellwig

author

2008-10-28 09:16 UTC #2 lore raw

Quoted 7 lines · Dave Chinner

Quoted 7 lines

fs/xfs/xfs_log_priv.h +0 -7

-typedef union xlog_in_core2 {
-	xlog_rec_header_t	hic_header;
-	xlog_rec_ext_header_t	hic_xheader;
-	char			hic_sector[XLOG_HEADER_SIZE];
-} xlog_in_core_2_t;
-
-typedef struct xlog_in_core {

Quoted 8 lines

fs/xfs/xfs_log_priv.h typedef struct xlog_iclog_fields { · line 376
Quoted 5 lines
fs/xfs/xfs_log_priv.h +2 -2
-	xlog_iclog_fields_t	hic_fields;
-	xlog_in_core_2_t	*hic_data;
+	xlog_in_core_2_t	*ic_data;
+#define ic_header	ic_data->hic_header
 } xlog_in_core_t;
The ic_data pointer should not be on the same cacheline as the reference count seeing as it is read-only field.

Makes sense, but I'll leave that for a separate patch to keep things bisectable.

Dave Chinner

Reviewed-by

2008-10-28 21:51 UTC #3 lore raw

Quoted 17 lines · Christoph Hellwig

Quoted 7 lines · Dave Chinner
Quoted 7 lines
fs/xfs/xfs_log_priv.h +0 -7
-typedef union xlog_in_core2 {
-	xlog_rec_header_t	hic_header;
-	xlog_rec_ext_header_t	hic_xheader;
-	char			hic_sector[XLOG_HEADER_SIZE];
-} xlog_in_core_2_t;
-
-typedef struct xlog_in_core {
Quoted 8 lines
fs/xfs/xfs_log_priv.h typedef struct xlog_iclog_fields { · line 376
Quoted 5 lines
fs/xfs/xfs_log_priv.h +2 -2
-	xlog_iclog_fields_t	hic_fields;
-	xlog_in_core_2_t	*hic_data;
+	xlog_in_core_2_t	*ic_data;
+#define ic_header	ic_data->hic_header
 } xlog_in_core_t;
The ic_data pointer should not be on the same cacheline as the reference count seeing as it is read-only field.

Quoted 2 lines

Makes sense, but I'll leave that for a separate patch to keep things bisectable.

ok.

Reviewed-by Dave Chinner <[email protected]>

Signature · 2 lines

Dave Chinner
[email protected]