[PATCH v2 03/10] xfs: consolidate xfs_inumbers

[PATCH v2 03/10] xfs: consolidate xfs_inumbers

[Jeff Liu]

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

To fetch the file system number tables, we currently just ignore the
errors and proceed to loop over the next AG or bump agino to the next
chunk in case of btree operations failed, that is not properly because
those errors might hint us potential file system problems.

This patch rework xfs_inumbers() to handle the btree operation errors
as well as the loop conditions.  Also, add pre-checkups for the given
inode, we can save alloc/free the format buffer once against an invalid
inode number.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_itable.c | 159 ++++++++++++++++++++++------------------------------
 1 file changed, 67 insertions(+), 92 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index f34e95a..2025bd0 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -559,12 +559,12 @@ xfs_bulkstat_single(
 int
 xfs_inumbers_fmt(
 	void			__user *ubuffer, /* buffer to write to */
-	const xfs_inogrp_t	*buffer,	/* buffer to read from */
+	const struct xfs_inogrp	*buffer,	/* buffer to read from */
 	long			count,		/* # of elements to read */
 	long			*written)	/* # of bytes written */
 {
 	if (copy_to_user(ubuffer, buffer, count * sizeof(*buffer)))
-		return -EFAULT;
+		return XFS_ERROR(EFAULT);
 	*written = count * sizeof(*buffer);
 	return 0;
 }
@@ -574,126 +574,101 @@ xfs_inumbers_fmt(
  */
 int					/* error status */
 xfs_inumbers(
-	xfs_mount_t	*mp,		/* mount point for filesystem */
-	xfs_ino_t	*lastino,	/* last inode returned */
-	int		*count,		/* size of buffer/count returned */
-	void		__user *ubuffer,/* buffer with inode descriptions */
-	inumbers_fmt_pf	formatter)
+	struct xfs_mount	*mp,/* mount point for filesystem */
+	xfs_ino_t		*lastino,/* last inode returned */
+	int			*count,/* size of buffer/count returned */
+	void			__user *ubuffer,/* buffer with inode desc */
+	inumbers_fmt_pf		formatter)
 {
-	xfs_buf_t	*agbp;
-	xfs_agino_t	agino;
-	xfs_agnumber_t	agno;
-	int		bcount;
-	xfs_inogrp_t	*buffer;
-	int		bufidx;
-	xfs_btree_cur_t	*cur;
-	int		error;
-	xfs_inobt_rec_incore_t r;
-	int		i;
-	xfs_ino_t	ino;
-	int		left;
-	int		tmp;
-
-	ino = (xfs_ino_t)*lastino;
-	agno = XFS_INO_TO_AGNO(mp, ino);
-	agino = XFS_INO_TO_AGINO(mp, ino);
-	left = *count;
+	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, *lastino);
+	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, *lastino);
+	struct xfs_btree_cur	*cur = NULL;
+	struct xfs_buf		*agbp = NULL;
+	struct xfs_inogrp	*buffer;
+	int			left = *count;
+	int			bcount;
+	int			bufidx;
+	int			error;
+
 	*count = 0;
+	if (agno >= mp->m_sb.sb_agcount ||
+	    *lastino != XFS_AGINO_TO_INO(mp, agno, agino))
+		return 0;
+
 	bcount = MIN(left, (int)(PAGE_SIZE / sizeof(*buffer)));
 	buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP);
