xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: Don't allocate new buffers on every call to _xfs_bu

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: Don't allocate new buffers on every call to _xfs_buf_find
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 10 Aug 2011 15:48:27 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110809075151.GA22968@xxxxxxxxxxxxx>
References: <1312786271-10871-1-git-send-email-david@xxxxxxxxxxxxx> <1312786271-10871-2-git-send-email-david@xxxxxxxxxxxxx> <20110809075151.GA22968@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Aug 09, 2011 at 03:51:51AM -0400, Christoph Hellwig wrote:
> On Mon, Aug 08, 2011 at 04:51:10PM +1000, Dave Chinner wrote:
> > -xfs_buf_t *
> > +struct xfs_buf *
> >  xfs_buf_get(
> > -   xfs_buftarg_t           *target,/* target for buffer            */
> > +   struct xfs_buftarg      *target,/* target for buffer            */
> >     xfs_off_t               ioff,   /* starting offset of range     */
> >     size_t                  isize,  /* length of range              */
> >     xfs_buf_flags_t         flags)
> >  {
> > -   xfs_buf_t               *bp, *new_bp;
> > +   struct xfs_buf          *bp;
> > +   struct xfs_buf          *new_bp = NULL;
> >     int                     error = 0;
> >  
> > -   new_bp = xfs_buf_allocate(flags);
> > -   if (unlikely(!new_bp))
> > -           return NULL;
> > -
> > +again:
> >     bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
> > -   if (bp == new_bp) {
> > +   if (!(bp || new_bp)) {
> > +           new_bp = xfs_buf_allocate(flags);
> > +           if (unlikely(!new_bp))
> > +                   return NULL;
> > +
> > +           _xfs_buf_initialize(new_bp, target, ioff << BBSHIFT,
> > +                           isize << BBSHIFT, flags);
> > +           goto again;
> > +   }
> > +
> > +   if (!bp) {
> > +           xfs_buf_deallocate(new_bp);
> > +           return NULL;
> > +   } else if (bp == new_bp) {
> >             error = xfs_buf_allocate_memory(bp, flags);
> >             if (error)
> >                     goto no_buffer;
> > -   } else {
> > +   } else if (new_bp)
> >             xfs_buf_deallocate(new_bp);
> > -           if (unlikely(bp == NULL))
> > -                   return NULL;
> > -   }
> 
> The while loop looks a bit confusing to me.  Why not unroll it similar
> to how it's done for the VFS inode cache:
> 
>       bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
>       if (bp)
>               goto found;
> 
>       new_bp = xfs_buf_allocate(flags);
>       if (unlikely(!new_bp))
>               return NULL;
>       _xfs_buf_initialize(new_bp, target, ioff << BBSHIFT, isize << BBSHIFT,
>                           flags);
>       bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
>       if (!bp) {
>               xfs_buf_deallocate(new_bp);
>               return NULL;
>       }
> 
>       if (bp == new_bp) {
>               error = xfs_buf_allocate_memory(bp, flags);
>               if (error)
>                       goto no_buffer;
>       }
> found:

Makes sense.

> or even better make sure all buffer initialization is done either as
> part of xfs_buf_find or the caller - although we'd need a variant
> with caller locking if we want to go down the latter route.  This
> variant would be nessecary if we ever want to add shared/exclusive or
> RCU locking for buffer lookup anyway.

I'll leave that problem for when RCU-izing the lookup - I haven't
quite worked out what the best way of doing that is yet, anyway.

> There's also some other weird things here that should be cleaned up
> when touching it:
> 
>  - we should never be able to find a buffer with the wrong mapped
>    state, so this could be replaced by an assert

OK.

>  - there should be no need to re-assign b_bn and b_count_desired
>    when we find a buffer

Yes, definitely.

I'm also tempted to rename some variables to make it more obvious
what units they are in and to reduce the conversions we do from
one type to another.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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