[GIT PULL] Add ftruncate #1004

tontinton · 2023-11-24T10:00:03Z

To accompany the patches I sent for adding ftruncate, here are the changes for usermode.

git request-pull output:

The following changes since commit 63342497bec87b72d4b415c8bed930c15ff83b0d:

  test/read-mshot: check that IORING_CQE_F_MORE isn't set on error (2024-01-22 14:56:08 -0700)

are available in the Git repository at:

  git@github.com:tontinton/liburing.git truncate

for you to fetch changes up to c023393f3a3b960e3df77e5bcfd1b2e93e3b5e0f:

  man/io_uring_prep_ftruncate: Add the new ftruncate command (2024-01-31 17:52:13 +0200)

----------------------------------------------------------------
Tony Solomonik (4):
      Add ftruncate helpers
      test/truncate: Add test for ftruncate
      test/truncate: Add test for failure on truncate path
      man/io_uring_prep_ftruncate: Add the new ftruncate command

 man/io_uring_prep_ftruncate.3   |  48 ++++++++++++++++++++++++++++++++
 src/include/liburing.h          |   6 ++++
 src/include/liburing/io_uring.h |   1 +
 src/liburing-ffi.map            |   1 +
 test/Makefile                   |   1 +
 test/truncate.c                 | 179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 236 insertions(+)
 create mode 100644 man/io_uring_prep_ftruncate.3
 create mode 100644 test/truncate.c

Click to show/hide pull request guidelines

Pull Request Guidelines

To make everyone easily filter pull request from the email
notification, use [GIT PULL] as a prefix in your PR title.

[GIT PULL] Your Pull Request Title

Follow the commit message format rules below.
Follow the Linux kernel coding style (see: https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst).

Commit message format rules:

