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

[Dave Allan]

Dave Allan wrote:
> 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.

Attached is what I think is a final version of the scsi host pool code. 
  It's the set of patches we've been discussing rolled up into a single 
patch, so it should look ok, but let me know if you have additional 
comments.  I allowed both disk and rom and took out the XML enhancement 
for device type.

Dave
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Add-SCSI-pool-support.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20090331/2944f59f/attachment-0001.ksh>