Hi Karen,
thank you very much for code review.
Please see my response in line.
Jan
On 09/20/10 09:24 PM, Karen Tung wrote:
Hi Jan,
I got the following questions and comments on the
boot_archive_archive.py file.
1) The fix you have for the bug seem kinda complicated and involve a
lot of new code.
To be honest, l am not quite convinced about this.
Most of the things which were not already there are comments which original
compress() function was lacking.
I agree that I have reordered the code and removed redundant or
inefficient parts.
Is there any problem with the following approach:
a) Move the cpio related stuff from lines 437-448 (of the old
boot_archive_archive.py file)
into a function.
b) For x86 and uncompress sparc case, call that function.
I believe that we should take advantage of that code for all scenarios,
since it is more efficient way to accomplish that task - it basically
does following:
[...]
os.system('cd <src> ; /usr/bin/find . | /usr/bin/cpio -pdum <dst>')
[...]
Now compare with the approach used in compress() function:
[...]
os.chdir(src)
compress_flist = find(["."])
for cfile in compress_flist:
if os.access(cpio_file, os.F_OK):
cmd = "echo " + cpio_file + " | " + CPIO + " -pdum " + dst + " 2>
/dev/null"
status = os.system(cmd)
[...]
So in latter case we are spawning subshell for each copied file.
Moreover, this is the root cause of 6984618, since this per-file
copy approach breaks hardlinks, so we can't use it as-is anyway.
c) For sparc and dcfs compress case, call the old "compress()" function.
d) Change the old compress() function to check for hard links, and skip
the compression for those that have hard links. So, just one additional
check in line 109 of the old file.
This would not be sufficient, since it would not fix the root cause
of 6984618 - see above.
This seem to make the changes less invasive. Also, it is probably
more efficient
since we are not doing many find() calls to generate large lists and
having to compute
the difference between those lists.
I believe that the opposite is the true. The original code is
inefficient, as it does
following:
[1] copy is done on per-file bases, subshell is spawned for each copy
operation - lines 96-99
[2] Each file is compressed using fiocompression at lines 109-112, even
those which have to be
left uncompressed (more than 1000 files, 90MB, mostly in /platform and
/kernel directories)
[3] Later those files are re-copied again from source to target at lines
165-176
The suggested fix basically addresses those aspects in following way:
[1] Files are copied all at once utilizing existing mechanism:
os.system('cd <src> ; /usr/bin/find . | /usr/bin/cpio -pdum <dst>')
[2+3] List of files eligible for compression is created and only those
are compressed.
With respect to number of find() calls, we aren't introducing new ones,
just taking advantage of those which were already there.
Out of curiosity, I have measured how long it takes to Python on Ultra 45
to generate the list of files to be compressed:
$ python
Python 2.6.4 (r264:75706, Aug 30 2010, 10:20:35) [C] on sunos5
Type "help", "copyright", "credits" or "license" for more information.
>>> import timeit
>>> s="""
... from osol_install.install_utils import find
... import os
...
os.chdir("/export/home/dc/text/text_sparc_6984618_147/step_ba-arch-clone/boot_archive/")
... ba_flist = find(["."])
... print len(ba_flist)
... uc=(['./etc/cluster/nodeid', './etc/dacf.conf', './etc/mach',
'./kernel', './platform', './usr/kernel'])
... exp_uc_flist = find(uc)
... print len(exp_uc_flist)
... compress_fset = set(ba_flist) - set(exp_uc_flist)
... print len(compress_fset)
... """
>>> t = timeit.Timer(stmt=s)
>>> t.timeit(number=1)
12154
1306
10848
23.369796991348267
>>>
So the whole operation took 23 seconds including all find() calls.
Please note that the most time consuming operation is the first find()
(which couldn't be avoided anyway), as this is where files get cached.
Subsequent calls to find() as well as computing difference between
two sets is pretty fast. In order to demonstrate that, I have
run that sequence for the second time now when caches are in use:
$ python
Python 2.6.4 (r264:75706, Aug 30 2010, 10:20:35) [C] on sunos5
Type "help", "copyright", "credits" or "license" for more information.
>>> import timeit
>>> s="""
... from osol_install.install_utils import find
... import os
...
os.chdir("/export/home/dc/text/text_sparc_6984618_147/step_ba-arch-clone/boot_archive/")
... ba_flist = find(["."])
... print len(ba_flist)
... uc=(['./etc/cluster/nodeid', './etc/dacf.conf', './etc/mach',
'./kernel', './platform', './usr/kernel'])
... exp_uc_flist = find(uc)
... print len(exp_uc_flist)
... compress_fset = set(ba_flist) - set(exp_uc_flist)
... print len(compress_fset)
... """
>>> t.timeit(number=1)
12154
1306
10848
1.0574219226837158
>>>
So subsequent call took just 1 second. That means, computing difference
between those two sets is not time consuming operation.
2) lines 108-109, 123-125: I think you are pre-pending "./" to all the
files so you can call "find()"
to expand the uncompressed list. Is the check to see whether the file
name start with './'
necessary?
We do not want to pre-pend './' if it is already there. Right now this
check is redundant,
since DC manifest as well as filelist.ramdisk contain entried w/o that './'.
That said, I would like to have it there, since pre-pending './' in case
it is already
there would break things.
Is it every a case that people are allowed to specify a relative
path in either
the DC manifest or the filelist.ramdisk?
All those paths are relative, since DC as well as filelist.ramdisk consumers
(e.g. bootadm(1m)) are alternate root aware, i.e. those paths are
relative to boot archive mountpoint.
3) line 346-348: compression level always need to be specified. For
SPARC,
if they don't want compression, I think users should be required
to specify the string "none" in the manifest. That's what they are
required to do
for x86.
Compression 'level' is applicable only to gzip compression, so not
applicable
to Sparc. On the other hand, compression 'type' is mandatory - we require
that it is provided:
[...]
BA_COMPR_TYPE = get_manifest_value(MANIFEST_READER_OBJ,
BOOT_ARCHIVE_COMPRESSION_TYPE)
if BA_COMPR_TYPE is None:
raise Exception, (sys.argv[0] +
": boot archive compression type missing from manifest")
[...]
Thanks,
--Karen
On 09/20/10 08:53, Jan Damborsky wrote:
Hello,
could I please get a code review for fix for following bug:
6984618 Not preserving hard links in boot archive during
fiocompression affects Sparc text media
webrev is available at:
http://cr.opensolaris.org/~dambi/bug-6984618/
Thank you,
Jan
tests accomplished:
[1] Sparc AI
* AI Sparc image based on 148 was built with DC containing the fix
* using that image, build 148 was installed
[2] Sparc text installer
* Text Sparc image based on 147 was built with DC containing the fix
* using that image, build 147 was installed
* resulting system was updated via pkg image-update to build 148
[3] regression tests
* x86 LiveCD image based on 148 was built with DC containing the fix
* using that image, build 148 was installed in Virtual Box guest
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
