Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   CentOS Development (http://www.linux-archive.org/centos-development/)
-   -   PATCH fix 510970, 529551, 530541 (http://www.linux-archive.org/centos-development/279606-patch-fix-510970-529551-530541-a.html)

Jerry Vonau 11-12-2009 08:00 PM

PATCH fix 510970, 529551, 530541
 
This is currently untested, but I'm looking for feedback. Think most of
the issues with the size of /boot can be traced to preupgrade or
backend.py:

free = anaconda.id.storage.fsFreeSpace
self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
free[0][0])

This ends up placing self._loopbackFile on /mnt/sysimage/boot and is
causing anaconda to not have enough room to copy the install.img. I just
changed the path to be /mnt/sysimage/tmp, should have lots of room
there.

While in the preupgrade case, /boot may not be able to hold the new
kernel due to having /boot/upgrade/install.img on its filesystem. This
gets copied to /tmp and just takes up space on boot once anaconda loads.
As a bonus once install.img is copied to /mnt/sysimage/tmp, anaconda's
/tmp is purged of the install.img, if present, freeing up ram.

Hoping I'm on the right track,

Jerry

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hans de Goede 11-13-2009 08:16 AM

PATCH fix 510970, 529551, 530541
 
Hi Jerry,

Many thanks for the patch, I'll let others who know this part
of the code better review it, but I still wanted to say thanks :)

Regards,

Hans


On 11/12/2009 10:00 PM, Jerry Vonau wrote:

This is currently untested, but I'm looking for feedback. Think most of
the issues with the size of /boot can be traced to preupgrade or
backend.py:

free = anaconda.id.storage.fsFreeSpace
self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
free[0][0])

This ends up placing self._loopbackFile on /mnt/sysimage/boot and is
causing anaconda to not have enough room to copy the install.img. I just
changed the path to be /mnt/sysimage/tmp, should have lots of room
there.

While in the preupgrade case, /boot may not be able to hold the new
kernel due to having /boot/upgrade/install.img on its filesystem. This
gets copied to /tmp and just takes up space on boot once anaconda loads.
As a bonus once install.img is copied to /mnt/sysimage/tmp, anaconda's
/tmp is purged of the install.img, if present, freeing up ram.

Hoping I'm on the right track,

Jerry




_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Jerry Vonau 11-16-2009 11:16 PM

PATCH fix 510970, 529551, 530541
 
On Fri, 2009-11-13 at 10:16 +0100, Hans de Goede wrote:
> Hi Jerry,
>
> Many thanks for the patch, I'll let others who know this part
> of the code better review it, but I still wanted to say thanks :)
>
> Regards,
>
> Hans
>
revised patch & tested with both preupgrade and 12.46-2 on a usbkey

anaconda.log from uskey install:

15:12:59 DEBUG : isys.py:mount()- going to mount /dev/loop1
on /mnt/source as
iso9660 with options defaults,ro
15:12:59 INFO : mediaDevice is None
15:12:59 INFO : Using /mnt/source/images/install.img as stage2 image
15:12:59 INFO : Full image path
is /mnt/sysimage/tmp/rhinstall-install.img
15:12:59 INFO : OVERRIDE Using /tmp/install.img as stage2 image
15:13:02 INFO : set mediaid of repo InstallationRepo to:
1257724710.186325
15:13:28 INFO : moving (1) to step tasksel

Sorry, forgot to save the one from the preupgrade session.

Jerry


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hans de Goede 11-20-2009 09:25 AM

PATCH fix 510970, 529551, 530541
 
Hi Jerry et all,

Below is a review of your patch:


diff -up ./backend.py.orig ./backend.py
--- ./backend.py.orig 2009-11-16 17:58:30.000000000 -0600
+++ ./backend.py 2009-11-16 18:00:15.000000000 -0600
@@ -153,29 +153,45 @@ class AnacondaBackend:
if not flags.setupFilesystems:
return

- if self._loopbackFile and os.path.exists(self._loopbackFile):
- return
+ log.info("mediaDevice is %s" % anaconda.mediaDevice )
+ # If they booted with a boot.iso, just continue using that install.img.

