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. > > perform_slim_install.c: line 754: Nit - Sentence seems incomplete. > " .. if swap slice needs to be created" changed > > 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. > > 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 > > 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. > > 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? > > 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". > > disk_slices.c: line 398: What tag will be returned if none > was set in the first place? Have you verified it is V_UNASSIGNED? Yes, "unassigned" is partition tag == V_UNASSIGNED (zero). > > disk_slices.c: line 406: Nit - I don't think the comment needs to > include the type of this variable removed > > 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. > > In other words, this seems to be true only for AI installs and if > auto-install were to be changed to reorder how it executes > manifest actions, that could lead to problems here. I can't think of any slice action ordering that will break this code. Do you have an example in mind? > > disk_slices.c: line 607: Again this comment is not clear. > Suggest rewording as "If default slice action is specified (i.e. no > customizations), create a swap slice if deemed neccesary for the > install to proceed (typically true only > for low memory systems)" Took suggested wording. > > 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? > > disk_slices.c: The thing I'm still not clear about is whether > AI installs done prior to this bug fix will have V_SWAP set > for the swap slice. Will they? Yes, I checked this - the partition tag was (and is) being set to V_SWAP in /a/etc/vfstab. William
