On Thu, Oct 26, 2023 at 07:41:41PM +0200, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> io_cancel() never succeeds, and even if it did, io_getevents() must
> still be called to reclaim the IO resources from the kernel.
> Don't pretend the opposite by resetting ct->running, or freeing the
> memory, before io_getevents() has indicated that the request is finished.
>
> In the test code, don't bother about the return value of __wrap_io_cancel().
>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> libmultipath/checkers/directio.c | 14 ++++----------
> tests/directio.c | 27 ++-------------------------
> 2 files changed, 6 insertions(+), 35 deletions(-)
>
> diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
> index 25b2383..83ab29f 100644
> --- a/libmultipath/checkers/directio.c
> +++ b/libmultipath/checkers/directio.c
> @@ -232,15 +232,15 @@ void libcheck_free (struct checker * c)
> }
> }
>
> - if (ct->running &&
> - (ct->req->state != PATH_PENDING ||
> - io_cancel(ct->aio_grp->ioctx, &ct->req->io, &event) == 0))
> + if (ct->running && ct->req->state != PATH_PENDING)
> ct->running = 0;
> if (!ct->running) {
> free(ct->req->buf);
> free(ct->req);
> ct->aio_grp->holders--;
> } else {
> + /* Currently a no-op */
> + io_cancel(ct->aio_grp->ioctx, &ct->req->io, &event);
> ct->req->state = PATH_REMOVED;
> list_add(&ct->req->node, &ct->aio_grp->orphans);
> check_orphaned_group(ct->aio_grp);
> @@ -351,13 +351,7 @@ check_state(int fd, struct directio_context *ct, int sync, int timeout_secs)
>
> LOG(3, "abort check on timeout");
>
> - r = io_cancel(ct->aio_grp->ioctx, &ct->req->io, &event);
> - /*
> - * Only reset ct->running if we really
> - * could abort the pending I/O
> - */
> - if (!r)
> - ct->running = 0;
> + io_cancel(ct->aio_grp->ioctx, &ct->req->io, &event);
> rc = PATH_DOWN;
> } else {
> LOG(4, "async io pending");
> diff --git a/tests/directio.c b/tests/directio.c
> index db9643e..5201d21 100644
> --- a/tests/directio.c
> +++ b/tests/directio.c
> @@ -141,10 +141,9 @@ int __real_io_cancel(io_context_t ctx, struct iocb *iocb, struct io_event *evt);
> int __wrap_io_cancel(io_context_t ctx, struct iocb *iocb, struct io_event *evt)
> {
> #ifdef DIO_TEST_DEV
> - mock_type(int);
> return __real_io_cancel(ctx, iocb, evt);
> #else
> - return mock_type(int);
> + return 0;
> #endif
> }
>
> @@ -439,14 +438,8 @@ static void test_check_state_timeout(void **state)
> do_libcheck_init(&c, 4096, NULL);
> aio_grp = get_aio_grp(&c);
> return_io_getevents_none();
> - will_return(__wrap_io_cancel, 0);
> do_check_state(&c, 1, 30, PATH_DOWN);
> check_aio_grp(aio_grp, 1, 0);
> -#ifdef DIO_TEST_DEV
> - /* io_cancel will return negative value on timeout, so it happens again
> - * when freeing the checker */
> - will_return(__wrap_io_cancel, 0);
> -#endif
> libcheck_free(&c);
> do_libcheck_reset(1);
> }
> @@ -468,12 +461,8 @@ static void test_check_state_async_timeout(void **state)
> return_io_getevents_none();
> do_check_state(&c, 0, 3, PATH_PENDING);
> return_io_getevents_none();
> - will_return(__wrap_io_cancel, 0);
> do_check_state(&c, 0, 3, PATH_DOWN);
> check_aio_grp(aio_grp, 1, 0);
> -#ifdef DIO_TEST_DEV
> - will_return(__wrap_io_cancel, 0);
> -#endif
> libcheck_free(&c);
> do_libcheck_reset(1);
> }
> @@ -501,13 +490,8 @@ static void test_free_with_pending(void **state)
> check_aio_grp(aio_grp, 2, 0);
> libcheck_free(&c[0]);
> check_aio_grp(aio_grp, 1, 0);
> - will_return(__wrap_io_cancel, 0);
> libcheck_free(&c[1]);
> -#ifdef DIO_TEST_DEV
> - check_aio_grp(aio_grp, 1, 1); /* real cancel doesn't remove request */
> -#else
> - check_aio_grp(aio_grp, 0, 0);
> -#endif
> + check_aio_grp(aio_grp, 1, 1); /* cancel doesn't remove request */
> do_libcheck_reset(1);
> }
>
> @@ -533,7 +517,6 @@ static void test_orphaned_aio_group(void **state)
> assert_int_equal(i, 1);
> for (i = 0; i < AIO_GROUP_SIZE; i++) {
> assert_true(is_checker_running(&c[i]));
> - will_return(__wrap_io_cancel, -1);
> if (i == AIO_GROUP_SIZE - 1) {
> /* remove the orphaned group and create a new one */
> will_return(__wrap_io_destroy, 0);
> @@ -559,12 +542,10 @@ static void test_timeout_cancel_failed(void **state)
> do_libcheck_init(&c[i], 4096, &reqs[i]);
> aio_grp = get_aio_grp(c);
> return_io_getevents_none();
> - will_return(__wrap_io_cancel, -1);
> do_check_state(&c[0], 1, 30, PATH_DOWN);
> assert_true(is_checker_running(&c[0]));
> check_aio_grp(aio_grp, 2, 0);
> return_io_getevents_none();
> - will_return(__wrap_io_cancel, -1);
> do_check_state(&c[0], 1, 30, PATH_DOWN);
> assert_true(is_checker_running(&c[0]));
> return_io_getevents_nr(NULL, 1, &reqs[0], &res[0]);
> @@ -600,7 +581,6 @@ static void test_async_timeout_cancel_failed(void **state)
> return_io_getevents_none();
> do_check_state(&c[1], 0, 2, PATH_PENDING);
> return_io_getevents_none();
> - will_return(__wrap_io_cancel, -1);
> do_check_state(&c[0], 0, 2, PATH_DOWN);
> #ifndef DIO_TEST_DEV
> /* can't pick which even gets returned on real devices */
> @@ -608,7 +588,6 @@ static void test_async_timeout_cancel_failed(void **state)
> do_check_state(&c[1], 0, 2, PATH_UP);
> #endif
> return_io_getevents_none();
> - will_return(__wrap_io_cancel, -1);
> do_check_state(&c[0], 0, 2, PATH_DOWN);
> assert_true(is_checker_running(&c[0]));
> return_io_getevents_nr(NULL, 2, reqs, res);
> @@ -637,7 +616,6 @@ static void test_orphan_checker_cleanup(void **state)
> aio_grp = get_aio_grp(c);
> return_io_getevents_none();
> do_check_state(&c[0], 0, 30, PATH_PENDING);
> - will_return(__wrap_io_cancel, -1);
> check_aio_grp(aio_grp, 2, 0);
> libcheck_free(&c[0]);
> check_aio_grp(aio_grp, 2, 1);
> @@ -662,7 +640,6 @@ static void test_orphan_reset_cleanup(void **state)
> orphan_aio_grp = get_aio_grp(&c);
> return_io_getevents_none();
> do_check_state(&c, 0, 30, PATH_PENDING);
> - will_return(__wrap_io_cancel, -1);
> check_aio_grp(orphan_aio_grp, 1, 0);
> libcheck_free(&c);
> check_aio_grp(orphan_aio_grp, 1, 1);
> --
> 2.42.0
