REVIEW: Fix for incore extent corruption.

Reference:
http://oss.sgi.com/archives/xfs/2008-06/msg00209.html

It turns out that the code in question is still broken.

xfs_iext_irec_compact_full will still corrupt the incore extent list if it does the the partial copy of extents from one page to the next.
I haven't quit figured out where things get out of sync but somehow if_real_bytes which tracks the total number of bytes available in the extent buffers and if_bytes (which tracks the total bytes used for extents.

So at some point the inode thinks is has more extents than allocated pages allow. So what happens is xfs_iext_idx_to_irec which uses idxp to pass IN the absolute extent index is suppose to change idxp on the way OUT and erp_idxp to be a buffer index pair. What happens is that it doesn't find the extent so idxp is left holding the same value as was passed in and erp_idx is 0. This causes the extent code to then index way past the end of extent buffer 0 into garbage mem.

with 4k ext buffers max extent count per buffer is 256. example being:
IN:
idxp = 400
should become:
idexp = 144
erp_idxp = 1

but we end up not finding the extent so
we have
idxp = 400
erp_idxp =0

so we now index 6400 bytes into a 4k buffer.

Which often times is a pages of mostly 0 so we end up with access to block 0 errors.

The more I looked at this code the more it didn't make sense to do partial moves.
Since the list of extent buffers is only scanned once vs restarting the list whenever a partial move is done, it is very unlikely to actually free an extent buffer. (granted it's possible but unlikely)

xfs_iext_irec_compact_pages does the same thing as xfs_iext_irec_compact_full except that doesn't handle partial moves.

xfs_iext_irec_compact is written such that ratio of current extents has to be significantly smaller than the current allocated space
xfs_inode: 4513
nextents < ( nlists * XFS_LINEAR_EXT) >> 3

As it turns out 99% of the time it calls xfs_iext_irec_compact_pages (which is why it has been so hard to track this bug down).

If you change the 3 to a 1 so the code alway calls compact_full vs compact_pages the extent list will corrupt amost immediately.

Since the code is broken, almost never used, and provides only micro optimization of incore space I propose we just remove it all together.

I'm also including an xfsidbg patch that for xexlist that prints out buffer offset and index.

-Russell Cattelan

Quoted 49 lines · Russell Cattelan

Reference:
http://oss.sgi.com/archives/xfs/2008-06/msg00209.html

It turns out that the code in question is still broken.

xfs_iext_irec_compact_full will still corrupt the incore extent list if it does the the partial copy of extents from one page to the next.
I haven't quit figured out where things get out of sync but somehow if_real_bytes which tracks the total number of bytes available in the extent buffers and if_bytes (which tracks the total bytes used for extents.

So at some point the inode thinks is has more extents than allocated pages allow. So what happens is xfs_iext_idx_to_irec which uses idxp to pass IN the absolute extent index is suppose to change idxp on the way OUT and erp_idxp to be a buffer index pair. What happens is that it doesn't find the extent so idxp is left holding the same value as was passed in and erp_idx is 0. This causes the extent code to then index way past the end of extent buffer 0 into garbage mem.

with 4k ext buffers max extent count per buffer is 256. example being:
IN:
idxp = 400
should become:
idexp = 144
erp_idxp = 1

but we end up not finding the extent so
we have
idxp = 400
erp_idxp =0

so we now index 6400 bytes into a 4k buffer.

Which often times is a pages of mostly 0 so we end up with access to block 0 errors.

The more I looked at this code the more it didn't make sense to do partial moves.
Since the list of extent buffers is only scanned once vs restarting the list whenever a partial move is done, it is very unlikely to actually free an extent buffer. (granted it's possible but unlikely)

xfs_iext_irec_compact_pages does the same thing as xfs_iext_irec_compact_full except that doesn't handle partial moves.

xfs_iext_irec_compact is written such that ratio of current extents has to be significantly smaller than the current allocated space
xfs_inode: 4513
nextents < ( nlists * XFS_LINEAR_EXT) >> 3

Quoted 5 lines

As it turns out 99% of the time it calls xfs_iext_irec_compact_pages (which is why it has been so hard to track this bug down).

If you change the 3 to a 1 so the code alway calls compact_full vs compact_pages the extent list will corrupt amost immediately.

Awesome work Russell, we'll give it a go.

Quoted 1 line

Quoted 2 lines

Since the code is broken, almost never used, and provides only micro optimization of incore space I propose we just remove it all together.

Are you sure the bug is in xfs_iext_irec_compact_full?

Is it possible that we are still indexing beyond the page when using xfs_iext_irec_compact_pages but the pages just happen to be sequential so the indexing gets the extent anyway?

Quoted 7 lines

I'm also including an xfsidbg patch that for xexlist that prints out buffer offset and index.

-Russell Cattelan

Quoted 58 lines · Lachlan McIlroy

Quoted 49 lines · Russell Cattelan

Reference:
http://oss.sgi.com/archives/xfs/2008-06/msg00209.html

It turns out that the code in question is still broken.

xfs_iext_irec_compact_full will still corrupt the incore extent list if it does the the partial copy of extents from one page to the next.
I haven't quit figured out where things get out of sync but somehow if_real_bytes which tracks the total number of bytes available in the extent buffers and if_bytes (which tracks the total bytes used for extents.

So at some point the inode thinks is has more extents than allocated pages allow. So what happens is xfs_iext_idx_to_irec which uses idxp to pass IN the absolute extent index is suppose to change idxp on the way OUT and erp_idxp to be a buffer index pair. What happens is that it doesn't find the extent so idxp is left holding the same value as was passed in and erp_idx is 0. This causes the extent code to then index way past the end of extent buffer 0 into garbage mem.

with 4k ext buffers max extent count per buffer is 256. example being:
IN:
idxp = 400
should become:
idexp = 144
erp_idxp = 1

but we end up not finding the extent so
we have
idxp = 400
erp_idxp =0

so we now index 6400 bytes into a 4k buffer.

Which often times is a pages of mostly 0 so we end up with access to block 0 errors.

The more I looked at this code the more it didn't make sense to do partial moves.
Since the list of extent buffers is only scanned once vs restarting the list whenever a partial move is done, it is very unlikely to actually free an extent buffer. (granted it's possible but unlikely)

xfs_iext_irec_compact_pages does the same thing as xfs_iext_irec_compact_full except that doesn't handle partial moves.

xfs_iext_irec_compact is written such that ratio of current extents has to be significantly smaller than the current allocated space
xfs_inode: 4513
nextents < ( nlists * XFS_LINEAR_EXT) >> 3

Quoted 5 lines

As it turns out 99% of the time it calls xfs_iext_irec_compact_pages (which is why it has been so hard to track this bug down).

If you change the 3 to a 1 so the code alway calls compact_full vs compact_pages the extent list will corrupt amost immediately.

Awesome work Russell, we'll give it a go.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

Since the code is broken, almost never used, and provides only micro optimization of incore space I propose we just remove it all together.

Are you sure the bug is in xfs_iext_irec_compact_full?

Is it possible that we are still indexing beyond the page when using xfs_iext_irec_compact_pages but the pages just happen to be sequential so the indexing gets the extent anyway?

I added a bunch of printk to track this down, the compact_pages path is hit
a lot in fact as far as I can tell all running file systems that shrink extents and don't crash :-)

I should have done this originally my I'm including the modified makeextents that I used to tickle this problem.
It reserves a bunch of space to create a contiguous extents then in unreserves space to poke a bunch of holes creating a big extent list, it then goes back and writes the whole file hopefully collapsing extents as it goes.

i.e.
makeextents -v -c 512 foo ; xfs_bmap -v foo should give you 1024 extents
makeextents -v -f -c 512 foo ; xfs_bmap -v foo will do the same thing but fill in the file with writes. The number of resulting extents vary, but sometimes you end up with one extent. sometimes more.

If you change the 3 to a 1 in the current code so compact_full is used vs compact_pages and run the test it will hit some problem every time.
xexlist in kdb will show the corruption in the incore list.

This will run the code through all 3 formats so if you are lucky you end up hitting all the cases indirect > 256, direct <= 256, and inline <= 2

note: xfs_iext_indirect_to_direct does call compact_full but in that case we are already down to under 256 extents (at least we should be ) and at that point compact_full will behave just like compact_pages.

Quoted 8 lines

Quoted 7 lines

I'm also including an xfsidbg patch that for xexlist that prints out buffer offset and index.

-Russell Cattelan

Quoted 58 lines · Lachlan McIlroy

Quoted 49 lines · Russell Cattelan

Reference:
http://oss.sgi.com/archives/xfs/2008-06/msg00209.html

It turns out that the code in question is still broken.

xfs_iext_irec_compact_full will still corrupt the incore extent list if it does the the partial copy of extents from one page to the next.
I haven't quit figured out where things get out of sync but somehow if_real_bytes which tracks the total number of bytes available in the extent buffers and if_bytes (which tracks the total bytes used for extents.

So at some point the inode thinks is has more extents than allocated pages allow. So what happens is xfs_iext_idx_to_irec which uses idxp to pass IN the absolute extent index is suppose to change idxp on the way OUT and erp_idxp to be a buffer index pair. What happens is that it doesn't find the extent so idxp is left holding the same value as was passed in and erp_idx is 0. This causes the extent code to then index way past the end of extent buffer 0 into garbage mem.

with 4k ext buffers max extent count per buffer is 256. example being:
IN:
idxp = 400
should become:
idexp = 144
erp_idxp = 1

but we end up not finding the extent so
we have
idxp = 400
erp_idxp =0

so we now index 6400 bytes into a 4k buffer.

Which often times is a pages of mostly 0 so we end up with access to block 0 errors.

The more I looked at this code the more it didn't make sense to do partial moves.
Since the list of extent buffers is only scanned once vs restarting the list whenever a partial move is done, it is very unlikely to actually free an extent buffer. (granted it's possible but unlikely)

xfs_iext_irec_compact_pages does the same thing as xfs_iext_irec_compact_full except that doesn't handle partial moves.

xfs_iext_irec_compact is written such that ratio of current extents has to be significantly smaller than the current allocated space
xfs_inode: 4513
nextents < ( nlists * XFS_LINEAR_EXT) >> 3

Quoted 5 lines

As it turns out 99% of the time it calls xfs_iext_irec_compact_pages (which is why it has been so hard to track this bug down).

If you change the 3 to a 1 so the code alway calls compact_full vs compact_pages the extent list will corrupt amost immediately.

Awesome work Russell, we'll give it a go.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

Since the code is broken, almost never used, and provides only micro optimization of incore space I propose we just remove it all together.

Are you sure the bug is in xfs_iext_irec_compact_full?

Is it possible that we are still indexing beyond the page when using xfs_iext_irec_compact_pages but the pages just happen to be sequential so the indexing gets the extent anyway?

I added a bunch of printk to track this down, the compact_pages path is hit
a lot in fact as far as I can tell all running file systems that shrink extents and don't crash :-)

I should have done this originally my I'm including the modified makeextents that I used to tickle this problem.
It reserves a bunch of space to create a contiguous extents then in unreserves space to poke a bunch of holes creating a big extent list, it then goes back and writes the whole file hopefully collapsing extents as it goes.

i.e.
makeextents -v -c 512 foo ; xfs_bmap -v foo should give you 1024 extents
makeextents -v -f -c 512 foo ; xfs_bmap -v foo will do the same thing but fill in the file with writes. The number of resulting extents vary, but sometimes you end up with one extent. sometimes more.

If you change the 3 to a 1 in the current code so compact_full is used vs compact_pages and run the test it will hit some problem every time.
xexlist in kdb will show the corruption in the incore list.

This will run the code through all 3 formats so if you are lucky you end up hitting all the cases indirect > 256, direct <= 256, and inline <= 2

note: xfs_iext_indirect_to_direct does call compact_full but in that case we are already down to under 256 extents (at least we should be ) and at that point compact_full will behave just like compact_pages.

Quoted 8 lines

Quoted 7 lines

I'm also including an xfsidbg patch that for xexlist that prints out buffer offset and index.

-Russell Cattelan

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

Quoted 5 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c xfs_iext_irec_compact_full( · line 4628

Quoted 9 lines

fs/xfs/xfs_inode.c +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
 					(XFS_LINEAR_EXTS -
 						erp_next->er_extcount) *
 					sizeof(xfs_bmbt_rec_t));
+				erp_next->er_extoff += ext_diff;
 			}
 		}

Lachlan, I concur. I spent way too long last night looking at this and arrived at the same conclusion about the root cause of the problem, but didn't hae *quite* the right solution. I blame it on 2am ;) Your fix looks right.

From some tracing when I hit this path:

Quoted 6 lines · Eric Sandeen

Quoted 5 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

Quoted 13 lines

fs/xfs/xfs_inode.c xfs_iext_irec_compact_full( · line 4628

Quoted 9 lines

fs/xfs/xfs_inode.c +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
 					(XFS_LINEAR_EXTS -
 						erp_next->er_extcount) *
 					sizeof(xfs_bmbt_rec_t));
+				erp_next->er_extoff += ext_diff;
 			}
 		}

