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

[Bob Liu <bob.liu@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel