On Thu, Aug 14, 2014 at 01:11:36PM +1000, Dave Chinner wrote: > On Wed, Aug 13, 2014 at 11:42:29AM -0400, Brian Foster wrote: > > On Fri, Aug 08, 2014 at 02:49:26PM -0400, Brian Foster wrote: > > > A file collapse stress test workload reproduces collapse failures > > > mid-operation due to changes in the inode fork extent count across > > > extent shift cycles. xfs_collapse_file_space() currently calls > > > xfs_bmap_shift_extents() to shift one extent at a time per transaction. > > > The extent index is used to track the next extent to shift after each > > > iteration. > > Right, it does so after writing back all the dirty pages and > invalidating the cache. This is done under the IOLOCK_EXCL, so we > should end up with nothing being able to newly dirty the inode while > the collapse range operation is in progress. > > > > > > A concurrent fsx and fsstress workload reproduces a scenario where the > > > extent count changes during this sequence, causing the 'current_ext' > > > index to become inaccurate and possibly skip shifting an extent. The > > > likely result of this behavior is the subsequent shift attempt will not > > > find a hole in the area of the skipped extent and fail, leaving the file > > > in a partially collapsed state. > > > > > > This occurs because the ilock is released and acquired across each > > > transaction and each individual extent shift. Tracepoint output shows > > > that once the ilock is released after an extent shift, a pending > > > blocking writeback (e.g., sync) can acquire the lock and proceed before > > > the next extent is shifted down. If the writeback converts part of a > > > delayed allocation earlier in the file, for example, it can insert a new > > > extent into the map. Tracing confirms a call to > > > xfs_bmap_add_extent_delay_real() in this particular instance. > > Are we getting a dirty extent in the range being collapsed, or does > it exist outside the range being shifted? If outside, then shouldn't > we just sync the entire file first? i.e. treat it the same way as we > do xfs_swap_extents()? > The dirty extent is prior to the range being collapsed, presumably already dirty by the time we acquire the iolock. The free space portion of the collapse currently only syncs from the start of the range to EOF. Syncing the entire file is something I was planning to experiment with, particularly since it seems like the remaining failure I mentioned in the series intro appears to be related to hitting a delalloc extent during the collapse. I don't yet know how that occurs, but my last test dumped enough trace data to show a startblock value that includes the mask used for delalloc extents. The collapse attempts to look up an extent in the bmap btree with these values and falls over when it's not found. > Realistically, cur_extent is not valid once we drop the ILOCK. > Perhaps we should keep the offset of the extent we are up to in the > shift, and then look up the offset to get current extent map index > once we pick the ILOCK back up. That way we avoid issues with other > parts of the extent map changing between ILOCK hold contexts. > I noticed that one or more of the other bmap functions do something like this. Unless there's some other reason that we don't want to drop ilock here, that sounds like a safer approach. Thanks. > > > To prevent this scenario, hold the ilock across the entire extent shift > > > loop in xfs_collapse_file_space(). > > > > > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx> > > > --- > > > fs/xfs/xfs_bmap_util.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > > index 2f1e30d..96eb97b 100644 > > > --- a/fs/xfs/xfs_bmap_util.c > > > +++ b/fs/xfs/xfs_bmap_util.c > > > @@ -1474,6 +1474,8 @@ xfs_collapse_file_space( > > > if (error) > > > return error; > > > > > > + xfs_ilock(ip, XFS_ILOCK_EXCL); > > > + > > > > I realized this moves the lock outside of the xfs_trans_reserve(), thus > > opening a potential deadlock scenario with regard to the log. I suppose > > this might be harder to hit in real life than a sync() causing the > > operation to fall over mid-sequence, so I'm still Ok with keeping this > > unless anybody objects. > > We can't do that. Inode locking rules explicitly forbid it - we've > avoided needing to do break this rule for 20 years, so let's not > start making exceptions now just because it's a "simple fix". > Sounds good, let's drop this one then and I'll revisit it. Are we still Ok with the first patch? I think that one is still warranted since it limits the current flaws of collapse to the collapse operation itself. Brian > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
