[RFC PATCH 2/4] xfs: introduce a new helper xfs_inobt_lookup_grab_ichunk()

[RFC PATCH 2/4] xfs: introduce a new helper xfs_inobt_lookup_grab_ichunk()

[Jeff Liu]

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

introduce a new helper xfs_inobt_lookup_grab_ichunk().  This function
can be used to lookup the inode chunk that the given inode lives in,
and then get the record if we found that chunk.  If the inode was not
the last in the chunk and there are some left allocated, update the
data for the pointed-to record.

Refactor xfs_bulkstat() to make use of the founction.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_ialloc.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_ialloc.h |    9 +++++++++
 fs/xfs/xfs_itable.c |   45 +++++++++--------------------------------
 3 files changed, 73 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 86436e7..e34e16f 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -111,6 +111,61 @@ xfs_inobt_get_rec(
 }
 
 /*
+ * Lookup the inode chunk that the given inode lives in and then get
+ * the record if we found the chunk.  If the inode was not the last
+ * in the chunk and there are some left allocated, update the data
+ * for the pointed-to record.
+ */
+int
+xfs_inobt_lookup_grab_ichunk(
+	struct xfs_btree_cur		*cur,	/* btree cursor */
+	xfs_agino_t			agino,	/* starting inode of chunk */
+	struct xfs_inobt_rec_incore	*irec,	/* btree record */
+	int				*stat)	/* success/failure */
+{
+	int				idx;	/* Index into inode chunk */
+	int				error;
+
+	/* Lookup the inode chunk that this inode lives in */
+	error = xfs_inobt_lookup(cur, agino, XFS_LOOKUP_LE, stat);
+	if (error || !*stat)
+		return error;
+
+	/* Get the record, should always work */
+	error = xfs_inobt_get_rec(cur, irec, stat);
+	ASSERT(!error && *stat);
+	if (error || !*stat)
+		return error;
+
+	*stat = 0;
+	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
+		return error;
+
+	idx = agino - irec->ir_startino + 1;
+	/*
+	 * We got a right chunk and there are some left inodes allocated at it.
+	 */
+	if (idx < XFS_INODES_PER_CHUNK &&
+	    (xfs_inobt_maskn(idx, XFS_INODES_PER_CHUNK - idx) & ~irec->ir_free)) {
+		int	i;
+
+		/*
+		 * Grab the chunk record.  Mark all the uninteresting inodes free
+		 * (because they're before our start point).
+		 */
+		for (i = 0; i < idx; i++) {
+			if (XFS_INOBT_MASK(i) & ~irec->ir_free)
+				irec->ir_freecount++;
+		}
+
+		irec->ir_free |= xfs_inobt_maskn(0, idx);
+		*stat = 1;
+	}
+
+	return error;
+}
+
+/*
  * 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.
diff --git a/fs/xfs/xfs_ialloc.h b/fs/xfs/xfs_ialloc.h
index 65b2bef..4b032d6 100644
--- a/fs/xfs/xfs_ialloc.h
+++ b/fs/xfs/xfs_ialloc.h
@@ -166,4 +166,13 @@ int xfs_ialloc_inode_init(struct xfs_mount *mp, struct xfs_trans *tp,
 void xfs_inobt_reada_chunk(struct xfs_mount *mp, xfs_agnumber_t agno,
 			   struct xfs_inobt_rec_incore *irec);
 
+/*
+ * Lookup the inode chunk that the given inode lives in and then get
+ * the record if we found the chunk.  If the inode was not the last
+ * in the chunk and there are some left allocated, update the data
+ * for the pointed-to record.
+ */
+int xfs_inobt_lookup_grab_ichunk(struct xfs_btree_cur *cur, xfs_agino_t agino,
+				 struct xfs_inobt_rec_incore *irec, int *stat);
+
 #endif	/* __XFS_IALLOC_H__ */
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 9ac69cd..3611fa3 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -274,50 +274,23 @@ xfs_bulkstat(
 		 * we need to get the remainder of the chunk we're in.
 		 */
 		if (agino > 0) {
-			xfs_inobt_rec_incore_t r;
-
-			/*
-			 * Lookup the inode chunk that this inode lives in.
-			 */
-			error = xfs_inobt_lookup(cur, agino, XFS_LOOKUP_LE,
-						 &tmp);
-			if (!error &&	/* no I/O error */
-			    tmp &&	/* lookup succeeded */
-					/* got the record, should always work */
-			    !(error = xfs_inobt_get_rec(cur, &r, &i)) &&
-			    i == 1 &&
-					/* this is the right chunk */
-			    agino < r.ir_startino + XFS_INODES_PER_CHUNK &&
-					/* lastino was not last in chunk */
-			    (chunkidx = agino - r.ir_startino + 1) <
-				    XFS_INODES_PER_CHUNK &&
-					/* there are some left allocated */
-			    xfs_inobt_maskn(chunkidx,
-				    XFS_INODES_PER_CHUNK - chunkidx) &
-				    ~r.ir_free) {
+			xfs_inobt_rec_incore_t	r;
+			error = xfs_inobt_lookup_grab_ichunk(cur, agino, &r,
+							     &tmp);
+			if (error || !tmp) {
 				/*
-				 * Grab the chunk record.  Mark all the
-				 * uninteresting inodes (because they're
-				 * before our start point) free.
+				 * If any of those tests failed, bump the
+				 * inode number (just in case).
 				 */
-				for (i = 0; i < chunkidx; i++) {
-					if (XFS_INOBT_MASK(i) & ~r.ir_free)
-						r.ir_freecount++;
-				}
-				r.ir_free |= xfs_inobt_maskn(0, chunkidx);
+				agino++;
+				icount = 0;
+			} else {
 				irbp->ir_startino = r.ir_startino;
 				irbp->ir_freecount = r.ir_freecount;
 				irbp->ir_free = r.ir_free;
 				irbp++;
 				agino = r.ir_startino + XFS_INODES_PER_CHUNK;
 				icount = XFS_INODES_PER_CHUNK - r.ir_freecount;
-			} else {
-				/*
-				 * If any of those tests failed, bump the
-				 * inode number (just in case).
-				 */
-				agino++;
-				icount = 0;
 			}
 			/*
 			 * In any case, increment to the next record.
-- 
1.7.9.5

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

Re: [RFC PATCH 2/4] xfs: introduce a new helper xfs_inobt_lookup_grab_ichunk()

[Christoph Hellwig]

> Refactor xfs_bulkstat() to make use of the founction.

                                         ... function.

> +int
> +xfs_inobt_lookup_grab_ichunk(
> +	struct xfs_btree_cur		*cur,	/* btree cursor */
> +	xfs_agino_t			agino,	/* starting inode of chunk */
> +	struct xfs_inobt_rec_incore	*irec,	/* btree record */
> +	int				*stat)	/* success/failure */
> +{
> +	int				idx;	/* Index into inode chunk */
> +	int				error;
> +
> +	/* Lookup the inode chunk that this inode lives in */
> +	error = xfs_inobt_lookup(cur, agino, XFS_LOOKUP_LE, stat);
> +	if (error || !*stat)
> +		return error;
> +
> +	/* Get the record, should always work */
> +	error = xfs_inobt_get_rec(cur, irec, stat);
> +	ASSERT(!error && *stat);

If it wails it's a corruption error, so this shouldn't be an assert,
but rather an XFS_WANT_CORRUPTED_GOTO/RETURN

> +	if (error || !*stat)
> +		return error;
> +
> +	*stat = 0;

Btw, I don't think we need to pass around the status pointer here,
the caller doesn't care about the internal lookup status, just if
we succeeded or failed.

> +	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
> +		return error;

error will usually be zero here, shouldn't we return a real error?

> +	idx = agino - irec->ir_startino + 1;
> +	/*
> +	 * We got a right chunk and there are some left inodes allocated at it.
> +	 */
> +	if (idx < XFS_INODES_PER_CHUNK &&
> +	    (xfs_inobt_maskn(idx, XFS_INODES_PER_CHUNK - idx) & ~irec->ir_free)) {
> +		int	i;
> +
> +		/*
> +		 * Grab the chunk record.  Mark all the uninteresting inodes free
> +		 * (because they're before our start point).
> +		 */
> +		for (i = 0; i < idx; i++) {
> +			if (XFS_INOBT_MASK(i) & ~irec->ir_free)
> +				irec->ir_freecount++;
> +		}
> +
> +		irec->ir_free |= xfs_inobt_maskn(0, idx);
> +		*stat = 1;
> +	}


At this point the function is so bulkstate specific that it should stay
in xfs_itable.c and have a name more like xfs_bulkstate_grab_ichunk.

I still like factoring it out though, thanks for the work!

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

Re: [RFC PATCH 2/4] xfs: introduce a new helper xfs_inobt_lookup_grab_ichunk()

[Jeff Liu]

On 11/16 2013 01:13 AM, Christoph Hellwig wrote:
>> Refactor xfs_bulkstat() to make use of the founction.
> 
>                                          ... function.
> 
>> +int
>> +xfs_inobt_lookup_grab_ichunk(
>> +	struct xfs_btree_cur		*cur,	/* btree cursor */
>> +	xfs_agino_t			agino,	/* starting inode of chunk */
>> +	struct xfs_inobt_rec_incore	*irec,	/* btree record */
>> +	int				*stat)	/* success/failure */
>> +{
>> +	int				idx;	/* Index into inode chunk */
>> +	int				error;
>> +
>> +	/* Lookup the inode chunk that this inode lives in */
>> +	error = xfs_inobt_lookup(cur, agino, XFS_LOOKUP_LE, stat);
>> +	if (error || !*stat)
>> +		return error;
>> +
>> +	/* Get the record, should always work */
>> +	error = xfs_inobt_get_rec(cur, irec, stat);
>> +	ASSERT(!error && *stat);
> 
> If it wails it's a corruption error, so this shouldn't be an assert,
> but rather an XFS_WANT_CORRUPTED_GOTO/RETURN
Indeed.
> 
>> +	if (error || !*stat)
>> +		return error;
>> +
>> +	*stat = 0;
> 
> Btw, I don't think we need to pass around the status pointer here,
> the caller doesn't care about the internal lookup status, just if
> we succeeded or failed.
Yes, I'll fix it in next version.
> 
>> +	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
>> +		return error;
> 
> error will usually be zero here, shouldn't we return a real error?
In xfs_imap_lookup(), it return EINVAL if the given inode is not inside the
returned record, looks we should follow up the same rule here.

> 
>> +	idx = agino - irec->ir_startino + 1;
>> +	/*
>> +	 * We got a right chunk and there are some left inodes allocated at it.
>> +	 */
>> +	if (idx < XFS_INODES_PER_CHUNK &&
>> +	    (xfs_inobt_maskn(idx, XFS_INODES_PER_CHUNK - idx) & ~irec->ir_free)) {
>> +		int	i;
>> +
>> +		/*
>> +		 * Grab the chunk record.  Mark all the uninteresting inodes free
>> +		 * (because they're before our start point).
>> +		 */
>> +		for (i = 0; i < idx; i++) {
>> +			if (XFS_INOBT_MASK(i) & ~irec->ir_free)
>> +				irec->ir_freecount++;
>> +		}
>> +
>> +		irec->ir_free |= xfs_inobt_maskn(0, idx);
>> +		*stat = 1;
>> +	}
> 
> 
> At this point the function is so bulkstate specific that it should stay
> in xfs_itable.c and have a name more like xfs_bulkstate_grab_ichunk.
Nice point. :)

Thanks,
-Jeff

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