Alok, Alok Aggarwal wrote: > > 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. You have a point - slice 2 is permitted (incorrectly, I believe) for install_slice_number in the schema, ai_manifest.rng, so if you specify slice 2 as the install slice, the schema will validate. Adding check in om_get_device_target_info() for install slice, prohibiting reserved slices from being the install slice. > >>> 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. Changed per request. > >>> 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. Here is udiff from the webrev: > @@ -1499,21 +1499,26 @@ > > return (IDM_E_VTOC_FAILED); > } > > /* > - * look, if default layout is to be used. If this is the case, > dedicate > - * slice 1 to swap (if required) and remaining space on disk/Solaris2 > - * partition to slice 0 > + * check attribute that is set if a swap slice is required, > + * used for low-memory systems > */ > + (void) nvlist_lookup_boolean_value(attrs, > + TI_ATTR_CREATE_SWAP_SLICE, &create_swap_slice); > > + /* > + * If default slice layout is indicated, > + * - dedicate slice 1 to swap (if swap slice is required) > + * - remaining space on disk/Solaris2 partition goes to slice 0 > + */ > + > if ((nvlist_lookup_boolean_value(attrs, TI_ATTR_SLICE_DEFAULT_LAYOUT, > &fl_slice_def_layout) == 0) && fl_slice_def_layout) { > > - if (nvlist_lookup_boolean_value(attrs, > - TI_ATTR_CREATE_SWAP_SLICE, &create_swap_slice) == 0 && > - create_swap_slice) { > + if (create_swap_slice) { > idm_debug_print(LS_DBGLVL_INFO, "vtoc: Default > layout " > "required with a swap slice, s1 will be > dedicated " > "to swap, s0 will occupy remaining space\n"); > > slice_num = 2; > Does this sufficiently explain it? > >>> 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. Indeed, but cscope data must be kept up-to-date, and token searches within the editor are much faster when you are coding. > >>> 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? I added some comments to the definition of RESERVED_SLICE() > >>> 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? I will leave the code as it is, mostly because of the release deadline. It might also be solved with an initialization routine for custom slices, since the objective is to insure that slice 1 is created, however it is not trivial, since the user might specify a delete action of slice 1, which would affect the logic. > > At a minimum, I think this code needs a giant comment > stating the conditions under which this function can > be called recursively. Added comment to create_swap_slice_if_necessary(), which performs recursive call.
Reissued webrev. Thanks for thorough review, William
