On Sat, 2010-11-13 at 20:40 -0800, Akshay Lal wrote:
> Hey Lucas,
>
> First up thanks for the comments. I had to do a bit of digging around
> the code and it was really helpful.
> I have made some changes and am running the regression right now. I do
> have a few comments on the code review.
Excellent. I do have a minor remark to make below:
> >> + def _reset_device(self, mount_point, file_system):
> >> + """Reset the test. Reinitialize sparse file.
> >> +
> >> + @param mount_point: the absolute path to the mount_point.
> >> + @param file_system: the type of file_system.
> >> + """
> >> + # Umount device.
> >> + self._execute('umount %s' % mount_point, error_check_skip=True)
> >> +
> >> + # Remove sparse_file.
> >> + self._execute('rm -rf %s' % self.sparse_file)
> >> +
> >> + # Recreate sparse_file.
> >> + self._execute('dd if=/dev/zero of=%s bs=1M seek=1000 count=1' %
> >> + self.sparse_file)
> >> +
> >> + # Format sparse_file.
> >> + self._execute('echo "y" | mkfs -t %s %s' %
> >> + (file_system, self.sparse_file),
> >> error_check_skip=True)
> >> +
> >> + # Mount sparse_file.
> >> + self._execute('mount -o loop -t %s %s %s' % (file_system,
> >> + self.sparse_file,
> >> + mount_point))
> >
> > ^ We do have code in client/bin/partition.py to do the above. In
> > initialize, we could do:
> >
> > def initialize(self):
> > self._create_partition()
> >
> > def _create_partition(self):
> > self.partition = job.partition(self.sparse_file, 1024, job.tmpdir)
> > self.partition.mkfs(fstype=self.file_system)
> > self.partition.mount()
> >
> > def _reset_device(self):
> > self.partition.umount()
> > os.remove(self.sparse_file)
> > self._create_partition()
> >
> > Or something like that, I think it'd avoid to write code for something
> > that we already have good code written for.
> >
> This makes perfect sense however, the "partition" class doesn't create
> a sparse file. What we need in the tests is a sparse file so that we
> can check the size increase every time more data is written into it.
^ I see, true. What we could do is to add an extra param to the
partition class constructor, something like
def __init__(self, job, device, loop_size=0, mountpoint=None,
sparse=None):
And when sparse is defined seek is added to the dd command line with the
value of skipped blocks defined on sparse. Then we could use a partition
instead of the code like:
self.partition = job.partition(device=self.sparse_file, loop_size=1024,
mountpoint=job.tmpdir, sparse=1000)
But of course, this is not mandatory. I'll send a patch adding this to
partition, but don't feel obligated to move your code to it. We can do
that at a later time, if you want. I believe your new version of the
code can be checked in as is.
> >> +
> >> + def _get_disk_usage(self, file_name):
> >> + """Returns the disk usage of given file.
> >> +
> >> + @param file_name: the name of the file.
> >> +
> >> + @returns: the disk usage as an integer.
> >> + """
> >> + # Check du stats.
> >> + cmd = '%s %s' % (self._DU_CMD, file_name)
> >> +
> >> + # Expected value for output = '1028\tfoo'
> >> + ret,output = self._execute(cmd)
> >> +
> >> + # Desired ouput = (1028, foo)
> >> + output = output.split('\t')
> >> +
> >> + return int(output[0])
> >
> > ^ It would be simpler to use os.stat, it seems.
>
> The reason I explicitly use "du" is so that we know the actual disk usage.
> For instance:
>
> In [2]: os.system('dd if=/dev/zero of=big-file bs=1M count=1 seek=1000')
> 1+0 records in
> 1+0 records out
> 1048576 bytes (1.0 MB) copied, 0.0180325 s, 58.1 MB/s
> Out[2]: 0
>
> In [3]: os.stat('big-file').st_size
> Out[3]: 1049624576
>
> In [4]: os.system('du big-file')
> 1036 big-file
> Out[4]: 0
>
Makes sense, I haven't thought about it. Thanks for clarifying!
>
> I think everything else I have fixed as you mentioned. I'll send the
> patch in once my regression test completes.
>
> Thanks again for you help and your patience.
My pleasure!
Lucas
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
