[OE-core] [PATCH] wic: add --reserved-size wks option
Maciej Borzęcki
maciej.borzecki at rndity.com
Tue Oct 18 08:37:22 UTC 2016
On Tue, Oct 18, 2016 at 9:31 AM, Ed Bartosh <ed.bartosh at linux.intel.com> wrote:
> On Mon, Oct 17, 2016 at 04:46:00PM +0200, Maciej Borzęcki wrote:
>> On Mon, Oct 17, 2016 at 3:22 PM, Ed Bartosh <ed.bartosh at linux.intel.com> wrote:
>> > Hi Maciej,
>> >
>> > There is already --size and --extra-space options.
>> > Can we get the same or similar result by just using them? Do we really
>> > need new option for similar purpose?
>>
>> --reserved-size serves a different purpose, it establishes an upper
>> bound on the size of a partition during layout. Unlike
>> --size/--extra-space does not depend on the size of the filesystem
>> image.
>>
>> For instance, assume I'm creating an image for SD card/eMMC with a
>> fixed partition layout (something simple: boot partition, primary &
>> secondary rootfs partitions, some data partition). Because future
>> system updates are delivered as filesystem image, I want to make sure
>> that there is exactly xxx MBs for my current and future rootfs images
>> (regardless of current image size). Neither --size nor --extra-space
>> can do that. I could use, say `--size 200 --overhead-factor 1`, but
>> this will needlessly create a 200MB rootfs image and if I happen to
>> cross the 200MB boundary I will not get an error.
>>
>> I had a private patch that added --fixed-size to enforce --size, but
>> that would still end up creating filesystem image to fill the whole
>> space.
> I didn't get the difference between enforcing partition size and below
> implementation. Can you elaborate a bit?
`--fixed-size` was something that I had added to my fork back in 2014,
even before `--overhead-factor` came in. The problem is that depending
on a project you might want to have more control over how partitions
are laid out, or even need to have a fixed layout. Adding
`--fixed-size` would had a similar effect to what `--overhead-factor
1` does right now. Combined with `--size` would ensure that rootfs is
say, 200MB large. The downside was that wic would actually create a
200MB rootfs, something that is not really necessary. In fact, I only
wanted to have 200MB gap so that I have some spare space for future
updates (where update is just a rootfs image you dd to the partition).
>
>> Another workaround we used until now was to create a filesystem image
>> manually and use fsimage source plugin. To get a fixed layout with
>> rootfs image that may grow/shrink between builds, you still need to
>> fiddle with --align (for instance, set it to 215040 to have the
>> partition start at 210MB offset).
>>
>
> Thanks for the explanations. I'm almost convinced.
>
> See my comments below.
>
>> >
>> >
>> > On Mon, Oct 17, 2016 at 03:06:18PM +0200, Maciej Borzecki wrote:
>> >> Added new option --reserved-size to wks. The option can be used to
>> >> indicate how much space should be reserved for a partition. This is
>> >> useful if the disk will be a subject to full filesystem image updates
>> >> and puts an upper limit of the size of filesystem images.
>> >>
>> >> The actual filesystem image may be smaller than the reserved space, thus
>> >> leaving some room for growth. If it is larger, an error will be raised.
>> >>
>> >> Signed-off-by: Maciej Borzecki <maciej.borzecki at rndity.com>
>> >> ---
>> >> scripts/lib/wic/help.py | 11 ++++++++++
>> >> scripts/lib/wic/imager/direct.py | 3 ++-
>> >> scripts/lib/wic/ksparser.py | 1 +
>> >> scripts/lib/wic/partition.py | 1 +
>> >> scripts/lib/wic/utils/partitionedfs.py | 37 +++++++++++++++++++++++++---------
>> >> 5 files changed, 43 insertions(+), 10 deletions(-)
>> >>
>> >> diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
>> >> index e5347ec4b7c900c68fc64351a5293e75de0672b3..1a5c7020ba0cdc5ef2e477a2b14360e09098a896 100644
>> >> --- a/scripts/lib/wic/help.py
>> >> +++ b/scripts/lib/wic/help.py
>> >> @@ -646,6 +646,17 @@ DESCRIPTION
>> >> not specified, the size is in MB.
>> >> You do not need this option if you use --source.
>> >>
>> >> + --reserved-size: This option specifies that there should be
>> >> + at least that many bytes reserved for
> According to the code if partition size > reserved size wic will throw
> an error. This somehow conflicts with 'at least that many bytes' part of the
> description in my opinion.
>
> As far as I uderstood your code this parameter specifies hard upper
> limit for partition size, right?
Thanks, the wording is a bit off. I should rephrase it to 'that many
bytes' or just state that it's an upper limit.
>
>> >> + the partition during layout. This is useful
>> >> + when the target disk will be a subject
>> >> + to full system image updates in the future.
>> >> + Specifying --reserved-size ensures that
>> >> + there is extra space in the partition allowing
>> >> + for future growth of the file system stored
>> >> + inside. Value format is the same as for
>> >> + --size option.
>> >> +
>> >> --source: This option is a wic-specific option that names the
>> >> source of the data that will populate the
>> >> partition. The most common value for this option
>> >> diff --git a/scripts/lib/wic/imager/direct.py b/scripts/lib/wic/imager/direct.py
>> >> index edf5e5d2214f8e78b6c2a98d7f6cd45fcc0065c4..02e293b9d744b760fcdf17610505dafef3e164ad 100644
>> >> --- a/scripts/lib/wic/imager/direct.py
>> >> +++ b/scripts/lib/wic/imager/direct.py
>> >> @@ -301,7 +301,8 @@ class DirectImageCreator(BaseImageCreator):
>> >> no_table=part.no_table,
>> >> part_type=part.part_type,
>> >> uuid=part.uuid,
>> >> - system_id=part.system_id)
>> >> + system_id=part.system_id,
>> >> + reserved_size=part.reserved_size)
>> >>
>> >> if fstab_path:
>> >> shutil.move(fstab_path + ".orig", fstab_path)
>> >> diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py
>> >> index 0894e2b199a299fbbed272f2e1c95e9d692e3ab1..4118bffdf4337f2d2d393d7e096632cd7aa37402 100644
>> >> --- a/scripts/lib/wic/ksparser.py
>> >> +++ b/scripts/lib/wic/ksparser.py
>> >> @@ -137,6 +137,7 @@ class KickStart():
>> >> part.add_argument('--part-type')
>> >> part.add_argument('--rootfs-dir')
>> >> part.add_argument('--size', type=sizetype, default=0)
>> >> + part.add_argument('--reserved-size', type=sizetype, default=0)
>> >> part.add_argument('--source')
>> >> part.add_argument('--sourceparams')
>> >> part.add_argument('--system-id', type=systemidtype)
>> >> diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
>> >> index 90f65a1e3976a5460cd1b265b238168cce22781f..162a3a289de891ccf81437876c1f7a6f3c797b3b 100644
>> >> --- a/scripts/lib/wic/partition.py
>> >> +++ b/scripts/lib/wic/partition.py
>> >> @@ -54,6 +54,7 @@ class Partition():
>> >> self.part_type = args.part_type
>> >> self.rootfs_dir = args.rootfs_dir
>> >> self.size = args.size
>> >> + self.reserved_size = args.reserved_size
>> >> self.source = args.source
>> >> self.sourceparams = args.sourceparams
>> >> self.system_id = args.system_id
>> >> diff --git a/scripts/lib/wic/utils/partitionedfs.py b/scripts/lib/wic/utils/partitionedfs.py
>> >> index cb03009fc7e3c97305079629ded7d2ff01eba4c4..5d3b1588231459dedf0142f807114736f0bb28ea 100644
>> >> --- a/scripts/lib/wic/utils/partitionedfs.py
>> >> +++ b/scripts/lib/wic/utils/partitionedfs.py
>> >> @@ -91,7 +91,7 @@ class Image():
>> >>
>> >> def add_partition(self, size, disk_name, mountpoint, source_file=None, fstype=None,
>> >> label=None, fsopts=None, boot=False, align=None, no_table=False,
>> >> - part_type=None, uuid=None, system_id=None):
>> >> + part_type=None, uuid=None, system_id=None, reserved_size=0):
>> >> """ Add the next partition. Prtitions have to be added in the
>> >> first-to-last order. """
>> >>
>> >> @@ -99,9 +99,11 @@ class Image():
>> >>
>> >> # Converting kB to sectors for parted
>> >> size = size * 1024 // self.sector_size
>> >> + reserved_size = reserved_size * 1024 // self.sector_size
>> >>
>> >> part = {'ks_pnum': ks_pnum, # Partition number in the KS file
>> >> 'size': size, # In sectors
>> >> + 'reserved_size': reserved_size, # space on disk reserved for partition
>> >> 'mountpoint': mountpoint, # Mount relative to chroot
>> >> 'source_file': source_file, # partition contents
>> >> 'fstype': fstype, # Filesystem type
>> >> @@ -192,7 +194,19 @@ class Image():
>> >> disk['offset'] += align_sectors
>> >>
>> >> part['start'] = disk['offset']
>> >> - disk['offset'] += part['size']
>> >> +
>> >> + if part['reserved_size']:
>> >> + if part['size'] > part['reserved_size']:
>> >> + msger.error("Partition %d on disk %s is larger (%s bytes) than its"
>> >> + " reserved space %s bytes" %
>> >> + (disk['numpart'], part['disk_name'],
>> >> + part['size'] * self.sector_size,
>> >> + part['reserved_size'] * self.sector_size))
>> >> + # next partition starts after the space reserved for the
>> >> + # current one
>> >> + disk['offset'] += part['reserved_size']
>> >> + else:
>> >> + disk['offset'] += part['size']
>> >>
>> >> part['type'] = 'primary'
>> >> if not part['no_table']:
>> >> @@ -207,10 +221,11 @@ class Image():
>> >>
>> >> disk['partitions'].append(num)
>> >> msger.debug("Assigned %s to %s%d, sectors range %d-%d size %d "
>> >> - "sectors (%d bytes)." \
>> >> + "sectors (%d bytes), reserved %d bytes." \
>> >> % (part['mountpoint'], part['disk_name'], part['num'],
>> >> - part['start'], part['start'] + part['size'] - 1,
>> >> - part['size'], part['size'] * self.sector_size))
>> >> + part['start'], disk['offset'] - 1,
>> >> + part['size'], part['size'] * self.sector_size,
>> >> + part['reserved_size'] * self.sector_size))
> This will produce incorrect output about partition size as in reality partition size will
> be set to part['reserved_size'].
I tried to avoid confusing the reader here. Partition size is one
thing and reservation is another. That is why reservation is logged
explicitly. Logging just the size would raise questions about why the
partition has grown.
Perhaps it makes sense to log reservation like this instead?
part['reserved_size'] * self.sector_size if part['reserved_size']
else part['size'] * self.sector_size
>
> "reserved 0 bytes" message will be produced if --reserved-size option
> is not used. It's a bit confusing as 'reserved' is too generic word. Can
> we use 'reserved size' instead?
Yes, 'reserved size' sounds much better.
>
>> >>
>> >> # Once all the partitions have been layed out, we can calculate the
>> >> # minumim disk sizes.
>> >> @@ -288,17 +303,21 @@ class Image():
>> >> # Type for ext2/ext3/ext4/btrfs
>> >> parted_fs_type = "ext2"
>> >>
>> >> + psize = part['size']
>> >> + if part['reserved_size']:
>> >> + psize = part['reserved_size']
>> >> +
> Here I'm getting confused. How this new parameters differs from --size if it behaves
> the same way, i.e. partition size is set to its value.
That's intentional, Partition.prepare() is not be affected by the
reservation. I just want to reserve some space during layout, but not
necessarily enlarge the rootfs right now.
> The only
> difference I see is that if rootfs size is greater than reserved size
> wic will fail to create partition. Would it make sense to name it
> --max-size in this case?
Just wording, reservation seems more fitting, but I'm open to suggestions.
>
>> >> # Boot ROM of OMAP boards require vfat boot partition to have an
>> >> # even number of sectors.
>> >> if part['mountpoint'] == "/boot" and part['fstype'] in ["vfat", "msdos"] \
>> >> - and part['size'] % 2:
>> >> - msger.debug("Substracting one sector from '%s' partition to " \
>> >> + and psize % 2:
>> >> + msger.debug("Subtracting one sector from '%s' partition to " \
>> >> "get even number of sectors for the partition" % \
>> >> part['mountpoint'])
>> >> - part['size'] -= 1
>> >> + psize -= 1
>> >>
>> >> self.__create_partition(disk['disk'].device, part['type'],
>> >> - parted_fs_type, part['start'], part['size'])
>> >> + parted_fs_type, part['start'], psize)
>> >>
>> >> if part['part_type']:
>> >> msger.debug("partition %d: set type UID to %s" % \
>
> Can you cover this functionality(and may be other size-related
> functionality too) with tests? You can add testcases to existing
> oe-selftest wic module.
I'll take a look at it.
--
Maciej Borzecki
RnDity
More information about the Openembedded-core
mailing list