xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [RFC PATCH 1/4] xfs: introduce a new helper xfs_inobt_reada_chunk()
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Sun, 17 Nov 2013 20:24:45 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131115170325.GA16942@xxxxxxxxxxxxx>
References: <5281F509.7020105@xxxxxxxxxx> <20131115170325.GA16942@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
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

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