On 03/18/10 06:03 PM, Ethan Quach wrote:
>
>
> On 03/18/10 04:14, Matt Keenan wrote:
>> 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/
>
> The updates look good.
>
>>
>>
>> On 03/18/10 02:04 AM, Ethan Quach wrote:
>>
>>> 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.
>
> This would imply that for the case where the user specified
> explicit swap and dump sizes as 0 in the manifest,
> prepare_zfs_volume_attrs() will never be called. If this
> is the case, then okay. Otherwise, it would seem that a
> OM_SUCCESS should be returned instead.
>
This is what is implied if user specifies both swap and dump as zero, then
prepare_zfs_volume_attrs() will never get called, so OM_FAILURE is correct I
think in this case.
>>
>>> 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.
>
> For the user-specified 0 case, we've already decided for that
> to mean they don't want to create that device at all, and we
> treat that specially.
Yes in that we don't create it at all :-)
>
>>
>> The question then is what makes sense as a reasonable minimum size,
>> 1MB ? for both swap and dump ?
>
> Perhaps defer back to the underlying component being
> a zfs volume? so whatever the minimum for a volume can
> be?
From man page the size is rounded to nearest 128KB, so I think enforcing a 1MB
minimum in this scenario should be fine.
I've updated calc_dump_size() and calc_swap_size() to have 1MB min limit of the
sizes were explicitly requested.
I've updated calculate_available_swap_dump_space() to remove the limits
checking
completely. It was only validating the limits if a positive value was specified
for either swap or dump, as we now don't care about a max the limit checking is
not necessary.
Updated webrev posted :
http://cr.opensolaris.org/~mattman/bug-10115-12960
Thanks again for the review.
cheers
Matt
>> 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.
>
> completely agree. my comment was only meant for the
> code path where we're handling an explicit user-specified
> size.
>
>
> thanks,
> -ethan
>
>>
>> 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
>>
