[OE-core] [PATCH] wic: add --reserved-size wks option

[Ed Bartosh]

On Tue, Oct 18, 2016 at 10:37:22AM +0200, Maciej Borzęcki wrote:
> 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).
>
Thanks for the explanations. Now I got it - reserved size is not a part
of partition, it's a gap between partitions.

What's the advantage of creating unusable gap over creating partition of
the same size that can be used? Even if that space is not needed it
doesn't harm to have it, does it?

> >> >>
> >> >> +         --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.
>
Upper limit is a wrong term here. I'm sorry for confusing you.
I proposed it because I misunderstood that it's not a partition size but a reserved
space after partition. Anyway, I think above description should be
modified to explain the option in a clear way. Using ascii picture of
the partition layout would probably help to show what this option means.

> >> >>              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?
> 
What about using 'reserved %d bytes of space after partition' or
something like that?

>    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.
We can also skip this part if reserved size is 0.

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

This made the whole thing clear for me! Thank you.

> > 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.
>
I agree. --max-size would suggest that wic will make partition with this
size.

I'm still wondering what's the benefit of having unused space between
partitions though.

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

--
Regards,
Ed