-	error = bufidx = 0;
-	cur = NULL;
-	agbp = NULL;
-	while (left > 0 && agno < mp->m_sb.sb_agcount) {
-		if (agbp == NULL) {
+	bufidx = error = 0;
+	do {
+		struct xfs_inobt_rec_incore	r;
+		int				stat;
+
+		if (!agbp) {
 			error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
-			if (error) {
-				/*
-				 * If we can't read the AGI of this ag,
-				 * then just skip to the next one.
-				 */
-				ASSERT(cur == NULL);
-				agbp = NULL;
-				agno++;
-				agino = 0;
-				continue;
-			}
+			if (error)
+				break;
 			cur = xfs_inobt_init_cursor(mp, NULL, agbp, agno);
 			error = xfs_inobt_lookup(cur, agino, XFS_LOOKUP_GE,
-						 &tmp);
-			if (error) {
-				xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
-				cur = NULL;
-				xfs_buf_relse(agbp);
-				agbp = NULL;
-				/*
-				 * Move up the last inode in the current
-				 * chunk.  The lookup_ge will always get
-				 * us the first inode in the next chunk.
-				 */
-				agino += XFS_INODES_PER_CHUNK - 1;
-				continue;
-			}
-		}
-		error = xfs_inobt_get_rec(cur, &r, &i);
-		if (error || i == 0) {
-			xfs_buf_relse(agbp);
-			agbp = NULL;
-			xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
-			cur = NULL;
-			agno++;
-			agino = 0;
-			continue;
+						 &stat);
+			if (error)
+				break;
+			if (!stat)
+				goto next_ag;
 		}
+
+		error = xfs_inobt_get_rec(cur, &r, &stat);
+		if (error || !stat)
+			break;
+
 		agino = r.ir_startino + XFS_INODES_PER_CHUNK - 1;
 		buffer[bufidx].xi_startino =
 			XFS_AGINO_TO_INO(mp, agno, r.ir_startino);
 		buffer[bufidx].xi_alloccount =
 			XFS_INODES_PER_CHUNK - r.ir_freecount;
 		buffer[bufidx].xi_allocmask = ~r.ir_free;
-		bufidx++;
-		left--;
-		if (bufidx == bcount) {
-			long written;
-			if (formatter(ubuffer, buffer, bufidx, &written)) {
-				error = XFS_ERROR(EFAULT);
+		if (++bufidx == bcount) {
+			long	written;
+
+			error = formatter(ubuffer, buffer, bufidx, &written);
+			if (error)
 				break;
-			}
 			ubuffer += written;
 			*count += bufidx;
 			bufidx = 0;
 		}
-		if (left) {
-			error = xfs_btree_increment(cur, 0, &tmp);
-			if (error) {
-				xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
-				cur = NULL;
-				xfs_buf_relse(agbp);
-				agbp = NULL;
-				/*
-				 * The agino value has already been bumped.
-				 * Just try to skip up to it.
-				 */
-				agino += XFS_INODES_PER_CHUNK;
-				continue;
-			}
-		}
-	}
+		if (!--left)
+			break;
+
+		error = xfs_btree_increment(cur, 0, &stat);
+		if (error)
+			break;
+		if (stat)
+			continue;
+
+next_ag:
+		xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
+		cur = NULL;
+		xfs_buf_relse(agbp);
+		agbp = NULL;
+		agino = 0;
+	} while (++agno < mp->m_sb.sb_agcount);
+
 	if (!error) {
 		if (bufidx) {
-			long written;
-			if (formatter(ubuffer, buffer, bufidx, &written))
-				error = XFS_ERROR(EFAULT);
-			else
+			long	written;
+
+			error = formatter(ubuffer, buffer, bufidx, &written);
+			if (!error)
 				*count += bufidx;
 		}
 		*lastino = XFS_AGINO_TO_INO(mp, agno, agino);
 	}
+
 	kmem_free(buffer);
 	if (cur)
 		xfs_btree_del_cursor(cur, (error ? XFS_BTREE_ERROR :
 					   XFS_BTREE_NOERROR));
 	if (agbp)
 		xfs_buf_relse(agbp);
+
 	return error;
 }
-- 
1.8.3.2

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

Re: [PATCH v2 03/10] xfs: consolidate xfs_inumbers

[Christoph Hellwig]

On Fri, Apr 18, 2014 at 08:58:26AM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> To fetch the file system number tables, we currently just ignore the
> errors and proceed to loop over the next AG or bump agino to the next
> chunk in case of btree operations failed, that is not properly because
> those errors might hint us potential file system problems.
> 
> This patch rework xfs_inumbers() to handle the btree operation errors
> as well as the loop conditions.  Also, add pre-checkups for the given
> inode, we can save alloc/free the format buffer once against an invalid
> inode number.

