On Wed, Jul 01, 2015 at 11:06:54AM +1000, Dave Chinner wrote:
> 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.
"refc" as shorthand for reference count, perhaps?
> > + 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.
Ok, I'll draw some pictures. :)
> > + 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.
:)
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
