diff mbox series

[4/5] xfs_repair: fix dir_read_buf use of libxfs_da_read_buf

Message ID 158619916916.469742.10169263890587590189.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfsprogs: rollup of various fixes for 5.6 | expand

Commit Message

Darrick J. Wong April 6, 2020, 6:52 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

xfs_da_read_buf dropped the 'mappedbno' argument in favor of a flags
argument.  Foolishly, we're passing that parameter (which is -1 in all
callers) to xfs_da_read_buf, which gets us the wrong behavior.

Since mappedbno == -1 meant "complain if we fall into a hole" (which is
the default behavior of xfs_da_read_buf) we can fix this by passing a
zero flags argument and getting rid of mappedbno entirely.

Coverity-id: 1457898
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase6.c |   21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Comments

Allison Henderson April 6, 2020, 10:09 p.m. UTC | #1 
On 4/6/20 11:52 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> xfs_da_read_buf dropped the 'mappedbno' argument in favor of a flags
> argument.  Foolishly, we're passing that parameter (which is -1 in all
> callers) to xfs_da_read_buf, which gets us the wrong behavior.
> 
> Since mappedbno == -1 meant "complain if we fall into a hole" (which is
> the default behavior of xfs_da_read_buf) we can fix this by passing a
> zero flags argument and getting rid of mappedbno entirely.
> 
> Coverity-id: 1457898
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
I dont see any logical errors
Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> ---
>   repair/phase6.c |   21 +++++++++------------
>   1 file changed, 9 insertions(+), 12 deletions(-)
> 
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 3fb1af24..beceea9a 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -179,7 +179,6 @@ static int
>   dir_read_buf(
>   	struct xfs_inode	*ip,
>   	xfs_dablk_t		bno,
> -	xfs_daddr_t		mappedbno,
>   	struct xfs_buf		**bpp,
>   	const struct xfs_buf_ops *ops,
>   	int			*crc_error)
> @@ -187,14 +186,13 @@ dir_read_buf(
>   	int error;
>   	int error2;
>   
> -	error = -libxfs_da_read_buf(NULL, ip, bno, mappedbno, bpp,
> -				   XFS_DATA_FORK, ops);
> +	error = -libxfs_da_read_buf(NULL, ip, bno, 0, bpp, XFS_DATA_FORK, ops);
>   
>   	if (error != EFSBADCRC && error != EFSCORRUPTED)
>   		return error;
>   
> -	error2 = -libxfs_da_read_buf(NULL, ip, bno, mappedbno, bpp,
> -				   XFS_DATA_FORK, NULL);
> +	error2 = -libxfs_da_read_buf(NULL, ip, bno, 0, bpp, XFS_DATA_FORK,
> +			NULL);
>   	if (error2)
>   		return error2;
>   
> @@ -2035,8 +2033,7 @@ longform_dir2_check_leaf(
>   	int			fixit = 0;
>   
>   	da_bno = mp->m_dir_geo->leafblk;
> -	error = dir_read_buf(ip, da_bno, -1, &bp, &xfs_dir3_leaf1_buf_ops,
> -			     &fixit);
> +	error = dir_read_buf(ip, da_bno, &bp, &xfs_dir3_leaf1_buf_ops, &fixit);
>   	if (error == EFSBADCRC || error == EFSCORRUPTED || fixit) {
>   		do_warn(
>   	_("leaf block %u for directory inode %" PRIu64 " bad CRC\n"),
> @@ -2137,8 +2134,8 @@ longform_dir2_check_node(
>   		 * a node block, then we'll skip it below based on a magic
>   		 * number check.
>   		 */
> -		error = dir_read_buf(ip, da_bno, -1, &bp,
> -				     &xfs_da3_node_buf_ops, &fixit);
> +		error = dir_read_buf(ip, da_bno, &bp, &xfs_da3_node_buf_ops,
> +				&fixit);
>   		if (error) {
>   			do_warn(
>   	_("can't read leaf block %u for directory inode %" PRIu64 ", error %d\n"),
> @@ -2205,8 +2202,8 @@ longform_dir2_check_node(
>   		if (bmap_next_offset(NULL, ip, &next_da_bno, XFS_DATA_FORK))
>   			break;
>   
> -		error = dir_read_buf(ip, da_bno, -1, &bp,
> -				     &xfs_dir3_free_buf_ops, &fixit);
> +		error = dir_read_buf(ip, da_bno, &bp, &xfs_dir3_free_buf_ops,
> +				&fixit);
>   		if (error) {
>   			do_warn(
>   	_("can't read freespace block %u for directory inode %" PRIu64 ", error %d\n"),
> @@ -2367,7 +2364,7 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
>   		else
>   			ops = &xfs_dir3_data_buf_ops;
>   
> -		error = dir_read_buf(ip, da_bno, -1, &bplist[db], ops, &fixit);
> +		error = dir_read_buf(ip, da_bno, &bplist[db], ops, &fixit);
>   		if (error) {
>   			do_warn(
>   	_("can't read data block %u for directory inode %" PRIu64 " error %d\n"),
>
Eric Sandeen April 7, 2020, 7:05 p.m. UTC | #2 
On 4/6/20 1:52 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> xfs_da_read_buf dropped the 'mappedbno' argument in favor of a flags
> argument.  Foolishly, we're passing that parameter (which is -1 in all
> callers) to xfs_da_read_buf, which gets us the wrong behavior.
> 
> Since mappedbno == -1 meant "complain if we fall into a hole" (which is
> the default behavior of xfs_da_read_buf) we can fix this by passing a
> zero flags argument and getting rid of mappedbno entirely.
> 
> Coverity-id: 1457898
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good; I think this patch

Fixes: 5f356ae6d ("xfs: remove the mappedbno argument to xfs_da_read_buf")

(i.e. the merge of that kernel commit, which I biffed on, apparently)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Christoph Hellwig April 9, 2020, 7:44 a.m. UTC | #3 
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/repair/phase6.c b/repair/phase6.c
index 3fb1af24..beceea9a 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -179,7 +179,6 @@  static int
 dir_read_buf(
 	struct xfs_inode	*ip,
 	xfs_dablk_t		bno,
-	xfs_daddr_t		mappedbno,
 	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops,
 	int			*crc_error)
@@ -187,14 +186,13 @@  dir_read_buf(
 	int error;
 	int error2;
 
-	error = -libxfs_da_read_buf(NULL, ip, bno, mappedbno, bpp,
-				   XFS_DATA_FORK, ops);
+	error = -libxfs_da_read_buf(NULL, ip, bno, 0, bpp, XFS_DATA_FORK, ops);
 
 	if (error != EFSBADCRC && error != EFSCORRUPTED)
 		return error;
 
-	error2 = -libxfs_da_read_buf(NULL, ip, bno, mappedbno, bpp,
-				   XFS_DATA_FORK, NULL);
+	error2 = -libxfs_da_read_buf(NULL, ip, bno, 0, bpp, XFS_DATA_FORK,
+			NULL);
 	if (error2)
 		return error2;
 
@@ -2035,8 +2033,7 @@  longform_dir2_check_leaf(
 	int			fixit = 0;
 
 	da_bno = mp->m_dir_geo->leafblk;
-	error = dir_read_buf(ip, da_bno, -1, &bp, &xfs_dir3_leaf1_buf_ops,
-			     &fixit);
+	error = dir_read_buf(ip, da_bno, &bp, &xfs_dir3_leaf1_buf_ops, &fixit);
 	if (error == EFSBADCRC || error == EFSCORRUPTED || fixit) {
 		do_warn(
 	_("leaf block %u for directory inode %" PRIu64 " bad CRC\n"),
@@ -2137,8 +2134,8 @@  longform_dir2_check_node(
 		 * a node block, then we'll skip it below based on a magic
 		 * number check.
 		 */
-		error = dir_read_buf(ip, da_bno, -1, &bp,
-				     &xfs_da3_node_buf_ops, &fixit);
+		error = dir_read_buf(ip, da_bno, &bp, &xfs_da3_node_buf_ops,
+				&fixit);
 		if (error) {
 			do_warn(
 	_("can't read leaf block %u for directory inode %" PRIu64 ", error %d\n"),
@@ -2205,8 +2202,8 @@  longform_dir2_check_node(
 		if (bmap_next_offset(NULL, ip, &next_da_bno, XFS_DATA_FORK))
 			break;
 
-		error = dir_read_buf(ip, da_bno, -1, &bp,
-				     &xfs_dir3_free_buf_ops, &fixit);
+		error = dir_read_buf(ip, da_bno, &bp, &xfs_dir3_free_buf_ops,
+				&fixit);
 		if (error) {
 			do_warn(
 	_("can't read freespace block %u for directory inode %" PRIu64 ", error %d\n"),
@@ -2367,7 +2364,7 @@  longform_dir2_entry_check(xfs_mount_t	*mp,
 		else
 			ops = &xfs_dir3_data_buf_ops;
 
-		error = dir_read_buf(ip, da_bno, -1, &bplist[db], ops, &fixit);
+		error = dir_read_buf(ip, da_bno, &bplist[db], ops, &fixit);
 		if (error) {
 			do_warn(
 	_("can't read data block %u for directory inode %" PRIu64 " error %d\n"),