Details

Reviewers

kib
asomers

Commits

rG24fb405260ed: fusefs: Fix returned error code in fuse_device_write()
rGf9c9122d5ab9: fusefs: First take on exterrorizing

Summary

I'm pretty confident with my changes to fuse_device.c, but
fuse_vfsops.c wasn't that obvious.

Test Plan

make buildkernel

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

arrowd created this revision.Jun 13 2025, 10:41 AM

Herald added a subscriber: imp. · View Herald TranscriptJun 13 2025, 10:41 AM

arrowd requested review of this revision.Jun 13 2025, 10:41 AM

Harbormaster completed remote builds in B64841: Diff 156961.Jun 13 2025, 10:41 AM

kib added inline comments.Jun 13 2025, 11:17 AM

sys/fs/fuse/fuse_device.c
198
369	The point is to have different error messages for same error codes but different places.
410
411	Continuation line should have indent +4 spaces.
416	Also same note about indent, there and below about [].

This is an interesting new feature. But how do we use it? There aren't any examples yet, or any man page. We could easily add test cases for the mount and kqueue errors to the fusefs test suite.

sys/fs/fuse/fuse_device.c
412	"SET_ERROR0" includes the "ktrexterr" dtrace probe. That makes this custom SDT probe redundant. I think you should delete it. Also, delete every other "trace" probe that's adjacent to SET_ERROR0. The fusefs "trace" probes aren't very useful anyway. They're leftover from the original GSoC contribution. I rarely use them.
462
530	It's no longer a "Linux" errno by this point.
580	Because ohead.unique == 0, we aren't looking at an error code. We're looking at a notification code. And it isn't necessarily unknown; it could be a fallthrough from one of the unimplemented codes.
sys/fs/fuse/fuse_vfsops.c
412

This revision now requires changes to proceed.Jun 13 2025, 3:55 PM

arrowd added a parent revision: D50836: Make SET_ERRORX() an expression evaluating to the error number.Jun 13 2025, 5:45 PM

kib added inline comments.Jun 13 2025, 5:46 PM

sys/fs/fuse/fuse_device.c
412	There is no dtrace probe in SET_ERRORX(). It provides ktrace tracing only, at least ATM. Removal of the SDT probes should be a separate commit. It might be that Gleb should just not bother to de-dup the string literals, instead copy-pasting them from SDT to SET_ERROR() for this change.

asomers added inline comments.Jun 13 2025, 6:12 PM

sys/fs/fuse/fuse_device.c
412	What about fbt:kernel:ktrexterr:entry ?

kib added inline comments.Jun 13 2025, 7:44 PM

sys/fs/fuse/fuse_device.c
412	I do not object, but it should be made by somebody else, not me. Feel free to add it.

Address comments
Make use of SET_ERROR*() becoming an expression

Harbormaster completed remote builds in B64894: Diff 157088.Jun 16 2025, 7:06 AM

I'm going to submit another diff that removes trace probes after finishing with exterrorizing.

sys/fs/fuse/fuse_device.c
369	Yes, but I don't understand the code well enough to discern between these 3 ENODEV returns. Maybe @asomers can help to reformulate these exterrors? On the other hand, this is still more descriptive than just returning ENODEV without any exterror.

There is also D50845 waiting for review.

arrowd added a parent revision: D50845: exterr: convert SET_ERRORX() to pseudo-variadic EXTERROR().Jun 16 2025, 9:41 AM

Use EXTERROR

Harbormaster completed remote builds in B64907: Diff 157108.Jun 16 2025, 3:59 PM

kib added inline comments.Jun 16 2025, 4:12 PM

sys/fs/fuse/fuse_device.c
198	and remove the next line.

Address comments

Harbormaster completed remote builds in B64908: Diff 157110.Jun 16 2025, 4:20 PM

kib accepted this revision.Jun 16 2025, 4:26 PM

asomers requested changes to this revision.Jun 16 2025, 4:49 PM

asomers added inline comments.

sys/fs/fuse/fuse_vfsops.c
592–595	It looks to me like you rebased your code? There should be another EXTERROR here.

