[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: PATCH fix 510970, 529551, 530541



On Fri, 2009-11-20 at 21:04 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/20/2009 06:59 PM, Jerry Vonau wrote:
> >> +        if self._loopbackFile and os.path.exists(self._loopbackFile):
> >> +            log.info("and exists and removing %s" % self._loopbackFile )
> >> +            try:
> >> +                os.unlink(self._loopbackFile)
> >> +            except:
> >> +                pass
> >> +
> >>
> >> Not sure what you are trying to do here, self._loopbackFile is None (so false)
> >> the first time we go through this code path, and if we go through this code
> >> path multiple times, it should be a nop the second time (see the
> >> check for this you removed above).
> >>
> > Before coping the running install.img, if there is self._loopbackFile
> > present (from a previously failed install attempt), remove it. Think the
> > original check made a bad assumption, this file may not be the same
> > version as what is need by the currently booted version of anaconda.
> > self.anaconda.backend.mountInstallImage is only called once from
> > yuminstall.py
> >
> 
> Ah, this is a misunderstanding of what the check actually does, first of
> all it checks if we have generated a filename for storing install.img somewhere
> on a partition we are installing to / we are updating. If we have failed
> in a previous install and we need to transfer install.img, then that filename is
> not set yet, so we will transfer the current install.img, even if an older
> one is already in the same place.
> 
> This check only stops us from transefering the image if:
> 1) the filename was already generated (so this is not the first run
>     of this function this install)
> 2) a file with such name exists (iow the previous transfer attempt
>     was correct).
> 
> The current code for this is correct, and unless you have a really good reason
> to change it you should not.
> 
Yup, I misunderstood, badly..

> >> +        # This s/b the one that was copied into /tmp ....
> >> +        # Why was this copied to ram when you have passed stage2=
> >> +        # lets free the RAM
> >> +        if os.path.exists("/tmp/install.img"):
> >> +            log.info("OVERRIDE Using /tmp/install.img as stage2 image")
> >> +            stage2img = "/tmp/install.img"
> >> +            stage2ram = 1
> >> +        else:
> >> +            stage2img = installimg
> >>
> >> Ok, you've alsmost completely lost me here, I have some clue what
> >> you are trying to do, but it is completely unrelated to the previous parts
> >> of this patch.
> >>
> > No, it's related,  you need to have the source for install.img, that can
> > live on cd/dvd media, or for hard-drive and net installs in /tmp. I want
> > to recover the ram from /tmp. It's best if we copy the one that is
> > currently mounted by anaconda.
> >
> 
> It is related in that has to do with transfering the image, but it is not
> targeting the exact same purpose / fixing the exact same thing.
> 
> >> Please make *ONE* change per patch, and add a lot more verbose description
> >> of the why, what and how of the patch. So put in the description:
> >>
> >> 1) What you are trying to fix
> >> 2) What is the current behaviour (if applicable in various scenarios)
> >> 3) What are you changing and how does this fix this.
> >> 4) How does this change impact other install scenarios ?
> >>
> > Sorry, should of made incremental patches, I'll break up the patch, to
> > be more readable with some revisions.
> >
> 
> Ack, but your current split up is still not split up per purpose, I think
> I understand what you are trying to achieve, so let me suggest a split:
> 
> 1) A patch fixing the current copy code to use the fs with the most
>     free space instead of the least
> 
> 2) A patch to not only do the image transfer in case of a multiple disk
>     cd / dvd install, but also in the case of a hdiso / net install, so as
>     to free up memory used by install.img (which will be another patch).
>     This patch should only do this when memory is tight! Doing this
>     always is bad, as it is useless on systems with tons of memory, and
>     could potentially even cause issues there with for example diskspace
> 
> 3) A patch to actually achieve the freeing of memory 2)'s goal is by
>     unlinking the install.img from /tmp
> 
> 
Think I got the flow you want for the patches now, I'm digging at the
free variable for the first part now. That will take me some time as I
don't have much to spare. What I have now is less that ideal, I would
like see what is being returned, and just use / for the image till the
above has being sorted out. At least for my testing, the rest of my
patches depend on that one being in place.

> Notice that I'm not going with your suggested just always transfer
> install.img approach. This is not acceptable IMHO as it causes unnecessary
> slow down in many cases (net / disk install with plenty of ram, single dvd
> install), and it has the potential to trigger issues in all these cases
> which we would not hit if we did not do the transfer.
> 
> IOW doing the transfer has a price:
> 1) it consumes disk space which we may need
> 2) it causes us to take more "steps" then if we don't and each step we
> take can (and eventually will) cause issues, so avoiding extra steps where
> possible is good.
> 3) it causes a slowdown
> 
> So since it is not free we should not do it unless there are good reasons
> to, so far the only reason we had for doing this was a multiple cd/dvd
> install, but I'm willing to agree that for things like a network install
> on a low memory machine it would be a good thing to do too.
> 
Cool, then the question is at what amount of ram should this kick in at,
I went with MIN_GUI_RAM. 