Lachlan, I concur. I spent way too long last night looking at this and arrived at the same conclusion about the root cause of the problem, but didn't hae *quite* the right solution. I blame it on 2am ;) Your fix looks right.

FWIW; some supporting information from debugging etc.

xfs_iext_irec_compact_full:

Move 1 item from BUF2 into BUF1, and compact BUF2

                         copy                memmove/zero
         BUF1    BUF2    --->   BUF1     BUF2    --->    BUF1    BUF2
        +-----+ +-----+         +-----+ +-----+         +-----+ +-----+
        |  0  | |  3  |         |  0  | |     |         |  0  | |  4  |
        +-----+ +-----+         +-----+ +-----+         +-----+ +-----+
        |  1  | |  4  |         |  1  | |  4  |         |  1  | |     |
        +-----+ +-----+         +-----+ +-----+         +-----+ +-----+
        |  2  | |     |         |  2  | |     |         |  2  | |     |
        +-----+ +-----+         +-----+ +-----+         +-----+ +-----+
        |     | |     |         |  3  | |     |         |  3  | |     |
        +-----+ +-----+         +-----+ +-----+         +-----+ +-----+
er_count   3       2                                       4       1
er_offset  0       3                                       0       4

If we don't update er_offset properly in BUF2, then a lookup for extent index 3 may find the first one in BUF2, not the last one in BUF1 (both claim to be "extent index 3"

