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

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

[Jeff Liu]

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>
---
 fs/xfs/xfs_itable.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 0571012..f463382 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -209,9 +209,8 @@ xfs_bulkstat(
 	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
 	xfs_inobt_rec_incore_t	*irbufend; /* end of good irec buffer entries */
 	xfs_ino_t		lastino; /* last inode number returned */
-	int			nbcluster; /* # of blocks in a cluster */
-	int			nicluster; /* # of inodes in a cluster */
-	int			nimask;	/* mask for inode clusters */
+	int			blks_per_cluster; /* # of blocks per cluster */
+	int			inodes_per_cluster;/* # of inodes per cluster */
 	int			nirbuf;	/* size of irbuf */
 	int			rval;	/* return value error code */
 	int			tmp;	/* result value from btree calls */
@@ -243,11 +242,8 @@ xfs_bulkstat(
 	*done = 0;
 	fmterror = 0;
 	ubufp = ubuffer;
-	nicluster = mp->m_sb.sb_blocksize >= mp->m_inode_cluster_size ?
-		mp->m_sb.sb_inopblock :
-		(mp->m_inode_cluster_size >> mp->m_sb.sb_inodelog);
-	nimask = ~(nicluster - 1);
-	nbcluster = nicluster >> mp->m_sb.sb_inopblog;
+	blks_per_cluster = xfs_icluster_size_fsb(mp);
+	inodes_per_cluster = blks_per_cluster << mp->m_sb.sb_inopblog;
 	irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4);
 	if (!irbuf)
 		return ENOMEM;
@@ -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);
-- 
1.8.3.2

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

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

[Dave Chinner]

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....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Jeff Liu]

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