On Tue, Aug 29, 2017 at 07:48:31PM +0200, Christoph Hellwig wrote:
> For the first right move we need to look up next_fsb. That means
> our last fsb that contains next_fsb must also be the current extent,
> so take advantage of that by moving the code around a bit.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Though I gotta ask, for the cases where we check current_ext against
stop_extent and return -EIO, isn't that only possible if there's
something seriously wrong with the extent map? In which case
-EFSCORRUPTED might be a more appropriate choice?
(Separate fix, obviously...)
--D
> ---
> fs/xfs/libxfs/xfs_bmap.c | 54 ++++++++++++++++++++++++++----------------------
> 1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index c944987c485d..6af3cc1fc630 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6127,7 +6127,6 @@ xfs_bmap_shift_extents(
> ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> ASSERT(direction == SHIFT_LEFT || direction == SHIFT_RIGHT);
> - ASSERT(*next_fsb != NULLFSBLOCK || direction == SHIFT_RIGHT);
>
> ifp = XFS_IFORK_PTR(ip, whichfork);
> if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> @@ -6159,43 +6158,48 @@ xfs_bmap_shift_extents(
> * In case of first right shift, we need to initialize next_fsb
> */
> if (*next_fsb == NULLFSBLOCK) {
> - gotp = xfs_iext_get_ext(ifp, total_extents - 1);
> + ASSERT(direction == SHIFT_RIGHT);
> +
> + current_ext = total_extents - 1;
> + gotp = xfs_iext_get_ext(ifp, current_ext);
> xfs_bmbt_get_all(gotp, &got);
> *next_fsb = got.br_startoff;
> if (stop_fsb > *next_fsb) {
> *done = 1;
> goto del_cursor;
> }
> + } else {
> + /*
> + * Look up the extent index for the fsb where we start shifting. We can
> + * henceforth iterate with current_ext as extent list changes are locked
> + * out via ilock.
> + *
> + * gotp can be null in 2 cases: 1) if there are no extents or 2)
> + * *next_fsb lies in a hole beyond which there are no extents. Either
> + * way, we are done.
> + */
> + gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, ¤t_ext);
> + if (!gotp) {
> + *done = 1;
> + goto del_cursor;
> + }
> }
>
> /* Lookup the extent index at which we have to stop */
> if (direction == SHIFT_RIGHT) {
> - gotp = xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
> + xfs_iext_bno_to_ext(ifp, stop_fsb, &stop_extent);
> /* Make stop_extent exclusive of shift range */
> stop_extent--;
> - } else
> + if (current_ext <= stop_extent) {
> + error = -EIO;
> + goto del_cursor;
> + }
> + } else {
> stop_extent = total_extents;
> -
> - /*
> - * Look up the extent index for the fsb where we start shifting. We can
> - * henceforth iterate with current_ext as extent list changes are locked
> - * out via ilock.
> - *
> - * gotp can be null in 2 cases: 1) if there are no extents or 2)
> - * *next_fsb lies in a hole beyond which there are no extents. Either
> - * way, we are done.
> - */
> - gotp = xfs_iext_bno_to_ext(ifp, *next_fsb, ¤t_ext);
> - if (!gotp) {
> - *done = 1;
> - goto del_cursor;
> - }
> -
> - /* some sanity checking before we finally start shifting extents */
> - if ((direction == SHIFT_LEFT && current_ext >= stop_extent) ||
> - (direction == SHIFT_RIGHT && current_ext <= stop_extent)) {
> - error = -EIO;
> - goto del_cursor;
> + if (current_ext >= stop_extent) {
> + error = -EIO;
> + goto del_cursor;
> + }
> }
>
> while (nexts++ < num_exts) {
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