The first line is title (don't be more than 72 chars if possible).
Then an empty line.
Then a description (may be omitted for truly trivial changes).
Then an empty line again (if it has a description).
Then a Signed-off-by tag with your real name and email. For example:

Signed-off-by: Foo Bar <foo.bar@gmail.com>

The description should be word-wrapped at 72 chars. Some things should
not be word-wrapped. They may be some kind of quoted text - long
compiler error messages, oops reports, Link, etc. (things that have a
certain specific format).

Note that all of this goes in the commit message, not in the pull
request text. The pull request text should introduce what this pull
request does, and each commit message should explain the rationale for
why that particular change was made. The git tree is canonical source
of truth, not github.

Each patch should do one thing, and one thing only. If you find yourself
writing an explanation for why a patch is fixing multiple issues, that's
a good indication that the change should be split into separate patches.

If the commit is a fix for an issue, add a Fixes tag with the issue
URL.

Don't use GitHub anonymous email like this as the commit author:

123456789+username@users.noreply.github.com

Use a real email address!

Commit message example:

src/queue: don't flush SQ ring for new wait interface

If we have IORING_FEAT_EXT_ARG, then timeouts are done through the
syscall instead of by posting an internal timeout. This was done
to be both more efficient, but also to enable multi-threaded use
the wait side. If we touch the SQ state by flushing it, that isn't
safe without synchronization.

Fixes: https://github.com/axboe/liburing/issues/402
Signed-off-by: Jens Axboe <axboe@kernel.dk>

By submitting this pull request, I acknowledge that:

I have followed the above pull request guidelines.
I have the rights to submit this work under the same license.
I agree to a Developer Certificate of Origin (see https://developercertificate.org for more information).

axboe · 2024-01-23T15:43:29Z

Two minor things:

The prep helper should go into the ffi as well, and the ffi map (in the 2.6 section). See src/liburing-ffi.map.
The test case tests functionality, which is great, but it should also the failure case. Since we're potentially adding truncate support later where we'll put the path in sqe->addr, add a test case that issues an sqe where addr is set to a filename and verify that it fails with EINVAL.

axboe · 2024-01-25T16:32:01Z

I think this looks fine, but one problem is that if you run this test case on an older kernel, then the test will fail. If you get -EINVAL for the first ftruncate you try, just assume the kernel doesn't support it and return T_EXIT_SKIP for that and skip the other tests. That way it'll just skip the test case on older kernels, but test it on newer kernels.

See other test cases that do this very thing, usually with a static int no_ftruncate global that can be set by the first test invocation, and then checked again for non-zero when that first test case returns. If true, then it should return T_EXIT_SKIP.

tontinton · 2024-01-26T16:24:37Z

It already runs goto out which returns 0, when ftruncate is not supported.

axboe · 2024-01-26T16:26:36Z

test/truncate.c

+		if (ret < 0) {
+			if (ret == -EBADF || ret == -EINVAL) {
+				fprintf(stdout, "Ftruncate not supported, skipping\n");
+				goto out;


Can be a break?

If this is a break, it will go to test truncate with path failing, no reason to test that if ftruncate is unsupported I think

axboe · 2024-01-26T16:27:02Z

test/truncate.c

+		}
+	}
+
+out:


Should return T_EXIT_PASS for success, T_EXIT_SKIP if the test is skipped, and T_EXIT_FAIL if the test case failed.

axboe · 2024-01-26T16:28:15Z

It already runs goto out which returns 0, when ftruncate is not supported.

It does, but it doesn't return T_EXIT_SKIPPED. I think you should also distinguish between "we got EBADF/EINVAL on the first test, it's probably not supported, T_EXIT_SKIPPED" rather than just have any of the test cases returning EINVAL leading to a skipped test where it probably should've been a failure.

ammarfaizi2

Missing Signed-off-by tag.

ammarfaizi2 · 2024-01-31T14:37:17Z

src/liburing-ffi.map

@@ -191,4 +191,5 @@ LIBURING_2.6 {
 	global:
 		io_uring_prep_fixed_fd_install;
 		io_uring_buf_ring_available;
+    io_uring_prep_ftruncate;


Use tabs as indentation; don't use spaces.

ammarfaizi2 · 2024-01-31T14:39:45Z

test/truncate.c

+		goto err;
+	}
+
+  return 0;


Same indentation issue.

ammarfaizi2 · 2024-01-31T14:43:55Z

test/truncate.c

+	ret = cqe->res;
+	io_uring_cqe_seen(ring, cqe);
+	if (cqe->res != -EINVAL) {
+		fprintf(stderr, "unexpected truncate res %d\n", cqe->res);
+		goto err;
+	}
+
+  return 0;


After this assignment, ret = cqe->res, the ret value is not used. Shouldn't it be returned?

Also, goto err is return 1 while your caller checking is:

ret = test_truncate(&ring, fd); if (ret < 0) goto err;

So test_truncate() will always be assumed as a success because it never returns a negative value.

Signed-off-by: Tony Solomonik <tony.solomonik@gmail.com>

axboe · 2024-01-31T16:03:07Z

test/truncate.c

+	}
+	ret = cqe->res;
+	io_uring_cqe_seen(ring, cqe);
+	if (cqe->res != -EINVAL) {


Doesn't matter too much here, but you cannot use the cqe after calling io_uring_cqe_seen() as it could've been overwritten by a new cqe. You assign 'ret' here anyway, so please just use ret. Would be a shame if people copied an error like that for their app.

Signed-off-by: Tony Solomonik <tony.solomonik@gmail.com>

ammarfaizi2 · 2024-01-31T16:20:18Z

test/truncate.c

+	sqe = io_uring_get_sqe(ring);
+	if (!sqe) {
+		fprintf(stderr, "get sqe failed\n");
+		goto err;
+	}


goto err; contains return ret, but ret is uninitialized; stop using goto if you don't have any cleanup. Just return -1;

My mistake, it turns out return 1; Not return ret.

ammarfaizi2 · 2024-01-31T16:21:04Z

test/truncate.c

+	ret = io_uring_submit(ring);
+	if (ret <= 0) {
+		fprintf(stderr, "sqe submit failed: %d\n", ret);
+		goto err;
+	}


If we assume io_uring_submit() returns 0 is failed, then you must override it with return -1 because return 0 is okay from the caller prespective.

Sorry for the noise, I probably read the old code before you force pushed.

ammarfaizi2 · 2024-01-31T16:26:50Z

test/truncate.c

+err:
+	return 1;
+}


Return 1 is not an error as you have this in the caller:

ret = test_ftruncate(&ring, fd, test_sizes[i]); if (ret < 0) { // fail }

Must be return -1?

Ah no, it has an else if, my mistake again...

Introduces `opcode::Ftruncate` to support the ftruncate which got supported since Linux 6.9. Also updates the libc to the latest version to get the new sys::IORING_OP_FTRUNCATE flag from libc. See - `IORING_OP_FTRUNCATE` commit: torvalds/linux@b4bb190 - liburing PR: axboe/liburing#1004

tontinton force-pushed the truncate branch 2 times, most recently from a098ef9 to 627c71e Compare November 24, 2023 10:06

tontinton changed the title ~~Add truncate + ftruncate~~ [GIT PULL] Add truncate + ftruncate Nov 24, 2023

tontinton force-pushed the truncate branch 2 times, most recently from d4f6968 to aed55d3 Compare January 23, 2024 13:19

tontinton changed the title ~~[GIT PULL] Add truncate + ftruncate~~ [GIT PULL] Add ftruncate Jan 23, 2024

tontinton force-pushed the truncate branch 2 times, most recently from d555e2f to 8f0c5c6 Compare January 23, 2024 13:31

tontinton force-pushed the truncate branch 2 times, most recently from f51b164 to fd1bd0e Compare January 23, 2024 21:12

axboe reviewed Jan 26, 2024

View reviewed changes

tontinton force-pushed the truncate branch from fd1bd0e to a1e5529 Compare January 27, 2024 10:22

tontinton requested a review from axboe January 27, 2024 10:24

ammarfaizi2 suggested changes Jan 27, 2024

View reviewed changes

tontinton force-pushed the truncate branch from a1e5529 to f45c5c1 Compare January 27, 2024 11:57

tontinton requested a review from ammarfaizi2 January 27, 2024 11:57

ammarfaizi2 suggested changes Jan 31, 2024

View reviewed changes

Add ftruncate helpers

63a8e8a

Signed-off-by: Tony Solomonik <tony.solomonik@gmail.com>

tontinton force-pushed the truncate branch from f45c5c1 to c023393 Compare January 31, 2024 15:52

tontinton requested a review from ammarfaizi2 January 31, 2024 15:54

axboe reviewed Jan 31, 2024

View reviewed changes

tontinton force-pushed the truncate branch from c023393 to 71519a3 Compare January 31, 2024 16:10

tontinton requested a review from axboe January 31, 2024 16:10

test/truncate: Add test for ftruncate

d4af166

Signed-off-by: Tony Solomonik <tony.solomonik@gmail.com>

tontinton force-pushed the truncate branch from 71519a3 to 31acf01 Compare January 31, 2024 16:18

tontinton added 2 commits January 31, 2024 18:18

test/truncate: Add test for failure on truncate path

41950c1

Signed-off-by: Tony Solomonik <tony.solomonik@gmail.com>

man/io_uring_prep_ftruncate: Add the new ftruncate command

ed3e6f5

Signed-off-by: Tony Solomonik <tony.solomonik@gmail.com>

tontinton force-pushed the truncate branch from 31acf01 to ed3e6f5 Compare January 31, 2024 16:18

ammarfaizi2 suggested changes Jan 31, 2024

View reviewed changes

ammarfaizi2 reviewed Jan 31, 2024

View reviewed changes

axboe merged commit 5909e17 into axboe:master Feb 9, 2024

vmingchen mentioned this pull request Dec 5, 2024

Add support for ftruncate tokio-rs/io-uring#310

Closed

vmingchen mentioned this pull request Dec 30, 2024

feature: Add File::truncate for async file truncation tokio-rs/tokio-uring#317

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GIT PULL] Add ftruncate #1004

[GIT PULL] Add ftruncate #1004

tontinton commented Nov 24, 2023 •

edited

Loading

axboe commented Jan 23, 2024

axboe commented Jan 25, 2024

tontinton commented Jan 26, 2024

axboe Jan 26, 2024

tontinton Jan 27, 2024

axboe Jan 26, 2024

tontinton Jan 27, 2024

axboe commented Jan 26, 2024

ammarfaizi2 left a comment

ammarfaizi2 Jan 31, 2024

tontinton Jan 31, 2024

ammarfaizi2 Jan 31, 2024

tontinton Jan 31, 2024

ammarfaizi2 Jan 31, 2024

tontinton Jan 31, 2024

axboe Jan 31, 2024

tontinton Jan 31, 2024

ammarfaizi2 Jan 31, 2024

ammarfaizi2 Jan 31, 2024

ammarfaizi2 Jan 31, 2024

ammarfaizi2 Jan 31, 2024

ammarfaizi2 Jan 31, 2024

ammarfaizi2 Jan 31, 2024

+              		}
+              	}
+              out:

[GIT PULL] Add ftruncate #1004

[GIT PULL] Add ftruncate #1004

Conversation

tontinton commented Nov 24, 2023 • edited Loading

git request-pull output:

Pull Request Guidelines

Commit message format rules:

Commit message example:

By submitting this pull request, I acknowledge that:

axboe commented Jan 23, 2024

axboe commented Jan 25, 2024

tontinton commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axboe commented Jan 26, 2024

ammarfaizi2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tontinton commented Nov 24, 2023 •

edited

Loading