| Message ID | 158510668935.922633.2938909097570009707.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: random fixes | expand |
On Tue, Mar 24, 2020 at 08:24:49PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > The dirattr btree checking code uses the altpath substructure of the > dirattr state structure to check the sibling pointers of dir/attr tree > blocks. At the end of sibling checks, xfs_da3_path_shift could have > changed multiple levels of buffer pointers in the altpath structure. > Although we release the leaf level buffer, this isn't enough -- we also > need to release the node buffers that are unique to the altpath. > > Not releasing all of the altpath buffers leaves them locked to the > transaction. This is suboptimal because we should release resources > when we don't need them anymore. Fix the function to loop all levels of > the altpath, and fix the return logic so that we always run the loop. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/scrub/dabtree.c | 42 +++++++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 17 deletions(-) looks reasonable. Reviewed-by: Dave Chinner <dchinner@redhat.com>
On Wednesday, March 25, 2020 8:54 AM Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > The dirattr btree checking code uses the altpath substructure of the > dirattr state structure to check the sibling pointers of dir/attr tree > blocks. At the end of sibling checks, xfs_da3_path_shift could have > changed multiple levels of buffer pointers in the altpath structure. > Although we release the leaf level buffer, this isn't enough -- we also > need to release the node buffers that are unique to the altpath. > > Not releasing all of the altpath buffers leaves them locked to the > transaction. This is suboptimal because we should release resources > when we don't need them anymore. Fix the function to loop all levels of > the altpath, and fix the return logic so that we always run the loop. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> The patch indeed releases 'altpath' block buffers that are not referenced by by block buffers in 'path' after the sibling block is checked for inconsistencies. Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > --- > fs/xfs/scrub/dabtree.c | 42 +++++++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > > diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c > index 97a15b6f2865..9a2e27ac1300 100644 > --- a/fs/xfs/scrub/dabtree.c > +++ b/fs/xfs/scrub/dabtree.c > @@ -219,19 +219,21 @@ xchk_da_btree_block_check_sibling( > int direction, > xfs_dablk_t sibling) > { > + struct xfs_da_state_path *path = &ds->state->path; > + struct xfs_da_state_path *altpath = &ds->state->altpath; > int retval; > + int plevel; > int error; > > - memcpy(&ds->state->altpath, &ds->state->path, > - sizeof(ds->state->altpath)); > + memcpy(altpath, path, sizeof(ds->state->altpath)); > > /* > * If the pointer is null, we shouldn't be able to move the upper > * level pointer anywhere. > */ > if (sibling == 0) { > - error = xfs_da3_path_shift(ds->state, &ds->state->altpath, > - direction, false, &retval); > + error = xfs_da3_path_shift(ds->state, altpath, direction, > + false, &retval); > if (error == 0 && retval == 0) > xchk_da_set_corrupt(ds, level); > error = 0; > @@ -239,27 +241,33 @@ xchk_da_btree_block_check_sibling( > } > > /* Move the alternate cursor one block in the direction given. */ > - error = xfs_da3_path_shift(ds->state, &ds->state->altpath, > - direction, false, &retval); > + error = xfs_da3_path_shift(ds->state, altpath, direction, false, > + &retval); > if (!xchk_da_process_error(ds, level, &error)) > - return error; > + goto out; > if (retval) { > xchk_da_set_corrupt(ds, level); > - return error; > + goto out; > } > - if (ds->state->altpath.blk[level].bp) > - xchk_buffer_recheck(ds->sc, > - ds->state->altpath.blk[level].bp); > + if (altpath->blk[level].bp) > + xchk_buffer_recheck(ds->sc, altpath->blk[level].bp); > > /* Compare upper level pointer to sibling pointer. */ > - if (ds->state->altpath.blk[level].blkno != sibling) > + if (altpath->blk[level].blkno != sibling) > xchk_da_set_corrupt(ds, level); > - if (ds->state->altpath.blk[level].bp) { > - xfs_trans_brelse(ds->dargs.trans, > - ds->state->altpath.blk[level].bp); > - ds->state->altpath.blk[level].bp = NULL; > - } > + > out: > + /* Free all buffers in the altpath that aren't referenced from path. */ > + for (plevel = 0; plevel < altpath->active; plevel++) { > + if (altpath->blk[plevel].bp == NULL || > + (plevel < path->active && > + altpath->blk[plevel].bp == path->blk[plevel].bp)) > + continue; > + > + xfs_trans_brelse(ds->dargs.trans, altpath->blk[plevel].bp); > + altpath->blk[plevel].bp = NULL; > + } > + > return error; > } > > >
diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c index 97a15b6f2865..9a2e27ac1300 100644 --- a/fs/xfs/scrub/dabtree.c +++ b/fs/xfs/scrub/dabtree.c @@ -219,19 +219,21 @@ xchk_da_btree_block_check_sibling( int direction, xfs_dablk_t sibling) { + struct xfs_da_state_path *path = &ds->state->path; + struct xfs_da_state_path *altpath = &ds->state->altpath; int retval; + int plevel; int error; - memcpy(&ds->state->altpath, &ds->state->path, - sizeof(ds->state->altpath)); + memcpy(altpath, path, sizeof(ds->state->altpath)); /* * If the pointer is null, we shouldn't be able to move the upper * level pointer anywhere. */ if (sibling == 0) { - error = xfs_da3_path_shift(ds->state, &ds->state->altpath, - direction, false, &retval); + error = xfs_da3_path_shift(ds->state, altpath, direction, + false, &retval); if (error == 0 && retval == 0) xchk_da_set_corrupt(ds, level); error = 0; @@ -239,27 +241,33 @@ xchk_da_btree_block_check_sibling( } /* Move the alternate cursor one block in the direction given. */ - error = xfs_da3_path_shift(ds->state, &ds->state->altpath, - direction, false, &retval); + error = xfs_da3_path_shift(ds->state, altpath, direction, false, + &retval); if (!xchk_da_process_error(ds, level, &error)) - return error; + goto out; if (retval) { xchk_da_set_corrupt(ds, level); - return error; + goto out; } - if (ds->state->altpath.blk[level].bp) - xchk_buffer_recheck(ds->sc, - ds->state->altpath.blk[level].bp); + if (altpath->blk[level].bp) + xchk_buffer_recheck(ds->sc, altpath->blk[level].bp); /* Compare upper level pointer to sibling pointer. */ - if (ds->state->altpath.blk[level].blkno != sibling) + if (altpath->blk[level].blkno != sibling) xchk_da_set_corrupt(ds, level); - if (ds->state->altpath.blk[level].bp) { - xfs_trans_brelse(ds->dargs.trans, - ds->state->altpath.blk[level].bp); - ds->state->altpath.blk[level].bp = NULL; - } + out: + /* Free all buffers in the altpath that aren't referenced from path. */ + for (plevel = 0; plevel < altpath->active; plevel++) { + if (altpath->blk[plevel].bp == NULL || + (plevel < path->active && + altpath->blk[plevel].bp == path->blk[plevel].bp)) + continue; + + xfs_trans_brelse(ds->dargs.trans, altpath->blk[plevel].bp); + altpath->blk[plevel].bp = NULL; + } + return error; }