From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7D939C433E1 for ; Fri, 26 Jun 2020 09:13:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3D16E20675 for ; Fri, 26 Jun 2020 09:13:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3D16E20675 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 99BFD6B000E; Fri, 26 Jun 2020 05:13:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 926356B0010; Fri, 26 Jun 2020 05:13:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7ED226B0022; Fri, 26 Jun 2020 05:13:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0092.hostedemail.com [216.40.44.92]) by kanga.kvack.org (Postfix) with ESMTP id 61B0C6B000E for ; Fri, 26 Jun 2020 05:13:02 -0400 (EDT) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id D152C99AC for ; Fri, 26 Jun 2020 09:13:01 +0000 (UTC) X-FDA: 76970798562.20.vest10_38089c426e54 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin20.hostedemail.com (Postfix) with ESMTP id 9DE5D180C060E for ; Fri, 26 Jun 2020 09:13:01 +0000 (UTC) X-HE-Tag: vest10_38089c426e54 X-Filterd-Recvd-Size: 13779 Received: from esa2.hc3370-68.iphmx.com (esa2.hc3370-68.iphmx.com [216.71.145.153]) by imf39.hostedemail.com (Postfix) with ESMTP for ; Fri, 26 Jun 2020 09:13:00 +0000 (UTC) Authentication-Results: esa2.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: T38r9SNgToIC/w7aDuAM7EkY+pgK9okoEH0+lTSUJx07CqgWIROtb49rxOQRiXXPaeR6ZVZAGe StGKMh2imfI2uuyLlp3OGlC5nDgFxMKtx5pIVhpU3XlrJF0ZxKoJjUu+mxvKfsjopB5rUF3lff 2pwoaqIcMFJJG1jo2L4B6VpoibYfqee8fD8v+Oht/J1BEzkTqaoOvmUJaw5xhWN2jM7f74uyD+ Ln8OyB8paYUUr8zh+f8v799e4f5+l+Drl9NZqdD96XIitldvVaKU3Qv9lxaj1fFBR+HkW8UWif 1X8= X-SBRS: 2.7 X-MesageID: 21014931 X-Ironport-Server: esa2.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.75,283,1589256000"; d="scan'208";a="21014931" Date: Fri, 26 Jun 2020 11:12:39 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Anchal Agarwal CC: Boris Ostrovsky , "tglx@linutronix.de" , "mingo@redhat.com" , "bp@alien8.de" , "hpa@zytor.com" , "x86@kernel.org" , "jgross@suse.com" , "linux-pm@vger.kernel.org" , "linux-mm@kvack.org" , "Kamata, Munehisa" , "sstabellini@kernel.org" , "konrad.wilk@oracle.com" , "axboe@kernel.dk" , "davem@davemloft.net" , "rjw@rjwysocki.net" , "len.brown@intel.com" , "pavel@ucw.cz" , "peterz@infradead.org" , "Valentin, Eduardo" , "Singh, Balbir" , "xen-devel@lists.xenproject.org" , "vkuznets@redhat.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Woodhouse, David" , "benh@kernel.crashing.org" Subject: Re: [PATCH 06/12] xen-blkfront: add callbacks for PM suspend and hibernation] Message-ID: <20200626091239.GA735@Air-de-Roger> References: <7FD7505E-79AA-43F6-8D5F-7A2567F333AB@amazon.com> <20200604070548.GH1195@Air-de-Roger> <20200616214925.GA21684@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com> <20200617083528.GW735@Air-de-Roger> <20200619234312.GA24846@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com> <20200622083846.GF735@Air-de-Roger> <20200623004314.GA28586@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com> <20200623081903.GP735@Air-de-Roger> <20200625183659.GA26586@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <20200625183659.GA26586@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com> X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) X-Rspamd-Queue-Id: 9DE5D180C060E X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam02 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Jun 25, 2020 at 06:36:59PM +0000, Anchal Agarwal wrote: > On Tue, Jun 23, 2020 at 10:19:03AM +0200, Roger Pau Monn=C3=A9 wrote: > > CAUTION: This email originated from outside of the organization. Do n= ot click links or open attachments unless you can confirm the sender and = know the content is safe. > >=20 > >=20 > >=20 > > On Tue, Jun 23, 2020 at 12:43:14AM +0000, Anchal Agarwal wrote: > > > On Mon, Jun 22, 2020 at 10:38:46AM +0200, Roger Pau Monn=C3=A9 wrot= e: > > > > CAUTION: This email originated from outside of the organization. = Do not click links or open attachments unless you can confirm the sender = and know the content is safe. > > > > > > > > > > > > > > > > On Fri, Jun 19, 2020 at 11:43:12PM +0000, Anchal Agarwal wrote: > > > > > On Wed, Jun 17, 2020 at 10:35:28AM +0200, Roger Pau Monn=C3=A9 = wrote: > > > > > > CAUTION: This email originated from outside of the organizati= on. Do not click links or open attachments unless you can confirm the sen= der and know the content is safe. > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jun 16, 2020 at 09:49:25PM +0000, Anchal Agarwal wrot= e: > > > > > > > On Thu, Jun 04, 2020 at 09:05:48AM +0200, Roger Pau Monn=C3= =A9 wrote: > > > > > > > > CAUTION: This email originated from outside of the organi= zation. Do not click links or open attachments unless you can confirm the= sender and know the content is safe. > > > > > > > > On Wed, Jun 03, 2020 at 11:33:52PM +0000, Agarwal, Anchal= wrote: > > > > > > > > > CAUTION: This email originated from outside of the org= anization. Do not click links or open attachments unless you can confirm = the sender and know the content is safe. > > > > > > > > > > + xenbus_dev_error(dev, err, "Freezin= g timed out;" > > > > > > > > > > + "the device may be= come inconsistent state"); > > > > > > > > > > > > > > > > > > Leaving the device in this state is quite bad, as i= t's in a closed > > > > > > > > > state and with the queues frozen. You should make a= n attempt to > > > > > > > > > restore things to a working state. > > > > > > > > > > > > > > > > > > You mean if backend closed after timeout? Is there a wa= y to know that? I understand it's not good to > > > > > > > > > leave it in this state however, I am still trying to fi= nd if there is a good way to know if backend is still connected after tim= eout. > > > > > > > > > Hence the message " the device may become inconsistent = state". I didn't see a timeout not even once on my end so that's why > > > > > > > > > I may be looking for an alternate perspective here. may= be need to thaw everything back intentionally is one thing I could think= of. > > > > > > > > > > > > > > > > You can manually force this state, and then check that it= will behave > > > > > > > > correctly. I would expect that on a failure to disconnect= from the > > > > > > > > backend you should switch the frontend to the 'Init' stat= e in order to > > > > > > > > try to reconnect to the backend when possible. > > > > > > > > > > > > > > > From what I understand forcing manually is, failing the fre= eze without > > > > > > > disconnect and try to revive the connection by unfreezing t= he > > > > > > > queues->reconnecting to backend [which never got diconnecte= d]. May be even > > > > > > > tearing down things manually because I am not sure what sta= te will frontend > > > > > > > see if backend fails to to disconnect at any point in time.= I assumed connected. > > > > > > > Then again if its "CONNECTED" I may not need to tear down e= verything and start > > > > > > > from Initialising state because that may not work. > > > > > > > > > > > > > > So I am not so sure about backend's state so much, lets say= if xen_blkif_disconnect fail, > > > > > > > I don't see it getting handled in the backend then what wil= l be backend's state? > > > > > > > Will it still switch xenbus state to 'Closed'? If not what = will frontend see, > > > > > > > if it tries to read backend's state through xenbus_read_dri= ver_state ? > > > > > > > > > > > > > > So the flow be like: > > > > > > > Front end marks XenbusStateClosing > > > > > > > Backend marks its state as XenbusStateClosing > > > > > > > Frontend marks XenbusStateClosed > > > > > > > Backend disconnects calls xen_blkif_disconnect > > > > > > > Backend fails to disconnect, the above function retu= rns EBUSY > > > > > > > What will be state of backend here? > > > > > > > > > > > > Backend should stay in state 'Closing' then, until it can fin= ish > > > > > > tearing down. > > > > > > > > > > > It disconnects the ring after switching to connected state too. > > > > > > > Frontend did not tear down the rings if backend does= not switches the > > > > > > > state to 'Closed' in case of failure. > > > > > > > > > > > > > > If backend stays in CONNECTED state, then even if we mark i= t Initialised in frontend, backend > > > > > > > > > > > > Backend will stay in state 'Closing' I think. > > > > > > > > > > > > > won't be calling connect(). {From reading code in frontend_= changed} > > > > > > > IMU, Initialising will fail since backend dev->state !=3D X= enbusStateClosed plus > > > > > > > we did not tear down anything so calling talk_to_blkback ma= y not be needed > > > > > > > > > > > > > > Does that sound correct? > > > > > > > > > > > > I think switching to the initial state in order to try to att= empt a > > > > > > reconnection would be our best bet here. > > > > > > > > > > > It does not seems to work correctly, I get hung tasks all over = and all the > > > > > requests to filesystem gets stuck. Backend does shows the state= as connected > > > > > after xenbus_dev_suspend fails but I think there may be somethi= ng missing. > > > > > I don't seem to get IO interrupts thereafter i.e hitting the fu= nction blkif_interrupts. > > > > > I think just marking it initialised may not be the only thing. > > > > > Here is a short description of what I am trying to do: > > > > > So, on timeout: > > > > > Switch XenBusState to "Initialized" > > > > > unquiesce/unfreeze the queues and return > > > > > mark info->connected =3D BLKIF_STATE_CONNECTED > > > > > > > > If xenbus state is Initialized isn't it wrong to set info->connec= ted > > > > =3D=3D CONNECTED? > > > > > > > Yes, you are right earlier I was marking it explicitly but that was= not right, > > > the connect path for blkfront will do that. > > > > You should tear down all the internal state (like a proper close)= ? > > > > > > > Isn't that similar to disconnecting in the first place that failed = during > > > freeze? Do you mean re-try to close but this time re-connect after = close > > > basically do everything you would at "restore"? > >=20 > > Last time I checked blkfront supported reconnections (ie: disconnect > > from a backend and connect again). I was assuming we could apply the > > same here on timeout, and just follow the same path where the fronten= d > > waits indefinitely for the backend to close and then attempts to > > reconnect. > >=20 > > > Also, I experimented with that and it works intermittently. I want = to take a > > > step back on this issue and ask few questions here: > > > 1. Is fixing this recovery a blocker for me sending in a V2 version= ? > >=20 > > At the end of day it's your feature. I would certainly prefer for it > > to work as good as possible, this being a recovery in case of failure > > just make sure it does something sane (ie: crash/close the frontend) > > and add a TODO note. > >=20 > > > 2. In our 2-3 years of supporting this feature at large scale we ha= ven't seen this issue > > > where backend fails to disconnect. What we are trying to do here is= create a > > > hypothetical situation where we leave backend in Closing state and = try and see how it > > > recovers. The reason why I think it "may not" occur and the timeout= of 5HZ is > > > sufficient is because we haven't come across even a single use-case= where it > > > caused hibernation to fail. > > > The reason why I think "it may" occur is if we are running a really= memory > > > intensive workload and ring is busy and is unable to complete all t= he requests > > > in the given timeout. This is very unlikely though. > >=20 > > As said above I would generally prefer for code to handle possible > > failures the best way, and hence I think here it would be nice to > > fallback to the normal disconnect path and just wait for the backend > > to close. > > > Do you mind throwing some light in here, what that path may be, if its > straight forward to fix I would like to debug it a bit more. May be I a= m > missing some of the context here. So the frontend should do: - Switch to Closed state (and cleanup everything required). - Wait for backend to switch to Closed state (must be done asynchronously, handled in blkback_changed). - Switch frontend to XenbusStateInitialising, that will in turn force the backend to switch to XenbusStateInitWait. - After that it should just follow the normal connection procedure. I think the part that's missing is the frontend doing the state change to XenbusStateInitialising when the backend switches to the Closed state. > I was of the view we may just want to mark frontend closed which should= do=20 > the job of freeing resources and then following the same flow as > blkfront_restore. That does not seems to work correctly 100% of the tim= e. I think the missing part is that you must wait for the backend to switch to the Closed state, or else the switch to XenbusStateInitialising won't be picked up correctly by the backend (because it's still doing it's cleanup). Using blkfront_restore might be an option, but you need to assert the backend is in the initial state before using that path. > > You likely have this very well tuned to your own environment and > > workloads, since this will now be upstream others might have more > > contended systems where it could start to fail. > >=20 > I agree, however, this is also from the testing I did with 100 of runs=20 > outside of EC2 running few tests of my own.=20 > > > 3) Also, I do not think this may be straight forward to fix and exp= ect > > > hibernation to work flawlessly in subsequent invocations. I am open= to > > > all suggestions. > >=20 > > Right, adding a TODO would seem appropriate then. > > > Just to double check, I will send in a V2 with this marked as TO-DO? I think that's fine. Please clearly describe what's missing, so others know what they might have to implement. Thanks, Roger.