xfs
[Top] [All Lists]

Re: [PATCH 3/3] xfs: Sort delayed write buffers before dispatch

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/3] xfs: Sort delayed write buffers before dispatch
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 8 Jan 2010 22:17:44 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100108111152.GA29812@xxxxxxxxxxxxx>
References: <1262649861-28530-1-git-send-email-david@xxxxxxxxxxxxx> <1262649861-28530-4-git-send-email-david@xxxxxxxxxxxxx> <20100108111152.GA29812@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Fri, Jan 08, 2010 at 06:11:52AM -0500, Christoph Hellwig wrote:
> > +/*
> > + * Compare function is more complex than it needs to be because
> > + * the return value is only 32 bits and we are doing comparisons
> > + * on 64 bit values
> > + */
> > +int
> > +xfs_buf_cmp(
> 
> Should be marked static.
> 
> > +void
> > +xfs_buf_delwri_sort(
> > +   xfs_buftarg_t   *target,
> > +   struct list_head *list)
> > +{
> > +   list_sort(NULL, list, xfs_buf_cmp);
> > +}
> 
> Same here.  Not sure I would even bother with the wrapper.  Also the
> first argument is entirely unused.
> 
> >  STATIC int
> >  xfsbufd(
> >     void            *data)
> >  {
> > +   xfs_buftarg_t   *target = (xfs_buftarg_t *)data;
> >  
> >     current->flags |= PF_MEMALLOC;
> >  
> > @@ -1739,6 +1774,8 @@ xfsbufd(
> >     do {
> >             long    age = xfs_buf_age_centisecs * msecs_to_jiffies(10);
> >             long    tout = age;
> > +           int     count = 0;
> > +           struct list_head tmp;
> >  
> >             if (unlikely(freezing(current))) {
> >                     set_bit(XBT_FORCE_SLEEP, &target->bt_flags);
> > @@ -1753,11 +1790,10 @@ xfsbufd(
> >             schedule_timeout_interruptible(tout);
> >  
> >             xfs_buf_delwri_split(target, &tmp, age);
> > +           xfs_buf_delwri_sort(target, &tmp);
> >             while (!list_empty(&tmp)) {
> > +                   struct xfs_buf *bp;
> > +                   bp = list_first_entry(&tmp, struct xfs_buf, b_list);
> >                     list_del_init(&bp->b_list);
> >                     xfs_buf_iostrategy(bp);
> >                     count++;
> > 
> >  
> >     if (wait)
> >             blk_run_address_space(target->bt_mapping);
> >  
> > +   /* Now wait for IO to complete if required. */
> > +   while (!list_empty(&wait_list)) {
> > +           bp = list_first_entry(&wait_list, struct xfs_buf, b_list);
> >  
> >             list_del_init(&bp->b_list);
> >             xfs_iowait(bp);
> 
> As a tiny optimization you might want to move this into the if (wait)
> block

All makes sense, will update.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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