- # 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):
+ if os.path.exists("/mnt/stage2/images/install.img"):
+ log.info("Don't need to transfer stage2 image")
return


This is not correct, we may need to unmount stage2 when it is an install CD / DVD
as we may need to change DVD / CD as the needed packages may be split over multiple
disks.

-
- free = anaconda.id.storage.fsFreeSpace
+
+ log.info("Using %s as stage2 image" % installimg)
self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
- free[0][0])
+ "/tmp")
+ log.info("Full image path is %s" % self._loopbackFile )
+

So you're hardcoding the use of /tmp here, but there are no ideas /tmp is a
good candidate in case of an upgrade it might even be a tmpfs for people who
have modified their fstab to make it such.

You are on to something here, the old code is using the first
element of anaconda.id.storage.fsFreeSpace, which is sorted by size, but
that is the element with the smallest size! I think this is a bug and the
code should use the last element

+ 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).

+ # 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.

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 ?


try:
win = anaconda.intf.waitWindow(_("Copying File"),
- _("Transferring install image to hard drive"))
- shutil.copyfile(installimg, self._loopbackFile)
+ _("Transferring install image to hard drive..."))
+ shutil.copyfile(stage2img, self._loopbackFile)
win.pop()
except Exception, e:
if win:
win.pop()

- log.critical("error transferring install.img: %s" %(e,))
+ log.critical("error transferring stage2img: %s" %(e,))

if isinstance(e, IOError) and e.errno == 5:
msg = _("An error occurred transferring the install image "
@@ -195,7 +211,21 @@ class AnacondaBackend:
return 1

isys.lochangefd("/dev/loop0", self._loopbackFile)
- isys.umount("/mnt/stage2")
+
+ # install.img is now in /tmp get rid of the original in boot
+ if os.path.exists("/mnt/sysimage/boot/upgrade/install.img"):
+ log.info("REMOVING install.img from /mnt/sysimage/boot/upgrade")
+ os.unlink("/mnt/sysimage/boot/upgrade/install.img")
+
+ if stage2ram:
+ try:
+ os.unlink(stage2img)
+ except:
+ pass
+ try:
+ isys.umount("/mnt/stage2")
+ except:
+ pass

def removeInstallImage(self):
if self._loopbackFile:


diff -up ./yuminstall.py.orig ./yuminstall.py
--- ./yuminstall.py.orig 2009-11-16 17:59:45.000000000 -0600
+++ ./yuminstall.py 2009-11-16 18:00:15.000000000 -0600
@@ -625,6 +625,9 @@ class AnacondaYum(YumSorter):


def doConfigSetup(self, fn='/tmp/anaconda-yum.conf', root='/'):
+ stage2img = "%s/images/install.img" % self.tree
+ self.anaconda.backend.mountInstallImage(self.anaco nda, stage2img)
+
if hasattr(self, "preconf"):
self.preconf.fn = fn
self.preconf.root = root
@@ -828,12 +831,6 @@ class AnacondaYum(YumSorter):
mkeys = self.tsInfo.reqmedia.keys()
mkeys.sort(mediasort)

- if len(mkeys) > 1:
- stage2img = "%s/images/install.img" % self.tree
- if self.anaconda.backend.mountInstallImage(self.anaco nda, stage2img):
- self.anaconda.id.storage.umountFilesystems()
- return DISPATCH_BACK
-
for i in mkeys:
self.tsInfo.curmedia = i
if i > 0:


Again, this seems yet another changed put in one and the same patch without any
explanation. Please split this patch up and thoroughly explain the reasoning behind
the patch. Also it seems wrong, as you are now transfering the image to HD, even when
we are doing a single dvd install, eating all that precious hard disk space you
are trying to avoid us running out of.

Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Jerry Vonau 11-20-2009 04:59 PM

PATCH fix 510970, 529551, 530541
 
On Fri, 2009-11-20 at 11:25 +0100, Hans de Goede wrote:
> Hi Jerry et all,
>
> Below is a review of your patch:
>
>
> diff -up ./backend.py.orig ./backend.py
> --- ./backend.py.orig 2009-11-16 17:58:30.000000000 -0600
> +++ ./backend.py 2009-11-16 18:00:15.000000000 -0600
> @@ -153,29 +153,45 @@ class AnacondaBackend:
> if not flags.setupFilesystems:
> return
>
> - if self._loopbackFile and os.path.exists(self._loopbackFile):
> - return
> + log.info("mediaDevice is %s" % anaconda.mediaDevice )
> + # If they booted with a boot.iso, just continue using that install.img.
>
> - # 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):
> + if os.path.exists("/mnt/stage2/images/install.img"):
> + log.info("Don't need to transfer stage2 image")
> return
>
>
> This is not correct, we may need to unmount stage2 when it is an install CD / DVD
> as we may need to change DVD / CD as the needed packages may be split over multiple
> disks.
>
Shoot, blew that one, I was focusing in on the hd install method. That
is an easy fix.

> -
> - free = anaconda.id.storage.fsFreeSpace
> +
> + log.info("Using %s as stage2 image" % installimg)
> self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
> - free[0][0])
> + "/tmp")
> + log.info("Full image path is %s" % self._loopbackFile )
> +
>
> So you're hardcoding the use of /tmp here, but there are no ideas /tmp is a
> good candidate in case of an upgrade it might even be a tmpfs for people who
> have modified their fstab to make it such.
>
Your saying that anaconda will mount whatever is in the /etc/fstab in an
upgrade situation? Just wondering, how about NFS shares? In any case how
about creating a directory on /mnt/sysimage to hold self._loopbackFile?
Then clean that up when done, like some sort of scratch space. Or just
dump it into /?

