Re: [PATCH 40/63] xfs: store in-progress CoW allocations in the refcount btree

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

On Fri, Oct 07, 2016 at 02:04:05PM -0400, Brian Foster wrote:
> On Thu, Sep 29, 2016 at 08:09:59PM -0700, Darrick J. Wong wrote:
> > Due to the way the CoW algorithm in XFS works, there's an interval
> > during which blocks allocated to handle a CoW can be lost -- if the FS
> > goes down after the blocks are allocated but before the block
> > remapping takes place.  This is exacerbated by the cowextsz hint --
> > allocated reservations can sit around for a while, waiting to get
> > used.
> > 
> > Since the refcount btree doesn't normally store records with refcount
> > of 1, we can use it to record these in-progress extents.  In-progress
> > blocks cannot be shared because they're not user-visible, so there
> > shouldn't be any conflicts with other programs.  This is a better
> > solution than holding EFIs during writeback because (a) EFIs can't be
> > relogged currently, (b) even if they could, EFIs are bound by
> > available log space, which puts an unnecessary upper bound on how much
> > CoW we can have in flight, and (c) we already have a mechanism to
> > track blocks.
> > 
> > At mount time, read the refcount records and free anything we find
> > with a refcount of 1 because those were in-progress when the FS went
> > down.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: Use the deferred operations system to avoid deadlocks and blowing
> > out the transaction reservation.  This allows us to unmap a CoW
> > extent from the refcountbt and into a file atomically.
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c     |   11 +
> >  fs/xfs/libxfs/xfs_format.h   |    3 
> >  fs/xfs/libxfs/xfs_refcount.c |  336 +++++++++++++++++++++++++++++++++++++++++-
> >  fs/xfs/libxfs/xfs_refcount.h |   10 +
> >  fs/xfs/xfs_mount.c           |   12 ++
> >  fs/xfs/xfs_refcount_item.c   |   12 ++
> >  fs/xfs/xfs_reflink.c         |  150 +++++++++++++++++++
> >  fs/xfs/xfs_reflink.h         |    1 
> >  fs/xfs/xfs_super.c           |    9 +
> >  fs/xfs/xfs_trace.h           |    4 +
> >  10 files changed, 537 insertions(+), 11 deletions(-)
> > 
> > 
> ...
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index c95cdc3..673ecc1 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> ...
> > @@ -772,3 +787,138 @@ xfs_reflink_end_cow(
> >  	trace_xfs_reflink_end_cow_error(ip, error, _RET_IP_);
> >  	return error;
> >  }
> ...
> > +STATIC int
> > +xfs_reflink_recover_cow_ag(
> > +	struct xfs_mount		*mp,
> > +	xfs_agnumber_t			agno)
> > +{
> > +	struct xfs_trans		*tp;
> > +	struct xfs_btree_cur		*cur;
> > +	struct xfs_buf			*agbp;
> > +	struct xfs_reflink_recovery	*rr, *n;
> > +	struct list_head		debris;
> > +	union xfs_btree_irec		low;
> > +	union xfs_btree_irec		high;
> > +	struct xfs_defer_ops		dfops;
> > +	xfs_fsblock_t			fsb;
> > +	int				error;
> > +
> > +	error = xfs_alloc_read_agf(mp, NULL, agno, 0, &agbp);
> > +	if (error)
> > +		return error;
> > +	cur = xfs_refcountbt_init_cursor(mp, NULL, agbp, agno, NULL);
> > +
> > +	/* Find all the leftover CoW staging extents. */
> > +	INIT_LIST_HEAD(&debris);
> > +	memset(&low, 0, sizeof(low));
> > +	memset(&high, 0, sizeof(high));
> > +	low.rc.rc_startblock = 0;
> > +	high.rc.rc_startblock = -1U;
> > +	error = xfs_btree_query_range(cur, &low, &high,
> > +			xfs_reflink_recover_extent, &debris);
> > +	if (error)
> > +		goto out_error;
> 
> Potential memory leak of debris list entries on error.

Ugh, the error handling in this whole function needs reworking.

> > +	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> > +	xfs_buf_relse(agbp);
> > +
> > +	/* Now iterate the list to free the leftovers */
> > +	list_for_each_entry(rr, &debris, rr_list) {
> > +		/* Set up transaction. */
> > +		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0, 0, &tp);
> > +		if (error)
> > +			goto out_free;
> > +
> > +		trace_xfs_reflink_recover_extent(mp, agno, &rr->rr_rrec);
> > +
> > +		/* Free the orphan record */
> > +		xfs_defer_init(&dfops, &fsb);
> > +		fsb = XFS_AGB_TO_FSB(mp, agno, rr->rr_rrec.rc_startblock);
> > +		error = xfs_refcount_free_cow_extent(mp, &dfops, fsb,
> > +				rr->rr_rrec.rc_blockcount);
> > +		if (error)
> > +			goto out_defer;
> > +
> > +		/* Free the block. */
> > +		xfs_bmap_add_free(mp, &dfops, fsb,
> > +				rr->rr_rrec.rc_blockcount, NULL);
> > +
> > +		error = xfs_defer_finish(&tp, &dfops, NULL);
> > +		if (error)
> > +			goto out_defer;
> > +
> > +		error = xfs_trans_commit(tp);
> > +		if (error)
> > +			goto out_cancel;
> > +	}
> > +	goto out_free;
> 
> return 0 ?

