diff mbox series

[v2,02/14] libmultipath: directio: don't reset ct->running after io_cancel()

Message ID 20231026174153.1133-3-mwilck@suse.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show
Series multipath: aio, systemd, and documentation improvements | expand

Commit Message

Martin Wilck Oct. 26, 2023, 5:41 p.m. UTC 
From: Martin Wilck <mwilck@suse.com>

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@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 libmultipath/checkers/directio.c | 14 ++++----------
 tests/directio.c                 | 27 ++-------------------------
 2 files changed, 6 insertions(+), 35 deletions(-)

Comments

Benjamin Marzinski Oct. 27, 2023, 6:54 p.m. UTC | #1 
On Thu, Oct 26, 2023 at 07:41:41PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> 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@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  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
diff mbox series

Patch

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);