Re: [PATCH 1/4] metadump: handle corruption errors without aborting

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Apr 27, 2022 at 09:44:50AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@kernel.org>

--D

> Reported-and-tested-by: Sean Caron <scaron@umich.edu>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  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
>