Various fixes for ggatec and ggated

[jo_bruelltuete.com]

Summary

Dynamically size buffers in ggatec. Instead of static size on the stack.
Add flush support.

Test Plan

Tested it on my machine. I'm still on 12-stable.

[jo_bruelltuete.com]

sbin/ggate/ggatec/ggatec.c

155

don't need this anymore now that recv thread will resize its buffer.

[jo_bruelltuete.com]

sbin/ggate/ggatec/ggatec.c

117

there's diff context missing here.
but there's a switch statement that has ENOMEM ifdef'd off. but without handling ENOMEM ggatec is almost totally useless tbh. i'm constantly running into that condition just trying some simple stuff...
i've got an updated patch for it.

[jo_bruelltuete.com]

Handle oversized write buffer coming from zfs

[peterj]

In general, this is a definite improvement over fixed size buffers (that are actually liable to subtle overruns).

sbin/ggate/ggatec/ggatec.c

97

Why ssize_t? The value can never be negative and it's only used to assign to an off_t. I would suggest this be size_t.

131

I think this only warrants LOG_DEBUG, not LOG_NOTICE. Particularly with ZFS, I/Os can be arbitrary sized so it's quite normal for them to exceed MAXPHYS. That's likely to make this noisy, unless there's a particularly write early on.

132

style(9) requires that all variable declarations come at the top of the function.

208

As per the comment above, I don't think ssize_t is the most appropriate type here.

245

Variable declarations should be at the top of the function.

[jo_bruelltuete.com]

sbin/ggate/ggatec/ggatec.c

97

copy pasta, size_t is good.

131

Yep noticed quite a bit of log spam myself.

132

If we ever update style I think that'd be a good rule to get rid of... i'm a big fan of declare & define & init on first use, esp since it has safety implications.
btw is the compiler supposed to warn/complain about this? (it didn't)

[jo_bruelltuete.com]

sbin/ggate/ggatec/ggatec.c

208

ah, ggio.gctl_length is off_t and at line 244 we have a comparison to buf_capacity.
with ssize_t we can avoid casting and its visual clutter.

[jo_bruelltuete.com]

sbin/ggate/ggated/ggated.c

753 ↗

(On Diff #95348)

this seems prudent.
otherwise unsupported requests (like maybe flush or trim from a newer version of ggatec) would get EIO, but in a not so obvious way.

[jo_bruelltuete.com]

add flush support

[asomers]

sbin/ggate/ggatec/ggatec.c

104

In CURRENT, maxphys can vary at runtime. You need to look it up with sysctlbyname. The node is kern.maxphys.

255

You should cuddle the else { on the same line as the }

[jo_bruelltuete.com]

sbin/ggate/ggatec/ggatec.c

104

it being maxphys is kinda a red herring, just need a (any) value for the initial buffer size.
we could put a literal 4096 here. The buffer is resized as necessary.
I've been using this patch for a few weeks now (on 12-stable) and the buffer size will eventually rise up to 1 MB.

[jo_bruelltuete.com]

sbin/ggate/ggatec/ggatec.c

104

hi @asomers, coming back to this. Is it important that a running ggatec adapts to the changing value of maxphys?

[asomers]

sbin/ggate/ggatec/ggatec.c

104

Yes, it should use the runtime value rather than the compile-time one. It doesn't need to adapt if maxphys changes after ggatec starts, however.

[jo_bruelltuete.com]

MAXPHYS no more, instead use sysctl kern.maxphys

[asomers]

LGTM, except for the style nit on line 256 of ggatec.c .

[jo_bruelltuete.com]

Wrap a few long lines.

[jo_bruelltuete.com]

Missed the else cuddle