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.
