dlm: Reset fs_notified when check_fs_done

Hi,

About the issue that dlm_controld and fs_controld sit spinning, retrying and replying for the fs_notified check, I have a suspision that another scenario may also hit that logic:

If the node->fs_notified has been set to 1 by previous change, when a new change comes and needs to check the node->fs_notified, because it has not been reset to 0, so check_fs_done will succeed even if dlm_controld has not received the notification from fs_controld this time.
For example, given that the following membership changes n, n+1, n+2, we see what happens on node X:
Step 1: cg n: node Y leaves with CPG_REASON_NODEDOWN reason,

        eventually in node X's ls->node_history, node Y's fs_notified
        = 1

Step 2: cg n+1: node Y joins ...
Step 3: cg n+2: node Y leaves with CPG_REASON_NODEDOWN reason, one
possible scenario is: before fs_controld's notification arrives, dlm_controld has known node Y is down from CPG message and done a lot of work, and it saw node Y's fs_notified = 1 (been set in Step 1) then passed the fs check wrongly. So node Y's check_fs reset to 0.
Step 4: fs_controld's notification arrives, it sees node Y's check_fs
= 0 and assumes dlm_controld has not known node Y is down and retries to send the notification. But in fact, dlm_controld has already known this and finished all the work, which will result in the spinning ...

I'm not sure if I read the code correctly :-) Below is the patch which reset the node->fs_notified. Review and comments are highly appreciated!

Thanks,
Jiaju

Signed-off-by Jiaju Zhang <[email protected]>

---
 group/dlm_controld/cpg.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

group/dlm_controld/cpg.c +1 -0

index d5245ce..b257595 100644
@@ -636,6 +636,7 @@ static int check_fs_done(struct lockspace *ls)
 
 		if (node->fs_notified) {
 			node->check_fs = 0;
+			node->fs_notified = 0;
 		} else {
 			log_group(ls, "check_fs nodeid %d needs fs notify",
 				  node->nodeid);

Luckily, things have changed now. One user met this issue two months ago and he's also very kindly to test the patch. The result is the patch really works.

Attached is the log before they apply the patch. This time the log has already included the debugging messages which were added by the commit 27b09badd40a2d1500500fa6945aeb532f75bd13 , so we can see what really happens on the user's site.
(The log is a bit large when it was uncompressed, this is because the spinning would print many messages to the log.)

I rebased the patch against current upstream code now. Thank you for your review in advance :)

Quoted 11 lines · Jiaju Zhang

Luckily, things have changed now. One user met this issue two months ago and he's also very kindly to test the patch. The result is the patch really works.

Attached is the log before they apply the patch. This time the log has already included the debugging messages which were added by the commit 27b09badd40a2d1500500fa6945aeb532f75bd13 , so we can see what really happens on the user's site.
(The log is a bit large when it was uncompressed, this is because the spinning would print many messages to the log.)

Quoted 2 lines

I rebased the patch against current upstream code now. Thank you for your review in advance :)

Thanks, the patch looks good, I'll push this out.

Quoted 12 lines · David Teigland

Quoted 11 lines · Jiaju Zhang

Luckily, things have changed now. One user met this issue two months ago and he's also very kindly to test the patch. The result is the patch really works.

Attached is the log before they apply the patch. This time the log has already included the debugging messages which were added by the commit 27b09badd40a2d1500500fa6945aeb532f75bd13 , so we can see what really happens on the user's site.
(The log is a bit large when it was uncompressed, this is because the spinning would print many messages to the log.)

Quoted 4 lines

Quoted 2 lines

I rebased the patch against current upstream code now. Thank you for your review in advance :)

Thanks, the patch looks good, I'll push this out.

Hi David, I haven't found this patch from
git://git.fedorahosted.org/dlm.git, was it being missed?