Quoted 1 line

From some tracing when I hit this path:

...
250: ffff810065c61fa0 startoff 251 startblock 263 blockcount 1 flag 1 251: ffff810065c61fb0 startoff 252 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
252: ffff810065c61fc0 startoff 253 startblock 265 blockcount 1 flag 1 253: ffff810065c61fd0 startoff 254 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
254: ffff810065c90000 startoff 255 startblock 267 blockcount 1 flag 1 255: ffff810065c90010 startoff 256 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
256: ffff810065c90020 startoff 257 startblock 269 blockcount 1 flag 1 257: ffff810065c90030 startoff 258 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
258: ffff810065c90040 startoff 259 startblock 271 blockcount 1 flag 1 259: ffff810065c90050 startoff 260 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
...

move enough to fill the previous page:
copy 2 (32) from ffff810065c90000 to ffff810065c61fe0

next page is not empty, so shift up:

move 254 (4064) from ffff810065c90020 to ffff810065c90000

But then I ran through the entire extent list for all indexes in order, and:

250: ffff810065c61fa0 startoff 251 startblock 263 blockcount 1 flag 1 251: ffff810065c61fb0 startoff 252 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
252: ffff810065c61fc0 startoff 253 startblock 265 blockcount 1 flag 1 253: ffff810065c61fd0 startoff 254 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
--- XXX where are starting offsets 255, 256 XXX ---
254: ffff810065c90000 startoff 257 startblock 269 blockcount 1 flag 1 255: ffff810065c90010 startoff 258 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
256: ffff810065c90020 startoff 259 startblock 271 blockcount 1 flag 1