Yep.  Good catch.

--D

> 
> Brian
> 
> > +
> > +out_defer:
> > +	xfs_defer_cancel(&dfops);
> > +out_cancel:
> > +	xfs_trans_cancel(tp);
> > +
> > +out_free:
> > +	/* Free the leftover list */
> > +	list_for_each_entry_safe(rr, n, &debris, rr_list) {
> > +		list_del(&rr->rr_list);
> > +		kmem_free(rr);
> > +	}
> > +
> > +	return error;
> > +
> > +out_error:
> > +	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> > +	xfs_buf_relse(agbp);
> > +	return error;
> > +}
> > +
> > +/*
> > + * Free leftover CoW reservations that didn't get cleaned out.
> > + */
> > +int
> > +xfs_reflink_recover_cow(
> > +	struct xfs_mount	*mp)
> > +{
> > +	xfs_agnumber_t		agno;
> > +	int			error = 0;
> > +
> > +	if (!xfs_sb_version_hasreflink(&mp->m_sb))
> > +		return 0;
> > +
> > +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > +		error = xfs_reflink_recover_cow_ag(mp, agno);
> > +		if (error)
> > +			break;
> > +	}
> > +
> > +	return error;
> > +}
> > diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> > index c0c989a..1d2f180 100644
> > --- a/fs/xfs/xfs_reflink.h
> > +++ b/fs/xfs/xfs_reflink.h
> > @@ -42,5 +42,6 @@ extern int xfs_reflink_cancel_cow_range(struct xfs_inode *ip, xfs_off_t offset,
> >  		xfs_off_t count);
> >  extern int xfs_reflink_end_cow(struct xfs_inode *ip, xfs_off_t offset,
> >  		xfs_off_t count);
> > +extern int xfs_reflink_recover_cow(struct xfs_mount *mp);
> >  
> >  #endif /* __XFS_REFLINK_H */
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 26b45b3..e6aaa91 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1306,6 +1306,15 @@ xfs_fs_remount(
> >  		xfs_restore_resvblks(mp);
> >  		xfs_log_work_queue(mp);
> >  		xfs_queue_eofblocks(mp);
> > +
> > +		/* Recover any CoW blocks that never got remapped. */
> > +		error = xfs_reflink_recover_cow(mp);
> > +		if (error) {
> > +			xfs_err(mp,
> > +	"Error %d recovering leftover CoW allocations.", error);
> > +			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > +			return error;
> > +		}
> >  	}
> >  
> >  	/* rw -> ro */
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index 8e89223..ca0930b 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -2916,14 +2916,18 @@ DEFINE_AG_ERROR_EVENT(xfs_refcount_update_error);
> >  /* refcount adjustment tracepoints */
> >  DEFINE_AG_EXTENT_EVENT(xfs_refcount_increase);
> >  DEFINE_AG_EXTENT_EVENT(xfs_refcount_decrease);
> > +DEFINE_AG_EXTENT_EVENT(xfs_refcount_cow_increase);
> > +DEFINE_AG_EXTENT_EVENT(xfs_refcount_cow_decrease);
> >  DEFINE_REFCOUNT_TRIPLE_EXTENT_EVENT(xfs_refcount_merge_center_extents);
> >  DEFINE_REFCOUNT_EXTENT_EVENT(xfs_refcount_modify_extent);
> > +DEFINE_REFCOUNT_EXTENT_EVENT(xfs_reflink_recover_extent);
> >  DEFINE_REFCOUNT_EXTENT_AT_EVENT(xfs_refcount_split_extent);
> >  DEFINE_REFCOUNT_DOUBLE_EXTENT_EVENT(xfs_refcount_merge_left_extent);
> >  DEFINE_REFCOUNT_DOUBLE_EXTENT_EVENT(xfs_refcount_merge_right_extent);
> >  DEFINE_REFCOUNT_DOUBLE_EXTENT_AT_EVENT(xfs_refcount_find_left_extent);
> >  DEFINE_REFCOUNT_DOUBLE_EXTENT_AT_EVENT(xfs_refcount_find_right_extent);
> >  DEFINE_AG_ERROR_EVENT(xfs_refcount_adjust_error);
> > +DEFINE_AG_ERROR_EVENT(xfs_refcount_adjust_cow_error);
> >  DEFINE_AG_ERROR_EVENT(xfs_refcount_merge_center_extents_error);
> >  DEFINE_AG_ERROR_EVENT(xfs_refcount_modify_extent_error);
> >  DEFINE_AG_ERROR_EVENT(xfs_refcount_split_extent_error);
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html