On Wed, Dec 18, 2019 at 07:10:33AM -0500, Brian Foster wrote:
> On Tue, Dec 17, 2019 at 06:37:26PM -0800, Darrick J. Wong wrote:
> > On Fri, Dec 13, 2019 at 12:12:57PM -0500, Brian Foster wrote:
> > > The insert range operation uses a unique transaction and ilock cycle
> > > for the extent split and each extent shift iteration of the overall
> > > operation. While this works, it is risks racing with other
> > > operations in subtle ways such as COW writeback modifying an extent
> > > tree in the middle of a shift operation.
> > >
> > > To avoid this problem, make insert range atomic with respect to
> > > ilock. Hold the ilock across the entire operation, replace the
> > > individual transactions with a single rolling transaction sequence
> > > and relog the inode to keep it moving in the log. This guarantees
> > > that nothing else can change the extent mapping of an inode while
> > > an insert range operation is in progress.
> > >
> > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_bmap_util.c | 32 +++++++++++++-------------------
> > > 1 file changed, 13 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > > index 829ab1a804c9..555c8b49a223 100644
> > > --- a/fs/xfs/xfs_bmap_util.c
> > > +++ b/fs/xfs/xfs_bmap_util.c
> > > @@ -1134,47 +1134,41 @@ xfs_insert_file_space(
> > > if (error)
> > > return error;
> > >
> > > - /*
> > > - * The extent shifting code works on extent granularity. So, if stop_fsb
> > > - * is not the starting block of extent, we need to split the extent at
> > > - * stop_fsb.
> > > - */
> > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write,
> > > XFS_DIOSTRAT_SPACE_RES(mp, 0), 0, 0, &tp);
> > > if (error)
> > > return error;
> > >
> > > xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> > > + xfs_trans_ijoin(tp, ip, 0);
> > >
> > > + /*
> > > + * The extent shifting code works on extent granularity. So, if stop_fsb
> > > + * is not the starting block of extent, we need to split the extent at
> > > + * stop_fsb.
> > > + */
> > > error = xfs_bmap_split_extent(tp, ip, stop_fsb);
> > > if (error)
> > > goto out_trans_cancel;
> > >
> > > - error = xfs_trans_commit(tp);
> > > - if (error)
> > > - return error;
> > > -
> > > - while (!error && !done) {
> > > - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0, 0,
> > > - &tp);
> >
> > I'm a little concerned about the livelock potential here, if there are a lot of
> > other threads that have eaten all the transaction reservation and are trying to
> > get our ILOCK, while at the same time this thread has the ILOCK and is trying
> > to roll the transaction to move another extent, having already rolled the
> > transaction more than logcount times.
> >
>
> My understanding is that the regrant mechanism is intended to deal with
> that scenario. Even after the initial (res * logcount) reservation is
> consumed, a regrant is different from an initial reservation in that the
> reservation head is unconditionally updated with a new reservation unit.
> We do wait on the write head in regrant, but IIUC that should be limited
> to the pool of already allocated transactions (and is woken before the
> reserve head waiters IIRC). I suppose something like this might be
> possible in theory if we were blocked on regrant and the entirety of
> remaining log space was consumed by transactions waiting on our ilock,
> but I think that is highly unlikely since we also hold the iolock here.
True. The only time I saw this happen was with buffered COW writeback
completions (which hold lock other than ILOCK), which should have been
fixed by the patch I made to put all the writeback items to a single
inode queue and run /one/ worker thread to process them all. So maybe
my fears are unfounded nowadays. :)
The only other place I can think of that does a lot of transaction
rolling on a single inode is online repair, and it always holds all
three exclusive locks.
> Also note that this approach is based on the current truncate algorithm,
> which is probably a better barometer of potential for this kind of issue
> as it is a less specialized operation. I'd argue that if this is safe
> enough for truncate, it should be safe enough for range shifting.
Hehehe.
> > I think the extent shifting loop starts with the highest offset mapping and
> > shifts it up and continues in order of decreasing offset until it gets to
> > @stop_fsb, correct?
> >
>
> Yep, for insert range at least.
>
> > Can we use "alloc trans; ilock; move; commit" for every extent higher than the
> > one that crosses @stop_fsb, and use "alloc trans; ilock; split; roll;
> > insert_extents; commit" to deal with that one extent that crosses @stop_fsb?
> > tr_write pre-reserves enough space to that the roll won't need to get more,
> > which would eliminate that potential problem, I think.
> >
>
> We'd have to reorder the extent split for that kind of approach, which I
> think you've noted in the sequence above, as the race window is between
> the split and subsequent shift. Otherwise I think that would work.
>
> That said, I'd prefer not to introduce the extra complexity and
> functional variance unless it were absolutely necessary, and it's not
> clear to me that it is. If it is, we'd probably have seen similar issues
> in truncate and should target a fix there before worrying about range
> shift.
Ok. Looking back through lore I don't see any complaints about insert
range, so I guess it's fine.
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> Brian
>
> > --D
> >
> > > + do {
> > > + error = xfs_trans_roll_inode(&tp, ip);
> > > if (error)
> > > - break;
> > > + goto out_trans_cancel;
> > >
> > > - xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> > > error = xfs_bmap_insert_extents(tp, ip, &next_fsb, shift_fsb,
> > > &done, stop_fsb);
> > > if (error)
> > > goto out_trans_cancel;
> > > + } while (!done);
> > >
> > > - error = xfs_trans_commit(tp);
> > > - }
> > > -
> > > + error = xfs_trans_commit(tp);
> > > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > > return error;
> > >
> > > out_trans_cancel:
> > > xfs_trans_cancel(tp);
> > > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > > return error;
> > > }
> > >
> > > --
> > > 2.20.1
> > >
> >
>
