svn commit: r238936 - in head/sys: fs/fifofs kern sys

[Bruce Evans]

On Tue, 31 Jul 2012, David Xu wrote:

> On 2012/7/31 15:22, Giovanni Trematerra wrote:
>> On Tue, Jul 31, 2012 at 7:48 AM, David Xu <davidxu at freebsd.org> wrote:
>>> Log:
>>>    I am comparing current pipe code with the one in 8.3-STABLE r236165,
>>>    I found 8.3 is a history BSD version using socket to implement FIFO
>>>    pipe, it uses per-file seqcount to compare with writer generation
>>>    stored in per-pipe object. The concept is after all writers are gone,
>>>    the pipe enters next generation, all old readers have not closed the
>>>    pipe should get the indication that the pipe is disconnected, result
>>>    is they should get EPIPE, SIGPIPE or get POLLHUP in poll().
>>>    But newcomer should not know that previous writters were gone, it
>>>    should treat it as a fresh session.

Good commit message.  Almost worth quoting in 3 followups :-).  I wrote
most of the code and forgotten some details, and the above made them
clear.

>>>    I am trying to bring back FIFO pipe to history behavior. It is still
>>>    unclear that if single EOF flag can represent SBS_CANTSENDMORE and
>>>    SBS_CANTRCVMORE which socket-based version is using, but I have run
>>>    the poll regression test in tool directory, output is same as the one
>>>    on 8.3-STABLE now.

Not very historic.  Only FreeBSD-8 (maybe 9?) did that.

>>>    I think the output "not ok 18 FIFO state 6b: poll result 0 expected 1.
>>>    expected POLLHUP; got 0" might be bogus, because newcomer should not
>>>    know that old writers were gone. I got the same behavior on Linux.

6b is intentionally different from Linux.   I forget if it is to reduce
races with readers or just to simply the implementation and understanding
it.  New readers simply joing old readers with a hangup set for all if
they manage to open the fifo (necessarily using O_NONBLOCK) after the
hangup but before the old readers go away.  Since this seems to increase
races, I may remember it backwards

>>>    Our implementation always return POLLIN for disconnected pipe even it
>>>    should return POLLHUP, but I think it is not wise to remove POLLIN for
>>>    compatible reason, this is our history behavior.

This is historical back to FreeBSD-3 (earlier versions didn't have
poll()).  I think it is just a bug.  POLLHUP was unimplemented for
most file types before FreeBSD-8, and setting POLLIN works around this
for most callers.  I tried to get it fixed for at least fifos when I
fixed POLLHUP for some file types.  No one uses fifos, so they are
safer to fix than sockets :-).

>> I'm sorry but I'm failing to understand the reason for this change.
>> Can you point me out a test that confirm that the change is needed.
>> The only thing I see is an increase in the memory footprint for the pipes.
>> There was a lot of discussions on this topic on -arch mailing list

Many poll regression tests fail.

>> http://lists.freebsd.org/pipermail/freebsd-arch/2012-January/012131.html
>> http://lists.freebsd.org/pipermail/freebsd-arch/2012-February/012314.html

There are also a lot of old PRs about this for poll() (not for your new
fifo implementation).  I think the PRs are mentioned in these threads.

> The old code broke some history semantic of FIFO pipe, you can try the test
> tool /usr/src/tools/regression/poll/pipepoll, try it before and after my
> commit, also compare the result with 8.3-STABLE, without this commit,
> both sub-tests 6c and 6d failed.
>
> I think old code did not mimic original code correctly,
> in 8.3-STABLE code, seqcount is stored in each file, writer generation
> detection is based on each copy of seqcount, but your code stored single
> copy of seqcount in fifoinfo object which is store as vnode data, and
> made the writer generation flag global by setting PIPE_SAMEWGEN in pipe
> object and used this flag to determine if it should return POLLHUP/POLLIN
> or not, this is wrong, for example:
> when there is no writer but have old readers, new incoming reader will
> executes:
> line 174 and 175:
>    fip->fi_seqcount = fip->fi_wgen - fip->fi_writers;
>    FIFO_WPDWGEN(fip, fpipe);
>
> this causes fi_seqcount to be equal to fi_wgen because fi_writer is zero,
> and FIFO_WPDWGEN() turns on flag PIPE_SAMEWGEN.
> When PIPE_SAMEWGEN is on, pipe_poll() ignores EOF, this breaks old readers,
> it causes old reader to get nothing while it should get POLLHUP from poll().
>
> The new incoming reader should get nothing, so I think sub-tests 6b
> is wrong.

Luckily I have forgotten the details for fifos and never understood them
all for nameless pipes, so you get to fix it :-).

Bruce