Message ID | 158619916916.469742.10169263890587590189.stgit@magnolia (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | xfsprogs: rollup of various fixes for 5.6 | expand |
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"), >
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>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
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"),