REVIEW: fix xfs_repair phase 4 ag_stride with prefetch

REVIEW: fix xfs_repair phase 4 ag_stride with prefetch

[Barry Naujok]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]


AG stride testing was performed on a system with ample amounts
of memory, so prefetching with AG stride during Phase 4 was
missed. The attached patch fixes this.

32 AGs, ag_stride = 4:

Phase 3 - for each AG...
         - scan and clear agi unlinked lists...
         - process known inodes and perform inode discovery...
         - agno = 0
         - agno = 4
         - agno = 8
         - agno = 12
         - agno = 16
         - agno = 20
         - agno = 24
         - agno = 28

which is correct... but in Phase 4:

Phase 4 - check for duplicate blocks...
         - setting up duplicate extent list...
         - check for inodes claiming duplicate blocks...
         - agno = 0
         - agno = 1
         - agno = 2
         - agno = 5
         - agno = 6
         - agno = 4
         - agno = 7
         - agno = 3

[-- Attachment #2: fix_ag_stride --]
[-- Type: application/octet-stream, Size: 1069 bytes --]

Index: repair/xfsprogs/repair/phase4.c
===================================================================
--- repair.orig/xfsprogs/repair/phase4.c	2007-07-24 15:34:39.000000000 +1000
+++ repair/xfsprogs/repair/phase4.c	2007-08-03 12:04:38.201176283 +1000
@@ -137,6 +137,7 @@
 	xfs_mount_t		*mp)
 {
 	int 			i, j;
+	xfs_agnumber_t 		agno;
 	work_queue_t		*queues;
 	prefetch_args_t		*pf_args[2];
 
@@ -153,12 +154,13 @@
 			/*
 			 * create one worker thread for each segment of the volume
 			 */
-			for (i = 0; i < thread_count; i++) {
+			for (i = 0, agno = 0; i < thread_count; i++) {
 				create_work_queue(&queues[i], mp, 1);
 				pf_args[0] = NULL;
-				for (j = i; j < mp->m_sb.sb_agcount; j += ag_stride) {
-					pf_args[0] = start_inode_prefetch(j, 0, pf_args[0]);
-					queue_work(&queues[i], process_ag_func, j, pf_args[0]);
+				for (j = 0; j < ag_stride && agno < mp->m_sb.sb_agcount;
+						j++, agno++) {
+					pf_args[0] = start_inode_prefetch(agno, 0, pf_args[0]);
+					queue_work(&queues[i], process_ag_func, agno, pf_args[0]);
 				}
 			}
 			/*

Re: REVIEW: fix xfs_repair phase 4 ag_stride with prefetch

[Timothy Shimmin]

Barry Naujok wrote:
> 
> AG stride testing was performed on a system with ample amounts
> of memory, so prefetching with AG stride during Phase 4 was
> missed. The attached patch fixes this.
> 
> 32 AGs, ag_stride = 4:
> 
> Phase 3 - for each AG...
>         - scan and clear agi unlinked lists...
>         - process known inodes and perform inode discovery...
>         - agno = 0
>         - agno = 4
>         - agno = 8
>         - agno = 12
>         - agno = 16
>         - agno = 20
>         - agno = 24
>         - agno = 28
> 
> which is correct... but in Phase 4:
> 
> Phase 4 - check for duplicate blocks...
>         - setting up duplicate extent list...
>         - check for inodes claiming duplicate blocks...
>         - agno = 0
>         - agno = 1
>         - agno = 2
>         - agno = 5
>         - agno = 6
>         - agno = 4
>         - agno = 7
>         - agno = 3


Okay I replied to this in the bug but forgot about the review
email request so I will reply here too:)


==========================
ADDITIONAL INFORMATION (ADD)
From: timothy shimmin <tes@sgi.com>
Date: Aug 09 2007 10:59:01PM
[BugWorks mailnews processor v1.7.1]
==========================

bnaujok@sgi.com via BugWorks wrote:
 >
 > [ EXCESSIVE QUOTED TEXT DELETED ]
 >
 >         - 12:00:16: setting up duplicate extent list - 32 of 32 allocation groups done
 >         - check for inodes claiming duplicate blocks...
 >         - agno = 0
 >         - agno = 1
 >         - agno = 2
 >         - agno = 5
 >         - agno = 6
 >         - agno = 4
 >         - agno = 7
 >         - agno = 3

So you need to replicate the logic/code you used in phase 3 and put it
into phase4 I presume.

And the changes are in phase4.c/process_ags() (like in phase3.c/process_ags).

So previously it would iterate thru the thread-count and then
prefetch and queue up to the ag-count stepping by the ag-stride.

And now we go thru each AG but only do an ag-stride worth at a time for
a thread. So each thread will get an ag-stride worth of ags to deal with:

Fixed code:
Thread 0 - gets ag 0-3  (go thru a loop doing a fetch & queue work on ag 0-3)
Thread 1 - gets ag 4-7
Thread 2 - gets ag 8-11
etc...

Okay, starting to get it now :)

So previously (broken code) it would do:
Thread 0 - gets ag 0,4,8,12,16,...
Thread 1 - gets ag 1,5,9,13,17,...
Thread 2 - gets ag 2,6,10,14,18,...

I presume the bad phase 4 msgs above are coming out in a different order
than what I said above
just because of the threading. Or did I make a mistake in my reading
of the code.

Pity we couldn't share some code there between the phases but I haven't
looked to see the difficulties with that are - do tell me.
Would be nice to guarantee handling the striding/threading the same in both phases by
both using the same iterator code - particularly if it needs
to be changed in the future etc. But whatever.

And I presume the fix is so that the prefetching for a thread is done in
ag-order (and they are just given a chunks worth)
instead of the old bad way where the prefetching would be done with a jump
over AGs by the stride instead of in order.

So it looks fine if my understanding is correct.

Cheers,
Tim.