On Wed, Apr 27, 2022 at 09:44:50AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@xxxxxxxxxx> > > Sean Caron reported that a metadump terminated after givin gthis > warning: > > xfs_metadump: inode 2216156864 has unexpected extents > > Metadump is supposed to ignore corruptions and continue dumping the > filesystem as best it can. Whilst it warns about many situations > where it can't fully dump structures, it should stop processing that > structure and continue with the next one until the entire filesystem > has been processed. > > Unfortunately, some warning conditions also return an "abort" error > status, causing metadump to abort if that condition is hit. Most of > these abort conditions should really be "continue on next object" > conditions so that the we attempt to dump the rest of the > filesystem. > > Fix the returns for warnings that incorrectly cause aborts > such that the only abort conditions are read errors when > "stop-on-read-error" semantics are specified. Also make the return > values consistently mean abort/continue rather than returning -errno > to mean "stop because read error" and then trying to infer what > the error means in callers without the context it occurred in. I was almost about to say "This variable should be named success", but then noticed that there already /are/ variables named success. Yuck. rval==0 means abort? and rval!=0 means continue? If so, Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> --D > Reported-and-tested-by: Sean Caron <scaron@xxxxxxxxx> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > --- > db/metadump.c | 94 +++++++++++++++++++++++++-------------------------- > 1 file changed, 47 insertions(+), 47 deletions(-) > > diff --git a/db/metadump.c b/db/metadump.c > index 48cda88a3ea5..a21baa2070d9 100644 > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -1645,7 +1645,7 @@ process_symlink_block( > { > struct bbmap map; > char *link; > - int ret = 0; > + int rval = 1; > > push_cur(); > map.nmaps = 1; > @@ -1658,8 +1658,7 @@ process_symlink_block( > > print_warning("cannot read %s block %u/%u (%llu)", > typtab[btype].name, agno, agbno, s); > - if (stop_on_read_error) > - ret = -1; > + rval = !stop_on_read_error; > goto out_pop; > } > link = iocur_top->data; > @@ -1682,10 +1681,11 @@ process_symlink_block( > } > > iocur_top->need_crc = 1; > - ret = write_buf(iocur_top); > + if (write_buf(iocur_top)) > + rval = 0; > out_pop: > pop_cur(); > - return ret; > + return rval; > } > > #define MAX_REMOTE_VALS 4095 > @@ -1843,8 +1843,8 @@ process_single_fsb_objects( > typnm_t btype, > xfs_fileoff_t last) > { > + int rval = 1; > char *dp; > - int ret = 0; > int i; > > for (i = 0; i < c; i++) { > @@ -1858,8 +1858,7 @@ process_single_fsb_objects( > > print_warning("cannot read %s block %u/%u (%llu)", > typtab[btype].name, agno, agbno, s); > - if (stop_on_read_error) > - ret = -EIO; > + rval = !stop_on_read_error; > goto out_pop; > > } > @@ -1925,16 +1924,17 @@ process_single_fsb_objects( > } > > write: > - ret = write_buf(iocur_top); > + if (write_buf(iocur_top)) > + rval = 0; > out_pop: > pop_cur(); > - if (ret) > + if (!rval) > break; > o++; > s++; > } > > - return ret; > + return rval; > } > > /* > @@ -1952,7 +1952,7 @@ process_multi_fsb_dir( > xfs_fileoff_t last) > { > char *dp; > - int ret = 0; > + int rval = 1; > > while (c > 0) { > unsigned int bm_len; > @@ -1978,8 +1978,7 @@ process_multi_fsb_dir( > > print_warning("cannot read %s block %u/%u (%llu)", > typtab[btype].name, agno, agbno, s); > - if (stop_on_read_error) > - ret = -1; > + rval = !stop_on_read_error; > goto out_pop; > > } > @@ -1998,18 +1997,19 @@ process_multi_fsb_dir( > } > iocur_top->need_crc = 1; > write: > - ret = write_buf(iocur_top); > + if (write_buf(iocur_top)) > + rval = 0; > out_pop: > pop_cur(); > mfsb_map.nmaps = 0; > - if (ret) > + if (!rval) > break; > } > c -= bm_len; > s += bm_len; > } > > - return ret; > + return rval; > } > > static bool > @@ -2039,15 +2039,15 @@ process_multi_fsb_objects( > return process_symlink_block(o, s, c, btype, last); > default: > print_warning("bad type for multi-fsb object %d", btype); > - return -EINVAL; > + return 1; > } > } > > /* inode copy routines */ > static int > process_bmbt_reclist( > - xfs_bmbt_rec_t *rp, > - int numrecs, > + xfs_bmbt_rec_t *rp, > + int numrecs, > typnm_t btype) > { > int i; > @@ -2059,7 +2059,7 @@ process_bmbt_reclist( > xfs_agnumber_t agno; > xfs_agblock_t agbno; > bool is_multi_fsb = is_multi_fsb_object(mp, btype); > - int error; > + int rval = 1; > > if (btype == TYP_DATA) > return 1; > @@ -2123,16 +2123,16 @@ process_bmbt_reclist( > > /* multi-extent blocks require special handling */ > if (is_multi_fsb) > - error = process_multi_fsb_objects(o, s, c, btype, > + rval = process_multi_fsb_objects(o, s, c, btype, > last); > else > - error = process_single_fsb_objects(o, s, c, btype, > + rval = process_single_fsb_objects(o, s, c, btype, > last); > - if (error) > - return 0; > + if (!rval) > + break; > } > > - return 1; > + return rval; > } > > static int > @@ -2331,7 +2331,7 @@ process_inode_data( > return 1; > } > > -static int > +static void > process_dev_inode( > xfs_dinode_t *dip) > { > @@ -2339,15 +2339,13 @@ process_dev_inode( > if (show_warnings) > print_warning("inode %llu has unexpected extents", > (unsigned long long)cur_ino); > - return 0; > - } else { > - if (zero_stale_data) { > - unsigned int size = sizeof(xfs_dev_t); > + return; > + } > + if (zero_stale_data) { > + unsigned int size = sizeof(xfs_dev_t); > > - memset(XFS_DFORK_DPTR(dip) + size, 0, > - XFS_DFORK_DSIZE(dip, mp) - size); > - } > - return 1; > + memset(XFS_DFORK_DPTR(dip) + size, 0, > + XFS_DFORK_DSIZE(dip, mp) - size); > } > } > > @@ -2365,11 +2363,10 @@ process_inode( > xfs_dinode_t *dip, > bool free_inode) > { > - int success; > + int rval = 1; > bool crc_was_ok = false; /* no recalc by default */ > bool need_new_crc = false; > > - success = 1; > cur_ino = XFS_AGINO_TO_INO(mp, agno, agino); > > /* we only care about crc recalculation if we will modify the inode. */ > @@ -2390,32 +2387,34 @@ process_inode( > /* copy appropriate data fork metadata */ > switch (be16_to_cpu(dip->di_mode) & S_IFMT) { > case S_IFDIR: > - success = process_inode_data(dip, TYP_DIR2); > + rval = process_inode_data(dip, TYP_DIR2); > if (dip->di_format == XFS_DINODE_FMT_LOCAL) > need_new_crc = 1; > break; > case S_IFLNK: > - success = process_inode_data(dip, TYP_SYMLINK); > + rval = process_inode_data(dip, TYP_SYMLINK); > if (dip->di_format == XFS_DINODE_FMT_LOCAL) > need_new_crc = 1; > break; > case S_IFREG: > - success = process_inode_data(dip, TYP_DATA); > + rval = process_inode_data(dip, TYP_DATA); > break; > case S_IFIFO: > case S_IFCHR: > case S_IFBLK: > case S_IFSOCK: > - success = process_dev_inode(dip); > + process_dev_inode(dip); > need_new_crc = 1; > break; > default: > break; > } > nametable_clear(); > + if (!rval) > + goto done; > > /* copy extended attributes if they exist and forkoff is valid */ > - if (success && XFS_DFORK_DSIZE(dip, mp) < XFS_LITINO(mp)) { > + if (XFS_DFORK_DSIZE(dip, mp) < XFS_LITINO(mp)) { > attr_data.remote_val_count = 0; > switch (dip->di_aformat) { > case XFS_DINODE_FMT_LOCAL: > @@ -2425,11 +2424,11 @@ process_inode( > break; > > case XFS_DINODE_FMT_EXTENTS: > - success = process_exinode(dip, TYP_ATTR); > + rval = process_exinode(dip, TYP_ATTR); > break; > > case XFS_DINODE_FMT_BTREE: > - success = process_btinode(dip, TYP_ATTR); > + rval = process_btinode(dip, TYP_ATTR); > break; > } > nametable_clear(); > @@ -2442,7 +2441,8 @@ done: > > if (crc_was_ok && need_new_crc) > libxfs_dinode_calc_crc(mp, dip); > - return success; > + > + return rval; > } > > static uint32_t inodes_copied; > @@ -2541,7 +2541,7 @@ copy_inode_chunk( > > /* process_inode handles free inodes, too */ > if (!process_inode(agno, agino + ioff + i, dip, > - XFS_INOBT_IS_FREE_DISK(rp, ioff + i))) > + XFS_INOBT_IS_FREE_DISK(rp, ioff + i))) > goto pop_out; > > inodes_copied++; > @@ -2800,7 +2800,7 @@ copy_ino( > xfs_agblock_t agbno; > xfs_agino_t agino; > int offset; > - int rval = 0; > + int rval = 1; > > if (ino == 0 || ino == NULLFSINO) > return 1; > -- > 2.35.1 >