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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
