[Pvfs2-developers] Re: the halloween bug fixed

[Sam Lang]

On Oct 11, 2007, at 10:15 AM, Phil Carns wrote:

> 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.
>
Hi Phil,

I can add a server flag to the tcp addr struct, and only call  
forget_addr in that case, but it seems like a bit of hack.  Can we  
just toss the ref list in the bmi control layer and force methods to  
manage their addresses?  The ref list is only being used to map an  
opaque address pointer to a particular method.  Could we just make  
the address pointer not opaque and encode the method in there somehow?

-sam

> 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
>