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

[Jeff Liu <jeff.liu@oracle.com>]

Thanks for so much detailed comments!

On 11/16 2013 01:03 AM, Christoph Hellwig wrote:
> 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.
Okay.
> 
>> +	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.
Yup, I also thought this can be factored out, however, I can not figure out
a meaningful function name at that time due to my poor skill...

How about if we introduce an inline helper to xfs_ialloc.h as below?

/* Helper function to extract the # of blocks/inodes/buffers hint per cluster */
static inline void
xfs_ialloc_get_cluster_hints(
	struct xfs_mount	*mp,
	int			*nblks;
	int			*ninodes;
	int			*nbufs)
{
	....
}
> 
>> +/*
>> + * 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
Ok.

Thanks,
-Jeff

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