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

[Bob Liu]

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.

Btw: could you please ack [PATCH v2 2/3] xen-blkfront: introduce blkif_set_queue_limits()?

Thank you!
Bob