[Pvfs2-developers] Re: the halloween bug fixed

[Phil Carns]

Sam Lang wrote:
> 
> The attached patch is the proposed fix for this problem.  When the  tcp 
> method receives a disconnect from a peer, it invokes a callback  
> (bmi_method_addr_forget_callback) into the bmi control layer to  remove 
> the address reference from the list.  Maybe I should also add  a counter 
> and limit on how bit the list can get, al though that would  involve 
> potentially forcing long-lived connections to reconnect  periodically, 
> and all methods would have to implement BMI_set_info (DROP_ADDR).
> 
> With tcp, new connections are registered, even if they are from the  
> same host/port on the peer, whereas the other methods seem to only  
> register new host/port endpoints that haven't been seen before.  So  its 
> not completely clear to me when the other methods need to call  this 
> callback, if at all.  There needs to be a matching  
> bmi_method_addr_forget_callback for each  bmi_method_addr_reg_callback, 
> but if the method only registers a  single address per client, the list 
> won't keep growing, unless we  ever plan to support millions of clients.

Hi Sam,

Thanks for tracking this down!  I've been out of the office for a few 
days so I am a little behind on the conversation.  I have a comment on 
the patch, though.

I think that for the most part, it is calling 
bmi_method_addr_forget_callback() on pretty much any tcp network error, 
since it is being invoked from the tcp_forget_addr() function which is a 
general purpose function.

This is fine on the server side, but I suspect (haven't been able to 
confirm yet) that it would cause problems on the client side.  Clients 
resolve a tcp://servername:3334 address into a PVFS_BMI_addr_t once and 
then hang onto it forever.  If a network error occurs, then the state 
machines do not re-resolve it; they will just retry communication on the 
same PVFS_BMI_addr_t, which will have been invalidated by the 
forget_callback() function.

Would it be better to only call bmi_method_addr_forget_callback() on 
addresses within bmi_tcp that were registered using 
bmi_method_addr_reg_callback()?  I haven't looked yet, but that may 
require an extra flag somewhere to record which addresses this applies 
to.  That way, the only person invalidating these things on errors will 
be servers which have anonymous addresses that need to be cleaned out. 
Clients with long lived address resolutions would not be affected.  It 
also makes a little more sense from an API point of view if these two 
functions are companions that are called in the same scenario.

By the way, I'm glad you are doing some work on hash tables for these 
things- these linked lists have always bugged me :)

-Phil