xfs: fix error handling in xfs_bulkstat
PATCH v2 5/10 xfs: fix error handling in xfs_bulkstat
From: Jie Liu <[email protected]>
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.
--- fs/xfs/xfs_itable.c | 40 ++++++---------------------------------- 1 file changed, 6 insertions(+), 34 deletions(-)
fs/xfs/xfs_itable.c +6 -34
@@ -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 ) {
Signature · 6 lines
1.8.3.2 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
fs/xfs/xfs_itable.c xfs_bulkstat( · line 235
Quoted 6 lines
Moreover, this fix also get rid of the redundant user buffer count pre-checkups as it has already been validated in upper callers.
fs/xfs/xfs_itable.c +0
-3-if (!ubcountp || *ubcountp <=0 ) { -return EINVAL ; - }
Probably better to have this as a separate patch.
fs/xfs/xfs_itable.c xfs_bulkstat( · line 355
Quoted 13 lines
fs/xfs/xfs_itable.c +0
-13-/* -* 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.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 8 lines · Christoph Hellwig
fs/xfs/xfs_itable.c xfs_bulkstat( · line 235
Quoted 6 lines
Moreover, this fix also get rid of the redundant user buffer count pre-checkups as it has already been validated in upper callers.
fs/xfs/xfs_itable.c +0
-3-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.
Quoted 19 lines
Quoted 4 lines
fs/xfs/xfs_itable.c +0
-3-/* -* Loop as long as we're unable to read the -* inode btree. fs/xfs/xfs_itable.c xfs_bulkstat( · line 355
Quoted 10 lines
fs/xfs/xfs_itable.c +0
-10-*/ -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.
Quoted 1 line
Signed-off-by Christoph Hellwig <[email protected]>
Thanks,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 18 lines · Christoph Hellwig
fs/xfs/xfs_itable.c xfs_bulkstat( · line 235
Quoted 6 lines
Moreover, this fix also get rid of the redundant user buffer count pre-checkups as it has already been validated in upper callers.
fs/xfs/xfs_itable.c +0
-3-if (!ubcountp || *ubcountp <=0 ) { -return EINVAL ; - }Probably better to have this as a separate patch.
Quoted 9 lines
fs/xfs/xfs_itable.c +0
-9-/* -* 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 ;
Quoted 8 lines
fs/xfs/xfs_itable.c xfs_bulkstat( · line 355
Quoted 4 lines
fs/xfs/xfs_itable.c +0
-4- 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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs