geom_gate: Fix Distinguish between errors
ClosedPublic
Actions

Authored by peterj on Jan 21 2022, 11:02 PM.

Details

Reviewers

jo_bruelltuete.com
asomers

Commits

rGa65f526addaf: geom_gate: Distinguish between classes of errors
rGafcd1210246b: geom_gate: Distinguish between classes of errors

Summary

The geom_gate API provides 2 distinct paths for exchanging error details between the kernel and the userland client: Including an error code in the g_gate_ctl_io structure passed in the ioctl(2) call or having the ioctl(2) call return -1. The latter reflects errors in the ioctl(2) call itself whilst the former reflects errors within the geom_gate instance.

The G_GATE_CMD_START ioctl blocks waiting for an I/O request to be directed to the geom_gate instance and the wait can fail (necessitating an error return) if the geom_gate instance is destroyed or if the msleep(9) fails. The code previously treated both error cases indentically: Returning ECANCELED as a geom_gate instance error (which the ggatec treats as a fatal error). Whilst this is the correct behaviour if the geom_gate instance is destroyed, a msleep(9) failure is unrelated to the geom_gate instance itself and should be reported as an ioctl(2) "failure". The distinction is important because msleep(9) can return ERESTART, which means the system call should be retried (and this will occur automatically as part of the generic syscall return processing).

The attached patch changes the msleep(9) handling to directly return the error code from msleep(9), which ensures ERESTART is correctly handled, rather than being treated as a fatal error.

Test Plan

Without this patch, I was fairly consistently seeing ECANCELED errors being returned to ggatec (actually, my reimplementation of it) when running geli init -P -K tank.key ggate0 on my SMP system. I couldn't reproduce the error on a UP VBox instance.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

peterj requested review of this revision.Jan 21 2022, 11:02 PM

peterj created this revision.

I think this is a good idea. But I'm not in a position to test this right now. I've also never noticed any cancelations. Does your ggatec get a lot of signals?

[...] can return ERESTART, which means the system call should be retried (and this will occur automatically as part of the generic syscall return processing).

This is interesting, to me. I didnt know nor expect this to happen. If any ioctl handler on the kernel side returns ERESTART then the user mode side will automatically call that same handler again? That needs great care to make sure that anything up to the returning bits was side-effect free.

sys/geom/gate/g_gate.c
864–866	do you think it's worth it to put a comment here, along the lines of: "can fail with ERESTART, without any negative affect on the ggate instance. ioctl handler will just call this code again automatically".

This revision is now accepted and ready to land.Jan 26 2022, 12:18 AM

In D33996#769676, @jo_bruelltuete.com wrote:

I think this is a good idea. But I'm not in a position to test this right now. I've also never noticed any cancelations. Does your ggatec get a lot of signals?

None at all as far as I'm aware. I can only assume that something in the geli initialisation path is sometimes causing ERESTART signals as part of its geom interaction inside the kernel.

[...] can return ERESTART, which means the system call should be retried (and this will occur automatically as part of the generic syscall return processing).

This is interesting, to me. I didnt know nor expect this to happen. If any ioctl handler on the kernel side returns ERESTART then the user mode side will automatically call that same handler again? That needs great care to make sure that anything up to the returning bits was side-effect free.

The ERESTART handling is part of the machine-dependent code - see eg https://cgit.freebsd.org/src/tree/sys/amd64/amd64/vm_machdep.c#n569, which patches the return address to point back at the syscall that is being returned from, so the code returns to userland and then immediately repeats the syscall.

I agree that it's important that the syscall be idempotent up until the ERESTART point but I believe that it's safe in this case because the msleep(9) can't be hit if any IO processing has occurred inside the geom_gate code.

sys/geom/gate/g_gate.c
864–866	do you think it's worth it to put a comment here, along the lines of: "can fail with ERESTART, without any negative affect on the ggate instance. ioctl handler will just call this code again automatically". It can return either ERESTART or EINTR. The former is transparently handled inside the kernel. The latter will bubble up to userland and cause ggatec to abort (much the same as at present). I don't think the comment would add much value because my patch is altering the behaviour to match normal FreeBSD behaviour of just returning an "error" via the standard error path. The existing code is abnormal because it's bypassing the standard return path.