xfs: remedy small writes during wrapped-log recovery

A bisect still needs to be done to determine when the slow mount behavior started. It could very well be that somebody fixed the buffer_io_error messages that I saw long ago, and the solution made some mounts here rather miserable.

Hi! I like this patch and am confident with it on x86. However,
a) it has no x86_64 coverage; and b) xfstests xfs/306 in particular emits more of this output:

buffer_io_error: nnnn callbacks suppressed

Might someone evaluate this patch or the intent of the patch?

The intent:

For XFS filesystems that don't change much, such as the /boot and alternate / partitions here, mount times were about 17s instead of 0.4s while the log is in a wrapped state, write caches off. This patch fixes the issue on v4- and v5-superblock XFS filesystems.

xfs_repair can solve this issue short-term and also cut wrapped-log mount time in half short-term for v5 file systems. Don't know if that's a mkfs.xfs issue or just coincidence.

A bisect still needs to be done to determine when the slow mount behavior started. It could very well be that somebody fixed the buffer_io_error messages that I saw long ago, and the solution made some mounts here rather miserable.

Thanks!

Michael

The patch:

xlog_write_log_records() has an algorithm to "Greedily allocate a buffer big enough...," starting with ffs(blocks), adding two sensible checks, and then feeding it to a loop with checks of its own.

However, when blocks is an odd number, the number that becomes nbblks to the xlog_bwrite() function ends up being 2 (1 << 1). The most obvious effect is that when the log wraps, a write of two odd-sized log regions on an 8-GB XFS filesystem will take around 2049 calls to xlog_bwrite() instead of the "two separate I/Os" suggested in xlog_clear_stale_blocks().

Fix this by changing the ffs(blocks) to fls(blocks).

There is a similar ffs(blocks) check in xlog_find_verify_cycle(). This was not investigated.

Signed-off-by Michael L. Semon <[email protected]>

---
 fs/xfs/xfs_log_recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_log_recover.c +0 -0

index a5a945f..13381eb 100644

fs/xfs/xfs_log_recover.c +0 -0

 	while (bufblks > log->l_logBBsize)
 		bufblks >>= 1;
 	while (!(bp = xlog_get_bp(log, bufblks))) {

Signature · 6 lines

1.8.4

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

Quoted 56 lines · Michael L. Semon

Hi! I like this patch and am confident with it on x86. However,
a) it has no x86_64 coverage; and b) xfstests xfs/306 in particular emits more of this output:

buffer_io_error: nnnn callbacks suppressed

Might someone evaluate this patch or the intent of the patch?

The intent:

For XFS filesystems that don't change much, such as the /boot and alternate / partitions here, mount times were about 17s instead of 0.4s while the log is in a wrapped state, write caches off. This patch fixes the issue on v4- and v5-superblock XFS filesystems.

xfs_repair can solve this issue short-term and also cut wrapped-log mount time in half short-term for v5 file systems. Don't know if that's a mkfs.xfs issue or just coincidence.

A bisect still needs to be done to determine when the slow mount behavior started. It could very well be that somebody fixed the buffer_io_error messages that I saw long ago, and the solution made some mounts here rather miserable.

Thanks!

Michael

The patch:

xlog_write_log_records() has an algorithm to "Greedily allocate a buffer big enough...," starting with ffs(blocks), adding two sensible checks, and then feeding it to a loop with checks of its own.

However, when blocks is an odd number, the number that becomes nbblks to the xlog_bwrite() function ends up being 2 (1 << 1). The most obvious effect is that when the log wraps, a write of two odd-sized log regions on an 8-GB XFS filesystem will take around 2049 calls to xlog_bwrite() instead of the "two separate I/Os" suggested in xlog_clear_stale_blocks().

Fix this by changing the ffs(blocks) to fls(blocks).

There is a similar ffs(blocks) check in xlog_find_verify_cycle(). This was not investigated.

Signed-off-by Michael L. Semon <[email protected]>

---
 fs/xfs/xfs_log_recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_log_recover.c +0 -0

index a5a945f..13381eb 100644

