xfs: consolidate xfs_inumbers
PATCH v2 3/10 xfs: consolidate xfs_inumbers
From: Jie Liu <[email protected]>
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.
--- fs/xfs/xfs_itable.c | 159 ++++++++++++++++++++++------------------------------ 1 file changed, 67 insertions(+), 92 deletions(-)
fs/xfs/xfs_itable.c +67 -92
@@ -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; }
Signature · 6 lines
1.8.3.2 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 11 lines · Jeff Liu
From: Jie Liu <[email protected]>
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.
fs/xfs/xfs_itable.c xfs_bulkstat_single( · line 567
Quoted 10 lines
xfs_inumbers_fmt(
fs/xfs/xfs_itable.c +2
-2void __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.
fs/xfs/xfs_itable.c xfs_inumbers_fmt( · line 596
Quoted 4 lines
fs/xfs/xfs_itable.c +3
-0*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?
fs/xfs/xfs_itable.c xfs_inumbers_fmt( · line 600
Quoted 2 lines
fs/xfs/xfs_itable.c +1
-0buffer =kmem_alloc (bcount *sizeof (*buffer),KM_SLEEP ); + bufidx = error =0 ;
Why not initialize bufidx and error at declaration time?
fs/xfs/xfs_itable.c xfs_inumbers_fmt( · line 620
Quoted 3 lines
fs/xfs/xfs_itable.c +3
-0+ 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Hi Christoph,
Sorry for my too late response! I missed your response somehow.
Quoted 19 lines · Christoph Hellwig
Quoted 11 lines · Jeff Liu
From: Jie Liu <[email protected]>
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.
Quoted 12 lines
fs/xfs/xfs_itable.c xfs_bulkstat_single( · line 567
Quoted 10 lines
xfs_inumbers_fmt(
fs/xfs/xfs_itable.c +2
-2void __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_compatwill need the same treatment.
Yup.
Quoted 7 lines
fs/xfs/xfs_itable.c xfs_inumbers_fmt( · line 596
Quoted 5 lines
fs/xfs/xfs_itable.c +3
-0*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.
Quoted 5 lines
fs/xfs/xfs_itable.c xfs_inumbers_fmt( · line 600
Quoted 3 lines
fs/xfs/xfs_itable.c +1
-0buffer =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.
Quoted 6 lines
fs/xfs/xfs_itable.c xfs_inumbers_fmt( · line 620
Quoted 4 lines
fs/xfs/xfs_itable.c +3
-0+ 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs