fcntl(F_SETFL): Don't unconditionally invoke FIONBIO and FIOASYNC

[jhb]

Summary

Currently, F_SETFL always invokes FIONBIO and FIOASYNC ioctls on the
file descriptor even if the state of the associated flag has not
changed. This means that a character device driver that implements
non-blocking I/O but not async I/O needs a handler for FIOASYNC that
permits setting the value to 0. This also means that fcntl(fd,
F_SETFL, fcntl(fd, F_GETFL)) can fail for a character device driver
that does not handle both FIONBIO and FIOASYNC. These requirements
are not obvious nor well documented.

Instead, only invoke FIONBIO and FIOASYNC if the relevant flag changes
state. This only requires a device driver to implement support for
FIONBIO or FIOASYNC if it supports the corresponding flag.

While here, if a request aims to toggle both F_NOBLOCK and F_ASYNC and
FIOASYNC fails, pass the previous state of F_NONBLOCK to FIONBIO
instead of always disabling non-blocking I/O and then possibly
reverting the flag back to on in f_flags.

[jhb]

The bug of always reverting back to disabling FIONBIO dates back to the very first version of fcntl() 42 years ago: https://svnweb.freebsd.org/csrg/sys/kern/kern_descrip.c?r1=12748&r2=12747&pathrev=12748

While @sam merged the commit, the log claims it is "bill's code", so presumably a Bill Joy bug?

[kib]

We need some file lock to prevent parallel threads from corrupting nonblock/async state vs flags.

[mckusick]

Interesting history of this bug.

[emaste]

We need some file lock to prevent parallel threads from corrupting nonblock/async state vs flags.

Worth an XXX comment in the code for now?

[jhb]

In D52403#1196680, @kib wrote:

We need some file lock to prevent parallel threads from corrupting nonblock/async state vs flags.

That is an old bug though? If it is just F_SETFL I would be happy to use a pool of sx locks or the like just for F_SETFL.

[kib]

In D52403#1198039, @jhb wrote:

In D52403#1196680, @kib wrote:

We need some file lock to prevent parallel threads from corrupting nonblock/async state vs flags.

That is an old bug though? If it is just F_SETFL I would be happy to use a pool of sx locks or the like just for F_SETFL.

This is a separate bug and should get a separate fix.

Do we want to introduce sx pool? I think a pair of bit in f_vnread_flag similar to FOFFSET_LOCKED/WANT_LOCK and the existing mtx pool would be enough.

[jhb]

In D52403#1198184, @kib wrote:

In D52403#1198039, @jhb wrote:

In D52403#1196680, @kib wrote:

We need some file lock to prevent parallel threads from corrupting nonblock/async state vs flags.

That is an old bug though? If it is just F_SETFL I would be happy to use a pool of sx locks or the like just for F_SETFL.

This is a separate bug and should get a separate fix.

Do we want to introduce sx pool? I think a pair of bit in f_vnread_flag similar to FOFFSET_LOCKED/WANT_LOCK and the existing mtx pool would be enough.

Well, an sx lock would be less overhead (2 atomics to lock/unlock vs 4 to lock/unlock the mutex twice in the uncontested case), and home-grown locks aren't covered by WITNESS.