Skip to contentSkip to conversation

xfs: track the log sector size rather than log2(that)

PATCH In review Alex Elder 2 participants 3 messages Apr 12, 2010 – Apr 14, 2010 XFS filesystem

lore
Share & tools
lore

Alex Elder

author
#0 lore raw

PATCH v2 10/10 xfs: track the log sector size rather than log2(that)

Change struct log so it keeps track of the size (in basic blocks) of a log sector in l_sectsize rather than the log-base-2 of that value (previously, l_sectbb_log).

---
fs/xfs/xfs_log_recover.c | 21 +++++++++++++++++----

 fs/xfs/xfs_log.c         |   33 ++++++++++++++++++---------------
 fs/xfs/xfs_log_priv.h    |    2 +-
 fs/xfs/xfs_log_recover.c |   27 ++++++++++++---------------
 3 files changed, 31 insertions(+), 31 deletions(-)
fs/xfs/xfs_log.c +18 -15
Index: b/fs/xfs/xfs_log.c
===================================================================
@@ -1025,6 +1025,7 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	int			i;
 	int			iclogsize;
 	int			error = ENOMEM;
+	uint			sector_bits = 0;
 
 	log = kmem_zalloc(sizeof(xlog_t), KM_MAYFAIL);
 	if (!log) {
@@ -1050,29 +1051,31 @@ xlog_alloc_log(xfs_mount_t	*mp,
 
 	error = EFSCORRUPTED;
 	if (xfs_sb_version_hassector(&mp->m_sb)) {
-		log->l_sectbb_log = mp->m_sb.sb_logsectlog - BBSHIFT;
-		if (log->l_sectbb_log < 0 ||
-		    log->l_sectbb_log > mp->m_sectbb_log) {
-			xlog_warn("XFS: Log sector size (0x%x) out of range.",
-						log->l_sectbb_log);
+	        sector_bits = mp->m_sb.sb_logsectlog;
+		if (sector_bits < BBSHIFT) {
+			xlog_warn("XFS: Log sector byte size too small "
+				"(0x%x < 0x%x)", sector_bits, BBSHIFT);
 			goto out_free_log;
 		}
 
-		/* for larger sector sizes, must have v2 or external log */
-		if (log->l_sectbb_log != 0 &&
-		    (log->l_logBBstart != 0 &&
-		     !xfs_sb_version_haslogv2(&mp->m_sb))) {
-			xlog_warn("XFS: log sector size (0x%x) invalid "
-				  "for configuration.", log->l_sectbb_log);
+	        sector_bits -= BBSHIFT;
+		if (sector_bits > mp->m_sectbb_log) {
+			xlog_warn("XFS: Log sector basic block size too large "
+				"(0x%x > 0x%x)", sector_bits, mp->m_sectbb_log);
 			goto out_free_log;
 		}
-		if (mp->m_sb.sb_logsectlog < BBSHIFT) {
-			xlog_warn("XFS: Log sector log (0x%x) too small.",
-						mp->m_sb.sb_logsectlog);
+
+		/* for larger sector sizes, must have v2 or external log */
+		if (sector_bits && (log->l_logBBstart > 0 &&
+					!xfs_sb_version_haslogv2(&mp->m_sb))) {
+
+			xlog_warn("XFS: log sector size (0x%x) invalid "
+				  "for configuration.", sector_bits);
 			goto out_free_log;
 		}
 	}
-	log->l_sectbb_mask = (1 << log->l_sectbb_log) - 1;
+	log->l_sectsize = 1 << sector_bits;
+	log->l_sectbb_mask = log->l_sectsize - 1;
 
 	xlog_get_iclog_buffer_size(mp, log);
 
fs/xfs/xfs_log_priv.h +1 -1
Index: b/fs/xfs/xfs_log_priv.h
===================================================================
@@ -396,7 +396,7 @@ typedef struct log {
 	struct xfs_buf_cancel	**l_buf_cancel_table;
 	int			l_iclog_hsize;  /* size of iclog header */
 	int			l_iclog_heads;  /* # of iclog header sectors */
-	uint			l_sectbb_log;   /* log2 of sector size in BBs */
+	uint			l_sectsize;     /* sector size in BBs */
 	uint			l_sectbb_mask;  /* sector size (in BBs)
 						 * alignment mask */
 	int			l_iclog_size;	/* size of log in bytes */
fs/xfs/xfs_log_recover.c +12 -15
Index: b/fs/xfs/xfs_log_recover.c
===================================================================
@@ -60,9 +60,6 @@ STATIC void	xlog_recover_check_summary(x
  * Sector aligned buffer routines for buffer create/read/write/access
  */
 
-/* Number of basic blocks in a log sector */
-#define xlog_sectbb(log) (1 << (log)->l_sectbb_log)
-
 /*
  * Verify the given count of basic blocks is valid number of blocks
  * to specify for an operation involving the given XFS log buffer.
@@ -110,9 +107,9 @@ xlog_get_bp(
 	 * extend the buffer by one extra log sector to ensure
 	 * there's space to accomodate this possiblility.
 	 */
-	if (nbblks > 1 && log->l_sectbb_log)
-		nbblks += xlog_sectbb(log);
-	nbblks = round_up(nbblks, xlog_sectbb(log));
+	if (nbblks > 1 && log->l_sectsize > 1)
+		nbblks += log->l_sectsize;
+	nbblks = round_up(nbblks, log->l_sectsize);
 
 	return xfs_buf_get_noaddr(BBTOB(nbblks), log->l_mp->m_logdev_targp);
 }
@@ -133,7 +130,7 @@ xlog_align(
 {
 	xfs_caddr_t	ptr;
 
-	if (!log->l_sectbb_log)
+	if (log->l_sectsize == 1)
 		return XFS_BUF_PTR(bp);
 
 	ptr = XFS_BUF_PTR(bp) + BBTOB((int)blk_no & log->l_sectbb_mask);
@@ -162,8 +159,8 @@ xlog_bread_noalign(
 		return EFSCORRUPTED;
 	}
 
-	blk_no = round_down(blk_no, xlog_sectbb(log));
-	nbblks = round_up(nbblks, xlog_sectbb(log));
+	blk_no = round_down(blk_no, log->l_sectsize);
+	nbblks = round_up(nbblks, log->l_sectsize);
 
 	ASSERT(nbblks > 0);
 	ASSERT(bp);
@@ -222,8 +219,8 @@ xlog_bwrite(
 		return EFSCORRUPTED;
 	}
 
-	blk_no = round_down(blk_no, xlog_sectbb(log));
-	nbblks = round_up(nbblks, xlog_sectbb(log));
+	blk_no = round_down(blk_no, log->l_sectsize);
+	nbblks = round_up(nbblks, log->l_sectsize);
 
 	ASSERT(nbblks > 0);
 	ASSERT(BBTOB(nbblks) <= XFS_BUF_SIZE(bp));
@@ -417,7 +414,7 @@ xlog_find_verify_cycle(
 	bufblks = 1 << ffs(nbblks);
 	while (!(bp = xlog_get_bp(log, bufblks))) {
 		bufblks >>= 1;
-		if (bufblks < xlog_sectbb(log))
+		if (bufblks < log->l_sectsize)
 			return ENOMEM;
 	}
 
@@ -1188,7 +1185,7 @@ xlog_write_log_records(
 	xfs_caddr_t	offset;
 	xfs_buf_t	*bp;
 	int		balign, ealign;
-	int		sectbb = xlog_sectbb(log);
+	int		sectbb = log->l_sectsize;
 	int		end_block = start_block + blocks;
 	int		bufblks;
 	int		error = 0;
@@ -1203,7 +1200,7 @@ xlog_write_log_records(
 	bufblks = 1 << ffs(blocks);
 	while (!(bp = xlog_get_bp(log, bufblks))) {
 		bufblks >>= 1;
-		if (bufblks < xlog_sectbb(log))
+		if (bufblks < log->l_sectsize)
 			return ENOMEM;
 	}
 
@@ -3532,7 +3529,7 @@ xlog_do_recovery_pass(
 			hblks = 1;
 		}
 	} else {
-		ASSERT(log->l_sectbb_log == 0);
+		ASSERT(log->l_sectsize == 1);
 		hblks = 1;
 		hbp = xlog_get_bp(log, 1);
 		h_size = XLOG_BIG_RECORD_BSIZE;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#1 lore raw
Quoted 5 lines · Alex Elder

Change struct log so it keeps track of the size (in basic blocks) of a log sector in l_sectsize rather than the log-base-2 of that value (previously, l_sectbb_log).

OK, this is what I commented on earlier....

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1057

Quoted 13 lines
fs/xfs/xfs_log.c +4 -5
@@ -1050,29 +1051,31 @@ xlog_alloc_log(xfs_mount_t	*mp,
 
 	error = EFSCORRUPTED;
 	if (xfs_sb_version_hassector(&mp->m_sb)) {
-		log->l_sectbb_log = mp->m_sb.sb_logsectlog - BBSHIFT;
-		if (log->l_sectbb_log < 0 ||
-		    log->l_sectbb_log > mp->m_sectbb_log) {
-			xlog_warn("XFS: Log sector size (0x%x) out of range.",
-						log->l_sectbb_log);
+	        sector_bits = mp->m_sb.sb_logsectlog;
+		if (sector_bits < BBSHIFT) {
+			xlog_warn("XFS: Log sector byte size too small "
+				"(0x%x < 0x%x)", sector_bits, BBSHIFT);

"sector_bits"? That doesn't seem to fit the units the log sector size being read from and checked against. i.e. it's not bits, nor is it bytes (as the error message says). It's really log2(sector size), so perhaps calling it sector_size_log would be more accurate.

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1063

Quoted 12 lines
fs/xfs/xfs_log.c +3 -6
 			goto out_free_log;
 		}
 
-		/* for larger sector sizes, must have v2 or external log */
-		if (log->l_sectbb_log != 0 &&
-		    (log->l_logBBstart != 0 &&
-		     !xfs_sb_version_haslogv2(&mp->m_sb))) {
-			xlog_warn("XFS: log sector size (0x%x) invalid "
-				  "for configuration.", log->l_sectbb_log);
+	        sector_bits -= BBSHIFT;
+		if (sector_bits > mp->m_sectbb_log) {
+			xlog_warn("XFS: Log sector basic block size too large "

It's just the log sector size is too large - no need for "basic block" in that error message.

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1070

Quoted 11 lines
fs/xfs/xfs_log.c +5 -3
+				"(0x%x > 0x%x)", sector_bits, mp->m_sectbb_log);
 			goto out_free_log;
 		}
 
-		if (mp->m_sb.sb_logsectlog < BBSHIFT) {
-			xlog_warn("XFS: Log sector log (0x%x) too small.",
-						mp->m_sb.sb_logsectlog);
+
+		/* for larger sector sizes, must have v2 or external log */
+		if (sector_bits && (log->l_logBBstart > 0 &&
+					!xfs_sb_version_haslogv2(&mp->m_sb))) {

no need for the extra ()s there.

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1078

Quoted 9 lines
fs/xfs/xfs_log.c +5 -1
+
+			xlog_warn("XFS: log sector size (0x%x) invalid "
+				  "for configuration.", sector_bits);
 			goto out_free_log;
 		}
 	}
-	log->l_sectbb_mask = (1 << log->l_sectbb_log) - 1;
+	log->l_sectsize = 1 << sector_bits;
+	log->l_sectbb_mask = log->l_sectsize - 1;

There's only only place left that uses l_sectbb_mask left (in xlog_align()), so you could probably kill it while you are at it.

Everything else looks OK.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#2 lore raw
Quoted 11 lines · Dave Chinner
Quoted 5 lines · Alex Elder

Change struct log so it keeps track of the size (in basic blocks) of a log sector in l_sectsize rather than the log-base-2 of that value (previously, l_sectbb_log).

OK, this is what I commented on earlier....

Quoted 2 lines
(quoted diff) +0 -0
@@ -1050,29 +1051,31 @@ xlog_alloc_log(xfs_mount_t	*mp,
 
Quoted 16 lines

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1057

Quoted 11 lines
fs/xfs/xfs_log.c +4 -5
 	error = EFSCORRUPTED;
 	if (xfs_sb_version_hassector(&mp->m_sb)) {
-		log->l_sectbb_log = mp->m_sb.sb_logsectlog - BBSHIFT;
-		if (log->l_sectbb_log < 0 ||
-		    log->l_sectbb_log > mp->m_sectbb_log) {
-			xlog_warn("XFS: Log sector size (0x%x) out of range.",
-						log->l_sectbb_log);
+	        sector_bits = mp->m_sb.sb_logsectlog;
+		if (sector_bits < BBSHIFT) {
+			xlog_warn("XFS: Log sector byte size too small "
+				"(0x%x < 0x%x)", sector_bits, BBSHIFT);

"sector_bits"? That doesn't seem to fit the units the log sector size being read from and checked against. i.e. it's not bits, nor is it bytes (as the error message says). It's really log2(sector size), so perhaps calling it sector_size_log would be more accurate.

My intent here was to avoid the use of "log" to mean "logarithm" in the log (meaning "log") code... The "bits" here is the number of bits required to represent a log sector. Sort of like CHAR_BIT is 8. I also intentionally avoided encoded units in the name, since what it represents changes from representing a number of bytes to representing a number of blocks.

I'll try to reformulate this, see if I can come up with something more clear.

Quoted 15 lines

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1063

Quoted 12 lines
fs/xfs/xfs_log.c +3 -6
 			goto out_free_log;
 		}
 
-		/* for larger sector sizes, must have v2 or external log */
-		if (log->l_sectbb_log != 0 &&
-		    (log->l_logBBstart != 0 &&
-		     !xfs_sb_version_haslogv2(&mp->m_sb))) {
-			xlog_warn("XFS: log sector size (0x%x) invalid "
-				  "for configuration.", log->l_sectbb_log);
+	        sector_bits -= BBSHIFT;
+		if (sector_bits > mp->m_sectbb_log) {
+			xlog_warn("XFS: Log sector basic block size too large "

It's just the log sector size is too large - no need for "basic block" in that error message.

I differentiated because the earlier value was really
log2(bytes in a sector)
while this one is
log2(basic blocks in a sector)

If someone were really looking at these, alongside the code, they could figure out the difference, so I don't really care much about this. I'll take another shot at it and will repost.

Quoted 5 lines
Quoted 4 lines
fs/xfs/xfs_log.c +1 -0
+				"(0x%x > 0x%x)", sector_bits, mp->m_sectbb_log);
 			goto out_free_log;
 		}
Quoted 9 lines

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1070

Quoted 7 lines
fs/xfs/xfs_log.c +4 -3
-		if (mp->m_sb.sb_logsectlog < BBSHIFT) {
-			xlog_warn("XFS: Log sector log (0x%x) too small.",
-						mp->m_sb.sb_logsectlog);
+
+		/* for larger sector sizes, must have v2 or external log */
+		if (sector_bits && (log->l_logBBstart > 0 &&
+					!xfs_sb_version_haslogv2(&mp->m_sb))) {

no need for the extra ()s there.

You're right; they were there in the original code. I don't like them and will remove them.

Quoted 4 lines
Quoted 4 lines
fs/xfs/xfs_log.c +3 -0
+
+			xlog_warn("XFS: log sector size (0x%x) invalid "
+				  "for configuration.", sector_bits);
 			goto out_free_log;
Quoted 8 lines

fs/xfs/xfs_log.c xlog_alloc_log(xfs_mount_t *mp, · line 1078

Quoted 5 lines
fs/xfs/xfs_log.c +2 -1
 		}
 	}
-	log->l_sectbb_mask = (1 << log->l_sectbb_log) - 1;
+	log->l_sectsize = 1 << sector_bits;
+	log->l_sectbb_mask = log->l_sectsize - 1;

There's only only place left that uses l_sectbb_mask left (in xlog_align()), so you could probably kill it while you are at it.

Agreed. I will add another patch to do that.

Quoted 1 line

Everything else looks OK.

Thanks for the reviews.

-Alex

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs