Re: [PATCH 1/5] xfs: introduce xfs_ialloc_blks_per_cluster

[Jeff Liu <jeff.liu@xxxxxxxxxx>]

On 12/12 2013 07:14 AM, Dave Chinner wrote:
> On Wed, Dec 11, 2013 at 09:48:01PM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> Introduce a common routine xfs_ialloc_blks_per_cluster() to calculate
>> and return the number of blocks per inode cluster.
>>
>> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
>> ---
>>  fs/xfs/xfs_ialloc.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_ialloc.h b/fs/xfs/xfs_ialloc.h
>> index a8f76a5..f4bfd32 100644
>> --- a/fs/xfs/xfs_ialloc.h
>> +++ b/fs/xfs/xfs_ialloc.h
>> @@ -37,6 +37,16 @@ struct xfs_btree_cur;
>>  #define     XFS_INODE_BIG_CLUSTER_SIZE      8192
>>  #define     XFS_INODE_CLUSTER_SIZE(mp)      (mp)->m_inode_cluster_size
>>  
>> +/* Calculate and return the number of blocks per inode cluster */
>> +static inline int
>> +xfs_ialloc_blks_per_cluster(
>> +    struct xfs_mount        *mp)
>> +{
>> +    if (mp->m_sb.sb_blocksize >= XFS_INODE_CLUSTER_SIZE(mp))
>> +            return 1;
>> +    return XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_blocklog;
>> +}
>
> I'd avoid using the XFS_INODE_CLUSTER_SIZE() macro - it's just a
> useless wrapper around mp->m_inode_cluster_size. Hence if you are
> cleaning this code up, the first thing I'd do is remove the macro.
> 
> /* Calculate and return the number of blocks per inode cluster */
> static inline int
> xfs_ialloc_blks_per_cluster(
>       struct xfs_mount        *mp)
> {
>       if (mp->m_sb.sb_blocksize >= mp->m_inode_cluster_size)
>               return 1;
>       return mp->m_inode_cluster_size >> mp->m_sb.sb_blocklog;
> }
> 
> Is much less shouty, and just as easy to read ;)
Yup, yup! I also considered to just remove this macros as 
mp->m_inode_cluster_size
is quite understood and actually, we avoid this macros in xfs_inobp_check(). :)
But that means I need to remove it from xlog_recover_buffer_pass2() and fix it's
comments in this patch as well.

Thanks,
-Jeff