On Tue, Nov 20, 2018 at 05:36:05PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > Long saga. There have been days spent following this through dead end > after dead end in multi-GB event traces. This morning, after writing > a trace-cmd wrapper that enabled me to be more selective about XFS > trace points, I discovered that I could get just enough essential > tracepoints enabled that there was a 50:50 chance the fsx config > would fail at ~115k ops. If it didn't fail at op 115547, I stopped > fsx at op 115548 anyway. > > That gave me two traces - one where the problem manifested, and one > where it didn't. After refining the traces to have the necessary > information, I found that in the failing case there was a real > extent in the COW fork compared to an unwritten extent in the > working case. > > Walking back through the two traces to the point where the CWO fork > extents actually diverged, I found that the bad case had an extra > unwritten extent in it. This is likely because the bug it led me to > had triggered multiple times in those 115k ops, leaving stray > COW extents around. What I saw was a COW delalloc conversion to an > unwritten extent (as they should always be through > xfs_iomap_write_allocate()) resulted in a /written extent/: > > xfs_writepage: dev 259:0 ino 0x83 pgoff 0x17000 size 0x79a00 offset 0 length 0 > xfs_iext_remove: dev 259:0 ino 0x83 state RC|LF|RF|COW cur 0xffff888247b899c0/2 offset 32 block 152 count 20 flag 1 caller xfs_bmap_add_extent_delay_real > xfs_bmap_pre_update: dev 259:0 ino 0x83 state RC|LF|RF|COW cur 0xffff888247b899c0/1 offset 1 block 4503599627239429 count 31 flag 0 caller xfs_bmap_add_extent_delay_real > xfs_bmap_post_update: dev 259:0 ino 0x83 state RC|LF|RF|COW cur 0xffff888247b899c0/1 offset 1 block 121 count 51 flag 0 caller xfs_bmap_add_ex > > Basically, Cow fork before: > > 0 1 32 52 > +H+DDDDDDDDDDDD+UUUUUUUUUUU+ > PREV RIGHT > > COW delalloc conversion allocates: > > 1 32 > +uuuuuuuuuuuu+ > NEW > > And the result according to the xfs_bmap_post_update trace was: > > 0 1 32 52 > +H+wwwwwwwwwwwwwwwwwwwwwwww+ > PREV > > Which is clearly wrong - it should be a merged unwritten extent, > not an unwritten extent. > > That lead me to look at the LEFT_FILLING|RIGHT_FILLING|RIGHT_CONTIG > case in xfs_bmap_add_extent_delay_real(), and sure enough, there's > the bug. > > It takes the old delalloc extent (PREV) and adds the length of the > RIGHT extent to it, takes the start block from NEW, removes the > RIGHT extent and then updates PREV with the new extent. > > What it fails to do is update PREV.br_state. For delalloc, this is > always XFS_EXT_NORM, while in this case we are converting the > delayed allocation to unwritten, so it needs to be updated to > XFS_EXT_UNWRITTEN. This LF|RF|RC case does not do this, and so > the resultant extent is always written. > > And that's the bug I've been chasing for a week - a bmap btree bug, > not a reflink/dedupe/copy_file_range bug, but a BMBT bug introduced > with the recent in core extent tree scalability enhancements. > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> Looks ok, seems to fix the fsx failures I know how to reproduce... :P Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> --D > --- > fs/xfs/libxfs/xfs_bmap.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 2d78fd53e822..39eaa2b86060 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -1694,10 +1694,13 @@ xfs_bmap_add_extent_delay_real( > case BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING | BMAP_RIGHT_CONTIG: > /* > * Filling in all of a previously delayed allocation extent. > - * The right neighbor is contiguous, the left is not. > + * The right neighbor is contiguous, the left is not. Take care > + * with delay -> unwritten extent allocation here because the > + * delalloc record we are overwriting is always written. > */ > PREV.br_startblock = new->br_startblock; > PREV.br_blockcount += RIGHT.br_blockcount; > + PREV.br_state = new->br_state; > > xfs_iext_next(ifp, &bma->icur); > xfs_iext_remove(bma->ip, &bma->icur, state);
