lkl: posix: use preadv/pwritev for block I/O #563

ddiss · 2025-02-17T01:32:23Z

preadv and pwritev have been around for a long time, and should be usable on BSD and Linux (v2.6.30, glibc 2.10, Bionic API level 24). This reduces the number of syscalls for multi-iov block requests and also fixes the do_rw() callback "strict-prototypes" compiler errors.

TODO:

short read/write fallback or error?
IOV_MAX checks

ddiss · 2025-02-17T01:46:56Z

I kept this as a draft, as I'm not sure whether short read/write fallback is needed... For Linux I think there are fifo and possibly network FS edge cases which might need fallback.

ddiss · 2025-02-17T09:41:40Z

v2:

add short read/write fallback by retaining old do_rw() helper

ddiss · 2025-02-17T09:48:55Z

TODO:
...
* IOV_MAX checks

preadv/pwritev errors are propagated for undersize or oversize iovcnt.

ddiss · 2025-02-17T10:00:00Z

android-aarch64 is failing:

/home/ubuntu/project/tools/lkl/liblkl.a(liblkl-in.o): In function `blk_request':
/home/ubuntu/project/tools/lkl/lib/posix-host.c:699: undefined reference to `pwritev'
/home/ubuntu/project/tools/lkl/lib/posix-host.c:689: undefined reference to `preadv'
collect2: error: ld returned 1 exit status
Makefile:107: recipe for target '/home/ubuntu/project/tools/lkl/tests/disk' failed
make: *** [/home/ubuntu/project/tools/lkl/tests/disk] Error 1

I did check to confirm that Bionic carried preadv/pwritev (API level 24). Not sure why we're getting a linker error here. Will need to investigate further...

ddiss · 2025-02-17T10:08:02Z

android-aarch64 is failing:
...
I did check to confirm that Bionic carried preadv/pwritev (API level 24). Not sure why we're getting a linker error here. Will need to investigate further...

Bionic commit 6f4594d5d ("Add preadv/pwritev.") is dated 2015.

ddiss · 2025-02-17T12:59:37Z

android-aarch64 is failing:
...
I did check to confirm that Bionic carried preadv/pwritev (API level 24). Not sure why we're getting a linker error here. Will need to investigate further...

Bionic commit 6f4594d5d ("Add preadv/pwritev.") is dated 2015.

I came across https://raw.githubusercontent.com/thehajime/lkl-docker/refs/heads/master/circleci/android/arm64/Dockerfile which indicates that we're using an ancient android-ndk-r15b with --api 23. It might just be a matter of bumping it to --api 24, but we should probably use this chance to move to a more modern Android build process.
@thehajime would cimg/android:2025.02.1-ndk from https://circleci.com/developer/images/image/cimg/android perhaps be an option?

tavip · 2025-02-17T18:59:44Z

We used preadv until 04a86da which I think we removed to support Android ARM (32bits) which at the time didn't had support for it.

LGTM once we fix the Android build issue. Perhaps ifdef for Android to use do_rw while we work on upgrading the CI?

tavip · 2025-02-17T19:02:09Z

tools/lkl/lib/posix-host.c

+			break;
+		}
+		if (len != sum_iovlen(req->buf, req->count))
+			err = do_rw(pwrite_fn, disk.fd, req);


Is it worth continuing with preadv/pwritev from where we left off instead of starting from scratch with read/write?

I did consider that, but given that short read/write should be very rare, I don't think it's worth the extra complexity.

ddiss · 2025-02-17T22:39:26Z

We used preadv until 04a86da which I think we removed to support Android ARM (32bits) which at the time didn't had support for it.

Ah interesting, thanks for the background (and code review!)

LGTM once we fix the Android build issue. Perhaps ifdef for Android to use do_rw while we work on upgrading the CI?

I'll see how I go with updating the CI images and proceed from there.

ddiss · 2025-02-17T23:36:24Z

...

LGTM once we fix the Android build issue. Perhaps ifdef for Android to use do_rw while we work on upgrading the CI?

I'll see how I go with updating the CI images and proceed from there.

I've submitted the lkl: posix: rework do_rw() types change separately via #565 and will keep this open to track the Android CI work required for preadv/pwritev use.

thehajime · 2025-02-18T08:48:53Z

I came across https://raw.githubusercontent.com/thehajime/lkl-docker/refs/heads/master/circleci/android/arm64/Dockerfile which indicates that we're using an ancient android-ndk-r15b with --api 23. It might just be a matter of bumping it to --api 24, but we should probably use this chance to move to a more modern Android build process.
@thehajime would cimg/android:2025.02.1-ndk from https://circleci.com/developer/images/image/cimg/android perhaps be an option?

with a quick patch of below to lkl-docker,

diff --git a/circleci/android/arm64/Dockerfile b/circleci/android/arm64/Dockerfile
index e86fd6d..10abae4 100644
--- a/circleci/android/arm64/Dockerfile
+++ b/circleci/android/arm64/Dockerfile
@@ -4,7 +4,7 @@ FROM $IMAGE_NAME
 LABEL authors="Hajime Tazaki <[email protected]>, Octavian Purdila <[email protected]>"

 # install toolchain from NDK
-RUN ./android-ndk-r15b/build/tools/make_standalone_toolchain.py --arch arm64 --api 23 --install-dir ./aarch64-linux-android && \
+RUN ./android-ndk-r15b/build/tools/make_standalone_toolchain.py --arch arm64 --api 24 --install-dir ./aarch64-linux-android && \
     rm -rf android-ndk-r15b

 # update toolchain, see https://github.com/lkl/linux/issues/348#issuecomment-312409409

the following commit can build without the link failure.

commit e825ddcd72ed66cdb52f50329cc726a3cd6bafc8 (HEAD -> pr-563, origin/pr-563, origin/HEAD)
Author: David Disseldorp <[email protected]>
Date:   Fri Feb 14 14:27:49 2025 +0100

    lkl: posix: use preadv/pwritev for block I/O

    preadv and pwritev have been around for a long time, and should be
    usable on BSD and Linux (v2.6.30, glibc 2.10, Bionic API level 24).
    Short reads or writes trigger fallback to the old do_rw() handler.

    Signed-off-by: David Disseldorp <[email protected]>

updating build environment with, for example,cimg/android:2025.02.1-ndk, might be a right way but have no immediate time to work on. any volunteer would be helpful but if there are no one, I'll allocate my time in near future.

ddiss · 2025-02-18T09:14:26Z

I came across https://raw.githubusercontent.com/thehajime/lkl-docker/refs/heads/master/circleci/android/arm64/Dockerfile which indicates that we're using an ancient android-ndk-r15b with --api 23. It might just be a matter of bumping it to --api 24, but we should probably use this chance to move to a more modern Android build process.
@thehajime would cimg/android:2025.02.1-ndk from https://circleci.com/developer/images/image/cimg/android perhaps be an option?

with a quick patch of below to lkl-docker,
diff --git a/circleci/android/arm64/Dockerfile b/circleci/android/arm64/Dockerfile
index e86fd6d..10abae4 100644
--- a/circleci/android/arm64/Dockerfile
+++ b/circleci/android/arm64/Dockerfile
@@ -4,7 +4,7 @@ FROM $IMAGE_NAME
 LABEL authors="Hajime Tazaki <[email protected]>, Octavian Purdila <[email protected]>"

 # install toolchain from NDK
-RUN ./android-ndk-r15b/build/tools/make_standalone_toolchain.py --arch arm64 --api 23 --install-dir ./aarch64-linux-android && \
+RUN ./android-ndk-r15b/build/tools/make_standalone_toolchain.py --arch arm64 --api 24 --install-dir ./aarch64-linux-android && \
     rm -rf android-ndk-r15b

 # update toolchain, see https://github.com/lkl/linux/issues/348#issuecomment-312409409
the following commit can build without the link failure.

Great \o/

...

updating build environment with, for example,cimg/android:2025.02.1-ndk, might be a right way but have no immediate time to work on. any volunteer would be helpful but if there are no one, I'll allocate my time in near future.

I'm fine with using your change as a band-aid solution for now - can you commit it, or would you like me to submit a PR? Looking at https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md it appears that we should also be able to use the regular github CI images for Android NDK builds too.

thehajime · 2025-02-18T22:02:58Z

. can you commit it, or would you like me to submit a PR?

I will do that.

This is needed to support preadv/pwritev calls, which is available after Bionic API level 24. Link: lkl/linux#563 Signed-off-by: Hajime Tazaki <[email protected]>

preadv and pwritev have been around for a long time, and should be usable on BSD and Linux (v2.6.30, glibc 2.10, Bionic API level 24). Short reads or writes trigger fallback to the old do_rw() handler. Signed-off-by: David Disseldorp <[email protected]>

ddiss · 2025-02-20T00:45:42Z

v3:

rebase atop CI image changes

ddiss · 2025-02-20T02:29:57Z

The android build is now passing thanks to @thehajime 's image changes. The Windows failure appears unrelated (failure to download openvpn).

thehajime · 2025-02-20T02:53:25Z

thanks @ddiss !

The Windows failure appears unrelated (failure to download openvpn).

rerun the failed test and seems to be fine.

tavip

LGTM, thanks @ddiss !

ddiss force-pushed the preadv_pwritev_blkio branch from 78e582c to e825ddc Compare February 17, 2025 09:40

ddiss marked this pull request as ready for review February 17, 2025 09:40

tavip reviewed Feb 17, 2025

View reviewed changes

ddiss changed the title ~~WIP: lkl: posix: use preadv/pwritev for block I/O~~ lkl: posix: use preadv/pwritev for block I/O Feb 18, 2025

thehajime mentioned this pull request Feb 19, 2025

Fix android preadv lkl/lkl-docker#14

Merged

ddiss force-pushed the preadv_pwritev_blkio branch from e825ddc to ca21656 Compare February 20, 2025 00:44

tavip approved these changes Feb 20, 2025

View reviewed changes

tavip merged commit e2b7eed into lkl:master Feb 20, 2025
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lkl: posix: use preadv/pwritev for block I/O #563

lkl: posix: use preadv/pwritev for block I/O #563

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025 •

edited

Loading

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

tavip commented Feb 17, 2025 •

edited

Loading

tavip Feb 17, 2025

ddiss Feb 17, 2025

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

thehajime commented Feb 18, 2025

ddiss commented Feb 18, 2025

thehajime commented Feb 18, 2025

ddiss commented Feb 20, 2025

ddiss commented Feb 20, 2025

thehajime commented Feb 20, 2025

tavip left a comment

lkl: posix: use preadv/pwritev for block I/O #563

lkl: posix: use preadv/pwritev for block I/O #563

Conversation

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025 • edited Loading

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

tavip commented Feb 17, 2025 • edited Loading

tavip Feb 17, 2025

Choose a reason for hiding this comment

ddiss Feb 17, 2025

Choose a reason for hiding this comment

ddiss commented Feb 17, 2025

ddiss commented Feb 17, 2025

thehajime commented Feb 18, 2025

ddiss commented Feb 18, 2025

thehajime commented Feb 18, 2025

ddiss commented Feb 20, 2025

ddiss commented Feb 20, 2025

thehajime commented Feb 20, 2025

tavip left a comment

Choose a reason for hiding this comment

ddiss commented Feb 17, 2025 •

edited

Loading

tavip commented Feb 17, 2025 •

edited

Loading