[libvirt] [PATCHv3 3/4] storage: implement rudimentary glusterfs pool refresh

[Eric Blake]

On 11/12/2013 09:58 AM, Daniel P. Berrange wrote:
> On Mon, Nov 11, 2013 at 09:19:30PM -0700, Eric Blake wrote:
>> Actually put gfapi to use, by allowing the creation of a gluster
>> pool.  Right now, all volumes are treated as raw; further patches
>> will allow peering into files to allow for qcow2 files and backing
>> chains, and reporting proper volume allocation.
>>

>> +    /* FIXME: Currently hard-coded to tcp transport; XML needs to be
>> +     * extended to allow alternate transport */
>> +    if (VIR_ALLOC(ret->uri) < 0 ||
>> +        VIR_STRDUP(ret->uri->scheme, "gluster") < 0 ||
>> +        VIR_STRDUP(ret->uri->server, pool->def->source.hosts[0].name) < 0 ||
>> +        virAsprintf(&ret->uri->path, "%s%s",
>> +                    *pool->def->source.name == '/' ? "" : "/",
>> +                    pool->def->source.name) < 0)
>> +        goto error;
> 
> I'm not a huge fan of chaining together statements like this,
> since some gdb versions are pretty awful at telling you which
> one of these 4 statements has the problem on segv.

Easy enough to split; so consider it done.


>> +    /* Silently skip directories, including '.' and '..'.  FIXME:
>> +     * should non-'.' subdirectories be listed as type dir?  */
> 
> What behaviour does the 'fs' pool have in for non-'.' subdirs ?
> Seems we perhaps ought to match that.

Peter asked the same question - guess I'm stuck doing that :)

>> +
>> +    /* FIXME - must open files to determine if they are non-raw */
>> +    vol->type = VIR_STORAGE_VOL_NETWORK;
>> +    vol->target.format = VIR_STORAGE_FILE_RAW;
>> +    vol->capacity = vol->allocation = st->st_size;
> 
> So gluster has no concept of sparse volumes ?

Gluster has sparse support; I need to flesh this out a bit more, based
on how directory pools do it (both pools have a struct stat to work
with); I also need to populate ownership details.

>> +
>> +    if (!(state = virStorageBackendGlusterOpen(pool)))
>> +        goto cleanup;
> 
> What kind of overhead is there in opening / closing gluster connections ?
> Is it something we should just do in the pool start/stop methods, instead
> of on every operation like refresh.

I was just copying after rbd.  In gdb, I see several threads spawned for
every refresh operation; but I can certainly try to rework things to
open at pool start instead.  Should I do it on this patch, or save it
for a followup?

The other consideration is that eventually, the
src/util/virstoragefile.c code will need to call into the
src/storage/storage_driver.c code (indirectly, via the virConnectPtr)
when doing recursive backing chain checking; in that case, I'm
envisioning a new callback (internal-only, no public API needed) that
roughly says: "given this URI for a backing file, tell me if you have a
backend for parsing the file, and if so, grab the header from that
file"; which means my ultimate goal is to have a reusable method that
both the pool XML (given host and name, [or host, volume, subdir, based
on review of 1/4]) and the qemu URI (given a string
"gluster://host/vol/...").  So while pool start may be better than pool
refresh for opening a glfs connection, the ability to open a glfs
connection also needs to be modular enough to reuse for one-off parsing
of a URI.


>> +    if (glfs_statvfs(state->vol, state->dir, &sb) < 0) {
>> +        virReportSystemError(errno, _("cannot statvfs path '%s'"),                             pool->def->source.name);
> 
> Seems you lost the line break here.

Yep, Peter caught it too.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131112/7d0c6431/attachment-0001.sig>