[libvirt] [PATCH 1/1] Add SCSI pool support.

[Dave Allan]

Daniel P. Berrange wrote:
> On Mon, Mar 30, 2009 at 05:50:38PM -0400, Dave Allan wrote:
>> Daniel P. Berrange wrote:
>>> This works now on RHEL-5, F9 and F10.
>>>
>>> I still had to disable this code
>>>
>>>    if (STREQLEN(devpath, vol->target.path, PATH_MAX)) {
>>>        VIR_DEBUG(_("No stable path found for '%s' in '%s'"),
>>>                  devpath, pool->def->target.path);
>>>        retval = -1;
>>>        goto free_vol;
>>>    }
>>>
>>> because it breaks pools configured with a target dir of /dev/
>> That's a good point.  Let's allow people to use non-stable paths by 
>> specifying a target path of /dev.  That preserves the behavior I want, 
>> which is to prevent people from accidentally using non-stable paths when 
>> they asked for by-id, etc, but lets them use unstable paths if they want 
>> to.  That's a one-line change that I have implemented in the attached patch.
>>
>>> And add device_type == 5 to allow CDROMs to work
>>>
>>>    if (device_type != 0 &&
>>>        device_type != 5) {
>>>        retval = 0;
>>>        goto out;
>>>    }
>> The more I think about this question, the more I think we need to ensure 
>> that within a particular pool there is only one device type, also 
>> implemented in the attached patch.  The problem with mixing device types 
>> within the pool is that it forces the consumer application to do device 
>> type checking every time it uses a volume from the pool, because the 
>> different device types cannot be used identically in all cases.  I'm not 
>> entirely in agreement that we should allow device types other than disk 
>> in this API, but if we ensure that each pool contains only devices of a 
>> particular type, I don't see any harm in it.
> 
> I don't think it makes sense to restrict a pool to only contain devices
> of a single type. It is perfectly reasonable for an IDE controller to
> have a disk and a CDROM device on it, and restricting this would mean
> users needing to create 2 separate pools for the same controller just
> to be able to see all the devices. 

Having the consumer create a pool for each device type is the way I was 
envisioning its being used.

> Other pool types are quite happy to mix volume types, eg raw, qcow2,
> vmdk files in filesystem based pool, and I see no reason not to allow
> this for HBA based pools. 

That is true.  My assumption was that they were all being used by the
VMs as disks.  Is that not the case, i.e, are people using storage
pools to back virtual MMC devices as well?  If people are already mixing 
device types (from the guest's perspective) within pools, then making a 
distinction here is less important.  It still does weird things to pool 
capacity, and volume creation fails unless there's media in the drive.

I guess what it comes down to is I disagree with allowing anything not a 
disk in the pool, but if people are already doing it, then I don't 
disagree enough to continue to argue about it.

>> diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c
>> index 2c5168a..9f6a085 100644
>> --- a/src/storage_backend_scsi.c
>> +++ b/src/storage_backend_scsi.c
>> @@ -149,8 +149,11 @@ virStorageBackendSCSINewLun(virConnectPtr conn,
>>          goto free_vol;
>>      }
>>  
>> -    if (STREQLEN(devpath, vol->target.path, PATH_MAX)) {
>> -        VIR_DEBUG(_("No stable path found for '%s' in '%s'"),
>> +    if (STREQLEN(devpath, vol->target.path, PATH_MAX) &&
>> +        !(STREQ(pool->def->target.path, "/dev") ||
>> +          STREQ(pool->def->target.path, "/dev/"))) {
>> +
>> +         VIR_DEBUG(_("No stable path found for '%s' in '%s'"),
>>                    devpath, pool->def->target.path);
>>          retval = -1;
>>          goto free_vol;
> 
> ACK to this chunk.

Cool.  Thanks.

>> @@ -343,16 +346,19 @@ processLU(virConnectPtr conn,
>>  
>>      if (getDeviceType(conn, host, bus, target, lun, &device_type) < 0) {
>>          virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
>> -                              _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"),
>> +                              _("Failed to determine if %u:%u:%u:%u "
>> +                                "is a Direct-Access LUN"),
>>                                host, bus, target, lun);
>>          retval = -1;
>>          goto out;
>>      }
>>  
>> -    /* We don't use anything except Direct-Access devices, but finding
>> -     * one isn't an error, either. */
>> -    if (device_type != 0) {
>> +    /* Check to see if the discovered device is the correct type for
>> +     * the pool. */
>> +    if (device_type != pool->def->deviceType) {
>>          retval = 0;
>> +        VIR_DEBUG(_("Found a device of type %d but pool device type is %d"),
>> +                  device_type, pool->def->deviceType);
>>          goto out;
>>      }
>>  
> 
> IMHo we should just be allowing types 0 & 5 directly here, with no
> other restriction.
> 
> 
> Daniel