cvs commit: src/sys/dev/io iodev.c

[John Baldwin]

On Saturday 09 August 2008 12:35:50 am Bruce Evans wrote:
> On Fri, 8 Aug 2008, John Baldwin wrote:
> > On Friday 08 August 2008 09:43:56 am Ed Schouten wrote:
> >> ed          2008-08-08 13:43:56 UTC
> >>
> >>   Apart from this change, I think some fishy things may happen when
> >> using /dev/io in multithreaded applications. I haven't tested, but
> >> looking at the code, the flag doesn't get cleared when close() is called
> >> from another thread, but this may not be this important.
>
> Of course it isn't.  The flag only gets cleared on last close.  Threads
> probably involve mainly file descriptors, and for file descriptors close()
> doesn't even reach vn_close() until the fd reference count drops to 0.
> Then for files, vn_close() normally doesn't reach device close until the
> file reference count drops to 0.
>
> > It should be setting D_TRACKCLOSE though so that close() reliably clears
> > the flag even in single-threaded processes.  You can still get odd
> > behavior if
>
> You mean "should _not_ be setting D_TRACKCLOSE", so that close() works
> normally.
>
> close() can never reliably clear the flag, since even vn_close() is not
> reached after open()/dup()/close() or open()/fork()/close()...
>
> > you explicitly open it twice in an app and then close one of the two
> > fd's. You will no longer have IO permission even though you still have
> > one fd open. However, if you do that I think you deserve what you asked
> > for. :)
>
> You asked for normal last-close behaviour and deserve that not being broken
> by setting D_TRACKCLOSE.
>
> D_TRACKCLOSE allows different handling of non-last closes, but this is
> rarely wanted, and in theory it allows better determination of last
> closes, since vfs doesn't count last closes right.  However, it is
> difficult to count last closes right, and most uses of D_TRACKCLOSE handle
> last closes are worse than vfs:
> - ast: uses D_TRACKCLOSE to trash (write a filemark) and/or rewind tapes
>    on non-last closes.  After the trash and rewind, astclose() uses
>    count_dev() to avoid doing a full hardware close if the device is still
>    open.  This involves using count_dev(), but count_dev() is just an
>    interface to the vfs count, so it miscounts in the same way as vfs.  In
>    particular, it doesn't count incompleted opens.  I don't know if ast's
>    locking is sufficient to prevent close() being called while a new open
>    is in progress.  For most devices, it isn't.
> - drm: it uses its own count of opens and closes, and it doesn't use
>    count_dev().  The own count has some chance of working, but needs
>    delicate locking of device open and device close to prevent them
>    running into each other, and some defense against the vfs bugs which
>    at least used to result in missing and/or extra closes.
> - smb: like drm, except it limits its own count to 0 or 1 to give half-
>    baked exclusive access (fork() and dup(), etc, still give non-exclusive
>    access).  D_TRACKCLOSE thus has no effect except for its interaction
> with other bugs.
> - geom: unlike most or all of the others, this may actually need
>    D_TRACKCLOSE, to implement multiple logical drivers per physical device.
>    I don't understand its details.
> - apm: like drm
> - bpf: like smb

You failed to note that not using D_TRACKCLOSE doesn't actually reliable close 
devices.  I set D_TRACKCLOSE on bpf because under load at work (lots of 
concurrent nmap's) bpf devices would sometimes never get fully closed, so 
you'd have an unopened bpf device that was no longer available.  D_TRACKCLOSE 
gives much saner semantics for devices, each open() of an fd calls 
the 'd_open' method, and the last close of that fd (which could be in another 
process) calls 'd_close'.

/dev/io in particular is quite bogus since even though a child process 
inherits the file descriptor, it doesn't inherit the behavior of 
having /dev/io open.

-- 
John Baldwin