From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bob Peterson Date: Mon, 15 Feb 2016 16:16:17 -0500 (EST) Subject: [Cluster-devel] [DLM PATCH 0/6] Misc DLM Improvements Regarding Socket Errors In-Reply-To: <1845924157.21879838.1455215948999.JavaMail.zimbra@redhat.com> References: <1455130532-9317-1-git-send-email-rpeterso@redhat.com> <20160211172241.GA1737@redhat.com> <1845924157.21879838.1455215948999.JavaMail.zimbra@redhat.com> Message-ID: <538645327.23339354.1455570977299.JavaMail.zimbra@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit ----- Original Message ----- > ----- Original Message ----- > > On Wed, Feb 10, 2016 at 01:55:26PM -0500, Bob Peterson wrote: > > > I've been doing a bunch of recovery testing with DLM and discovered some > > > issues. This collection of 6 patches addresses those issues. Some of them > > > are of my own making, introduced by the recent patches that made DLM > > > print socket connection errors, and recovery from those errors. > > > > Thanks Bob, perhaps I've not been paying close enough attention, but it's > > unclear to me how this patch set relates the the most accute issue we have > > at the moment, which are the problems introduced here: > > > > From b3a5bbfd780d9e9291f5f257be06e9ad6db11657 Mon Sep 17 00:00:00 2001 > > From: Bob Peterson > > Date: Thu, 27 Aug 2015 09:34:47 -0500 > > Subject: [PATCH] dlm: print error from kernel_sendpage > > > > Print a dlm-specific error when a socket error occurs > > when sending a dlm message. > > > > Signed-off-by: Bob Peterson > > Signed-off-by: David Teigland > > > > Could we begin with one patch that's easy to track that directly resolves > > the issues with that commit (perhaps even a revert if it's not simple to > > fix directly)? That brings us back to a known-good place, from which we > > can look at cleanups and changes. > > > Hi Dave, > > My goal has always been to attain stability, which I think I've finally > achieved. > > The problem is: While testing the dlm in multiple recovery situations, > Nate and I discovered multiple problems. Until recently, no one has tried > to run recovery tests on an upstream DLM, so I think we're finding some > old bugs that have been there for a while, as well as bugs with b3a5bbfd, > which you mentioned. > > I agree that some of these patches might be unnecessary improvements. > I'll try to pare them down to what is absolutely necessary and what > is not. I'll also document exactly why the necessary ones are needed. Hi Dave, Here is some more information on the set of DLM patches I recently posted, and where things stand: 1. Patch: dlm: print error from kernel_sendpage Commit: b3a5bbfd780d9e9291f5f257be06e9ad6db11657 Advantages: It allows dlm to report socket errors Disadvantages: It caused some major problems: Problem #1: nodeid_to_addr ends up occasionally being called from softirq context, which is a problem because it takes a spinlock. Problem #2: The first condition also does "return;" rather than calling the original error report. This is a problem because the original error report needs to be called to do socket cleanup. The sunrpc implementation avoids this by doing that socket cleanup manually inside its own error_report function. Problem #3: It saves off the sk_error_report callback, but it never restores the callback to its original value. Problem #4: It only saves off the sk_error_report callback, but not any of the other three callbacks. All four really ought to be saved and restored once dlm is done with the socket, like sunrpc does. Problem #5: If two competing socket errors occur, lowcomms_error_report could, in theory, be called twice, causing socket cleanup (from the original error_report function) to happen twice, which results in a kernel panic (the details of which escape me, but I could maybe recreate it). 2. Patch: DLM: Replace nodeid_to_addr with kernel_getpeername Advanges: It fixes problem #1 above. Disadvantages: It doesn't fix any of the other problems. 3. Patch: DLM: Call original error report when socket is NULL Advantages: It fixes problem #2 above. Disadvantages: It introduces a new problem below. Problem: Error report recursion problem: Depending on timing, if/when add_sock is called multiple times for the same socket, it saves off the original sk_error_report multiple times. The first time, it saves off the proper one and replaces it with lowcomms_error_report. The second time, it saves lowcomms_error_report, which means when lowcomms_error_report is called the next time, it recurses and calls itself recursively an infinite number of times until the system crashes and is fenced. NOTE #1: This problem is, in fact, already in the code today, for the second two paths through lowcomms_error_report. This patch only makes the first path do the same thing. In other words, the problem is already there; this patch just makes it a lot more likely to happen. NOTE #2: There are two ways to fix it. The first is to make dlm do the socket cleanup, like sunrpc does. I don't like that because any cleanup introduced in the calling code needs to be echoed to dlm, and whomever makes that kind of change won't know to do it. The second is to clean up the socket code so it doesn't save itself as the original error_report callback, which is what subsequent patches do. 4. Patch: DLM: save / restore all socket callbacks Advantages: This tries to fix problems 3 and 4. Disadvantages: It has some sock-level locking, but not sk-callback_lock locking like sunrpc has, which means it does not fix problem #5 above. 5. Patch: DLM: Add locking to protect save callback assignments Advantages: This tries to fix problem #5 above. Disadvantages: None. 6. Patch: DLM: Don't create kernel socket until we have valid node address This is a cleanup, unrelated to the others. This makes the TCP code path similar to the SCTP code path. 7. Patch: DLM: Make consistent error path through tcp_create_listen_sock This is a cleanup, unrelated to the others. 8. Patch: DLM: Eliminate useless goto This is a cleanup, unrelated to the others. I think the "right thing to do" at this point is this: 1. Patch #1 is already upstream 2. Patch #2 stands on its own, so I think this should go forward. 3. Combine patches 3, 4 and 5, which ought to provide a comprehensive fix for the other problems listed in #1. 4. The rest of the patches, I can post as separate patches because they are code cleanups, not related to the original problems of #1. Let me know your thoughts on the subject. If you like this plan, I can re-test and post replacement patches tomorrow (hopefully). Regards, Bob Peterson Red Hat File Systems