[PATCH v2 05/10] xfs: fix error handling in xfs_bulkstat

[PATCH v2 05/10] xfs: fix error handling in xfs_bulkstat

[Jeff Liu]

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

We should not ignore the btree operation errors in xfs_bulkstat()
but to propagate them if any.  This patch fix two places in this
function and the remains things can be fixed with code refactoring
thereafter.

Moreover, this fix also get rid of the redundant user buffer count
pre-checkups as it has already been validated in upper callers.

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

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 62cf2f8..cdbcf4e 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -230,9 +230,6 @@ xfs_bulkstat(
 		*ubcountp = 0;
 		return 0;
 	}
-	if (!ubcountp || *ubcountp <= 0) {
-		return EINVAL;
-	}
 	ubcount = *ubcountp; /* statstruct's */
 	ubleft = ubcount * statstruct_size; /* bytes */
 	*ubcountp = ubelem = 0;
@@ -255,14 +252,8 @@ xfs_bulkstat(
 	while (XFS_BULKSTAT_UBLEFT(ubleft) && agno < mp->m_sb.sb_agcount) {
 		cond_resched();
 		error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
-		if (error) {
-			/*
-			 * Skip this allocation group and go to the next one.
-			 */
-			agno++;
-			agino = 0;
-			continue;
-		}
+		if (error)
+			break;
 		agi = XFS_BUF_TO_AGI(agbp);
 		/*
 		 * Allocate and initialize a btree cursor for ialloc btree.
@@ -333,34 +324,15 @@ xfs_bulkstat(
 			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &tmp);
 			icount = 0;
 		}
+		if (error)
+			break;
+
 		/*
 		 * Loop through inode btree records in this ag,
 		 * until we run out of inodes or space in the buffer.
 		 */
 		while (irbp < irbufend && icount < ubcount) {
-			xfs_inobt_rec_incore_t r;
-
-			/*
-			 * Loop as long as we're unable to read the
-			 * inode btree.
-			 */
-			while (error) {
-				agino += XFS_INODES_PER_CHUNK;
-				if (XFS_AGINO_TO_AGBNO(mp, agino) >=
-						be32_to_cpu(agi->agi_length))
-					break;
-				error = xfs_inobt_lookup(cur, agino,
-							 XFS_LOOKUP_GE, &tmp);
-				cond_resched();
-			}
-			/*
-			 * If ran off the end of the ag either with an error,
-			 * or the normal way, set end and stop collecting.
-			 */
-			if (error) {
-				end_of_ag = 1;
-				break;
-			}
+			struct xfs_inobt_rec_incore	r;
 
 			error = xfs_inobt_get_rec(cur, &r, &i);
 			if (error || i == 0) {
-- 
1.8.3.2

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

Re: [PATCH v2 05/10] xfs: fix error handling in xfs_bulkstat

[Christoph Hellwig]

> Moreover, this fix also get rid of the redundant user buffer count
> pre-checkups as it has already been validated in upper callers.

> -	if (!ubcountp || *ubcountp <= 0) {
> -		return EINVAL;
> -	}

Probably better to have this as a separate patch.

> -			/*
> -			 * Loop as long as we're unable to read the
> -			 * inode btree.
> -			 */
> -			while (error) {
> -				agino += XFS_INODES_PER_CHUNK;
> -				if (XFS_AGINO_TO_AGBNO(mp, agino) >=
> -						be32_to_cpu(agi->agi_length))
> -					break;
> -				error = xfs_inobt_lookup(cur, agino,
> -							 XFS_LOOKUP_GE, &tmp);
> -				cond_resched();
> -			}

This code goes back to 1995, but I still can't see how it would make
sense.  I think we should get rid of this, but I'd also love to have
Dave and Eric double check it as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH v2 05/10] xfs: fix error handling in xfs_bulkstat

[Jeff Liu]

On 04/25 2014 14:48 PM, Christoph Hellwig wrote:
>> Moreover, this fix also get rid of the redundant user buffer count
>> pre-checkups as it has already been validated in upper callers.
> 
>> -	if (!ubcountp || *ubcountp <= 0) {
>> -		return EINVAL;
>> -	}
> 
> Probably better to have this as a separate patch.

Sometimes, I'd to put such kind of trivial fixes into a relative effective patch
if possible. But from another point of view, yep, have it as a separate patch would
make it more convenient for reviewer.

> 
>> -			/*
>> -			 * Loop as long as we're unable to read the
>> -			 * inode btree.
>> -			 */
>> -			while (error) {
>> -				agino += XFS_INODES_PER_CHUNK;
>> -				if (XFS_AGINO_TO_AGBNO(mp, agino) >=
>> -						be32_to_cpu(agi->agi_length))
>> -					break;
>> -				error = xfs_inobt_lookup(cur, agino,
>> -							 XFS_LOOKUP_GE, &tmp);
>> -				cond_resched();
>> -			}
> 
> This code goes back to 1995, but I still can't see how it would make
> sense.  I think we should get rid of this, but I'd also love to have
> Dave and Eric double check it as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks,
-Jeff

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

Re: [PATCH v2 05/10] xfs: fix error handling in xfs_bulkstat

[Dave Chinner]

On Thu, Apr 24, 2014 at 11:48:15PM -0700, Christoph Hellwig wrote:
> > Moreover, this fix also get rid of the redundant user buffer count
> > pre-checkups as it has already been validated in upper callers.
> 
> > -	if (!ubcountp || *ubcountp <= 0) {
> > -		return EINVAL;
> > -	}
> 
> Probably better to have this as a separate patch.
> 
> > -			/*
> > -			 * Loop as long as we're unable to read the
> > -			 * inode btree.
> > -			 */
> > -			while (error) {
> > -				agino += XFS_INODES_PER_CHUNK;
> > -				if (XFS_AGINO_TO_AGBNO(mp, agino) >=
> > -						be32_to_cpu(agi->agi_length))
> > -					break;
> > -				error = xfs_inobt_lookup(cur, agino,
> > -							 XFS_LOOKUP_GE, &tmp);
> > -				cond_resched();
> > -			}
> 
> This code goes back to 1995, but I still can't see how it would make
> sense.  I think we should get rid of this, but I'd also love to have
> Dave and Eric double check it as well.

I can't see it makes much sense, except for handling IO errors
that occur during something like a path failover where a retry would
then succeed. However, I think that we'd do better for userspace to
handle this problem - a short bulkstat followed by userspace retry
rather than a potential endless loop in the kernel is a much better
way to handle the problem...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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