> You are on to something here, the old code is using the first
> element of anaconda.id.storage.fsFreeSpace, which is sorted by size, but
> that is the element with the smallest size! I think this is a bug and the
> code should use the last element
>
Glad something came out positive, if that is fixed then the above change
is unneeded.

> + 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

> + # 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.

> 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.

>
> try:
> win = anaconda.intf.waitWindow(_("Copying File"),
> - _("Transferring install image to hard drive"))
> - shutil.copyfile(installimg, self._loopbackFile)
> + _("Transferring install image to hard drive..."))
> + shutil.copyfile(stage2img, self._loopbackFile)
> win.pop()
> except Exception, e:
> if win:
> win.pop()
>
> - log.critical("error transferring install.img: %s" %(e,))
> + log.critical("error transferring stage2img: %s" %(e,))
>
> if isinstance(e, IOError) and e.errno == 5:
> msg = _("An error occurred transferring the install image "
> @@ -195,7 +211,21 @@ class AnacondaBackend:
> return 1
>
> isys.lochangefd("/dev/loop0", self._loopbackFile)
> - isys.umount("/mnt/stage2")
> +
This frees up /boot for preupgrade,

> + # install.img is now in /tmp get rid of the original in boot
> + if os.path.exists("/mnt/sysimage/boot/upgrade/install.img"):
> + log.info("REMOVING install.img from /mnt/sysimage/boot/upgrade")
> + os.unlink("/mnt/sysimage/boot/upgrade/install.img")
> +

This frees up the ram

> + if stage2ram:
> + try:
> + os.unlink(stage2img)
> + except:
> + pass
> + try:
> + isys.umount("/mnt/stage2")
> + except:
> + pass
>
> def removeInstallImage(self):
> if self._loopbackFile:
>
>
> diff -up ./yuminstall.py.orig ./yuminstall.py
> --- ./yuminstall.py.orig 2009-11-16 17:59:45.000000000 -0600
> +++ ./yuminstall.py 2009-11-16 18:00:15.000000000 -0600
> @@ -625,6 +625,9 @@ class AnacondaYum(YumSorter):
>
>
> def doConfigSetup(self, fn='/tmp/anaconda-yum.conf', root='/'):
> + stage2img = "%s/images/install.img" % self.tree
> + self.anaconda.backend.mountInstallImage(self.anaco nda, stage2img)
> +
> if hasattr(self, "preconf"):
> self.preconf.fn = fn
> self.preconf.root = root
> @@ -828,12 +831,6 @@ class AnacondaYum(YumSorter):
> mkeys = self.tsInfo.reqmedia.keys()
> mkeys.sort(mediasort)
>
> - if len(mkeys) > 1:
> - stage2img = "%s/images/install.img" % self.tree
> - if self.anaconda.backend.mountInstallImage(self.anaco nda, stage2img):
> - self.anaconda.id.storage.umountFilesystems()
> - return DISPATCH_BACK
> -
> for i in mkeys:
> self.tsInfo.curmedia = i
> if i > 0:
>
>
> Again, this seems yet another changed put in one and the same patch without any
> explanation. Please split this patch up and thoroughly explain the reasoning behind
> the patch. Also it seems wrong, as you are now transfering the image to HD, even when
> we are doing a single dvd install, eating all that precious hard disk space you
> are trying to avoid us running out of.
>

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? 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. At least
the behavior is the now the same between dvd and cdroms.

Revised patches, the numbers reflex the apply order.

Thanks for taking the time,

Jerry
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hans de Goede 11-20-2009 07:04 PM

PATCH fix 510970, 529551, 530541
 
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.


+ # 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


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.


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.

Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Jerry Vonau 11-22-2009 03:51 AM

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
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hans de Goede 11-22-2009 07:27 PM

PATCH fix 510970, 529551, 530541
 
Hi,

On 11/22/2009 05:51 AM, Jerry Vonau wrote:

<snip>


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.



Oh, but that is easy, simply change:
self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
free[0][0])
to:
self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
free[len(free)-1][0])

