Page MenuHomeFreeBSD

Various fixes for ggatec and ggated
ClosedPublic

Authored by jo_bruelltuete.com on Aug 30 2021, 1:22 AM.

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sbin/ggate/ggatec/ggatec.c
155

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

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.

Handle oversized write buffer coming from zfs

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.

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)

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 added inline comments.
sbin/ggate/ggated/ggated.c
753

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 retitled this revision from ggatec: dynamic buffers to Various fixes for ggatec and ggated.
jo_bruelltuete.com edited the summary of this revision. (Show Details)

add flush support

asomers requested changes to this revision.Oct 29 2021, 7:52 PM
asomers added inline comments.
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 }

This revision now requires changes to proceed.Oct 29 2021, 7:52 PM
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.

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?

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.

MAXPHYS no more, instead use sysctl kern.maxphys

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

This revision is now accepted and ready to land.Jan 2 2022, 3:36 AM
This revision now requires review to proceed.Jan 3 2022, 12:30 AM
This revision is now accepted and ready to land.Jan 3 2022, 12:45 AM
This revision was automatically updated to reflect the committed changes.