Re: [PATCH 2/4] libceph: have messages take a connection reference

[Alex Elder <elder@inktank.com>]

On 06/06/2012 12:06 PM, Sage Weil wrote:
> I have some reservations about this one.  Not because it's wrong, but 
> because I'm not sure what it will fix (now or later).  Right now the con 
> has references to the messages.  If we add refs the other way around, we 
> need to make sure those refs go away or else the con will leak.  And in 
> order to do that, the upper layer has to properly clean up.  Currently, if 
> it doesn't clean up, the messages would point to an invalid/released con, 
> but nothing would reference them.. they'd leak.  With this change, we 
> would also leak the con and it's containing structure.  I'm not sure 
> that's any better or worse.
> 
> The net effect here is that we're doing a lot of additional atomic 
> operations on the containing object's ref count.  Not sure if that cost is 
> important.
> 
> What do you think?

I think you make good points.

I don't know that this change directly fixes anything, but it might
help verify things as we do ongoing development.

I also think that an upper layer not properly cleaning up is a bug,
which we will fix (and which the reference counting *could* help
identify).  Also, I am not concerned about the cost of the atomic
operations, at least not now.  We have much more important performance
concerns to address before start worrying about that kind of extra
cost.  If we do find that the cost is excessive, we can remove them
(or maybe make them a debug-only feature).

The main thing I want is having the messenger pointing to its
connection, because if we are ever again debugging memory from
a crash it will be very helpful to have.  Beyond that though, the
reference counting is a nice thing to keep things validated.

So I guess--as long as it is working--I'd like to keep it in there.

					-Alex