Re: [PATCH 02/14] libxfs: adjust refcounts in reflink btree

[Dave Chinner <david@fromorbit.com>]

On Thu, Jun 25, 2015 at 04:39:23PM -0700, Darrick J. Wong wrote:
> Provide a function to adjust the reference counts for a range of
> blocks in the reflink btree.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_reflink_btree.c |  406 +++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_reflink_btree.h |    4 
>  2 files changed, 410 insertions(+)

As per previous comments, this all belongs in
fs/xfs/libxfs/xfs_reflink.c...

> 
> 
> diff --git a/fs/xfs/libxfs/xfs_reflink_btree.c b/fs/xfs/libxfs/xfs_reflink_btree.c
> index 8a0fa5d..380ed72 100644
> --- a/fs/xfs/libxfs/xfs_reflink_btree.c
> +++ b/fs/xfs/libxfs/xfs_reflink_btree.c
> @@ -529,3 +529,409 @@ xfs_reflinkbt_delete(
>  error0:
>  	return error;
>  }
> +
> +#ifdef REFLINK_DEBUG
> +static void
> +dump_cur_loc(
> +	struct xfs_btree_cur	*cur,
> +	const char		*str,
> +	int			line)
> +{
> +	xfs_agblock_t		gbno;
> +	xfs_extlen_t		glen;
> +	xfs_nlink_t		gnr;
> +	int			i;
> +
> +	xfs_reflink_get_rec(cur, &gbno, &glen, &gnr, &i);
> +	printk(KERN_INFO "%s(%d) cur[%d]:[%u,%u,%u,%d] ", str, line,
> +	       cur->bc_ptrs[0], gbno, glen, gnr, i);
> +	if (i && cur->bc_ptrs[0]) {
> +		cur->bc_ptrs[0]--;
> +		xfs_reflink_get_rec(cur, &gbno, &glen, &gnr, &i);
> +		printk("left[%d]:[%u,%u,%u,%d] ", cur->bc_ptrs[0],
> +		       gbno, glen, gnr, i);
> +		cur->bc_ptrs[0]++;
> +	}
> +
> +	if (i && cur->bc_ptrs[0] < xfs_reflinkbt_get_maxrecs(cur, 0)) {
> +		cur->bc_ptrs[0]++;
> +		xfs_reflink_get_rec(cur, &gbno, &glen, &gnr, &i);
> +		printk("right[%d]:[%u,%u,%u,%d] ", cur->bc_ptrs[0],
> +		       gbno, glen, gnr, i);
> +		cur->bc_ptrs[0]--;
> +	}
> +	printk("\n");
> +}
> +#else
> +# define dump_cur_loc(c, s, l)
> +#endif

Use trace points on lookup/update/insert/delete so debug like this
is unnecessary.


> +/*
> + * Adjust the ref count of a range of AG blocks.
> + */
> +int						/* error */
> +xfs_reflinkbt_adjust_refcount(
> +	struct xfs_mount	*mp,
> +	struct xfs_trans	*tp,		/* transaction pointer */
> +	struct xfs_buf		*agbp,		/* buffer for agf structure */
> +	xfs_agnumber_t		agno,		/* allocation group number */
> +	xfs_agblock_t		agbno,		/* start of range */
> +	xfs_extlen_t		aglen,		/* length of range */
> +	int			adj)		/* how much to change refcnt */

350 line function. Needs factoring. Also needs a comment explaining
the algorithm. 

> +{
> +	struct xfs_btree_cur	*cur;
> +	int			error;
> +	int			i, have;
> +	bool			real_crl;	/* cbno/clen is on disk? */
> +	xfs_agblock_t		lbno, cbno, rbno;	/* rlextent start */
> +	xfs_extlen_t		llen, clen, rlen;	/* rlextent length */
> +	xfs_nlink_t		lnr, cnr, rnr;		/* rlextent refcount */

"num" is the usual shorthand for "number". And in this case, nr is
extremely ambiguous: Number of records, number of reflinks, some
other number? I can't easily tell when I read the code, so the
variable names need to be better. factoring will certainly help
here.

> +	xfs_agblock_t		bno;		/* ag bno in the loop */
> +	xfs_agblock_t		agbend;		/* end agbno of the loop */
> +	xfs_extlen_t		len;		/* remaining len to add */
> +	xfs_nlink_t		new_cnr;	/* new refcount */
> +
> +	CHECK_AG_NUMBER(mp, agno);
> +	CHECK_AG_EXTENT(mp, agbno, aglen);

No real need for these checks - bad agno or extent sizes shoul dhave
been validated long before this.

> +
> +	/*
> +	 * Allocate/initialize a cursor for the by-number freespace btree.
> +	 */
> +	cur = xfs_reflinkbt_init_cursor(mp, tp, agbp, agno);

You can kill that incorrect comment.

> +
> +	/*
> +	 * Split a left rlextent that crosses agbno.
> +	 */

These comments need some ascii art displaying the before, current
extent and after states so it's clear what the intent is. As it is,
I'd probably split these into "left/right/middle" helper functions,
as there is no state created by these initial overlap splits
used later in the function. That would get rid of excessive
indentation, make the error handling more obvious, etc.

> +	error = xfs_reflink_lookup_le(cur, agbno, &have);
> +	if (error)
> +		goto error0;

		goto out_error;

> +	if (have) {

if I "have" what?  "found_rec" would be a better name, because then
the code reads clearly...

> +	/*
> +	 * Start iterating the range we're adjusting.  rlextent boundaries
> +	 * should be at agbno and agbend.
> +	 */

Trying to work my way through this loop, but the logic is hard to
follow. It's hurting my head trying to work out what it is supposed
to be doing, so I'm going to wait for more comments, ascii art, and
factoring before really looking at it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs