Extra ()

So if an error occurs in the loop, after some iovs are already queued, you return -1 and aio_aqueue_file() re-queues them ? Am I missing something ?

If aio_qbio returns -1, then aio_aqueue_file will queue it using the slow path, aio_process_rw.

So you confirm my observation, and it is a valid bug.

No, it's not a bug. aio_queue_file tries the fast path first, and if that doesn't work it tries the slow path. Perhaps you're thinking that aio_process_rw calls aio_qbio? It doesn't. Only aio_queue_file does.

The loop processes iovs one by one. If e.g. first iov was queued, but second causes an error, aio_queue_file() requeues everything. So first iov is queued twice.

Ok, I see what you're saying. Some of the iovs will be issued twice in that case. I think I can fix that.

struct bio * (space before star)

BTW might be mallocarray(9) is better

Why -i ?

Also the cleanup for kaio_buffer_count might be more logical in destroy_bios.

You need to qremove and unhold pages for already processed buffers.

Leftover from an earlier revision. Should be just iovcnt.

This needs to be done in the syscalls themselves as different ABIs need to populate the uio. For example, the freebsd32 versions need to use freebsd32_copyinuio rather than copyinuio. Either that or you have to make a new ops->copyinuio callback that matches the copyinuio signature. It can use copyinuio for the native ABI and freebsd32_copyinuio for freebsd32.

Good catch!

Space before * in the cast type.

Anonymous unions where added in C11. In other words, this change as is breaks system headers use in C99 mode.

Also AFAIR they are not in C++11, but you might want to recheck this claim.

aio_iovcnt is count of array elements, why it is int and not size_t, at least ? If this is done to match the uio_iovcnt type, then arguably it is type error (fortunately kernel-only).

It would definitely require an overflow check on lowering type rank assignment.

Is it a requirement for our headers to all be strictly C99 compatible? I notice that we already have many anonymous unions that aren't guarded by #ifdef KERNEL. For example, struct kinfo_file includes an anonymous union.

Even so, I could do the stupid "named union with #defines for each element` hack, if you would prefer.

I did it to match the signature of pwritev, where iovcnt is an int. Do you think I should change it?

I think it is a reasonable goal to have system headers usable for userspace in as many situations as possible.

Note that kinfo/user.h is very much FreeBSD-specific header, while aio.h is POSIX-mandated. POSIX still requires compliance with C99, so if we cannot provide aio.h usable with C99, we already lost POSIX.

I think it is a type error in preadv/pwritev as well. But I see that our preadv/pwritev have a signature matching Linux implementation. It is too late to fix this, perhaps.

Ok then.

The "named union with #defines" hack didn't work very well, because it clashed with local variables and other structs' fields that shared the same name. What do you think about this alternative? We're still POSIX compliant, if the right macros are defined. But we don't have to hack up the structure or abuse the preprocessor too badly.

Can you show the hack that 'did not work very well'.

Your patch currently provides different ABI for struct aiocb depending on the compiler flags, and most likely does not work with C++. Even if you fixed what you intended to do, it is ugly and really not needed. We have named unions working in more complex situations without a glitch.

How does it provide different ABIs depending on the compiler flags? The structure layout is the same.

Here is the named union hack:

--- a/sys/sys/aio.h
+++ b/sys/sys/aio.h
@@ -96,13 +96,13 @@ typedef struct aiocb {
 	int	aio_fildes;		/* File descriptor */
 	off_t	aio_offset;		/* File offset for I/O */
 	union {
-		volatile void *aio_buf;	/* I/O buffer in process space */
-		struct iovec *aio_iov;	/* I/O scatter/gather list */
-	};
+		volatile void *aio_un1_buf; /* I/O buffer in process space */
+		struct iovec *aio_un1_iov;  /* I/O scatter/gather list */
+	} aio_buf_un;
 	union {
-		size_t	aio_nbytes;	/* Number of bytes for I/O */
-		int	aio_iovcnt;	/* Length of aio_iov */
-	};
+		size_t	aio_un2_nbytes;	/* Number of bytes for I/O */
+		int	aio_un2_iovcnt;	/* Length of aio_iov */
+	} aio_nbytes_un;
 	int	__spare__[2];
 	void	*__spare2__;
 	int	aio_lio_opcode;		/* LIO opcode */
@@ -111,6 +111,11 @@ typedef struct aiocb {
 	struct	sigevent aio_sigevent;	/* Signal to deliver */
 } aiocb_t;
 
+#define	aio_buf		aio_buf_un.aio_un1_buf
+#define	aio_iov		aio_buf_un.aio_un1_iov
+#define	aio_nbytes	aio_nbytes_un.aio_un2_nbytes
+#define	aio_iovcnt	aio_nbytes_un.aio_un2_iovcnt
+
 #ifdef _KERNEL
 
 typedef void aio_cancel_fn_t(struct kaiocb *);

The aio_buf macro conflicted with the field of the same name in struct oaiocb, struct oaiocb32 and struct aiocb32. It would also conflict with any similarly named struct definitions or local variables in user code.

First, lets clear my note about ABI difference. I thought that your union hack changes alignment requirements but it seems that you are right.

That said, several notes:

1. userspace must not use any symbols starting with aio_, they are reserved by POSIX. See vol.2 2.2.2 Namespace section.
2. oaiocb of course can be fixed, while problem with aiocb32 perhaps means that this structure definition does not properly match struct aiocb and this needs fixing.

But looking at the stuff from some distance, why do you need aio_iov at all ? aio_buf is typed so that assignment of struct iovec * to it should just work. Then aio_nbytes is size_t. So if you use aio_buf/aio_nbytes as the pointer to iovec and counter, everything would work without the surgery on the struct aiocb.

You mean why not just overload aio_buf and aio_nbytes? I certainly could've. I used the union instead because I thought it would be clearer. As a matter of style, I don't like reusing even local variables for different purposes, let alone struct members. I'll update the review to show you what that option looks like.

This test and vectored_zvol_poll appear to be failing in CI recently (https://ci.freebsd.org/job/FreeBSD-main-amd64-test/19158/testReport/junit/sys.aio/aio_test/vectored_unaligned/). Any ideas why this could be happening?

I am not sure if it has ever succeeded in CI, most previous runs show it as skipped.

No, and I can't reproduce that failure myself. It might be a race condition in ZFS, or maybe for some other reason the zvol doesn't have the expected name.