fs/xfs/xfs_log_recover.c xlog_write_log_records( · line 1245

Quoted 5 lines

fs/xfs/xfs_log_recover.c +1 -1

@@ -1242,7 +1242,7 @@ xlog_write_log_records(
 	 * a smaller size.  We need to be able to write at least a
 	 * log sector, or we're out of luck.
 	 */
-	bufblks = 1 << ffs(blocks);
+	bufblks = 1 << fls(blocks);

Interesting, it does seem like there is a bug here. Thanks for the test code to help reproduce.

The fix seems reasonable to me, but I'm also wondering if there is at least one other bug in this code. In the middle of that loop, we have the following:

	...

	/* We may need to do a read at the end to fill in part of

(quoted diff) +0 -0

 	* the buffer in the final sector not covered by the write.
 	* If this is the same sector as the above read, skip it.
 	*/

	ealign = round_down(end_block, sectbb);
	if (j == 0 && (start_block + endcount > ealign)) {
		offset = bp->b_addr + BBTOB(ealign - start_block);
		error = xlog_bread_offset(log, ealign, sectbb,
						bp, offset);
		...
	}
	...

... but how have we really confirmed whether the end sector is equivalent to the first sector? It looks to me that we operate at basic block granularity but log I/O is managed at log sector alignment. So if the start basic block is not sector aligned, we read in the first sector and add records at the associated buffer offset. Similar if the end block is not sector aligned. If the buffer size spans multiple sectors and the start and end are not aligned, it looks like we could skip the read of the final sector.

Perhaps I'm missing some context as to why this wouldn't occur..? It also seems strange that the offset calculation above uses start_block as the baseline start block value of the buffer, but the pre-loop balign code suggests the buffer might not be aligned to start_block...

Brian

Quoted 99 lines · Brian Foster

Quoted 56 lines · Michael L. Semon

Hi! I like this patch and am confident with it on x86. However,
a) it has no x86_64 coverage; and b) xfstests xfs/306 in particular emits more of this output:

buffer_io_error: nnnn callbacks suppressed

Might someone evaluate this patch or the intent of the patch?

The intent:

For XFS filesystems that don't change much, such as the /boot and alternate / partitions here, mount times were about 17s instead of 0.4s while the log is in a wrapped state, write caches off. This patch fixes the issue on v4- and v5-superblock XFS filesystems.

xfs_repair can solve this issue short-term and also cut wrapped-log mount time in half short-term for v5 file systems. Don't know if that's a mkfs.xfs issue or just coincidence.

A bisect still needs to be done to determine when the slow mount behavior started. It could very well be that somebody fixed the buffer_io_error messages that I saw long ago, and the solution made some mounts here rather miserable.

Thanks!

Michael

The patch:

xlog_write_log_records() has an algorithm to "Greedily allocate a buffer big enough...," starting with ffs(blocks), adding two sensible checks, and then feeding it to a loop with checks of its own.

However, when blocks is an odd number, the number that becomes nbblks to the xlog_bwrite() function ends up being 2 (1 << 1). The most obvious effect is that when the log wraps, a write of two odd-sized log regions on an 8-GB XFS filesystem will take around 2049 calls to xlog_bwrite() instead of the "two separate I/Os" suggested in xlog_clear_stale_blocks().

Fix this by changing the ffs(blocks) to fls(blocks).

There is a similar ffs(blocks) check in xlog_find_verify_cycle(). This was not investigated.

Signed-off-by Michael L. Semon <[email protected]>

---
 fs/xfs/xfs_log_recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_log_recover.c +0 -0

index a5a945f..13381eb 100644

fs/xfs/xfs_log_recover.c xlog_write_log_records( · line 1245

Quoted 5 lines

fs/xfs/xfs_log_recover.c +1 -1

@@ -1242,7 +1242,7 @@ xlog_write_log_records(
 	 * a smaller size.  We need to be able to write at least a
 	 * log sector, or we're out of luck.
 	 */
-	bufblks = 1 << ffs(blocks);
+	bufblks = 1 << fls(blocks);

Interesting, it does seem like there is a bug here. Thanks for the test code to help reproduce.

The fix seems reasonable to me, but I'm also wondering if there is at least one other bug in this code. In the middle of that loop, we have the following:

	...

	/* We may need to do a read at the end to fill in part of

(quoted diff) +0 -0

 	* the buffer in the final sector not covered by the write.
 	* If this is the same sector as the above read, skip it.
 	*/

	ealign = round_down(end_block, sectbb);
	if (j == 0 && (start_block + endcount > ealign)) {
		offset = bp->b_addr + BBTOB(ealign - start_block);
		error = xlog_bread_offset(log, ealign, sectbb,
						bp, offset);
		...
	}
	...

... but how have we really confirmed whether the end sector is equivalent to the first sector? It looks to me that we operate at basic block granularity but log I/O is managed at log sector alignment. So if the start basic block is not sector aligned, we read in the first sector and add records at the associated buffer offset. Similar if the end block is not sector aligned. If the buffer size spans multiple sectors and the start and end are not aligned, it looks like we could skip the read of the final sector.

Perhaps I'm missing some context as to why this wouldn't occur..? It also seems strange that the offset calculation above uses start_block as the baseline start block value of the buffer, but the pre-loop balign code suggests the buffer might not be aligned to start_block...

Quoted 1 line

Brian

I'm currently stumped on getting this code to fire. For that matter, all blk_no and nbblks numbers are coming in to xlog_bwrite() neatly pre- rounded, so the rounding functions in there don't change anything. In all, an unsuccessful testing effort on my part.

Maybe the "env MKFS_OPTIONS='-m crc=1,finobt=1 -s size=4096' ./check -g auto" xfstests run will cough something up. It will be waiting at home, at the end of the day.

Should I use fdisk on a spare disk and deliberately misalign the partitions? Otherwise, there's a struggle to find misalignments, and my idea bucket was not very full, anyway ;-)

Thanks!

Michael