xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 044/119] xfs: propagate bmap updates to rmapbt
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Mon, 18 Jul 2016 08:56:24 -0400
Cc: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>, linux-fsdevel@xxxxxxxxxxxxxxx, vishal.l.verma@xxxxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20160718012122.GA1922@dastard>
References: <146612627129.12839.3827886950949809165.stgit@xxxxxxxxxxxxxxxx> <146612655409.12839.4069768871045909071.stgit@xxxxxxxxxxxxxxxx> <20160715183356.GD55338@xxxxxxxxxxxxxxx> <20160716072621.GC21529@xxxxxxxxxxxxxxxx> <20160718012122.GA1922@dastard>
User-agent: Mutt/1.6.1 (2016-04-27)
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@xxxxxxxxxx>
> .....
> > > > + * 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@xxxxxxxxxxxxx
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>