starting *offsets* 255, 256 are lost because the next buffer was still claiming to start at extent index 254 so it essentially jumped there, missing the 2 extents we added to the previous buffer.

in addition, since the er_startoff for this last buffer was wrong, so was the last extent record - off by one, and looked at uninit'd memory:

507: ffff810065c90fd0 startoff 510 startblock NULLSTARTBLOCK(5) blockcount 1 flag 0
508: ffff810065c92fc0 startoff 483406127300608 startblock 2014118168 blockcount 196608 flag 0
wtf, ext 509 out of order (1888313573376 < 483406127300608)?

hope that's more useful than confusing :)

Anyway I really looked closely at this and I think Lachlan is spot-on.

I might even suggest proposing this and the previous fix for -stable....

-Eric

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c xfs_iext_irec_compact_full( · line 4628

Quoted 6 lines

fs/xfs/xfs_inode.c +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
 					(XFS_LINEAR_EXTS -
 						erp_next->er_extcount) *
 					sizeof(xfs_bmbt_rec_t));
+				erp_next->er_extoff += ext_diff;
 			}
 		}

Lachlan, I concur. I spent way too long last night looking at this and arrived at the same conclusion about the root cause of the problem, but didn't hae *quite* the right solution. I blame it on 2am ;) Your fix looks right.

(though I'd probably move the erp_next changes into the else clause? Otherwise you're changing it then freeing it.)

Quoted 20 lines · Eric Sandeen

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c xfs_iext_irec_compact_full( · line 4628

Quoted 6 lines

fs/xfs/xfs_inode.c +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
 					(XFS_LINEAR_EXTS -
 						erp_next->er_extcount) *
 					sizeof(xfs_bmbt_rec_t));
+				erp_next->er_extoff += ext_diff;
 			}
 		}

Lachlan, I concur. I spent way too long last night looking at this and arrived at the same conclusion about the root cause of the problem, but didn't hae *quite* the right solution. I blame it on 2am ;) Your fix looks right.

Quoted 2 lines

(though I'd probably move the erp_next changes into the else clause? Otherwise you're changing it then freeing it.)

I don't understand what you mean by that. Could you elaborate?

Quoted 21 lines · Lachlan McIlroy

Quoted 20 lines · Eric Sandeen

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c xfs_iext_irec_compact_full( · line 4628

Quoted 6 lines

fs/xfs/xfs_inode.c +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
 					(XFS_LINEAR_EXTS -
 						erp_next->er_extcount) *
 					sizeof(xfs_bmbt_rec_t));
+				erp_next->er_extoff += ext_diff;
 			}
 		}

Lachlan, I concur. I spent way too long last night looking at this and arrived at the same conclusion about the root cause of the problem, but didn't hae *quite* the right solution. I blame it on 2am ;) Your fix looks right.

Quoted 2 lines

Quoted 1 line

(though I'd probably move the erp_next changes into the else clause? Otherwise you're changing it then freeing it.)

I don't understand what you mean by that. Could you elaborate?

Sorry I mis-read where the above hunk went... that makes sense as-is above.

For clarity having the erp_next->er_extoff and er_extcount adjustments together *might* make sense but no big deal.

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

Quoted 103 lines · Russell Cattelan

Quoted 58 lines · Lachlan McIlroy

Quoted 49 lines · Russell Cattelan

Reference:
http://oss.sgi.com/archives/xfs/2008-06/msg00209.html

It turns out that the code in question is still broken.

xfs_iext_irec_compact_full will still corrupt the incore extent list if it does the the partial copy of extents from one page to the next.
I haven't quit figured out where things get out of sync but somehow if_real_bytes which tracks the total number of bytes available in the extent buffers and if_bytes (which tracks the total bytes used for extents.

So at some point the inode thinks is has more extents than allocated pages allow. So what happens is xfs_iext_idx_to_irec which uses idxp to pass IN the absolute extent index is suppose to change idxp on the way OUT and erp_idxp to be a buffer index pair. What happens is that it doesn't find the extent so idxp is left holding the same value as was passed in and erp_idx is 0. This causes the extent code to then index way past the end of extent buffer 0 into garbage mem.

with 4k ext buffers max extent count per buffer is 256. example being:
IN:
idxp = 400
should become:
idexp = 144
erp_idxp = 1

but we end up not finding the extent so
we have
idxp = 400
erp_idxp =0

so we now index 6400 bytes into a 4k buffer.

Which often times is a pages of mostly 0 so we end up with access to block 0 errors.

