From mboxrd@z Thu Jan  1 00:00:00 1970
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH V5 2/2] xenbus: delay xenbus frontend resume
 if xenstored is not running
Date: Wed, 29 May 2013 11:44:59 -0400
Message-ID: <20130529154459.GE23775@phenom.dumpdata.com>
References: <1369760996-23616-1-git-send-email-aurelien.chartier@citrix.com>
	<1369760996-23616-3-git-send-email-aurelien.chartier@citrix.com>
	<51A5C45102000078000D9751@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <51A5C45102000078000D9751@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: Aurelien Chartier <aurelien.chartier@citrix.com>, xen-devel <xen-devel@lists.xen.org>, david.vrabel@citrix.com, ian.campbell@citrix.com
List-Id: xen-devel@lists.xenproject.org

On Wed, May 29, 2013 at 08:03:13AM +0100, Jan Beulich wrote:
> >>> On 28.05.13 at 19:09, Aurelien Chartier <aurelien.chartier@citrix.com> wrote:
> > If the xenbus frontend is located in a domain running xenstored, the device
> > resume is hanging because it is happening before the process resume. This
> > patch adds extra logic to the resume code to check if we are the domain
> > running xenstored and delay the resume if needed.
> > 
> > Signed-off-by: Aurelien Chartier <aurelien.chartier@citrix.com>
> > 
> > Changes in v2:
> > - Instead of bypassing the resume, process it in a workqueue
> > Changes in v3:
> > - Add a struct work in xenbus_device to avoid dynamic allocation
> > - Several small code fixes
> > Changes in v4:
> > - Use a dedicated workqueue
> > Changes in v5:
> > - Move create_workqueue error handling to xenbus_frontend_dev_resume
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Yet nevertheless I'm still seeing room for improvement:
> 
> > +static int xenbus_frontend_dev_resume(struct device *dev)
> > +{
> > +	/* 
> > +	 * If xenstored is running in this domain, we cannot access the backend
> > +	 * state at the moment, so we need to defer xenbus_dev_resume
> > +	 */
> > +	if (xen_store_domain_type == XS_LOCAL) {
> > +		struct xenbus_device *xdev = to_xenbus_device(dev);
> > +
> > +		if (!xenbus_frontend_wq) {
> > +			pr_err("%s: no workqueue to process delayed resume\n",
> > +			       xdev->nodename);
> > +			return -EFAULT;
> 
> Issuing a message here is fine, but I think you should also issue a
> pr_warn() at initialization time.
> 
> > +		}
> > +
> > +		INIT_WORK(&xdev->work, xenbus_frontend_delayed_resume);
> 
> And I also think that this would better be done once at initialization
> time too.
> 
> > +		queue_work(xenbus_frontend_wq, &xdev->work);
> > +
> > +		return 0;
> > +	}
> 
> Jan

Lets do that as a follow up patch. Aurlien, I've taken your patches in.

>