On Fri, Dec 07, 2012 at 12:16:08PM +0800, Osier Yang wrote: > On 2012年12月07日 09:00, Eric Blake wrote: > >On 12/05/2012 04:14 AM, Osier Yang wrote: > >>On 2012年12月05日 19:03, Jiri Denemark wrote: > >>>On Wed, Dec 05, 2012 at 18:54:42 +0800, Osier Yang wrote: > >>>... > >>>>>However, the mainly reason I choosed to use a sub-list of domain names > >>>>>is for future extenstion, I.E. Assuming there are other disk setting > >>>>>(you never known how many they will be), we have to guarantee they are > >>>>>same among guests in future. Looking up the disk def with domain and > >>>>>disk path gives us much flexibility IMHO. > >>>>> > >>>> > >>>>So the point of the argument is: the trade between the flexibility and > >>>>the uncomfortable locks. > >>> > >>>OK, I guess we can store more info in the sharedDisks list (either > >>>today or > >>>later when we need it), we may even store the domain list there, but > >>>we don't > >>>definitely want to go through all the domains to get the required > >>>details. > >>> > >> > >>I think it should be just a few guests share the disk in practice, > >>which means in practice the sharedDisks->disks[i]->ndomains should > >>be short. And that's the other reason why I can live with the > >>locks. Though in theory, it can be all domains (assuming it's large > >>number) share the disk. > >> > >>The other question is: Aren't we already go through all domain objects > >>in many places? > > > >I'm stepping in a bit late, and haven't fully looked at the series yet, > >but one of the things that I would love for libvirt to someday have is a > >way to get a list of all virStorageVolPtr associated with any given > >virDomainPtr, and conversely, given a virStorageVolPtr, return a list of > >all virDomainPtr that use the file (whether directly, or whether as a > >backing file). > > This is actually in my right next TODO list, I.E to associate the > domain and storage. See (I'm just having rest with this SG_IO set > at the half of persistent NPIV support): > > https://www.redhat.com/archives/libvir-list/2012-November/msg00878.html > > > To get to that point, I envision having a way for every > ><disk> element to be tied to a virStorageVolPtr, even if it means the > >implicit creation of a transient virStoragePoolPtr directory-based pool > >for any<disk> specified without an explicit pool. Then, creation of a > >new domain, as well as hot-plugging of any disks to an existing domain, > >will update each affected virStorageVolPtr; and you really _should_ be > >maintaining a list of all associated domains with the disk object. > >Thus, I think searching whether a shared disk is allowed should be a > >matter of asking that shared disk what domains it is already associated > >with, rather than asking each domain whether it uses the shared disk. > > I think this gets rid of the argument completely. We can introduce > a new struct as member of virStorageVolDef to store the disk settings > which have to be consistent among the domains share it. E.g. > > typedef struct virStorageVolSharedSettings virStorageVolSharedSettings; > struct virStorageVolSharedSettings { > int cdbfilter; > > /* Others in future */ > }; > > struct _virStorageVolDef { > char *name; > char *key; > int type; /* virStorageVolType enum */ > > ... > > virStorageVolSharedSettings sharedSettings; > > ... > }; > > As each domain disk is mapped to a volume object, we can just initialize > the sharedSettings during parsing or starting. The sharedSetting can > be further used to do checking when starting, attaching. I don't really like this becuase you are mixing up configuration with runtime state. virStorageVolDef should be exclusively about configuration data. Further I don't like the fact that information about domain state is being stored in the storage driver. I prefer to have strict separation between the two, with the only interaction being based on passing (uuid,name) between the domain & storage drivers. This is key to allowing us to split libvirtd up into multiple daemons in the future. I think this is rather over-engineering things for the needs of the cdb filter. Fundamentally we do not need to know /which/ domains are using the same disk, only how many. If the number currently using the disk is non-zero, then the new domain must match the current CDB filter setting. If the number using the disk is zero, then the new domain can have any CDB filter setting. This is really just a hashtable of (path, reference count). No need to store virDomainObjPtr instances, nor virStorageVolptr instances. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list