The more I looked at this code the more it didn't make sense to do partial moves.
Since the list of extent buffers is only scanned once vs restarting the list whenever a partial move is done, it is very unlikely to actually free an extent buffer. (granted it's possible but unlikely)

xfs_iext_irec_compact_pages does the same thing as xfs_iext_irec_compact_full except that doesn't handle partial moves.

xfs_iext_irec_compact is written such that ratio of current extents has to be significantly smaller than the current allocated space
xfs_inode: 4513
nextents < ( nlists * XFS_LINEAR_EXT) >> 3

Quoted 5 lines

As it turns out 99% of the time it calls xfs_iext_irec_compact_pages (which is why it has been so hard to track this bug down).

If you change the 3 to a 1 so the code alway calls compact_full vs compact_pages the extent list will corrupt amost immediately.

Awesome work Russell, we'll give it a go.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

Since the code is broken, almost never used, and provides only micro optimization of incore space I propose we just remove it all together.

Are you sure the bug is in xfs_iext_irec_compact_full?

Is it possible that we are still indexing beyond the page when using xfs_iext_irec_compact_pages but the pages just happen to be sequential so the indexing gets the extent anyway?

I added a bunch of printk to track this down, the compact_pages path is hit
a lot in fact as far as I can tell all running file systems that shrink extents and don't crash :-)

I should have done this originally my I'm including the modified makeextents that I used to tickle this problem.
It reserves a bunch of space to create a contiguous extents then in unreserves space to poke a bunch of holes creating a big extent list, it then goes back and writes the whole file hopefully collapsing extents as it goes.

i.e.
makeextents -v -c 512 foo ; xfs_bmap -v foo should give you 1024 extents
makeextents -v -f -c 512 foo ; xfs_bmap -v foo will do the same thing but fill in the file with writes. The number of resulting extents vary, but sometimes you end up with one extent. sometimes more.

If you change the 3 to a 1 in the current code so compact_full is used vs compact_pages and run the test it will hit some problem every time.
xexlist in kdb will show the corruption in the incore list.

This will run the code through all 3 formats so if you are lucky you end up hitting all the cases indirect > 256, direct <= 256, and inline <= 2

note: xfs_iext_indirect_to_direct does call compact_full but in that case we are already down to under 256 extents (at least we should be ) and at that point compact_full will behave just like compact_pages.

Quoted 8 lines

Quoted 7 lines

I'm also including an xfsidbg patch that for xexlist that prints out buffer offset and index.

-Russell Cattelan

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 xfs_iext_irec_compact_full( · line 4629

Quoted 7 lines

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 renamed from fs/xfs/xfs_inode.c 2008-09-18 18:48:46.000000000 +1000 +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
                    (XFS_LINEAR_EXTS -
                        erp_next->er_extcount) *
                    sizeof(xfs_bmbt_rec_t));
+                erp_next->er_extoff += ext_diff;
            }
        }
 

Cool I'll give it some run through when I done traveling.

I still think compact_full should simply be eliminated since it really doesn't help, and it's obviously confusing code. Or we should make sure it works and get rid of compact_pages since compact_full behaves just like compact_pages when not doing partial moves.

Quoted 18 lines · Russell Cattelan

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 xfs_iext_irec_compact_full( · line 4629

Quoted 7 lines

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 renamed from fs/xfs/xfs_inode.c 2008-09-18 18:48:46.000000000 +1000 +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
                    (XFS_LINEAR_EXTS -
                        erp_next->er_extcount) *
                    sizeof(xfs_bmbt_rec_t));
+                erp_next->er_extoff += ext_diff;
            }
        }
 

Cool I'll give it some run through when I done traveling.

Quoted 5 lines

I still think compact_full should simply be eliminated since it really doesn't help, and it's obviously confusing code. Or we should make sure it works and get rid of compact_pages since compact_full behaves just like compact_pages when not doing partial moves.

I'd agree with that, at least as far as reevaluating this packing stuff - given the seriousness of the bug when you do hit it, and how rarely it's ever hit, apparently this chunk of code is almost never run ....

Quoted 25 lines · Eric Sandeen

Quoted 18 lines · Russell Cattelan

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 xfs_iext_irec_compact_full( · line 4629

Quoted 7 lines

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 renamed from fs/xfs/xfs_inode.c 2008-09-18 18:48:46.000000000 +1000 +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
                    (XFS_LINEAR_EXTS -
                        erp_next->er_extcount) *
                    sizeof(xfs_bmbt_rec_t));
+                erp_next->er_extoff += ext_diff;
            }
        }
 

Cool I'll give it some run through when I done traveling.

Quoted 5 lines

I still think compact_full should simply be eliminated since it really doesn't help, and it's obviously confusing code. Or we should make sure it works and get rid of compact_pages since compact_full behaves just like compact_pages when not doing partial moves.

Quoted 4 lines

I'd agree with that, at least as far as reevaluating this packing stuff - given the seriousness of the bug when you do hit it, and how rarely it's ever hit, apparently this chunk of code is almost never run ....

I agree too. If any code is difficult to reach it's also difficult to test. We've had numerous reports of extent corruption that could be explained by this bug but we have not been able to reproduce the symptoms let alone devise a reliable test case.

What real benefit does compact_full have over compact_pages? Are there corner cases where compact_pages is not good enough?

Here's a patch to remove xfs_iext_irec_compact_full() like Russell did in his original patch - are you guys happy with this?

I'm putting it through it's paces now and so far it looks good.