So that you use the last element of the list, instead of the first, so that
you get the biggest partition, your testing of this fix is much appreciated :)


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.



That sounds reasonable. Maybe want to do use something like MIN_GUI_RAM + 100MB,
so as to have atleast MIN_GUI_RAM free when the install.img (which is approx
100Mb) lives in 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,


You are welcome, thanks for contributing to anaconda. I hope that once you
get the hang of it you keep on contributing :)

Here is a review of your latest set of patches:

1freefix.diff:
See above

2modcall.diff:
Please put the large comment in a commit msg, it would be great if you
could learn to use git. (Drop by on #anaconda during CET office hours and
I'll help you). Otherwise atleast learn to write commit messages, in a
.patch / .diff file you can put text above the
--- filename
+++ filename

Lines and patch (and other tools) will ignore this, please learn to put some
explanation of the patch there start with a single line summary, so for
example a good header for 2modcall.diff would be:

###
Remove unneeded check from mountInstallImage()

This is a preparation patch for transferring install.img from /tmp to
disk in low memory network and hdiso install scenarios to free up
memory used by install.img under /tmp.

Currently mountInstallImage only gets called when using cd's, based on
yuminstall's run, mkeys.sort(mediasort), if len(mkeys) > 1.
mediaDevice has already been validated in yuminstall.py and you can't
get here without mounting install.img, so there is no need for the
check this patch removes.
###

The second hunk of this patch should be part of the 3th patch (or separate)


3setcall.diff:

1) This does not belong in doConfigSetup(), setup() itself or a new method
called from setup() would be a better place.

2) Even if the mountInstallImage() fails you still unlink /tmp/install.img


4boot.diff:

Ah a new trick upi your sleef (atleast to me), nice one. But will this
work ? Does the loader (stage1) copy install.img from /boot/upgrade to
/tmp ? I'm asking because if it does not, then we will still have the
loopback mounted and the unlink will not free up the diskspace.


We are getting somewhere, I think this will greatly improve certain
types of installs on low memory machines, and it will help with some
pre-upgrade issues, thanks!

Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Jerry Vonau 11-22-2009 08:48 PM

PATCH fix 510970, 529551, 530541
 
On Sun, 2009-11-22 at 21:27 +0100, Hans de Goede wrote:
> Hi,
>
> On 11/22/2009 05:51 AM, Jerry Vonau wrote:
>
> <snip>
>
> >> 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.
> >
>
> Oh, but that is easy, simply change:
> self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
> free[0][0])
> to:
> self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
> free[len(free)-1][0])
>
Yea, I see that now, but I need to see what was being returned first ;-)

