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

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

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@oracle.com>
>>
>> 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@oracle.com>
> 
> Looks fine.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> 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

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