Re: [PATCH 2/3] xfs: rework insert range into an atomic operation

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@redhat.com>
> > > ---
> > >  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@oracle.com>

--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
> > > 
> > 
>