current fd allocation idiom

[Mateusz Guzik]

On Mon, Aug 11, 2014 at 11:24:51AM -0400, John Baldwin wrote:
> On Friday, July 18, 2014 3:19:28 pm Mateusz Guzik wrote:
> > On Fri, Jul 18, 2014 at 06:59:59PM +0300, Konstantin Belousov wrote:
> > > On Fri, Jul 18, 2014 at 04:40:12PM +0200, Mateusz Guzik wrote:
> > > > On Fri, Jul 18, 2014 at 04:06:29PM +0300, Konstantin Belousov wrote:
> > > > > It seems that all what is needed is conversion of places using
> > > > > falloc() to falloc_noinstall()/finstall().
> > > > > 
> > > > 
> > > > This postpones fd allocation to after interested function did all work
> > > > it wanted to do, which means we would need reliable ways of reverting
> > > > all the work in case allocation failed. I'm not so confident we can do
> > > > that for all current consumers and both current and my proposed approach
> > > > don't impose such requirement.
> > > Cleanup should be identical to the actions done on close(2).
> > > 
> > > > 
> > > > Of course postponing fd allocation where possible is definitely worth
> > > > doing.
> > > Yes, and after that the rest of the cases should be evaluated.
> > > But my gut feeling is that everything would be converted.
> > > 
> > 
> > So let's say you accept() a connection.
> > 
> > With current code, if you got to accept the connection you got it.
> > 
> > With your proposal you may find that you can't allocate any fd and have
> > to close fp. This will be visible as accept + close by the other end,
> > while the caller never saw the connection.
> > 
> > My guess is people would complain once they encounter such issue.
> 
> Can't you already get this if you overflow the listen queue?  (Having
> "accepted" connections aborted where the user application doesn't know):
> 
> 	} else {
> 		/*
> 		 * Keep removing sockets from the head until there's room for
> 		 * us to insert on the tail.  In pre-locking revisions, this
> 		 * was a simple if(), but as we could be racing with other
> 		 * threads and soabort() requires dropping locks, we must
> 		 * loop waiting for the condition to be true.
> 		 */
> 		while (head->so_incqlen > head->so_qlimit) {
> 			struct socket *sp;
> 			sp = TAILQ_FIRST(&head->so_incomp);
> 			TAILQ_REMOVE(&head->so_incomp, sp, so_list);
> 			head->so_incqlen--;
> 			sp->so_qstate &= ~SQ_INCOMP;
> 			sp->so_head = NULL;
> 			ACCEPT_UNLOCK();
> 			soabort(sp);
> 			ACCEPT_LOCK();
> 		}
> 		TAILQ_INSERT_TAIL(&head->so_incomp, so, so_list);
> 		so->so_qstate |= SQ_INCOMP;
> 		head->so_incqlen++;
> 
> I think the simplest approach would be to first convert as many places as
> possible to use falloc_noinstall() / finstall().  If you end up with all of
> them converted then you can just rename falloc_noinstall to falloc() and
> retire the old falloc().
> 

I would expect soabort to result in a timeout/reset as opposed to regular
connection close.

Comments around soabort suggest it should not be used as a replacement
for close, but maybe this is largely because of what the other end will
see. That will need to be investigated.

That said, I definitely support using delayed fd allocation (current
falloc_noinstall) where possible, but I'm not convinced it is safe for
all consumers.

-- 
Mateusz Guzik <mjguzik gmail.com>