xfs
[Top] [All Lists]

Re: [PATCHv2 5/5] xfs: kill off l_sectbb_mask

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCHv2 5/5] xfs: kill off l_sectbb_mask
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Sat, 17 Apr 2010 11:34:54 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <201004162054.o3GKs54f025204@xxxxxxxxxxxxxxxxxxxxxx>
References: <201004162054.o3GKs54f025204@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Fri, Apr 16, 2010 at 03:54:05PM -0500, Alex Elder wrote:
> There remains only one user of the l_sectbb_mask field in the log
> structure.  Just kill it off and compute the mask where needed from
> the power-of-2 sector size.
> 
> (Only update from last post is to accomodate the changes in the
> previous patch in the series.)
> 
> Signed-off-by: Alex Elder <aelder@xxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>       xfs_buf_t       *bp)
>  {
> +     xfs_daddr_t     offset;
>       xfs_caddr_t     ptr;
>  
> -     if (log->l_sectBBsize == 1)
> -             return XFS_BUF_PTR(bp);
> +     offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
> +     ptr = XFS_BUF_PTR(bp) + BBTOB(offset);
> +
> +     ASSERT(ptr + BBTOB(nbblks) <= XFS_BUF_PTR(bp) + XFS_BUF_SIZE(bp));

The ASSERT is more obfuscated than it needs to be. It's not obvious
that it is bounds checking offset+nbblks. i.e. I prefer the original
format like:

        ASSERT(BBTOB(offset + nbblks) <= XFS_BUF_SIZE(bp));

but otherwise it looks good. Anyway, minor nitpick, so:

Reviewed-by: Dave Chinner <david@xxxxxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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