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

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

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

Introduce a new helper xfs_inobt_reada_chunk(), it is used to loop
over all clusters in the next inode chunk, then performs readahead
if there are any allocated inodes in that cluster.

Refactor xfs_bulkstat() to make use of this function.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_ialloc.c |   29 +++++++++++++++++++++++++++++
 fs/xfs/xfs_ialloc.h |    7 +++++++
 fs/xfs/xfs_itable.c |   21 +--------------------
 3 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 14d732f..86436e7 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -111,6 +111,35 @@ xfs_inobt_get_rec(
 }
 
 /*
+ * 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.
+ */
+void
+xfs_inobt_reada_chunk(
+	struct xfs_mount		*mp,
+	xfs_agnumber_t			agno,
+	struct xfs_inobt_rec_incore	*irec)
+{
+	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;
+
+	for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
+	     chunkidx += nicluster, agbno += nbcluster) {
+		if (xfs_inobt_maskn(chunkidx, nicluster) & ~irec->ir_free)
+			xfs_btree_reada_bufs(mp, agno, agbno, nbcluster,
+					     &xfs_inode_buf_ops);
+	}
+}
+
+/*
  * Verify that the number of free inodes in the AGI is correct.
  */
 #ifdef DEBUG
diff --git a/fs/xfs/xfs_ialloc.h b/fs/xfs/xfs_ialloc.h
index a8f76a5..65b2bef 100644
--- a/fs/xfs/xfs_ialloc.h
+++ b/fs/xfs/xfs_ialloc.h
@@ -159,4 +159,11 @@ int xfs_ialloc_inode_init(struct xfs_mount *mp, struct xfs_trans *tp,
 			  xfs_agnumber_t agno, xfs_agblock_t agbno,
 			  xfs_agblock_t length, unsigned int gen);
 
+/*
+ * 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);
+
 #endif	/* __XFS_IALLOC_H__ */
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index c237ad1..9ac69cd 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -190,7 +190,6 @@ xfs_bulkstat(
 	char			__user *ubuffer, /* buffer with inode stats */
 	int			*done)	/* 1 if there are more stats to get */
 {
-	xfs_agblock_t		agbno=0;/* allocation group block number */
 	xfs_buf_t		*agbp;	/* agi header buffer */
 	xfs_agi_t		*agi;	/* agi header data */
 	xfs_agino_t		agino;	/* inode # in allocation group */
@@ -209,9 +208,6 @@ 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			nirbuf;	/* size of irbuf */
 	int			rval;	/* return value error code */
 	int			tmp;	/* result value from btree calls */
@@ -243,11 +239,6 @@ xfs_bulkstat(
 	*done = 0;
 	fmterror = 0;
 	ubufp = ubuffer;
-	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);
-	nimask = ~(nicluster - 1);
-	nbcluster = nicluster >> mp->m_sb.sb_inopblog;
 	irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4);
 	if (!irbuf)
 		return ENOMEM;
@@ -387,17 +378,7 @@ xfs_bulkstat(
 				 * inodes in that cluster.
 				 */
 				blk_start_plug(&plug);
-				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)
-						xfs_btree_reada_bufs(mp, agno,
-							agbno, nbcluster,
-							&xfs_inode_buf_ops);
-				}
+				xfs_inobt_reada_chunk(mp, agno, &r);
 				blk_finish_plug(&plug);
 				irbp->ir_startino = r.ir_startino;
 				irbp->ir_freecount = r.ir_freecount;
-- 
1.7.9.5

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

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

[Christoph Hellwig]

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.

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

> +/*
> + * 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.

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

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

[Jeff Liu]

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

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

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

[Christoph Hellwig]

> > 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)
> {
> 	....
> }

Probably fine to make it an inline.  I don't think we need the nbufs
parameter, as it requires the length to calculate, and it's a trivial
length / blks_per_cluster.

Similarly the ninodes value is trivially calculatable, so it might be
as easy as:

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_blocksize;
}


...

	blks_per_cluster = xfs_ialloc_blks_per_cluster(mp);
	nbufs = length / blks_per_cluster;
	ninodes = blks_per_cluster * mp->m_sb.sb_inopblock;

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

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

[Jeff Liu]

On 11/18 2013 07:01 PM, Christoph Hellwig wrote:

>>> 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)
>> {
>> 	....
>> }
> 
> Probably fine to make it an inline.  I don't think we need the nbufs
> parameter, as it requires the length to calculate, and it's a trivial
> length / blks_per_cluster.
> 
> Similarly the ninodes value is trivially calculatable, so it might be
> as easy as:
> 
> 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_blocksize;
> }
> 
> 
> ...
> 
> 	blks_per_cluster = xfs_ialloc_blks_per_cluster(mp);
> 	nbufs = length / blks_per_cluster;
> 	ninodes = blks_per_cluster * mp->m_sb.sb_inopblock;

Coool, your idea is better than mine.

Thanks,
-Jeff


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