[PATCH 4/5] [PATCH] xfs: simplify buffer to transaction matching
Alex Elder
aelder at sgi.com
Thu Apr 29 08:35:22 CDT 2010
On Sat, 2010-04-17 at 20:10 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-buf-match-cleanup)
> 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.
Dave suggested using the xfs_trans_*_item() routines (which maybe
could be renamed xfs_trans_item_*() someday). That is the right
thing to do, but not necessary at this point. In fact I prefer
this smaller change anyway, since it clearly just transforms
the original xfs_trans_buf_item_match_all() code directly to
the new result. The switch to use xfs_trans_*_item() can (and
should) happen as a follow-on patch.
The simplification is good.
Reviewed-by: Alex Elder <aelder at sgi.com>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
>
> 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 at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
More information about the xfs
mailing list