> > Like you stated above, you may have to change disks, you don't have to
> > right now with a dvd, but how long before you need 2 DVDs?
> 
> Quite long, there is no reason why we should not be able to fix a package
> set for a compete desktop install on a single dvd for years to come.
> 
> > The move to
> > doConfigSetup is based on the fact that net and hdiso installs hold the
> > install image in /tmp. In order to make that ram available for yum/rpm,
> > I figured that is the earliest point to trigger that call to backend.py
> > where that could now take place. I think the trade off of having the
> > memory recovered, is a good one for using that space on the HD.
> 
> The trade off is only a good one if memory is a scarce resource, see above.
> 
Think I have that addressed in this round of patches against 12.46-2.

Thanks for taking the time, 

Jerry
diff -up ./backend.py.orig ./backend.py
--- ./backend.py.orig	2009-11-21 19:22:34.000000000 -0600
+++ ./backend.py	2009-11-21 19:30:38.000000000 -0600
@@ -163,9 +163,13 @@ class AnacondaBackend:
             return
 
         free = anaconda.id.storage.fsFreeSpace
+        # revert this once "free" is fixed
+        # need to see what is returned
+        log.info("free = %s" %free )
+        # screw it just use / for now
         self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
-                                                             free[0][0])
-
+                                                            "/")  
+#                                                             free[0][0])
         try:
             win = anaconda.intf.waitWindow(_("Copying File"),
                     _("Transferring install image to hard drive"))
diff -up ./yuminstall.py.orig ./yuminstall.py
--- ./yuminstall.py.orig	2009-11-21 20:24:29.000000000 -0600
+++ ./yuminstall.py	2009-11-21 20:24:32.000000000 -0600
@@ -725,6 +725,13 @@ class AnacondaYum(YumSorter):
 
         self.repos.setCacheDir(self.conf.cachedir)
 
+        if os.path.exists("/mnt/sysimage/boot/upgrade/install.img"):
+            log.info("REMOVING stage2 image from /mnt/sysimage/boot/upgrade")
+            try:
+                os.unlink ("/mnt/sysimage/boot/upgrade/install.img")
+            except:
+                log.warning("failed to clean /boot/upgrade")
+
     def downloadHeader(self, po):
         while True:
             # retrying version of download header
diff -up ./yuminstall.py.orig ./yuminstall.py
--- ./yuminstall.py.orig	2009-11-21 05:16:04.000000000 -0600
+++ ./yuminstall.py	2009-11-21 16:36:47.000000000 -0600
@@ -625,6 +625,25 @@ class AnacondaYum(YumSorter):
 
 
     def doConfigSetup(self, fn='/tmp/anaconda-yum.conf', root='/'):
+        # installs that don't use /mnt/stage2 hold the install.img on 
+        # a tmpfs, free this ram if things are tight.
+        stage2img = "/tmp/install.img"
+
+        if os.path.exists(stage2img) and iutil.memInstalled() < isys.MIN_GUI_RAM: 
+            log.info("%s exists and low memory" % stage2img )
+
+        # free up /tmp for more memory before yum is called, 
+            try: 
+                self.anaconda.backend.mountInstallImage(self.anaconda, stage2img)
+            except: 
+                log.info("mountInstallImage failed")
+                   
+            try:
+                os.unlink(stage2img)
+            except:
+                log.info("clearing /tmp failed")
+                pass
+
         if hasattr(self, "preconf"):
             self.preconf.fn = fn
             self.preconf.root = root

--- backend.py.orig2	2009-11-21 19:52:50.000000000 -0600
+++ backend.py	2009-11-21 19:53:23.000000000 -0600
@@ -156,11 +156,19 @@
         if self._loopbackFile and os.path.exists(self._loopbackFile):
             return
 
-        # If we've booted off the first CD/DVD (so, not the boot.iso) then
-        # copy the install.img to the filesystem and switch loopback devices
-        # to there.  Otherwise we won't be able to unmount and swap media.
-        if not anaconda.mediaDevice or not os.path.exists(installimg):
-            return
+        # remove me later..
+
+        # You get here currently only if your using cd's, based on 
+        # yuminstall's run, mkeys.sort(mediasort), if len(mkeys) > 1.
+        # hd installs hold install.img in /tmp, while preupgrade might 
+        # be forced to download install.img again. mediaDevice has alrealy 
+        # been validated in yuminstall.py and you can't get here without 
+        # mounting install.img, removed the check to enable first patch.
+
+        # If we've booted off the first CD (so, not the boot.iso or DVD) 
+        # then copy the install.img to the filesystem and switch loopback 
+        # devices to there.  Otherwise we won't be able to unmount and 
+        # swap media.
 
         free = anaconda.id.storage.fsFreeSpace
         # revert this once "free" is fixed
@@ -199,7 +207,12 @@
             return 1
 
         isys.lochangefd("/dev/loop0", self._loopbackFile)
-        isys.umount("/mnt/stage2")
+
+        # http, ftp, hd installs don't mount /mnt/stage2
+        try:
+            isys.umount("/mnt/stage2")
+        except:
+            pass
 
     def removeInstallImage(self):
         if self._loopbackFile:

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]