xfs
[Top] [All Lists]

Re: [PATCH 5/9] xfs: convert the xfsaild threads to a workqueue

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/9] xfs: convert the xfsaild threads to a workqueue
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 7 Apr 2011 10:08:02 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110406181256.GA4356@xxxxxxxxxxxxx>
References: <1302070758-17312-1-git-send-email-david@xxxxxxxxxxxxx> <1302070758-17312-6-git-send-email-david@xxxxxxxxxxxxx> <20110406181256.GA4356@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed, Apr 06, 2011 at 02:12:56PM -0400, Christoph Hellwig wrote:
> On Wed, Apr 06, 2011 at 04:19:14PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Similar to the xfssyncd, the per-filesystem xfsaild threads can be
> > converted to a global workqueue and run periodically by delayed
> > works. This makes sense for the AIL pushing because it uses
> > variable timeouts depending on the work that needs to be done.
> > 
> > By removing the xfsaild, we simplify the AIL pushing code and
> > remove the need to spread the code to implement the threading
> > and pushing across multiple files.
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> >  fs/xfs/linux-2.6/xfs_super.c |  115 ++++++++++++------------------------
> >  fs/xfs/xfs_trans_ail.c       |  133 
> > ++++++++++++++++++++++++------------------
> >  fs/xfs/xfs_trans_priv.h      |   15 +++--
> >  3 files changed, 121 insertions(+), 142 deletions(-)
> 
> 
> 
> 
> > +   tout = 0;
> >     if (!count) {
> >             /* We're past our target or empty, so idle */
> > -           last_pushed_lsn = 0;
> > +           ailp->xa_last_pushed_lsn = 0;
> > +
> > +           /*
> > +            * Check for an updated push target before clearing the
> > +            * XFS_AIL_PUSHING_BIT. If the target changed, we've got more
> > +            * work to do.
> > +            */
> > +           smp_rmb();
> > +           if (ailp->xa_target == target)
> > +                   clear_bit(XFS_AIL_PUSHING_BIT, &ailp->xa_flags);
> > +           else
> > +                   tout = 50;
> 
> I thjink this could be simplified a bit by just returning here if we
> have matched the target, and then making the queueing up of work later
> on unconditional.

Ok, that's reasonable.

> > +/*
> > + * xfs_trans_ail_push
> > + *
> 
> Wha'ts the point of mentioning the function name like this in comments?

None really, was just being consistent with other functions. Will
fix.

> > +   /*
> > +    * En??ure that the new target is noticed in push code before it clears
> 
> There's some weird character here which my mailer can't display.

Oops.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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