xfs
[Top] [All Lists]

Re: [PATCH v3 4/5] xfs: fix the multi-segment log buffer format

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH v3 4/5] xfs: fix the multi-segment log buffer format
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sat, 8 Dec 2012 07:38:21 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121204224938.906216248@xxxxxxx>
References: <20121204231801.982422341@xxxxxxx> <20121204224938.906216248@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Dec 04, 2012 at 05:18:05PM -0600, Mark Tinguely wrote:
> Per Dave Chinner suggestion, this patch:
>  1) Corrects the detection of whether a multi-segment buffer is
>     still tracking data.
>  2) Clears all the buffer log formats for a multi-segment buffer.
> 
> Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_buf_item.c  |   13 ++++++++++---
>  fs/xfs/xfs_buf_item.c  |   13 ++++++++++---
>  fs/xfs/xfs_trans_buf.c |    7 +++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> Index: b/fs/xfs/xfs_buf_item.c
> ===================================================================
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -611,7 +611,7 @@ xfs_buf_item_unlock(
>  {
>       struct xfs_buf_log_item *bip = BUF_ITEM(lip);
>       struct xfs_buf          *bp = bip->bli_buf;
> -     int                     aborted;
> +     int                     aborted, clean, i;
>       uint                    hold;
>  
>       /* Clear the buffer's association with this transaction. */
> @@ -654,8 +654,15 @@ xfs_buf_item_unlock(
>        * If the buf item isn't tracking any data, free it, otherwise drop the
>        * reference we hold to it.
>        */
> -     if (xfs_bitmap_empty(bip->__bli_format.blf_data_map,
> -                          bip->__bli_format.blf_map_size))
> +     clean = 1;
> +     for (i = 0; i < bip->bli_format_count; i++) {
> +             if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
> +                          bip->bli_formats[i].blf_map_size)) {
> +                     clean = 0;
> +                     break;
> +             }
> +     }
> +     if (clean)
>               xfs_buf_item_relse(bp);
>       else
>               atomic_dec(&bip->bli_refcount);

Looks ok, although avoiding the clean variable would be even better:

        for (i = 0; i < bip->bli_format_count; i++) {
                if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
                                      bip->bli_formats[i].blf_map_size)) {
                        atomic_dec(&bip->bli_refcount);
                        goto out;
                }
        }

        xfs_buf_item_relse(bp);
out:

bu that might be getting a bit too much into bikeshedding.

What I'm worried more about is how we semi-duplicate this bli_refcount
decrement vs xfs_buf_item_relse in xfs_trans_brelse, but use the
xfs_buf_item_dirty (aka XFS_BLI_DIRTY) check there instead.


It seems like the proper fix might be to:

 - only set XFS_BLI_DIRTY in xfs_buf_item_log if we actually set
   any bits in a bitmap
 - use the XFS_BLI_DIRTY check in xfs_buf_item_unlock as well
 - kill the useless xfs_buf_item_dirty wrapper

Probably both of these aren't worth doing it for now as we'll need to
get fixes into Linus tree quickly, so:


Reviewed-by: Christoph Hellwig <hch@xxxxxx>

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