xfs
[Top] [All Lists]

Re: [PATCH v3 3/5] xfs: fix segment in xfs_buf_item_format_segment

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH v3 3/5] xfs: fix segment in xfs_buf_item_format_segment
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sat, 8 Dec 2012 07:29:21 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121204224938.820985192@xxxxxxx>
References: <20121204231801.982422341@xxxxxxx> <20121204224938.820985192@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Dec 04, 2012 at 05:18:04PM -0600, Mark Tinguely wrote:
> Not every segment in a multi-segment buffer is dirty in a
> transaction and they will not be outputted. The assert in
> xfs_buf_item_format_segment() that checks for the at least
> one chunk of data in the segment to be used is not necessary
> true for multi-segmented buffers.
> 
> Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> ---
>  fs/xfs/xfs_buf_item.c |   20 +++++++++++++++-----
>  1 file 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
> @@ -287,6 +287,17 @@ xfs_buf_item_format_segment(
>        */
>       base_size = offsetof(struct xfs_buf_log_format, blf_data_map) +
>                       (blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
> +
> +     nvecs = 0;
> +     first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);
> +     if (!(bip->bli_flags & XFS_BLI_STALE) && first_bit == -1) {
> +             /*
> +              * If the map is not be dirty in the transaction, mark
> +              * the size as zero and do not advance the vector pointer.
> +              */
> +             goto out;
> +     }
> +

I don't really like the style of this check.  What's the problem of
doing it this way:

  1) fill out the first vecp
  2) do the stale check as-is
  3) handle the the first_bit == -1 case  ala:

        if (first_bit == -1) {
                blfp->blf_size = 0;
                return vecp;
        }

  4) only then increcement vecp

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