Add sbuf streaming mode to pseudofs(9), use in linprocfs(5)

[cem]

Summary

Add a pseudofs node flag 'PFS_AUTODRAIN', which automatically emits sbuf
contents to the caller when the sbuf buffer fills. This is only
permissible if the corresponding PFS node fill function can sleep
whenever it appends to the sbuf.

linprocfs' /proc/self/maps node happens to meet this requirement.
Streaming out the file as it is composed avoids truncating the output
and also avoids preallocating a very large buffer.

Test Plan

Eliminates this problem triggered by, e.g., Chrome:

linux: pid 713 (chromium-browse): cannot fill /proc/self/maps; consider bumping PFS_MAXBUFSIZ

[emaste]

Nice work, thanks - will try to review soon if someone else doesn't first.

[emaste]

sys/fs/pseudofs/pseudofs.h

71 ↗

(On Diff #79051)

I guess these flags are documented only here

[emaste]

Looks fine to the extent of my knowledge of sbuf(9).
I mentioned this review to @markj and @kib out of band in case they have more insight.

sys/fs/pseudofs/pseudofs_vnops.c

732–735 ↗

(On Diff #79051)

I'm not sure the C99 compound literal here is an improvement over just

ssh.skip_bytes = uio->uio_offset;
ssh.uio = uio;

No real objection and not a big deal though; I mention it only in case there's some benefit I'm missing.

[emaste]

Also as it happens there was a reference to the issue fixed by this on -curent in the last day or two

[markj]

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

Why is the + 1 here? For AUTODRAIN routines it means that sbuf has to allocate 4097->8192 bytes.

[cem]

In D27047#603701, @emaste wrote:

Also as it happens there was a reference to the issue fixed by this on -curent in the last day or two

Yeah, I saw.

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

I’m not sure it makes any sense for the non-autodrain interface either. I haven’t investigated the history, but IMO it can be dropped.

732–735 ↗

(On Diff #79051)

The only real benefit is zeroing new struct fields as they are added in the future. Your proposal plus bzero would be similar. In this context I agree it is not especially useful.

[emaste]

sys/fs/pseudofs/pseudofs_vnops.c

732–735 ↗

(On Diff #79051)

Fair point. I guess padding is unspecified in the C99 compound literal still.

[cem]

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

I think the idea is that sbufs are always nul-terminated / always reserve a byte of the buffer for a trailing nul. So to the extent buflen is derived from uio_resid, rather than a truncated value, we want to fulfill the entire request. However, I think that just means we should move the + 1 above, something like:

buflen = io->uio_offset + uio->uio_resid + 1;
...
sb = sbuf_new(..., buflen, ...);
...
if (!(pn->pn_flags & PFS_AUTODRAIN)) {
    if (error == 0)
        buflen = sbuf_len(sb);
    else
        /* In overflow case, valid length does not include trailing nul. */
        buflen--;
    error = uiomove_frombuf(..., buflen, ...);
}

[cem]

• Fix buflen+1 logic. As a side effect, the maximal limit for pfs fill routines is one byte smaller (of 1MB).
• Drop compound literal for struct initialization.

[trasz]

This is much better than my workarounds, thank you!

[cem]

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

Hm, I'm reconsidering this.

(1) Chrome tries to read exactly a page at a time, and it's a bit silly to limit each chunk to 4095 bytes.

(2) sbuf itself shouldn't be 'extending' our given size, because we do not use the SBUF_AUTOEXTEND flag. So I think sbuf will just malloc(4097) bytes. Is the 4097 -> 8192 you're thinking coming from malloc(9)?

[cem]

sys/fs/pseudofs/pseudofs_vnops.c

654 ↗

(On Diff #79090)

This needs to return len + the number of bytes we skipped in the partial seek case above. Fixed in my local tree.

[markj]

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

Yes, my belief is that malloc() will end up using two whole pages for a 4097-byte allocation. Not the end of the world, but if we go that route I think it'd be worthwhile to add a comment.

Does SBUF_INCLUDENUL help you here?

[cem]

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

Ah, ok.

SBUF_INCLUDENUL doesn't help; it only changes how sbuf_len() behaves.

We'd probably prefer a new SBUF_BINARY mode (or something) which does not reserve a byte for trailing NUL, but the 4095 thing isn't the end of the world (nor 2 pages).

[cem]

• Fix sbuf drain length math when a partial seek occurred
• Early terminate pseudofs auto-drain fill routines when caller's uio is fully consumed

[markj]

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

Hmm, SBUF_INCLUDENUL also determines whether the nul byte is included in the buffer passed to the drain function, but that's not what we want here. So the choices are to add a new mode as you suggested, overallocate, or drain 4095 bytes at a time.

[cem]

sys/fs/pseudofs/pseudofs_vnops.c

725 ↗

(On Diff #79051)

For now, I've gone with the 4095-at-a-time option.