This revision now requires changes to proceed.Jun 16 2025, 4:49 PM

arrowd added inline comments.Jun 16 2025, 5:09 PM

sys/fs/fuse/fuse_vfsops.c
592–595	It looks to me like you rebased your code? Yes, I did that to make use of EXTERROR. There should be another EXTERROR here. Should I deduplicate the error string via static char array here?

asomers added inline comments.Jun 16 2025, 5:56 PM

sys/fs/fuse/fuse_vfsops.c
592–595	It looks to me like you rebased your code? Yes, I did that to make use of EXTERROR. There should be another EXTERROR here. Should I deduplicate the error string via static char array here? Yes please. Unlike the dtrace probe, the `fuse_warn` will not be going away.

exterrorize one more errno

Harbormaster completed remote builds in B64915: Diff 157122.Jun 16 2025, 6:13 PM

arrowd marked 2 inline comments as done.Jun 16 2025, 6:13 PM

This change is ok with me. But I would still like to see a man page and some usage examples from userland.

This revision is now accepted and ready to land.Jun 16 2025, 6:20 PM

What man page? The information on how to read exterrors is located in kdumps manpage.

In D50831#1161438, @arrowd wrote:

What man page? The information on how to read exterrors is located in kdumps manpage.

Where? That man page hasn't been changed since 2022. And uexterr_gettext and exterrctl need man pages.

In D50831#1161439, @asomers wrote:

Where? That man page hasn't been changed since 2022.

Ah, sorry, it is in ktrace man page: 96f4be881e8e9e0cb9a6ad2cd9f17f4440983600

And uexterr_gettext and exterrctl need man pages.

Yes, but this is orthogonal to this diff, I think?

In D50831#1161439, @asomers wrote:

In D50831#1161438, @arrowd wrote:

What man page? The information on how to read exterrors is located in kdumps manpage.

Where? That man page hasn't been changed since 2022. And uexterr_gettext and exterrctl need man pages.

We did not finished with stabilizing the kernel interface yet, and not much started looking at the proper userspace facilities. It might end up built-in into err()/warn() so uexterr_gettext() would be left as some internal function. I simply did not considered that we are there yet.

For the exterrctl(2), the man page would be eventually added, but with the warning similar to that for thr*(2) and sigfastblock(2) that the interface is for C runtime and should not be used by the app code.

In D50831#1161443, @kib wrote:

We did not finished with stabilizing the kernel interface yet, and not much started looking at the proper userspace facilities. It might end up built-in into err()/warn() so uexterr_gettext() would be left as some internal function. I simply did not considered that we are there yet.

For the exterrctl(2), the man page would be eventually added, but with the warning similar to that for thr*(2) and sigfastblock(2) that the interface is for C runtime and should not be used by the app code.

So is it intended that kdump/ktrace an err/warn will be the primary way to view these messages?

In D50831#1161444, @asomers wrote:

In D50831#1161443, @kib wrote:

We did not finished with stabilizing the kernel interface yet, and not much started looking at the proper userspace facilities. It might end up built-in into err()/warn() so uexterr_gettext() would be left as some internal function. I simply did not considered that we are there yet.

For the exterrctl(2), the man page would be eventually added, but with the warning similar to that for thr*(2) and sigfastblock(2) that the interface is for C runtime and should not be used by the app code.

So is it intended that kdump/ktrace an err/warn will be the primary way to view these messages?

Probably warn/err when I enable them. Ktrace is indeed already there, but it cannot be the primary user interface.
I do not want to do the more intrusive pass on userspace to avoid the needs to iterate, as I have to do for vm/vm_mmap.c in kernel.

Closed by commit rGf9c9122d5ab9: fusefs: First take on exterrorizing (authored by arrowd). · Explain WhyJun 17 2025, 6:11 AM

This revision was automatically updated to reflect the committed changes.

arrowd added a commit: rGf9c9122d5ab9: fusefs: First take on exterrorizing.

markj added a subscriber: markj.Jun 21 2025, 12:00 AM

markj added inline comments.