xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: Don't allocate new buffers on every call to _xfs_buf_find
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 9 Aug 2011 03:51:51 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1312786271-10871-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1312786271-10871-1-git-send-email-david@xxxxxxxxxxxxx> <1312786271-10871-2-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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:


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.

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
 - there should be no need to re-assign b_bn and b_count_desired
   when we find a buffer

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