xfs
[Top] [All Lists]

Re: [PATCH 4/5] [PATCH] xfs: simplify buffer to transaction matching

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH 4/5] [PATCH] xfs: simplify buffer to transaction matching
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 27 Apr 2010 10:16:49 -0400
In-reply-to: <20100418001058.677429475@xxxxxxxxxxxxxxxxxxxxxx>
References: <20100418001041.865247520@xxxxxxxxxxxxxxxxxxxxxx> <20100418001058.677429475@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
Alex, any reason you haven't picked this up?  Dave's cleanup might
be a nice addition, but let's get this going first.

On Sat, Apr 17, 2010 at 08:10:45PM -0400, Christoph Hellwig wrote:
> We currenly have a routine xfs_trans_buf_item_match_all which checks if any
> log item in a transaction contains a given buffer, and a second one that
> only does this check for the first, embedded chunk of log items.  We only
> use the second routine if we know we only have that log item chunk, so get
> rid of the limited routine and always use the more complete one.
> 
> Also rename the old xfs_trans_buf_item_match_all to xfs_trans_buf_item_match
> and update various surrounding comments, and move the remaining
> xfs_trans_buf_item_match on top of the file to avoid a forward prototype.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: xfs/fs/xfs/xfs_trans_buf.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_trans_buf.c   2010-03-09 22:58:31.307004136 +0100
> +++ xfs/fs/xfs/xfs_trans_buf.c        2010-03-11 11:46:45.052004554 +0100
> @@ -40,11 +40,51 @@
>  #include "xfs_rw.h"
>  #include "xfs_trace.h"
>  
> +/*
> + * Check to see if a buffer matching the given parameters is already
> + * a part of the given transaction.
> + */
> +STATIC struct xfs_buf *
> +xfs_trans_buf_item_match(
> +     struct xfs_trans        *tp,
> +     struct xfs_buftarg      *target,
> +     xfs_daddr_t             blkno,
> +     int                     len)
> +{
> +     xfs_log_item_chunk_t    *licp;
> +     xfs_log_item_desc_t     *lidp;
> +     xfs_buf_log_item_t      *blip;
> +     int                     i;
>  
> -STATIC xfs_buf_t *xfs_trans_buf_item_match(xfs_trans_t *, xfs_buftarg_t *,
> -             xfs_daddr_t, int);
> -STATIC xfs_buf_t *xfs_trans_buf_item_match_all(xfs_trans_t *, xfs_buftarg_t 
> *,
> -             xfs_daddr_t, int);
> +     len = BBTOB(len);
> +     for (licp = &tp->t_items; licp != NULL; licp = licp->lic_next) {
> +             if (xfs_lic_are_all_free(licp)) {
> +                     ASSERT(licp == &tp->t_items);
> +                     ASSERT(licp->lic_next == NULL);
> +                     return NULL;
> +             }
> +
> +             for (i = 0; i < licp->lic_unused; i++) {
> +                     /*
> +                      * Skip unoccupied slots.
> +                      */
> +                     if (xfs_lic_isfree(licp, i))
> +                             continue;
> +
> +                     lidp = xfs_lic_slot(licp, i);
> +                     blip = (xfs_buf_log_item_t *)lidp->lid_item;
> +                     if (blip->bli_item.li_type != XFS_LI_BUF)
> +                             continue;
> +
> +                     if (XFS_BUF_TARGET(blip->bli_buf) == target &&
> +                         XFS_BUF_ADDR(blip->bli_buf) == blkno &&
> +                         XFS_BUF_COUNT(blip->bli_buf) == len)
> +                             return blip->bli_buf;
> +             }
> +     }
> +
> +     return NULL;
> +}
>  
>  /*
>   * Add the locked buffer to the transaction.
> @@ -112,14 +152,6 @@ xfs_trans_bjoin(
>   * within the transaction, just increment its lock recursion count
>   * and return a pointer to it.
>   *
> - * Use the fast path function xfs_trans_buf_item_match() or the buffer
> - * cache routine incore_match() to find the buffer
> - * if it is already owned by this transaction.
> - *
> - * If we don't already own the buffer, use get_buf() to get it.
> - * If it doesn't yet have an associated xfs_buf_log_item structure,
> - * then allocate one and add the item to this transaction.
> - *
>   * If the transaction pointer is NULL, make this just a normal
>   * get_buf() call.
>   */
> @@ -149,11 +181,7 @@ xfs_trans_get_buf(xfs_trans_t    *tp,
>        * have it locked.  In this case we just increment the lock
>        * recursion count and return the buffer to the caller.
>        */
> -     if (tp->t_items.lic_next == NULL) {
> -             bp = xfs_trans_buf_item_match(tp, target_dev, blkno, len);
> -     } else {
> -             bp  = xfs_trans_buf_item_match_all(tp, target_dev, blkno, len);
> -     }
> +     bp = xfs_trans_buf_item_match(tp, target_dev, blkno, len);
>       if (bp != NULL) {
>               ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
>               if (XFS_FORCED_SHUTDOWN(tp->t_mountp))
> @@ -259,14 +287,6 @@ int      xfs_error_mod = 33;
>   * within the transaction and already read in, just increment its
>   * lock recursion count and return a pointer to it.
>   *
> - * Use the fast path function xfs_trans_buf_item_match() or the buffer
> - * cache routine incore_match() to find the buffer
> - * if it is already owned by this transaction.
> - *
> - * If we don't already own the buffer, use read_buf() to get it.
> - * If it doesn't yet have an associated xfs_buf_log_item structure,
> - * then allocate one and add the item to this transaction.
> - *
>   * If the transaction pointer is NULL, make this just a normal
>   * read_buf() call.
>   */
> @@ -328,11 +348,7 @@ xfs_trans_read_buf(
>        * If the buffer is not yet read in, then we read it in, increment
>        * the lock recursion count, and return it to the caller.
>        */
> -     if (tp->t_items.lic_next == NULL) {
> -             bp = xfs_trans_buf_item_match(tp, target, blkno, len);
> -     } else {
> -             bp = xfs_trans_buf_item_match_all(tp, target, blkno, len);
> -     }
> +     bp = xfs_trans_buf_item_match(tp, target, blkno, len);
>       if (bp != NULL) {
>               ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
>               ASSERT(XFS_BUF_FSPRIVATE2(bp, xfs_trans_t *) == tp);
> @@ -901,111 +917,3 @@ xfs_trans_dquot_buf(
>  
>       bip->bli_format.blf_flags |= type;
>  }
> -
> -/*
> - * Check to see if a buffer matching the given parameters is already
> - * a part of the given transaction.  Only check the first, embedded
> - * chunk, since we don't want to spend all day scanning large transactions.
> - */
> -STATIC xfs_buf_t *
> -xfs_trans_buf_item_match(
> -     xfs_trans_t     *tp,
> -     xfs_buftarg_t   *target,
> -     xfs_daddr_t     blkno,
> -     int             len)
> -{
> -     xfs_log_item_chunk_t    *licp;
> -     xfs_log_item_desc_t     *lidp;
> -     xfs_buf_log_item_t      *blip;
> -     xfs_buf_t               *bp;
> -     int                     i;
> -
> -     bp = NULL;
> -     len = BBTOB(len);
> -     licp = &tp->t_items;
> -     if (!xfs_lic_are_all_free(licp)) {
> -             for (i = 0; i < licp->lic_unused; i++) {
> -                     /*
> -                      * Skip unoccupied slots.
> -                      */
> -                     if (xfs_lic_isfree(licp, i)) {
> -                             continue;
> -                     }
> -
> -                     lidp = xfs_lic_slot(licp, i);
> -                     blip = (xfs_buf_log_item_t *)lidp->lid_item;
> -                     if (blip->bli_item.li_type != XFS_LI_BUF) {
> -                             continue;
> -                     }
> -
> -                     bp = blip->bli_buf;
> -                     if ((XFS_BUF_TARGET(bp) == target) &&
> -                         (XFS_BUF_ADDR(bp) == blkno) &&
> -                         (XFS_BUF_COUNT(bp) == len)) {
> -                             /*
> -                              * We found it.  Break out and
> -                              * return the pointer to the buffer.
> -                              */
> -                             break;
> -                     } else {
> -                             bp = NULL;
> -                     }
> -             }
> -     }
> -     return bp;
> -}
> -
> -/*
> - * Check to see if a buffer matching the given parameters is already
> - * a part of the given transaction.  Check all the chunks, we
> - * want to be thorough.
> - */
> -STATIC xfs_buf_t *
> -xfs_trans_buf_item_match_all(
> -     xfs_trans_t     *tp,
> -     xfs_buftarg_t   *target,
> -     xfs_daddr_t     blkno,
> -     int             len)
> -{
> -     xfs_log_item_chunk_t    *licp;
> -     xfs_log_item_desc_t     *lidp;
> -     xfs_buf_log_item_t      *blip;
> -     xfs_buf_t               *bp;
> -     int                     i;
> -
> -     bp = NULL;
> -     len = BBTOB(len);
> -     for (licp = &tp->t_items; licp != NULL; licp = licp->lic_next) {
> -             if (xfs_lic_are_all_free(licp)) {
> -                     ASSERT(licp == &tp->t_items);
> -                     ASSERT(licp->lic_next == NULL);
> -                     return NULL;
> -             }
> -             for (i = 0; i < licp->lic_unused; i++) {
> -                     /*
> -                      * Skip unoccupied slots.
> -                      */
> -                     if (xfs_lic_isfree(licp, i)) {
> -                             continue;
> -                     }
> -
> -                     lidp = xfs_lic_slot(licp, i);
> -                     blip = (xfs_buf_log_item_t *)lidp->lid_item;
> -                     if (blip->bli_item.li_type != XFS_LI_BUF) {
> -                             continue;
> -                     }
> -
> -                     bp = blip->bli_buf;
> -                     if ((XFS_BUF_TARGET(bp) == target) &&
> -                         (XFS_BUF_ADDR(bp) == blkno) &&
> -                         (XFS_BUF_COUNT(bp) == len)) {
> -                             /*
> -                              * We found it.  Break out and
> -                              * return the pointer to the buffer.
> -                              */
> -                             return bp;
> -                     }
> -             }
> -     }
> -     return NULL;
> -}
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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