xfs
[Top] [All Lists]

Re: [PATCH v2 5/8] xfs: use xfs_icluster_size_fsb in xfs_bulkstat

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2 5/8] xfs: use xfs_icluster_size_fsb in xfs_bulkstat
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Fri, 13 Dec 2013 10:20:50 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131212221826.GH10988@dastard>
References: <52A96800.5080706@xxxxxxxxxx> <20131212221826.GH10988@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
On 12/13 2013 06:18 AM, Dave Chinner wrote:
> On Thu, Dec 12, 2013 at 03:38:40PM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> Use xfs_icluster_size_fsb() in xfs_bulkstat(), make the related
>> variables more meaningful and remove an unused variable nimask
>> from it.
>>
>> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
> 
> Looks fine.
> 
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> At some point we need to factor this code a bit to get rid of the
> levels of indenting it has.....
> 
>> @@ -390,12 +386,12 @@ xfs_bulkstat(
>>                              agbno = XFS_AGINO_TO_AGBNO(mp, r.ir_startino);
>>                              for (chunkidx = 0;
>>                                   chunkidx < XFS_INODES_PER_CHUNK;
>> -                                 chunkidx += nicluster,
>> -                                 agbno += nbcluster) {
>> -                                    if (xfs_inobt_maskn(chunkidx, nicluster)
>> -                                                    & ~r.ir_free)
>> +                                 chunkidx += inodes_per_cluster,
>> +                                 agbno += blks_per_cluster) {
>> +                                    if (xfs_inobt_maskn(chunkidx,
>> +                                        inodes_per_cluster) & ~r.ir_free)
>>                                              xfs_btree_reada_bufs(mp, agno,
>> -                                                    agbno, nbcluster,
>> +                                                    agbno, blks_per_cluster,
>>                                                      &xfs_inode_buf_ops);
>>                              }
>>                              blk_finish_plug(&plug);
> 
> e.g. this readahead loop could be factored into
> 
> static void
> xfs_ichunk_ra(
>       *mp, *rec, inodes_per_cluster, blks_per_cluster)
> {
>       blk_start_plug(&plug);
>       agbno = XFS_AGINO_TO_AGBNO(mp, r.ir_startino);
>       for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
>            chunkidx += inodes_per_cluster, agbno += blks_per_cluster) {
>               if (xfs_inobt_maskn(chunkidx, inodes_per_cluster) & ~r.ir_free)
>                       xfs_btree_reada_bufs(mp, agno, agbno, blks_per_cluster,
>                                            &xfs_inode_buf_ops);
>       }
>       blk_finish_plug(&plug);
> }
> 
> Doing this to all the separate parts of the bulkstat code would make
> it an awful lot easier to read and modify in future....
Yup, I have a separate patch for doing that in my current quota check working 
branch.

Originally, I also did it as the 1st patch in RFC parallel quota check:
http://www.spinics.net/lists/xfs/msg23618.html

But looks I should isolate it at xfs_itable.c if no other users for now.

Thanks,
-Jeff

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