fs/xfs/xfs_inode.c +1 -1

@@ -4157,7 +4166,7 @@ xfs_iext_indirect_to_direct(
 	ASSERT(nextents <= XFS_LINEAR_EXTS);
 	size = nextents * sizeof(xfs_bmbt_rec_t);
 
-	xfs_iext_irec_compact_full(ifp);
+	xfs_iext_irec_compact_pages(ifp);
 	ASSERT(ifp->if_real_bytes == XFS_IEXT_BUFSZ);
 
 	ep = ifp->if_u1.if_ext_irec->er_extbuf;

Quoted 3 lines · Lachlan McIlroy

Here's a patch to remove xfs_iext_irec_compact_full() like Russell did in his original patch - are you guys happy with this?

Quoted 2 lines

I'm putting it through it's paces now and so far it looks good.

I'll have to think more about it, honestly. Probably fine, but I've not looked at all the surrounding code, I was so far just looking for the original bug.

(FWIW, compact_full *does* get called reasonably frequently, but the memmove case is what's hard to hit...)

-Eric

Quoted 12 lines

fs/xfs/xfs_inode.c +1 -1

@@ -4157,7 +4166,7 @@ xfs_iext_indirect_to_direct(
 	ASSERT(nextents <= XFS_LINEAR_EXTS);
 	size = nextents * sizeof(xfs_bmbt_rec_t);
 
-	xfs_iext_irec_compact_full(ifp);
+	xfs_iext_irec_compact_pages(ifp);
 	ASSERT(ifp->if_real_bytes == XFS_IEXT_BUFSZ);
 
 	ep = ifp->if_u1.if_ext_irec->er_extbuf;

...

Quoted 3 lines · Lachlan McIlroy

Here's a patch to remove xfs_iext_irec_compact_full() like Russell did in his original patch - are you guys happy with this?

Quoted 1 line

I'm putting it through it's paces now and so far it looks good.

I'll have to think more about it, honestly. Probably fine, but I've not looked at all the surrounding code, I was so far just looking for the original bug.

(FWIW, compact_full *does* get called reasonably frequently, but the memmove case is what's hard to hit...)

-Eric

Quoted 11 lines

fs/xfs/xfs_inode.c +1 -1

@@ -4157,7 +4166,7 @@ xfs_iext_indirect_to_direct(
 	ASSERT(nextents <= XFS_LINEAR_EXTS);
 	size = nextents * sizeof(xfs_bmbt_rec_t);
 
-	xfs_iext_irec_compact_full(ifp);
+	xfs_iext_irec_compact_pages(ifp);
 	ASSERT(ifp->if_real_bytes == XFS_IEXT_BUFSZ);
 
 	ep = ifp->if_u1.if_ext_irec->er_extbuf;

...

Here's a patch to remove xfs_iext_irec_compact_full() like Russell did in his original patch - are you guys happy with this?

I'm putting it through it's paces now and so far it looks good.

fs/xfs/xfs_inode.c 2008-09-19 13:16:34.000000000 +1000 renamed from fs/xfs/xfs_inode.c 2008-09-19 13:08:08.000000000 +1000 +1 -88

@@ -4157,7 +4166,7 @@ xfs_iext_indirect_to_direct(
    ASSERT(nextents <= XFS_LINEAR_EXTS);
    size = nextents * sizeof(xfs_bmbt_rec_t);
 
-    xfs_iext_irec_compact_full(ifp);
+    xfs_iext_irec_compact_pages(ifp);
    ASSERT(ifp->if_real_bytes == XFS_IEXT_BUFSZ);
 
    ep = ifp->if_u1.if_ext_irec->er_extbuf;

@@ -4510,8 +4519,6 @@ xfs_iext_irec_compact(
        xfs_iext_direct_to_inline(ifp, nextents);
    } else if (nextents <= XFS_LINEAR_EXTS) {
        xfs_iext_indirect_to_direct(ifp);
-    } else if (nextents < (nlists * XFS_LINEAR_EXTS) >> 3) {
-        xfs_iext_irec_compact_full(ifp);
    } else if (nextents < (nlists * XFS_LINEAR_EXTS) >> 1) {
        xfs_iext_irec_compact_pages(ifp);
    }

@@ -4555,91 +4562,6 @@ xfs_iext_irec_compact_pages(
 }
 
 /*
- * Fully compact the extent records managed by the indirection array.
- */
-void
-xfs_iext_irec_compact_full(
-    xfs_ifork_t    *ifp)            /* inode fork pointer */
-{
-    xfs_bmbt_rec_host_t *ep, *ep_next;    /* extent record pointers */
-    xfs_ext_irec_t    *erp, *erp_next;    /* extent irec pointers */
-    int        erp_idx = 0;        /* extent irec index */
-    int        ext_avail;        /* empty entries in ex list */
-    int        ext_diff;        /* number of exts to add */
-    int        nlists;            /* number of irec's (ex lists) */
-
-    ASSERT(ifp->if_flags & XFS_IFEXTIREC);
-
-    nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
-    erp = ifp->if_u1.if_ext_irec;
-    ep = &erp->er_extbuf[erp->er_extcount];
-    erp_next = erp + 1;
-    ep_next = erp_next->er_extbuf;
-
-    while (erp_idx < nlists - 1) {
-        /*
-         * Check how many extent records are available in this irec.
-         * If there is none skip the whole exercise.
-         */
-        ext_avail = XFS_LINEAR_EXTS - erp->er_extcount;
-        if (ext_avail) {
-
-            /*
-             * Copy over as many as possible extent records into
-             * the previous page.
-             */
-            ext_diff = MIN(ext_avail, erp_next->er_extcount);
-            memcpy(ep, ep_next, ext_diff * sizeof(xfs_bmbt_rec_t));
-            erp->er_extcount += ext_diff;
-            erp_next->er_extcount -= ext_diff;
-
-            /*
-             * If the next irec is empty now we can simply
-             * remove it.
-             */
-            if (erp_next->er_extcount == 0) {
-                /*
-                 * Free page before removing extent record
-                 * so er_extoffs don't get modified in
-                 * xfs_iext_irec_remove.
-                 */
-                kmem_free(erp_next->er_extbuf);
-                erp_next->er_extbuf = NULL;
-                xfs_iext_irec_remove(ifp, erp_idx + 1);
-                erp = &ifp->if_u1.if_ext_irec[erp_idx];
-                nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
-
-            /*
-             * If the next irec is not empty move up the content
-             * that has not been copied to the previous page to
-             * the beggining of this one.
-             */
-            } else {
-                memmove(erp_next->er_extbuf, &ep_next[ext_diff],
-                    erp_next->er_extcount *
-                    sizeof(xfs_bmbt_rec_t));
-                ep_next = erp_next->er_extbuf;
-                memset(&ep_next[erp_next->er_extcount], 0,
-                    (XFS_LINEAR_EXTS -
-                        erp_next->er_extcount) *
-                    sizeof(xfs_bmbt_rec_t));
-            }
-        }
-
-        if (erp->er_extcount == XFS_LINEAR_EXTS) {
-            erp_idx++;
-            if (erp_idx < nlists)
-                erp = &ifp->if_u1.if_ext_irec[erp_idx];
-            else
-                break;
-        }
-        ep = &erp->er_extbuf[erp->er_extcount];
-        erp_next = erp + 1;
-        ep_next = erp_next->er_extbuf;
-    }
-}
-
-/*
 * This is called to update the er_extoff field in the indirection
 * array when extents have been added or removed from one of the
 * extent lists. erp_idx contains the irec index to begin updating

Quoted 42 lines · Lachlan McIlroy

Quoted 25 lines · Eric Sandeen

Quoted 18 lines · Russell Cattelan

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 xfs_iext_irec_compact_full( · line 4629

Quoted 7 lines

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 renamed from fs/xfs/xfs_inode.c 2008-09-18 18:48:46.000000000 +1000 +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
                    (XFS_LINEAR_EXTS -
                        erp_next->er_extcount) *
                    sizeof(xfs_bmbt_rec_t));
+                erp_next->er_extoff += ext_diff;
            }
        }
 

Cool I'll give it some run through when I done traveling.

Quoted 5 lines

I still think compact_full should simply be eliminated since it really doesn't help, and it's obviously confusing code. Or we should make sure it works and get rid of compact_pages since compact_full behaves just like compact_pages when not doing partial moves.

Quoted 4 lines

I'd agree with that, at least as far as reevaluating this packing stuff - given the seriousness of the bug when you do hit it, and how rarely it's ever hit, apparently this chunk of code is almost never run ....

I agree too. If any code is difficult to reach it's also difficult to test. We've had numerous reports of extent corruption that could be explained by this bug but we have not been able to reproduce the symptoms let alone devise a reliable test case.

What real benefit does compact_full have over compact_pages? Are there corner cases where compact_pages is not good enough?

Quoted 4 lines · Lachlan McIlroy

Here's a patch to remove xfs_iext_irec_compact_full() like Russell did in his original patch - are you guys happy with this?

I'm putting it through it's paces now and so far it looks good.

I guess looking at my original patch it would have made sense to drop the call to xfs_iext_irec_irec_compact_pages/full in xfs_iext_indirect_to_direct or do what you did and keep the if else logic.

So ya this look good.

Quoted 159 lines

fs/xfs/xfs_inode.c 2008-09-19 13:16:34.000000000 +1000 renamed from fs/xfs/xfs_inode.c 2008-09-19 13:08:08.000000000 +1000 +1 -88

@@ -4157,7 +4166,7 @@ xfs_iext_indirect_to_direct(
    ASSERT(nextents <= XFS_LINEAR_EXTS);
    size = nextents * sizeof(xfs_bmbt_rec_t);
 
-    xfs_iext_irec_compact_full(ifp);
+    xfs_iext_irec_compact_pages(ifp);
    ASSERT(ifp->if_real_bytes == XFS_IEXT_BUFSZ);
 
    ep = ifp->if_u1.if_ext_irec->er_extbuf;

@@ -4510,8 +4519,6 @@ xfs_iext_irec_compact(
        xfs_iext_direct_to_inline(ifp, nextents);
    } else if (nextents <= XFS_LINEAR_EXTS) {
        xfs_iext_indirect_to_direct(ifp);
-    } else if (nextents < (nlists * XFS_LINEAR_EXTS) >> 3) {
-        xfs_iext_irec_compact_full(ifp);
    } else if (nextents < (nlists * XFS_LINEAR_EXTS) >> 1) {
        xfs_iext_irec_compact_pages(ifp);
    }

@@ -4555,91 +4562,6 @@ xfs_iext_irec_compact_pages(
 }
 
 /*
- * Fully compact the extent records managed by the indirection array.
- */
-void
-xfs_iext_irec_compact_full(
-    xfs_ifork_t    *ifp)            /* inode fork pointer */
-{
-    xfs_bmbt_rec_host_t *ep, *ep_next;    /* extent record pointers */
-    xfs_ext_irec_t    *erp, *erp_next;    /* extent irec pointers */
-    int        erp_idx = 0;        /* extent irec index */
-    int        ext_avail;        /* empty entries in ex list */
-    int        ext_diff;        /* number of exts to add */
-    int        nlists;            /* number of irec's (ex lists) */
-
-    ASSERT(ifp->if_flags & XFS_IFEXTIREC);
-
-    nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
-    erp = ifp->if_u1.if_ext_irec;
-    ep = &erp->er_extbuf[erp->er_extcount];
-    erp_next = erp + 1;
-    ep_next = erp_next->er_extbuf;
-
-    while (erp_idx < nlists - 1) {
-        /*
-         * Check how many extent records are available in this irec.
-         * If there is none skip the whole exercise.
-         */
-        ext_avail = XFS_LINEAR_EXTS - erp->er_extcount;
-        if (ext_avail) {
-
-            /*
-             * Copy over as many as possible extent records into
-             * the previous page.
-             */
-            ext_diff = MIN(ext_avail, erp_next->er_extcount);
-            memcpy(ep, ep_next, ext_diff * sizeof(xfs_bmbt_rec_t));
-            erp->er_extcount += ext_diff;
-            erp_next->er_extcount -= ext_diff;
-
-            /*
-             * If the next irec is empty now we can simply
-             * remove it.
-             */
-            if (erp_next->er_extcount == 0) {
-                /*
-                 * Free page before removing extent record
-                 * so er_extoffs don't get modified in
-                 * xfs_iext_irec_remove.
-                 */
-                kmem_free(erp_next->er_extbuf);
-                erp_next->er_extbuf = NULL;
-                xfs_iext_irec_remove(ifp, erp_idx + 1);
-                erp = &ifp->if_u1.if_ext_irec[erp_idx];
-                nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
-
-            /*
-             * If the next irec is not empty move up the content
-             * that has not been copied to the previous page to
-             * the beggining of this one.
-             */
-            } else {
-                memmove(erp_next->er_extbuf, &ep_next[ext_diff],
-                    erp_next->er_extcount *
-                    sizeof(xfs_bmbt_rec_t));
-                ep_next = erp_next->er_extbuf;
-                memset(&ep_next[erp_next->er_extcount], 0,
-                    (XFS_LINEAR_EXTS -
-                        erp_next->er_extcount) *
-                    sizeof(xfs_bmbt_rec_t));
-            }
-        }
-
-        if (erp->er_extcount == XFS_LINEAR_EXTS) {
-            erp_idx++;
-            if (erp_idx < nlists)
-                erp = &ifp->if_u1.if_ext_irec[erp_idx];
-            else
-                break;
-        }
-        ep = &erp->er_extbuf[erp->er_extcount];
-        erp_next = erp + 1;
-        ep_next = erp_next->er_extbuf;
-    }
-}
-
-/*
 * This is called to update the er_extoff field in the indirection
 * array when extents have been added or removed from one of the
 * extent lists. erp_idx contains the irec index to begin updating

Quoted 42 lines · Lachlan McIlroy

Quoted 25 lines · Eric Sandeen

Quoted 18 lines · Russell Cattelan

Quoted 8 lines · Lachlan McIlroy

Russell, this fixes xfs_iext_irec_compact_full(). If we don't move all the records from the next page into the current page then we need to update the er_extoff of the modified page as we move the remaining extents up. Would you mind giving it a go?

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 xfs_iext_irec_compact_full( · line 4629

Quoted 7 lines

fs/xfs/xfs_inode.c 2008-09-18 18:57:18.000000000 +1000 renamed from fs/xfs/xfs_inode.c 2008-09-18 18:48:46.000000000 +1000 +1 -0

@@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
                    (XFS_LINEAR_EXTS -
                        erp_next->er_extcount) *
                    sizeof(xfs_bmbt_rec_t));
+                erp_next->er_extoff += ext_diff;
            }
        }
 

Cool I'll give it some run through when I done traveling.

Quoted 5 lines

I still think compact_full should simply be eliminated since it really doesn't help, and it's obviously confusing code. Or we should make sure it works and get rid of compact_pages since compact_full behaves just like compact_pages when not doing partial moves.

Quoted 4 lines

I'd agree with that, at least as far as reevaluating this packing stuff - given the seriousness of the bug when you do hit it, and how rarely it's ever hit, apparently this chunk of code is almost never run ....

I agree too. If any code is difficult to reach it's also difficult to test. We've had numerous reports of extent corruption that could be explained by this bug but we have not been able to reproduce the symptoms let alone devise a reliable test case.

What real benefit does compact_full have over compact_pages? Are there corner cases where compact_pages is not good enough?