xfs
[Top] [All Lists]

Re: [PATCH 5/6] [XFS] Replace per-ag array with a radix tree

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/6] [XFS] Replace per-ag array with a radix tree
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 11 Dec 2009 11:43:53 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20091210234547.GA28289@xxxxxxxxxxxxx>
References: <1259734299-20306-1-git-send-email-david@xxxxxxxxxxxxx> <1259734299-20306-6-git-send-email-david@xxxxxxxxxxxxx> <20091210234547.GA28289@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Thu, Dec 10, 2009 at 06:45:47PM -0500, Christoph Hellwig wrote:
> On Wed, Dec 02, 2009 at 05:11:38PM +1100, Dave Chinner wrote:
> > -           down_read(&mp->m_peraglock);
> > +           pag = xfs_perag_get(mp, ag);
> >             while (blen < ap->alen) {
> > -                   pag = xfs_perag_get(mp, ag);
> >                     if (!pag->pagf_init &&
> >                         (error = xfs_alloc_pagf_init(mp, args.tp,
> >                                 ag, XFS_ALLOC_FLAG_TRYLOCK))) {
> >                             xfs_perag_put(pag);
> > -                           up_read(&mp->m_peraglock);
> >                             return error;
> >                     }
> >                     /*
> > @@ -2801,7 +2799,6 @@ xfs_bmap_btalloc(
> >                     } else
> >                             notinit = 1;
> >  
> > -                   xfs_perag_put(pag);
> 
> There's a lot of those xfs_perag_get/put moved around here.  Having
> those merged into the patch that adds them would be a lot cleaner.

OK, I'll clean all those up into the initial couple of patches.

> > +   /* allocate the new per-ag structures */
> >     if (nagcount > oagcount) {
> > +           /* XXX: (dgc) We don't need the filestream flush anymore? */
> >             xfs_filestream_flush(mp);
> 
> What was the reason to have it in the first time?

Filestreams keeps reference counts and state on the per-ag
structure. If it was to be re-allocated, then all the references had
to be dropped before reallocation, hence the flush. Now there is
no reallocation, I don't think we need the flush anymore. Removing
it also means updating comments in the filestream code, so I was
going to do all that in a subsequent patch....


> > index 3727104..d6de63d 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -207,13 +207,17 @@ STATIC void
> >  xfs_free_perag(
> >     xfs_mount_t     *mp)
> >  {
> > +   xfs_agnumber_t  agno;
> > +   struct xfs_perag *pag;
> > +
> > +   for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > +           spin_lock(&mp->m_perag_lock);
> > +           pag = radix_tree_delete(&mp->m_perag_tree, agno);
> > +           spin_unlock(&mp->m_perag_lock);
> > +           if (!pag)
> > +                   continue;
> 
> Shouldn't this be a BUG_ON/ASSERT?

Probably should be. Will change.

> > +   /*
> > +    * Walk the current per-ag tree so we don't try to initialise AGs
> > +    * that already exist (growfs case). Allocate and insert all the
> > +    * AGs we don't find ready for initialisation.
> > +    */
> > +   for (index = 0; index < agcount; index++) {
> > +           pag = xfs_perag_get(mp, index);
> > +           if (pag) {
> > +                   xfs_perag_put(pag);
> > +                   continue;
> > +           }
> > +           pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> > +           if (!pag)
> > +                   return -ENOMEM;
> > +           if (radix_tree_preload(GFP_NOFS))
> > +                   return -ENOMEM;
> 
> Leaks the pag object on failure.

Good catch.

> 
> >     mp->m_maxagi = xfs_initialize_perag(mp, sbp->sb_agcount);
> > +   if ((int)mp->m_maxagi < 0) {
> > +           cmn_err(CE_WARN, "XFS: Failed per-ag initialisation: %d",
> > +                           (int)mp->m_maxagi);
> > +           error = mp->m_maxagi;
> >
> Just assign it to error first and then later to mp->m_maxagi to avoid
> the cast?

Actually, to avoid all such sign issues, I think that mp->m_maxagi
sho┘ld be assigned in xfs_initialize_perag() and it only returns
error or success. Does that make sense?

> >  static inline xfs_perag_t *
> >  xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
> >  {
> > -   return &mp->m_perag[agno];
> > +   struct xfs_perag        *pag;
> > +
> > +   spin_lock(&mp->m_perag_lock);
> > +   pag = radix_tree_lookup(&mp->m_perag_tree, agno);
> > +   spin_unlock(&mp->m_perag_lock);
> > +   return pag;
> 
> Can't we do this as a lock-less (at least for lookups) radix tree?

I think it can be (RCU-based?) , but I think that makes sense as a
followup optimisation once we have confidence the code is working
as it should.

> And btw, I think we should still have a global sleeping lock to
> serialize the whole growfs operation against other potentional growfs
> callers.

Agreed - mp->m_growlock already does this and this patch series did
not touch it at all so it should still work ;)

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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