Re: [PATCH v3] xen-blkfront: dynamic configuration of per-vbd resources

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Jul 27, 2016 at 05:12:22PM +0800, Bob Liu wrote:
> 
> On 07/27/2016 04:07 PM, Roger Pau Monné wrote:
> ..[snip]..
> >> @@ -2443,6 +2674,22 @@ static void blkfront_connect(struct blkfront_info *info)
> >>  		return;
> >>  	}
> >>  
> >> +	err = device_create_file(&info->xbdev->dev, &dev_attr_max_ring_page_order);
> >> +	if (err)
> >> +		goto fail;
> >> +
> >> +	err = device_create_file(&info->xbdev->dev, &dev_attr_max_indirect_segs);
> >> +	if (err) {
> >> +		device_remove_file(&info->xbdev->dev, &dev_attr_max_ring_page_order);
> >> +		goto fail;
> >> +	}
> >> +
> >> +	err = device_create_file(&info->xbdev->dev, &dev_attr_max_queues);
> >> +	if (err) {
> >> +		device_remove_file(&info->xbdev->dev, &dev_attr_max_ring_page_order);
> >> +		device_remove_file(&info->xbdev->dev, &dev_attr_max_indirect_segs);
> >> +		goto fail;
> >> +	}
> >>  	xenbus_switch_state(info->xbdev, XenbusStateConnected);
> >>  
> >>  	/* Kick pending requests. */
> >> @@ -2453,6 +2700,12 @@ static void blkfront_connect(struct blkfront_info *info)
> >>  	add_disk(info->gd);
> >>  
> >>  	info->is_ready = 1;
> >> +	return;
> >> +
> >> +fail:
> >> +	blkif_free(info, 0);
> >> +	xlvbd_release_gendisk(info);
> >> +	return;
> > 
> > Hm, I'm not sure whether this chunk should be in a separate patch, it seems 
> > like blkfront_connect doesn't properly cleanup on error (if 
> > xlvbd_alloc_gendisk fails blkif_free will not be called). Do you think you 
> > could send the addition of the 'fail' label as a separate patch and fix the 
> > error path of xlvbd_alloc_gendisk?
> > 
> 
> Sure, will fix all of your comments above.
> 
> >>  }
> >>  
> >>  /**
> >> @@ -2500,8 +2753,16 @@ static void blkback_changed(struct xenbus_device *dev,
> >>  		break;
> >>  
> >>  	case XenbusStateClosed:
> >> -		if (dev->state == XenbusStateClosed)
> >> +		if (dev->state == XenbusStateClosed) {
> >> +			if (info->reconfiguring)
> >> +				if (blkfront_resume(info->xbdev)) {
> > 
> > Could you please join those two conditions:
> > 
> > if (info->reconfiguring && blkfront_resume(info->xbdev)) { ...
> > 
> > Also, I'm not sure this is correct, if blkfront sees the "Closing" state on 
> > blkback it will try to close the frontend and destroy the block device (see 
> > blkfront_closing), and this should be avoided. You should call 
> > blkfront_resume as soon as you see the backend move to the Closed or Closing 
> > states, without calling blkfront_closing.
> > 
> 
> I didn't get how this can happen, backend state won't be changed to 'Closing' before blkfront_closing() is called.
> So I think current logic is fine.

I see, in dynamic_reconfig_device you change the frontend state to "Closed". 
Then if the backend switches to state "Closing" blkfront will call 
blkfront_closing, but that won't do anything because the frontend state is 
already at the "Closed" state, at which point you just wait for the backend 
to finally reach state "Closed" in order to perform the reconnection. I 
stand corrected, I think the current logic is indeed fine.
 
Roger.