Ethan, Thanks for the review. I've updated webrev with recommended changes, further comments inline below. Specifically on limits checking.
Webrev : http://cr.opensolaris.org/~mattman/bug-10115-12960/ On 03/18/10 02:04 AM, Ethan Quach wrote: > Hi Matt, > > Thanks for fixing these. Besides the element naming > comments we'd had offline, just a couple additional > comments below ... Made changes e.g. s/device_swap/swap_device, and s/device_dump/dump_device. > perform_slim_install.c > ------------------------------------- > 480 - nit - For the case where we end up printing > requested sizes as -1, that would seem awkward. > Could we instead just print the appropriate message > inside and outside each if() above? Updated to print two different debug messages depending on whether swap/dump sizes are specified. > 758,884 - us -> use DONE > 1307,1313 - nit - these messages lack stating that the > other will be created. Done > 2801 - was there a reason for this added check here? This is just a sanity check and should never really be hit, in the code above this you can see vol_num is set if requested_swap_size or requested_dump_size are != 0, therefore it is potentially possible to get to this section with a vol_num of 0, and this will cause TI to fail, as you cannot instantiate 0 volumes. Bear in mind this is just a sanity check, and should really never be hit. > 819, 942 (and consequently 2572, 2580) - I am wondering > if we shouldn't be normalizing the explicitly requested > swap/dump size against our defined MIN / MAX numbers. > > Previously, we had to come up with a number from thin > air, so we had to have internal 'defines' to cap ourselves. > But if given an explicit size (as long as it fits in the disk) > seems we should just use it. > > For example, on some super large system, if someone a > wanted 48Gig swap, why not let them. Or for some test > environments, if 256Mb swap was desired, seems we > should allow that too. This is something I toiled with aswell, but did not come to a resolution myself, having no MAX_SIZE I am fine with as the actual amount of available disk space will limit this either way. The MIN_SIZE's however I really think we should have. Attempting to create a zvol of zero size for swap/dump will fail (zfs create -V 0m, will fail), and to be honest does not make sense. The question then is what makes sense as a reasonable minimum size, 1MB ? for both swap and dump ? Point to note, the current definitions of MIN_SWAP_SIZE, MIN_DUMP_SIZE, MAX_DUMP_SIZE and MAX_SWAP_SIZE, I would like to leave in place, as these values in the default formula for calculating the size of swap/dump to be created where these sizes are not explicitly set by the user. cheers Matt > thanks, > -ethan > > > On 03/12/10 08:15, Matt Keenan wrote: >> Code review request for following two bugs : >> 10115 - AI ignores desired swap listed in ai manifest >> http://defect.opensolaris.org/bz/show_bug.cgi?id=10115 >> >> 12960 - Should be possible to define dump ZVOL size in AI manifest >> http://defect.opensolaris.org/bz/show_bug.cgi?id=12960 >> >> Webrev : >> http://cr.opensolaris.org/~mattman/bug-10115-12960/ >> >> Both bugs were addressed together as they affect same code paths. >> >> 10115 >> ----- >> AI manifest currently provides specification for <ai_swap_size>, >> however the contents of this element had never been implemented, it >> was simply ignored. >> Solution, was to read <ai_swap_size> from manifest, if defined and >> pass through to liborchestrator via nvlist as new nvpair. >> >> 12960 >> ----- >> This is a request to allow the specification of dump size in AI manifest. >> Solution, was to define new section in AI manifest <ai_device_dump> >> which can >> contain new element <ai_dump_size>. Parse this element in AI and pass >> through to liborchestrator in the same manner as swap size. >> >> In both cases for swap and dump size, a specification of "0" size is >> indicative that the user does not want a swap/dump zvol created. >> >> The only instance where this is ignored is in the case of swap where a >> system memory is low e.g. < 700MB, then a default swap slice of 512 is >> created (see disk_slices.c) >> >> Swap and dump sizes are always optional, if neither are specified, >> then the original algorithm for calculating swap and dump size is >> used, which is >> simply RAM/2. >> >> If specified swap size takes precedence over dump size. >> >> >> Following testing was carried out. >> >> Machine : X86 Vbox Client >> Disk Size : 16GB (16001 MB) >> Ram Size : 1GB (1024 MB) >> Recommended Software Size : ((4096 * 1.2) * 2) + 2048 = 11878 MB >> Available disk space for swap/dump : (16001 MB - 11878 MB) = 4123 MB >> >> Following scenarios were tested : >> >> <ai_swap_size> <ai_dump_size> Swap Created Dump Created >> --------------- ---------------- --------------- ------------ >> Not Specified Not Specified 512 511 >> 0 Not Specified Not Created 511 >> 250 Not Specified 512 511 >> 1024 Not Specified 1024 511 >> 5000 Not Specified 3867 256 >> Not Specified 0 512 Not Created >> Not Specified 250 512 256 >> Not Specified 1024 512 1024 >> Not Specified 5000 512 3611 >> 0 0 Not Created Not Created >> 250 0 512 Not Created >> 1500 0 1500 Not Created >> 5000 0 4123 Not Created >> 0 250 Not Created 256 >> 0 1500 Not Created 1500 >> 0 5000 Not Created 4123 >> 1024 1024 1024 1024 >> 3000 3000 3000 1123 >> 5000 5000 3867 256 >> 1024 4000 1024 3099 >> 1024 5000 1024 3099 >> 4000 1024 3867 256 >> 5000 1024 3867 256 >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