The patch looks mostly good to me, but I really think it should be
split into two patches: one to do the formatting changes and code
consolidation, and then one that does the actual logic changes for
better error handling.  It's not easy to understand and verify with
these two different changes combined.

>  xfs_inumbers_fmt(
>  	void			__user *ubuffer, /* buffer to write to */
> -	const xfs_inogrp_t	*buffer,	/* buffer to read from */
> +	const struct xfs_inogrp	*buffer,	/* buffer to read from */
>  	long			count,		/* # of elements to read */
>  	long			*written)	/* # of bytes written */
>  {
>  	if (copy_to_user(ubuffer, buffer, count * sizeof(*buffer)))
> -		return -EFAULT;
> +		return XFS_ERROR(EFAULT);

xfs_inumbers_fmt_compat will need the same treatment.

>  	*count = 0;
> +	if (agno >= mp->m_sb.sb_agcount ||
> +	    *lastino != XFS_AGINO_TO_INO(mp, agno, agino))
> +		return 0;

Where is the lastino check coming from?

>  	buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP);
> +	bufidx = error = 0;

Why not initialize bufidx and error at declaration time?

> +		error = xfs_inobt_get_rec(cur, &r, &stat);
> +		if (error || !stat)
> +			break;

The old code moved on to the next AG here, why has this changed?

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

Re: [PATCH v2 03/10] xfs: consolidate xfs_inumbers

[Jeff Liu]

Hi Christoph,

Sorry for my too late response! I missed your response somehow.

On 04/23 2014 23:36 PM, Christoph Hellwig wrote:
> On Fri, Apr 18, 2014 at 08:58:26AM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> To fetch the file system number tables, we currently just ignore the
>> errors and proceed to loop over the next AG or bump agino to the next
>> chunk in case of btree operations failed, that is not properly because
>> those errors might hint us potential file system problems.
>>
>> This patch rework xfs_inumbers() to handle the btree operation errors
>> as well as the loop conditions.  Also, add pre-checkups for the given
>> inode, we can save alloc/free the format buffer once against an invalid
>> inode number.
> 
> The patch looks mostly good to me, but I really think it should be
> split into two patches: one to do the formatting changes and code
> consolidation, and then one that does the actual logic changes for
> better error handling.  It's not easy to understand and verify with
> these two different changes combined.
> 
>>  xfs_inumbers_fmt(
>>  	void			__user *ubuffer, /* buffer to write to */
>> -	const xfs_inogrp_t	*buffer,	/* buffer to read from */
>> +	const struct xfs_inogrp	*buffer,	/* buffer to read from */
>>  	long			count,		/* # of elements to read */
>>  	long			*written)	/* # of bytes written */
>>  {
>>  	if (copy_to_user(ubuffer, buffer, count * sizeof(*buffer)))
>> -		return -EFAULT;
>> +		return XFS_ERROR(EFAULT);
> 
> xfs_inumbers_fmt_compat will need the same treatment.

Yup.

> 
>>  	*count = 0;
>> +	if (agno >= mp->m_sb.sb_agcount ||
>> +	    *lastino != XFS_AGINO_TO_INO(mp, agno, agino))
>> +		return 0;
> 
> Where is the lastino check coming from?

Originally, I copied this check up from xfs_bulkstat(), it seems that it
is a redundant check as agno >= mp->m_sb.sb_agcount can handle an invalid
lastino input?  At least, I can not think out a case to make it happen for
now.

> 
>>  	buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP);
>> +	bufidx = error = 0;
> 
> Why not initialize bufidx and error at declaration time?

So they should be initialized there.

> 
>> +		error = xfs_inobt_get_rec(cur, &r, &stat);
>> +		if (error || !stat)
>> +			break;
> 
> The old code moved on to the next AG here, why has this changed?

Ah, it should be the old way.


Thanks,
-Jeff

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