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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
|