Page MenuHomeFreeBSD

GEOM: Reduce unnecessary log interleaving with sbufs
ClosedPublic

Authored by cem on Tue, Aug 6, 3:07 AM.

Details

Summary

Similar to what was done for device_printfs in r347229.

Test Plan

This is a proof of concept. If it seems reasonable, I'll extend it to the rest
of the copy-pasted DEBUG() macros in GEOM.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 25730
Build 24312: arc lint + arc unit

Event Timeline

cem created this revision.Tue, Aug 6, 3:07 AM
markj added a comment.Tue, Aug 6, 3:28 AM

It seems reasonable to me.

cem updated this revision to Diff 60491.Tue, Aug 6, 5:45 AM

Convert the rest of the GEOM drivers

cem updated this revision to Diff 60492.Tue, Aug 6, 6:01 AM
  • Fix a missed case or two (didn't have "DEBUG" in the names)
  • Inline remaining g_print_bio() callers and remove the API; document g_format_bio() in g_bio.9
rlibby added a comment.Tue, Aug 6, 6:28 AM

I like that this also moves the logging code text out of line.

sys/geom/geom_int.h
40–41 ↗(On Diff #60486)

I feel like this may cause consternation (non-standard, not really used in the project, and other geom headers use ifndef include guards). However, I have no love for the bad old way either.

46–47 ↗(On Diff #60486)

(Have we tried to get FreeBSD to take #define __UNIQ __CONCAT(__uniq, __COUNTER__) ?)

sys/geom/geom_io.c
1040

Why __unused?

sys/geom/label/g_label.h
37

I think it would be preferable not to include geom_int.h in headers. I don't think it's necessary just to define macros in terms of _GEOM_DEBUG().

cem added a comment.Tue, Aug 6, 3:45 PM

I like that this also moves the logging code text out of line.

Thanks for taking a look!

sys/geom/geom_int.h
40–41 ↗(On Diff #60486)

I sneak it in wherever I can, until someone complains. I'll replace it if you insist, but otherwise, it's supported by every compiler we could conceivably care about.

46–47 ↗(On Diff #60486)

I'm not sure. Do you know how __COUNTER__ interacts with reproducible builds? If it's per-CU, that seems more readily reproducible than any other scheme.

Would it provide any benefit here? Worst case, this is a new scope and these variables shadow the external ones, right? Triggering -Wshadow, which is annoying with -Werror.

Unless ctrlvar or biop is literally named __{control,level} I think we're ok without __UNIQ in this instance. I might be missing something :-).

sys/geom/geom_io.c
1040

(Context for readers: This is about the struct sbuf *sbp __unused, which is no longer here but can be found in geom_subr.c.)

I only use the pointer to error-check the sbuf_new() return. And the error checking is only a KASSERT(). KASSERT is less like Isilon's ASSERT() and more like our ASSERT_DEBUG(), i.e., it is compiled out in !INVARIANTS builds.

Without __unused, the compiler would produce -Wunused-but-set-variable (included in -Wall) for !INVARIANTS configs, breaking the -Werror build.

sys/geom/label/g_label.h
37

To avoid the include, I think I have two options:

  • I would have to move the geom_int.h include into a slightly greater number of C files instead, or
  • I would have to move the _GEOM_DEBUG define into geom.h, which is already included by everything.

Of these two, I'd prefer the latter. My reservation with it is that geom.h is kind of the public API of GEOM, and consumed more broadly than just GEOM object classes.

Did you have something else in mind? Or a preference among these two?

rlibby added inline comments.Tue, Aug 6, 6:01 PM
sys/geom/geom_int.h
46–47 ↗(On Diff #60486)

Yeah, it was just a question, I definitely wouldn't insist on doing that for this review. Actually I had thought it was in the standard, but apparently it isn't. Perhaps that's why we haven't tried to upstream it. Yes, I believe it's set to 0 at the start of each CU.

There's probably no practical benefit here, but a theoretical one. It's as you noted about ctlvar and biop, but it also affects the format varargs.

I ran into a similar issue recently with fail points, which had me thinking about it.

sys/geom/geom_io.c
1040

Ah, yes. Thanks.

sys/geom/label/g_label.h
37

One of those two seems fine. I would have a slight preference for the former, for the API reasons. Another alternative might be a new geom_log.h or geom_dbg.h, which you'd then have to go include appropriately.

This isn't something I would insist on in any case, it just seemed non-optimal to be including an internals header in other headers like that.

markj added a comment.Tue, Aug 6, 6:22 PM

My inline comments are just nitpicking.

Since we now have a few inline code fragments that create an sbuf and use it to print a BIO to the console with some optional prefix, doesn't that suggest that we should keep a version of g_bio_print() that does exactly that? For logging errors we could move the error value into the prefix. I think it would be inconvenient to have to write several lines of boilerplate any time one wants to just print a BIO to the console.

share/man/man9/Makefile
1008

This is no longer sorted.

share/man/man9/g_bio.9
288

It doesn't really make sense to have these be separate printf() calls.

sys/geom/geom_subr.c
82

gcls isn't used as a name anywhere else; class or classname would be more clear to me.

cem added a comment.Tue, Aug 6, 7:32 PM

Thanks!

Since we now have a few inline code fragments that create an sbuf and use it to print a BIO to the console with some optional prefix, doesn't that suggest that we should keep a version of g_bio_print() that does exactly that?

Sure, that would be fine.

For logging errors we could move the error value into the prefix.

Hm, I don't follow.

I think it would be inconvenient to have to write several lines of boilerplate any time one wants to just print a BIO to the console.

Well, sure. It appears no one is actually using this function => I just figured no one wants to actually do that.

share/man/man9/Makefile
1008

Will fix

share/man/man9/g_bio.9
288

Ah, sure. Will fix

sys/geom/geom_subr.c
82

I wanted to avoid using the C++ keyword class. classname is fine. Will fix.

sys/geom/label/g_label.h
37

New file is probably best, since geom_int.h is documented as "not included from classes," for whatever reason. I'll plan to do that.

markj added a comment.Tue, Aug 6, 7:59 PM

Since we now have a few inline code fragments that create an sbuf and use it to print a BIO to the console with some optional prefix, doesn't that suggest that we should keep a version of g_bio_print() that does exactly that?

Sure, that would be fine.

For logging errors we could move the error value into the prefix.

Hm, I don't follow.

I'm talking about g_vfs_done(), which prints "g_vfs_done: " <bio info> "error = %d".

I think it would be inconvenient to have to write several lines of boilerplate any time one wants to just print a BIO to the console.

Well, sure. It appears no one is actually using this function => I just figured no one wants to actually do that.

g_vfs_done(), the fdc driver and the example in the man page all do that. I can imagine it being useful for ad-hoc debugging outside of a GEOM class. (Within a class we have the debug macro.)

cem marked 7 inline comments as done.Wed, Aug 7, 3:57 PM
cem updated this revision to Diff 60544.

Address review feedback:

  • Push definition out to separate header, geom_dbg.h
  • Push includes out from class headers into individual compilation units
  • Restore a version of g_print_bio, which takes a prefix and formatted suffix (and use it in fdc, geom_vfs)
  • Update g_bio.9
  • Update / sort g_bio.9 MLINKS
markj accepted this revision.Wed, Aug 7, 4:48 PM
This revision is now accepted and ready to land.Wed, Aug 7, 4:48 PM
cem closed this revision.Wed, Aug 7, 8:35 PM