xfs
[Top] [All Lists]

Re: [PATCH 18/27] xfs: avoid usage of struct xfs_dir2_data

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 18/27] xfs: avoid usage of struct xfs_dir2_data
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 6 Jul 2011 13:02:28 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110701094606.003170984@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110701094321.936534538@xxxxxxxxxxxxxxxxxxxxxx> <20110701094606.003170984@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Fri, Jul 01, 2011 at 05:43:39AM -0400, Christoph Hellwig wrote:
> In most places we can simply pass around and use the struct xfs_dir2_data_hdr,
> which is the first and most important member of struct xfs_dir2_data instead
> of the full structure.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Only a couple of minor things.

....

> @@ -251,12 +258,13 @@ xfs_dir2_data_freeinsert(
>       xfs_dir2_data_free_t    new;            /* new bestfree entry */
>  
>  #ifdef __KERNEL__
> -     ASSERT(be32_to_cpu(d->hdr.magic) == XFS_DIR2_DATA_MAGIC ||
> -            be32_to_cpu(d->hdr.magic) == XFS_DIR2_BLOCK_MAGIC);
> +     ASSERT(be32_to_cpu(hdr->magic) == XFS_DIR2_DATA_MAGIC ||
> +            be32_to_cpu(hdr->magic) == XFS_DIR2_BLOCK_MAGIC);
>  #endif

You kill the ifdef __KERNEL__ there.

> @@ -286,36 +294,36 @@ xfs_dir2_data_freeinsert(
>   */
>  STATIC void
>  xfs_dir2_data_freeremove(
> -     xfs_dir2_data_t         *d,             /* data block pointer */
> +     xfs_dir2_data_hdr_t     *hdr,           /* data block header */
>       xfs_dir2_data_free_t    *dfp,           /* bestfree entry pointer */
>       int                     *loghead)       /* out: log data header */
>  {
>  #ifdef __KERNEL__
> -     ASSERT(be32_to_cpu(d->hdr.magic) == XFS_DIR2_DATA_MAGIC ||
> -            be32_to_cpu(d->hdr.magic) == XFS_DIR2_BLOCK_MAGIC);
> +     ASSERT(be32_to_cpu(hdr->magic) == XFS_DIR2_DATA_MAGIC ||
> +            be32_to_cpu(hdr->magic) == XFS_DIR2_BLOCK_MAGIC);
>  #endif

And there.

> @@ -335,23 +344,23 @@ xfs_dir2_data_freescan(
>       char                    *p;             /* current entry pointer */
>  
>  #ifdef __KERNEL__
> -     ASSERT(be32_to_cpu(d->hdr.magic) == XFS_DIR2_DATA_MAGIC ||
> -            be32_to_cpu(d->hdr.magic) == XFS_DIR2_BLOCK_MAGIC);
> +     ASSERT(hdr->magic == cpu_to_be32(XFS_DIR2_DATA_MAGIC) ||
> +            hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC));
>  #endif

I'll stop commenting on this now ;)

However, I have noticed that you've converted some of the magic
number compares to cpu_to_be32(XFS_DIR2_DATA_MAGIC) form and not
others. I'm not so concerned about the ASSERT()s, but some of the
real runtime checks are touched but not then not changed around.
Anyway, this can probably be done later as a separate cleanup.

>                       if (!needscan) {
> -                             xfs_dir2_data_freeremove(d, dfp, needlogp);
> -                             (void)xfs_dir2_data_freeinsert(d, newdup,
> +                             xfs_dir2_data_freeremove(hdr, dfp, needlogp);
> +                             (void)xfs_dir2_data_freeinsert(hdr, newdup,
>                                       needlogp);
> -                             (void)xfs_dir2_data_freeinsert(d, newdup2,
> +                             (void)xfs_dir2_data_freeinsert(hdr, newdup2,
>                                       needlogp);
>                       }
>               }

Kill the (void) casts?

Otherwise looks good.

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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