On Wed, 15 Apr 2009, William Schumann wrote:
> Alok,
> Responses below.
> Alok, Sanjay,
> Generated new webrev:
> http://cr.opensolaris.org/~wmsch/bug-7758/
> Previous webrev for reference:
> http://cr.opensolaris.org/~wmsch/bug-7758-prior/
>
> Alok Aggarwal wrote:
>>
>> On Tue, 14 Apr 2009, William Schumann wrote:
>>
>>> Alok,
>>>
>>> Alok Aggarwal wrote:
>>>> Hi William,
>>>>
>>>> On Thu, 9 Apr 2009, William Schumann wrote:
>>>>
>>>>> There are two problems addressed by this bug.
>>>>>
>>>>> 1) After initial AI installation, the swap slice is regarded as user
>>>>> data in subsequent AI installations instead of scratch
>>>>> 2) The swap slice space allocation in TI is not done with any awareness
>>>>> of AI customization
>>>>>
>>>>> The fix involving making AI aware of 1 and for AI to perform 2 instead
>>>>> of leaving everything to TI, which allocates the swap space for GUI.
>>>>
>>>> So, from a high level, is it fair to say that the policy with respect to
>>>> preserving slices by default isn't being
>>>> changed?
>>> Yes, the policy for preserving slices by default does not change.
>>>> And, the change you're making here is to create
>>>> a swap slice if either s1 doesn't exist or s1 exists with
>>>> a V_SWAP tag. Correct?
>>> The decision of whether a swap slice is created depends upon the memory
>>> size of the machine.
>>>>
>>>> Also now that 7718 has been reversed in the source code
>>>> only machines with less than 700MB memory will see this
>>>> problem on every other AI install after the *first* install.
>>> I think it would fail *all* AI installs after the first one (that didn't
>>> explicitly delete slice 1 per the workaround); otherwise, what you said
>>> is true. Also, without the fix, problem 2) above still exists - if a
>>> swap slice is called for, any slice customization or any existing slices
>>> which should be preserved will be destroyed instead.
>>
>> Okay, thanks for clarifying. Here are my comments -
>>
>> auto_install.c: Why is OM_ROOT being set for installs on
>> slices other than slice zero?
> OM_ROOT indicates that the VTOC partition tag for the install slice will be
> set to "root" - slices 0,1,3,4,5,6, or 7 could be the install slice.
I didn't read that expression correctly the first
time. I don't see how we exclude slice 2 from being
the install slice, and if we don't, it is something
we should revisit when we take a second look at the
relevant design.
>> perform_slim_install.c: line 2919: This function no longer
>> returns a boolean_t
> It returns the boolean_t values (0 or 1), which are evaluated in logical
> expressions as either zero or non-zero, so I don't see a problem. In
> principle, the definition of boolean_t could conceivably be changed, but
> much code would break. Also, I think of the boolean_t type as helpful in
> simplifying logical expressions making them more readable.
<sys/types.h> defines boolean_t as -
typedef enum { B_FALSE, B_TRUE } boolean_t;
Thus, it would be more appropriate for the return value to
be either B_FALSE or B_TRUE.
>> ti_dm.c: lines 1503 - 1515: It seems that this comment should
>> be made clearer to indicate under which conditions a swap slice
>> would be created - a) If it is explicitly specified or, b) if
>> the code figured out if the installation was being done on a low
>> memory machine
> added comments here
For some reason I still don't see the comments in
the updated webrev.
>> disk_slices.c: line 50: Why this change?
> I tried to explain this in the code review request - I was just changing the
> name from "install", which is obviously not a unique token, to "s_install",
> which would be more easily found in a text editor - that's the only reason.
> Vim has a nice feature - if you type '*' on a token, it grabs the token as
> the search string and jumps to the next occurrence.
I don't see this as a reason to change, I think
'install' is as good. But I don't have any strong
opinions on this, either way is fine.
Btw, if you haven't played with cscope it's worth
it. It's a much easier to search, among other things,
variable/function definitions.
>> disk_slices.c: line 365: How about check_slices_in_use() as
>> the name instead?
> I thought that "if (are_any_slices_in_table())" would be simple and readable
> for a function that asks the question "are there any slices already defined
> in the table?". Was this unclear? Too casual?
I think it's too casual but again I don't have any
strong opinions.
>> disk_slices.c: line 376: What is an exempt slice?
> A slice that is reserved for a special purpose and cannot be used for user
> data. Changed "exempt" to "reserved".
How about adding a blurb about what a reserved slice
is?
>> disk_slices.c: lines 430-432: Could you please expound on this?
>> There seems to be implicit ordering here that the callers must
>> maintain.
> Actually, the purpose of the code here is to avoid implicit ordering by the
> caller. If a swap slice is needed, it is easier to create it before
> creating any other slices - otherwise you have to decide which slice to take
> the space from. This create_swap_slice_if_necessary() call insures that the
> swap slice will be allocated before any other new slices are created. Also,
> create_swap_slice_if_necessary() is called during finalization for TI.
>
> Perhaps I should mention here that the creation of a swap slice cannot be
> specified in a manifest - it is automatic.
The last part is what I was missing, makes sense.
>> disk_slices.c: line 658: The thought of possible recursion
>> in this function makes me queasy. This seems like a perfect
>> candidate for some code refactoring in the future.
> Perhaps you understand this already, but I'll add the following just in
> case. There is no explicit initialization call in the Orchestrator for AI
> where the swap slice might be allocated. So, the first call from AI
> concerning slice customization could be om_create_slice(). Within
> om_create_slice() is a check to see if the swap slice is necessary. If it
> is, om_create_slice will be called recursively once, before it does any
> other work, so make sure that the swap slice is allocated. Furthermore,
> there is a guard so that it is not run more than once for the same purpose.
> Does this help?
Is this the only scenario in which this function can
be called recursively? Is there a way to restructure
this so it isn't called recursively or is it too risky
at this point?
At a minimum, I think this code needs a giant comment
stating the conditions under which this function can
be called recursively.
Alok
