xfs
[Top] [All Lists]

Re: [RFC PATCH 1/4] xfs: introduce a new helper xfs_inobt_reada_chunk()

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [RFC PATCH 1/4] xfs: introduce a new helper xfs_inobt_reada_chunk()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 15 Nov 2013 09:03:25 -0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5281F509.7020105@xxxxxxxxxx>
References: <5281F509.7020105@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
I like the factoring, but it could go a littler further.  Comments
below:

>  /*
> + * Loop over all clusters in a chunk for a given incore inode
> + * allocation btree record.  Do a readahead if there are any
> + * allocated inodes in that cluster.
> + */

Try to use the full 80 lines available to you for comment.

> +     xfs_agblock_t                   agbno = XFS_AGINO_TO_AGBNO(mp,
> +                                             irec->ir_startino);
> +     int                             nicluster, nbcluster;
> +     int                             chunkidx;
> +
> +     nicluster = mp->m_sb.sb_blocksize >= XFS_INODE_CLUSTER_SIZE(mp) ?
> +                 mp->m_sb.sb_inopblock :
> +                 (XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog);
> +     nbcluster = nicluster >> mp->m_sb.sb_inopblog;

I'd prefer to factor this out even further. xfs_ialloc_inode_init and
xfs_ifree_cluster already have two pieces of code that calculate these
two (with more readable names) and an additional nuber buffers counter
we won't need here, it might make most sense to factor that into a
single common helper.

> +/*
> + * Lookup clusters in inode chunk for a given incore inobt record,
> + * do readahead if there are any allocated inodes in that cluster.
> + */
> +void xfs_inobt_reada_chunk(struct xfs_mount *mp, xfs_agnumber_t agno,
> +                        struct xfs_inobt_rec_incore *irec);

No need to duplicate the comment in the header.

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