From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Snitzer Subject: Re: [PATCH v2] dm mpath: Add timeout mechanism for queue_if_no_path Date: Tue, 14 Jan 2020 11:53:29 -0500 Message-ID: <20200114165329.GA11499@redhat.com> References: <20200113224127.3367484-1-krisman@collabora.com> <20200114161505.GA48507@lobo> <85pnfmyp7t.fsf@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <85pnfmyp7t.fsf@collabora.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com Content-Disposition: inline To: Gabriel Krisman Bertazi Cc: dm-devel@redhat.com, kernel@collabora.com, khazhy@google.com List-Id: dm-devel.ids On Tue, Jan 14 2020 at 11:31am -0500, Gabriel Krisman Bertazi wrote: > Mike Snitzer writes: > > > On Mon, Jan 13 2020 at 5:41P -0500, > > Gabriel Krisman Bertazi wrote: > > > >> From: Anatol Pomazau > >> > >> Add a configurable timeout mechanism to disable queue_if_no_path without > >> assistance from multipathd. In reality, this reimplements the > >> no_path_retry mechanism from multipathd in kernel space, which is > >> interesting to prevent processes from hanging indefinitely in cases > >> where the daemon might be unable to respond, after a failure or for > >> whatever reason. > >> > >> Despite replicating the policy configuration on kernel space, it is > >> quite an important case to prevent IOs from hanging forever, waiting for > >> userspace to behave correctly. > >> > >> v2: > >> - Use a module parameter instead of configuring per table > >> - Simplify code > >> > >> Co-developed-by: Frank Mayhar > >> Signed-off-by: Frank Mayhar > >> Co-developed-by: Bharath Ravi > >> Signed-off-by: Bharath Ravi > >> Co-developed-by: Khazhismel Kumykov > >> Signed-off-by: Khazhismel Kumykov > >> Signed-off-by: Anatol Pomazau > >> Co-developed-by: Gabriel Krisman Bertazi > >> Signed-off-by: Gabriel Krisman Bertazi > > > > All these tags seem rather excessive (especially in that you aren't cc > > most of them on the submission). Please review/scrub. Just seems odd > > that so many had a hand in this relatively small patch. > > Hey, > > I removed some of the Cc's because those emails addresses were bouncing. > Still, I kept the lines for credits. I got the bounces when sending v1. Hmm, if their emails bounce, not a lot of point detailing them. > > The Signed-off-by for anatol@google.com seems wrong, or did Anatol > > shephard this patch at some point? Or did you mean Reviewed-by? > > Something else? > > Anatol was the main author, as listed in the From: header. This > seems correct with regard to the ordering rules of > Documentation/process/submitting-patches.rst, in particular the second > example, (Example of a patch submitted by a Co-developed-by: author::). > > Am I missing something? No, I missed that Anatol is main author. I'd have noticed once I applied the patch via 'git am' but... > > > > > Anyway, if in the end you hold these tags to reflect the development > > evolution of this patch then so be it ;) > > > > I've reviewed the changes and found various things I felt were > > worthwhile to modify. Summary: > > > > - included missing > > - added MODULE_PARM_DESC > > - moved new functions to reduce needed forward declarations > > - tweaked queue_if_no_path_timeout_work's DMWARN message > > - added lockdep_assert_held to enable_nopath_timeout > > - renamed static queue_if_no_path_timeout to queue_if_no_path_timeout_secs > > - use READ_ONCE when accessing queue_if_no_path_timeout_secs > > > > The changes you made look good to me and your version of the patch > passes my testcase. OK, thanks. Mike