Date: Tue, 25 Aug 2009 04:41:59 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@FreeBSD.org, Jilles Tjoelker <jilles@stack.nl>, svn-src-all@FreeBSD.org, glebius@FreeBSD.org, Konstantin Belousov <kib@FreeBSD.org>, svn-src-head@FreeBSD.org Subject: Re: svn commit: r196460 - head/sys/kern Message-ID: <20090825041842.I923@besplex.bde.org> In-Reply-To: <20090825031355.B729@besplex.bde.org> References: <200908231244.n7NCiFgc061095@svn.freebsd.org> <20090823213759.GA55039@stack.nl> <20090825031355.B729@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 25 Aug 2009, Bruce Evans wrote:
> On Sun, 23 Aug 2009, Jilles Tjoelker wrote:
>
>> I think poll on fifos should instead be fixed by closing the
>> half-connection corresponding to writing from fi_readsock to
>> fi_writesock. I have tried this out, see attached patch. With the patch,
>> pipepoll only gives "expected POLLHUP; got POLLIN | POLLHUP" errors and
>> an error in fifo case 6b caused by our distinction between new and old
>> readers.
>
> This sort of worked for me, but has several problems:
>
> % Index: sys/fs/fifofs/fifo_vnops.c
> % ===================================================================
> % --- sys/fs/fifofs/fifo_vnops.c (revision 196459)
> % +++ sys/fs/fifofs/fifo_vnops.c (working copy)
> % @@ -193,6 +193,10 @@
> % goto fail2;
> % fip->fi_writesock = wso;
> % error = soconnect2(wso, rso);
> % + if (error == 0)
> % + error = soshutdown(rso, SHUT_WR);
> % + if (error == 0)
> % + error = soshutdown(wso, SHUT_RD);
> % if (error) {
> % (void)soclose(wso);
> % fail2:
>
> The second soshutdown() is only harmful. I see the following state changes
> - the first soshutdown() sets SBS_CANTRCVMORE on rso like you would expect,
> and also sets SBS_CANTSENDMORE on wso. This gives the desired state.
> - the second soshutdown() then clears SBS_CANTRCVMORE on rso (without
> the first soshutdown() it leaves both flags clear in both directions).
> This clobbers the desired state. The failure shows in just one of my
> uncommitted regression tests (when there is a writer and there was
> a reader, poll() returns POLLOUT for the writer, but should return
> POLLHUP; the missing SBS_CANTRCVMORE on rso prevents it ever returning
> POLLHUP for writers).
SBS_CANTRCVMORE on rso is cleared here in ~5.2, but this seems to be fixed
in -current:
% void
% sorflush(struct socket *so)
% {
% struct sockbuf *sb = &so->so_rcv;
% struct protosw *pr = so->so_proto;
% struct sockbuf asb;
%
% /*
% * In order to avoid calling dom_dispose with the socket buffer mutex
% * held, and in order to generally avoid holding the lock for a long
% * time, we make a copy of the socket buffer and clear the original
% * (except locks, state). The new socket buffer copy won't have
% * initialized locks so we can only call routines that won't use or
% * assert those locks.
% *
% * Dislodge threads currently blocked in receive and wait to acquire
% * a lock against other simultaneous readers before clearing the
% * socket buffer. Don't let our acquire be interrupted by a signal
% * despite any existing socket disposition on interruptable waiting.
% */
% CURVNET_SET(so->so_vnet);
% socantrcvmore(so);
% (void) sblock(sb, SBL_WAIT | SBL_NOINTR);
%
% /*
% * Invalidate/clear most of the sockbuf structure, but leave selinfo
% * and mutex data unchanged.
% */
% SOCKBUF_LOCK(sb);
% bzero(&asb, offsetof(struct sockbuf, sb_startzero));
% bcopy(&sb->sb_startzero, &asb.sb_startzero,
% sizeof(*sb) - offsetof(struct sockbuf, sb_startzero));
% bzero(&sb->sb_startzero,
% sizeof(*sb) - offsetof(struct sockbuf, sb_startzero));
^^^^^
sb_state is at the end of the struct in 5.2, but is now just before the
startzero section.
sockbuf.h has usless history (no repo copy), but according to history
of socketvar.h, sb_state was moved mainly to fix clobbering of
SBS_CANTRCVMORE:
% RCS file: /home/ncvs/src/sys/sys/socketvar.h,v
% Working file: socketvar.h
% head: 1.171
% ----------------------------
% revision 1.137
% date: 2005/01/30 13:11:44; author: glebius; state: Exp; lines: +1 -1
% Move sb_state to the beginning of structure, above sb_startzero member.
% sb_state shouldn't be erased, when socket buffer is flushed by sorflush().
%
% When sb_state was bzero'ed, a recently set SBS_CANTRCVMORE flag was cleared.
% If a socket was shutdown(SHUT_RD), a subsequent read() would block on it.
%
% Reported by: Ed Maste, Gerrit Nagelhout
% Reviewed by: rwatson
% ----------------------------
What were previous symptoms of this bug? This bug lived in FreeBSD-5-2+
for about a year and for 4 more years in my version of 5.2 :-(. Older
versions didn't have it since the flags were all in so_state which has
never been cleared.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090825041842.I923>