Re: [PATCH 044/119] xfs: propagate bmap updates to rmapbt

Brian Foster
2016-07-18T12:56:30Z

On Mon, Jul 18, 2016 at 11:21:22AM +1000, Dave Chinner wrote:
> On Sat, Jul 16, 2016 at 12:26:21AM -0700, Darrick J. Wong wrote:
> > On Fri, Jul 15, 2016 at 02:33:56PM -0400, Brian Foster wrote:
> > > On Thu, Jun 16, 2016 at 06:22:34PM -0700, Darrick J. Wong wrote:
> > > > When we map, unmap, or convert an extent in a file's data or attr
> > > > fork, schedule a respective update in the rmapbt. Previous versions
> > > > of this patch required a 1:1 correspondence between bmap and rmap,
> > > > but this is no longer true.
> > > >
> > > > v2: Remove the 1:1 correspondence requirement now that we have the
> > > > ability to make interval queries against the rmapbt. Update the
> > > > commit message to reflect the broad restructuring of this patch.
> > > > Fix the bmap shift code to adjust the rmaps correctly.
> > > >
> > > > v3: Use the deferred operations code to handle redo operations
> > > > atomically and deadlock free. Plumb in all five rmap actions
> > > > (map, unmap, convert extent, alloc, free); we'll use the first
> > > > three now for file data, and reflink will want the last two.
> > > > Add an error injection site to test log recovery.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> .....
> > > > + * superblock and the AGF because we'll always grab them in the same
> > > > + * order.
> > > > + */
> > > > +int
> > > > +xfs_rmap_finish_one(
> > > > + struct xfs_trans *tp,
> > > > + enum xfs_rmap_intent_type type,
> > > > + __uint64_t owner,
> > > > + int whichfork,
> > > > + xfs_fileoff_t startoff,
> > > > + xfs_fsblock_t startblock,
> > > > + xfs_filblks_t blockcount,
> > > > + xfs_exntst_t state,
> > > > + struct xfs_btree_cur **pcur)
> > > > +{
> > > > + struct xfs_mount *mp = tp->t_mountp;
> > > > + struct xfs_btree_cur *rcur;
> > > > + struct xfs_buf *agbp = NULL;
> > > > + int error = 0;
> > > > + xfs_agnumber_t agno;
> > > > + struct xfs_owner_info oinfo;
> > > > + xfs_agblock_t bno;
> > > > + bool unwritten;
> > > > +
> > > > + agno = XFS_FSB_TO_AGNO(mp, startblock);
> > > > + ASSERT(agno != NULLAGNUMBER);
> > > > + bno = XFS_FSB_TO_AGBNO(mp, startblock);
> > > > +
> > > > + trace_xfs_rmap_deferred(mp, agno, type, bno, owner, whichfork,
> > > > + startoff, blockcount, state);
> > > > +
> > > > + if (XFS_TEST_ERROR(false, mp,
> > > > + XFS_ERRTAG_RMAP_FINISH_ONE,
> > > > + XFS_RANDOM_RMAP_FINISH_ONE))
> > > > + return -EIO;
> > > > +
> > > > + /*
> > > > + * If we haven't gotten a cursor or the cursor AG doesn't match
> > > > + * the startblock, get one now.
> > > > + */
> > > > + rcur = *pcur;
> > > > + if (rcur != NULL && rcur->bc_private.a.agno != agno) {
> > > > + xfs_rmap_finish_one_cleanup(tp, rcur, 0);
> > > > + rcur = NULL;
> > > > + *pcur = NULL;
> > > > + }
> > > > + if (rcur == NULL) {
> > > > + error = xfs_free_extent_fix_freelist(tp, agno, &agbp);
> > >
> > > Comment? Why is this here? (Maybe we should rename that function while
> > > we're at it..)
> >
> > /*
> > * Ensure the freelist is of a sufficient length to provide for any btree
> > * splits that could happen when we make changes to the rmapbt.
> > */
> >
> > (I don't know why the function has that name; Dave supplied it.)
>
> I named it that way because it was common code factored out of
> xfs_free_extent() for use by multiple callers on the extent freeing
> side of things. Feel free to name it differently if you can think of
> something more appropriate.
>

Right, that's why it stood out to me. I don't feel too strongly about
it, perhaps xfs_fix_freelist()? xfs_agf_fix_freelist()?

Brian

> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs