Date: Sun, 22 Dec 2002 14:21:39 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Alfred Perlstein <bright@mu.org> Cc: arch@freebsd.org Subject: Re: pipes and FIONBIO breakage? Message-ID: <20021222133251.G8167-100000@gamplex.bde.org> In-Reply-To: <20021222015737.GO23663@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 21 Dec 2002, Alfred Perlstein wrote:
> * Bruce Evans <bde@zeta.org.au> [021221 17:24] wrote:
> >
> > Things work correctly at the file descriptor level and almost correctly
> > for ordinary pipes. kern_descrip.c maintains the FNONBLOCK flag in
> > the file struct and arranges to pass it to read/write/etc. pipe_read()
> > and pipe_write() ignore their `flags' arg and determine the FNONBLOCK
> > flag by accessing the file struct direct. This is a layering violation.
> > Most device drivers can't even think about doing it without it being
> > a clear violation since they don't have a pointer to the file struct
> > in scope. However, accessing the current value of the flag may be
> > best since the value in the flags arg can't be changed by fcntl() or
> > FIONBIO and it may be right to permit unblocking of blocked i/o by
> > changing the flag.
>
> I don't see the FNONBLOCK being passed into dofileread as 'flags'.
Oops. I was thinking of vn_{read,write,ioctl[,other?]}. These are at
the same level as pipe_{read,write,...}. So it is correct for
pipe_{read,write,...} to look at the flag in *fp (and then not pass it
down since they do almost everything inline).
> Are you suggesting though that perhaps it would make sense for the
> FIONBIO ioctl to issue a wakeup on the object it is being set on
> to perhaps unblock already blocked threads after setting the state?
I want the ioctl to not exist so I don't want it to do more :-). At
most, I want lower levels to be able to see changes to the file-level
flag in some efficient way.
> I like the flexibility that introduces although the semantics of
> it are somewhat offbeat.
Now I think it wouldn't be very useful. In the non-broken case where
there is one copy of the flag per open file, processes sharing an open
file using dup()'ed or fork()ed descriptor(s) could affect each other,
but you couldn't do something like "fooctl --unblock /dev/foo" since
fooctl would open a new file.
> > FIONBIO handling is a little different and uglier. Both
> > fcntl(... F_SETFL ...) and ioctl(... FIONBIO ...) set the FNONBLOCK
> > flag in the file struct and call fo_ioctl(... FIONBIO ...) to duplicat
> > any bogus copies of the flag in lower level structs. Non-broken lower
> > levels don't have a bogus copy of the flag so FIONBIO is null at
> > their level. The sys_pipe.c level is one of these, so
> > pipe_ioctl(... FIONBIO ...) returns successfully after doing nothing
> > except fiddling with locks.
>
> I agree with you, you think that perhaps we could fix this by
> adding a parameter to the socket read/write functions so they
> would know if they could block or not?
I think this wouldn't work reasonably because the parameter would then
have to be passed to too many lower level functions.
> > > There does appear to be an issue where the fifo code temporarity
> > > ORs in the non-block state of the struct file into the socket behind
> > > it when performing reads and writes. This might cause someone else
> > > to block when multiple people open a fifo because of races.
> >
> > Yes, it seems to be easy for processes to clobber other processes'
> > blocking state state either way. fifo_read() and fifo_write() don't
> > even manage to restore the socket struct's copy of the flag to its
> > initial state. They always turn it off if they set it, even if it
> > was initially on. This implements clobbering of the state from
> > on to off, but not soon enough to help processes that actually want
> > it off, since the flag is assumed to be initially off and not changed
> > in the !(ap->a_ioflag & IO_NDELAY) case until after doing the i/o.
> >
> > This bug suite seems to mostly not affect normal sockets since open()
> > doesn't work on them so one copy of the file flags is sufficient.
>
> So this is somewhat worse than my second take on the problem. Any
> suggestions for a fix? Passing the blocking status down into the
> socket level calls seems like the right thing to do.
Perhaps block in fifo_read()/fifo_write() if there is already a
reader/write further in there (except in the non-blocking case of
course). I think this would just change the places where we block (or
give up), since processes must block at some level to get exclusive
access to the socket buffers.
> Same with devices I guess? Or is this just another reason device
> driver writers shouldn't allow more than one open() at a time? :)
Many device drivers do this correctly. I originally learned about this
by understanding the tty driver (the parts in tty.c). Exclusive access
only "fixes" the problem of having multiple file states but only 1
device state to make a copy of the open state, since multiple readers
and writers are always possible using fork()ed file descriptors.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021222133251.G8167-100000>