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

[Christoph Hellwig <hch@infradead.org>]

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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs