[Cluster-devel] Re: gfs2-utils: master - gfs_controld: Remove three unused functions

[Steven Whitehouse]

Hi,

On Fri, 2009-10-16 at 11:33 -0500, David Teigland wrote:
> On Fri, Oct 16, 2009 at 05:01:18PM +0100, Steven Whitehouse wrote:
> > Hi,
> > 
> > On Fri, 2009-10-16 at 10:59 -0500, David Teigland wrote:
> > > On Fri, Oct 16, 2009 at 03:56:05PM +0100, Steven Whitehouse wrote:
> > > > Hi,
> > > > 
> > > > On Wed, 2009-10-14 at 12:53 -0500, David Teigland wrote:
> > > > > On Wed, Oct 14, 2009 at 02:55:04PM +0000, Steven Whitehouse wrote:
> > > > > > gfs_controld: Remove three unused functions
> > > > > > 
> > > > > > These functions are not called from anywhere and appear
> > > > > > to be left over from earlier times.
> > > > > 
> > > > > They were just added, but in translating the dlm_controld patch to
> > > > > gfs_controld I missed the bits that called them (both in
> > > > > cluster.git/STABLE3 and gfs2-utils.git)  I'll reapply this bit with the
> > > > > bits that are missing.
> > > > > 
> > > > > Dave
> > > > > 
> > > > 
> > > > I'm not sure I understand the purpose of this code. Is there more to
> > > > come yet?
> > > > 
> > > > The function find_mg_id() still seems to be unused. So far as I can
> > > > figure out the purpose of the new code seems to be to maintain two
> > > > timestamps: cluster_add_time whose sole purpose seems to be to check
> > > > against cg->create_time but I'm not quite sure why, and
> > > > cluster_remove_time which seems to not do anything at all at the moment.
> > > > 
> > > > I can't get any clues from dlm_controld because cluster_remove_time
> > > > seems to be unused there as well,
> > > 
> > > Right, cluster_add_time is used, but cluster_remove_time isn't, although
> > > it can be very useful to know for debugging.
> > > 
> > > Dave
> > > 
> > Yes, but used for what exactly? What is the purpose of this bit of code?
> 
> This bit about cluster_add_time?
> 
> +       /* a node's start can't match a change if the node joined the cluster
> +          more recently than the change was created */
> +
> +       node = get_node_history(mg, hd->nodeid);
> +       if (!node) {
> +               log_group(mg, "match_change %d:%u skip cg %u no node history",
> +                         hd->nodeid, seq, cg->seq);
> +               return 0;
> +       }
> +
> +       if (node->cluster_add_time > cg->create_time) {
> +               log_group(mg, "match_change %d:%u skip cg %u created %llu "
> +                         "cluster add %llu", hd->nodeid, seq, cg->seq,
> +                         (unsigned long long)cg->create_time,
> +                         (unsigned long long)node->cluster_add_time);
> +               return 0;
> +       }
> 
Yes, and the other bits that were recently added too. It all seems to be
be related.

> The commit gave a brief summary and pointed to this other commit for the
> long description of the problems with sorting out events after partitions
> and merges:
> 
> http://git.fedorahosted.org/git/cluster.git?p=cluster.git;a=commitdiff;h=bcc5fdef8473d99399c624a7bc15423a2af645c1
> 
That assuming that you trace the commit log back from gfs2-utils into
dlm and thence into cluster.git.

The question really is why we have all these (apparently) different
ideas of cluster membership. Looking at gfs_controld itself, it uses two
CPGs (one for all gfs_controlds which seems to only be used in
negotiating the protocol, of which there seems to be only one anyway,
and the other on a per mount group basis), each of which have their own
idea of which cluster members exist.

So I guess one question I have is, can we be certain the the "per mount
group" CPG will always have a membership which is a subset of the "all
gfs_controlds" CPG? Will the sequencing of delivery of membership events
by synchronised between the two CPGs wrt to other events (i.e. message
delivery) ? I guess that question might be more in Steve Dake's line, so
I've cc'd him too.

Given all that, what is the relationship which the membership events
reported in this new quorum_callback() have with the above? From my
earlier investigations, it appeared that dlm_controld was in charge of
ensuring quorum was attained before fencing took place, so I'm not quite
sure why that should affect gfs_controld directly.

The thing that I've not quite figured out yet is why we need to record
the times at all. My understanding of corosync is that it gives us a
guaranteed ordering of events, so that I'd expect to see a sequence
number rather than an actual timestamp. That is always assuming that the
timestamp isn't just being used as a monotonic sequence number, of
course.

Steve.