> So that you use the last element of the list, instead of the first, so that
> you get the biggest partition, your testing of this fix is much appreciated :)
>
That I can test quickly. I get back to you in a bit. (firing up vmm)

> >> 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.
> >
>
> That sounds reasonable. Maybe want to do use something like MIN_GUI_RAM + 100MB,
> so as to have atleast MIN_GUI_RAM free when the install.img (which is approx
> 100Mb) lives in RAM.
>
That is do-able.

> >>> 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,
>
> You are welcome, thanks for contributing to anaconda. I hope that once you
> get the hang of it you keep on contributing :)
>
> Here is a review of your latest set of patches:
>
> 1freefix.diff:
> See above
>
> 2modcall.diff:
> Please put the large comment in a commit msg, it would be great if you
> could learn to use git. (Drop by on #anaconda during CET office hours and
> I'll help you). Otherwise atleast learn to write commit messages, in a
> .patch / .diff file you can put text above the
> --- filename
> +++ filename
>
I'm getting the hang of git, slowly but surely..

> Lines and patch (and other tools) will ignore this, please learn to put some
> explanation of the patch there start with a single line summary, so for
> example a good header for 2modcall.diff would be:
>
> ###
> Remove unneeded check from mountInstallImage()
>
> This is a preparation patch for transferring install.img from /tmp to
> disk in low memory network and hdiso install scenarios to free up
> memory used by install.img under /tmp.
>
> Currently mountInstallImage only gets called when using cd's, based on
> yuminstall's run, mkeys.sort(mediasort), if len(mkeys) > 1.
> mediaDevice has already been validated in yuminstall.py and you can't
> get here without mounting install.img, so there is no need for the
> check this patch removes.
> ###
>
> The second hunk of this patch should be part of the 3th patch (or separate)
>
>
> 3setcall.diff:
>
> 1) This does not belong in doConfigSetup(), setup() itself or a new method
> called from setup() would be a better place.
>
OK, I'll see what I come up with.

> 2) Even if the mountInstallImage() fails you still unlink /tmp/install.img
>
Yea, rushed that part, should be checking for a return code before
unlinking.

>
> 4boot.diff:
>
> Ah a new trick upi your sleef (atleast to me), nice one. But will this
> work ? Does the loader (stage1) copy install.img from /boot/upgrade to
> /tmp ? I'm asking because if it does not, then we will still have the
> loopback mounted and the unlink will not free up the diskspace.
>
The harddrive method never used to until preupgrade came along, but it
does now.

>
> We are getting somewhere, I think this will greatly improve certain
> types of installs on low memory machines, and it will help with some
> pre-upgrade issues, thanks!
>
At least if I don't have the time, the ideas are out there now.

Thanks,

Jerry


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Jerry Vonau 11-22-2009 10:25 PM

PATCH fix 510970, 529551, 530541
 
On Sun, 2009-11-22 at 15:48 -0600, Jerry Vonau wrote:
> On Sun, 2009-11-22 at 21:27 +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 11/22/2009 05:51 AM, Jerry Vonau wrote:
> >
> > <snip>
> >
> > >> 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.
> > >
> >
> > Oh, but that is easy, simply change:
> > self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
> > free[0][0])
> > to:
> > self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
> > free[len(free)-1][0])
> >
> Yea, I see that now, but I need to see what was being returned first ;-)
>
> > So that you use the last element of the list, instead of the first, so that
> > you get the biggest partition, your testing of this fix is much appreciated :)
> >
> That I can test quickly. I get back to you in a bit. (firing up vmm)

That works, I added some logging to return self._loopbackFile to my
updates.img, Then doing a vmm http install results in this logging:
http://members.shaw.ca/jvonau/pub/F12/looploc.png

Then a screen shot of df before/after
http://members.shaw.ca/jvonau/pub/F12/dfinstall.png

Got to do family time now,

Jerry





_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list


All times are GMT. The time now is 04:01 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.