| Message ID | 20260329134716.3185469-1-charles-antoine.couret@mind.be |
|---|---|
| Headers | show |
| Series | image_types: use IMAGE_FILE_MAXSIZE variable to create fixed partition size | expand |
On Sun, 2026-03-29 at 15:47 +0200, Charles-Antoine Couret via lists.openembedded.org wrote: > From: Charles-Antoine Couret <charles-antoine.couret@mind.be> > > Details are described in the first patch. > > Difference from v3: > > * Don't use bashism which fails for Debian derived systems by default > * Improve test checks > > Difference from v2: > > * Added working and failing tests in meta/lib/oeqa/selftest/cases/imagefeatures.py > * Split commit to send documentation to right mailing list > > Difference from v1: > > * Added documentation for IMAGE_FILE_MAXSIZE variable > * Added Python function to get the value of this variable from shell functions > otherwise parsing issue can happen > * Added an additional task to check the final result which works for all filesystems > and not only those created with dd command. > > Charles-Antoine Couret (5): > image_types: add python function to get the IMAGE_FILE_MAXSIZE:fstype > value > image_types: use IMAGE_FILE_MAXSIZE variable for ext2/3/4 image types > image_types: use IMAGE_FILE_MAXSIZE variable for btrfs image types > image_types: use IMAGE_FILE_MAXSIZE variable for f2fs image types > image: add check_image_max_size as post function to check file size > against IMAGE_FILE_MAXSIZE For context, these previous versions were in 2023. I think the general concern is we already have a lot of rootfs size related variables and adding more of them complicates the user interface. It isn't clear which ones "win" when multiple conflicting settings are made. In this case the max size "wins" above any of the others. It does appear that it isn't just a max size but a fixed size in that it will pad the filesystems to this size? That makes the name a bit confusing/misleading and was one of the concerns last time. Cheers, Richard
Le 29/03/26 à 18:15, Richard Purdie a écrit : > For context, these previous versions were in 2023. I think the general > concern is we already have a lot of rootfs size related variables and > adding more of them complicates the user interface. It isn't clear > which ones "win" when multiple conflicting settings are made. > > In this case the max size "wins" above any of the others. IMHO this makes sense because it's more specific and related to the real filesystem size. But indeed if you want to change the design behind, I'm open to it. > It does > appear that it isn't just a max size but a fixed size in that it will > pad the filesystems to this size? That makes the name a bit > confusing/misleading and was one of the concerns last time. In fact I realize that I've not read your previous answer last time, too focus on the CI error. Sorry, my mistake. That's why I've not taken this into account for my recent changes. Yes, this variable is doing both (fixed + max size of the filesystem), due to how filesystem tools are working. Maybe by having a more "intrusive" change, we can improve it. What I can suggest is: * IMAGE_ROOTFS_MAXSIZE would be used against filesystem sizes, and not against precomputed rootfs sizes as done today, it's likely not really relevant to keep current behavior as it. I don't know if we would be able to customize the value per filesystem type as done in my proposal or if a global setting is good enough. * if IMAGE_ROOTFS_EXTRA_SPACE is set to 0 and IMAGE_OVERHEAD_FACTOR set to 1.0, don't provide size parameter to mkfs to create a dynamic and minimal filesystem, to avoid the need to always provide manually extra margin just for filesystem overhead * Then introduce a new variable like IMAGE_ROOTFS_FIXED_SIZE which can be provided (if set) to mkfs tool to have a fixed partition size whatever are the other settings related to rootfs size. If unset (by default), previous logic can be used instead. I'm not an expert of all filesystems so maybe this suggestion can introduce problems. I don't see potential regressions for current users as well. What do you think? Another ideas? Thank you for your feedback. Regards,
On Sun, Mar 29, 2026 at 11:12 AM Charles-Antoine Couret via lists.openembedded.org <charles-antoine.couret=mind.be@lists.openembedded.org> wrote: > > Le 29/03/26 à 18:15, Richard Purdie a écrit : > > For context, these previous versions were in 2023. I think the general > > concern is we already have a lot of rootfs size related variables and > > adding more of them complicates the user interface. It isn't clear > > which ones "win" when multiple conflicting settings are made. > > > > In this case the max size "wins" above any of the others. > > IMHO this makes sense because it's more specific and related to the real > filesystem size. > > But indeed if you want to change the design behind, I'm open to it. > > > It does > > appear that it isn't just a max size but a fixed size in that it will > > pad the filesystems to this size? That makes the name a bit > > confusing/misleading and was one of the concerns last time. > > In fact I realize that I've not read your previous answer last time, too > focus on the CI error. Sorry, my mistake. That's why I've not taken this > into account for my recent changes. > > Yes, this variable is doing both (fixed + max size of the filesystem), > due to how filesystem tools are working. Maybe by having a more > "intrusive" change, we can improve it. > > > What I can suggest is: > > * IMAGE_ROOTFS_MAXSIZE would be used against filesystem sizes, and not > against precomputed rootfs sizes as done today, it's likely not really > relevant to keep current behavior as it. I don't know if we would be > able to customize the value per filesystem type as done in my proposal > or if a global setting is good enough. > > * if IMAGE_ROOTFS_EXTRA_SPACE is set to 0 and IMAGE_OVERHEAD_FACTOR set > to 1.0, don't provide size parameter to mkfs to create a dynamic and > minimal filesystem, to avoid the need to always provide manually extra > margin just for filesystem overhead > > * Then introduce a new variable like IMAGE_ROOTFS_FIXED_SIZE which can > be provided (if set) to mkfs tool to have a fixed partition size > whatever are the other settings related to rootfs size. If unset (by > default), previous logic can be used instead. Perhaps a better option would be some sort of "IMAGE_ROOTFS_ALLOW_GROW" variable that controls if an image can actually grow beyond the IMAGE_ROOTFS_SIZE + overhead amount? The current code basically takes the max of IMAGE_ROOTFS_SIZE & the actual install size to determine the size, but this variable could supress that to make it only look at IMAGE_ROOTFS_SIZE. It could also set IMAGE_ROOTFS_MAXSIZE to the sum for additional checks? I think this would fit into the current model of how these size variables work without needing to introduce a completely new variable that does different math? > > > I'm not an expert of all filesystems so maybe this suggestion can > introduce problems. I don't see potential regressions for current users > as well. > > What do you think? Another ideas? > > > Thank you for your feedback. > > Regards, > > -- > > Charles-Antoine Couret > > Embedded Software Developer > > +32 488 27 56 40 > Website <https://mind.be> • LinkedIn > <https://www.linkedin.com/company/mind_essensium/> • Newsletter > <http://eepurl.com/h-gy7v> > > <https://mind.be> logo > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#234153): https://lists.openembedded.org/g/openembedded-core/message/234153 > Mute This Topic: https://lists.openembedded.org/mt/118563509/3616693 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [JPEWhacker@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On 3/30/26 11:01 AM, Joshua Watt via lists.openembedded.org wrote: > On Sun, Mar 29, 2026 at 11:12 AM Charles-Antoine Couret via > lists.openembedded.org > <charles-antoine.couret=mind.be@lists.openembedded.org> wrote: >> >> Le 29/03/26 à 18:15, Richard Purdie a écrit : >>> For context, these previous versions were in 2023. I think the general >>> concern is we already have a lot of rootfs size related variables and >>> adding more of them complicates the user interface. It isn't clear >>> which ones "win" when multiple conflicting settings are made. >>> >>> In this case the max size "wins" above any of the others. >> >> IMHO this makes sense because it's more specific and related to the real >> filesystem size. >> >> But indeed if you want to change the design behind, I'm open to it. >> >>> It does >>> appear that it isn't just a max size but a fixed size in that it will >>> pad the filesystems to this size? That makes the name a bit >>> confusing/misleading and was one of the concerns last time. >> >> In fact I realize that I've not read your previous answer last time, too >> focus on the CI error. Sorry, my mistake. That's why I've not taken this >> into account for my recent changes. >> >> Yes, this variable is doing both (fixed + max size of the filesystem), >> due to how filesystem tools are working. Maybe by having a more >> "intrusive" change, we can improve it. >> >> >> What I can suggest is: >> >> * IMAGE_ROOTFS_MAXSIZE would be used against filesystem sizes, and not >> against precomputed rootfs sizes as done today, it's likely not really >> relevant to keep current behavior as it. I don't know if we would be >> able to customize the value per filesystem type as done in my proposal >> or if a global setting is good enough. >> >> * if IMAGE_ROOTFS_EXTRA_SPACE is set to 0 and IMAGE_OVERHEAD_FACTOR set >> to 1.0, don't provide size parameter to mkfs to create a dynamic and >> minimal filesystem, to avoid the need to always provide manually extra >> margin just for filesystem overhead >> >> * Then introduce a new variable like IMAGE_ROOTFS_FIXED_SIZE which can >> be provided (if set) to mkfs tool to have a fixed partition size >> whatever are the other settings related to rootfs size. If unset (by >> default), previous logic can be used instead. > > > Perhaps a better option would be some sort of > "IMAGE_ROOTFS_ALLOW_GROW" variable that controls if an image can > actually grow beyond the IMAGE_ROOTFS_SIZE + overhead amount? The > current code basically takes the max of IMAGE_ROOTFS_SIZE & the actual > install size to determine the size, but this variable could supress > that to make it only look at IMAGE_ROOTFS_SIZE. It could also set > IMAGE_ROOTFS_MAXSIZE to the sum for additional checks? This makes more sense to me. Looking at the existing variables: IMAGE_ROOTFS_SIZE - Defines the size in Kbytes for the generated image. Really this is the minimum size, computed with actual filesystem needs. IMAGE_OVERHEAD_FACTOR - Defines a multiplier that the build system applies to the initial image size for cases when the multiplier times the returned disk usage value for the image is greater than the sum of IMAGE_ROOTFS_SIZE and IMAGE_ROOTFS_EXTRA_SPACE. IMAGE_ROOTFS_EXTRA_SPACE - Defines additional free disk space created in the image in Kbytes. By default, this variable is set to '0'. IMAGE_ROOTFS_MAXSIZE - (not in the documentation.conf but...) This value is a QA check to ensure that the filesystem does not exceed a specific size after factoring in filesystem size, extra space and overhead factor. (this should be added to the docs!) Adding an additional boolean of "IMAGE_ROOTFS_ALLOW_GROW" would then allow those variables to be used "as-is" without the automatic growing. (I would assume existing behavior means ALLOW_GROW = 1 by default.) As far as I can tell, in this patch set that would mean: IMAGE_ROOTFS_ALLOW_GROW = "0" IMAGE_ROOTFS_SIZE = _the_ size of filesystem you want IMAGE_ROOTFS_EXTRA_SPACE = "0" IMAGE_OVERHEAD_FACTOR = "1" Then a patch similar to: def get_rootfs_size(d): import subprocess, oe.utils rootfs_alignment = int(d.getVar('IMAGE_ROOTFS_ALIGNMENT')) overhead_factor = float(d.getVar('IMAGE_OVERHEAD_FACTOR')) rootfs_req_size = int(d.getVar('IMAGE_ROOTFS_SIZE')) rootfs_extra_space = eval(d.getVar('IMAGE_ROOTFS_EXTRA_SPACE')) rootfs_maxsize = d.getVar('IMAGE_ROOTFS_MAXSIZE') rootfs_allow_grow = d.getVar('IMAGE_ROOTFS_ALLOW_GROW') or "1" ... size_kb = oe.utils.directory_size(d.getVar("IMAGE_ROOTFS")) / 1024 if rootfs_allow_grow: base_size = size_kb * overhead_factor bb.debug(1, '%f = %d * %f' % (base_size, size_kb, overhead_factor)) base_size2 = max(base_size, rootfs_req_size) + rootfs_extra_space bb.debug(1, '%f = max(%f, %d)[%f] + %d' % (base_size2, base_size, rootfs_req_size, max(base_size, rootfs_req_size), rootfs_extra_space)) else: base_size2 = rootfs_req_size + rootfs_extra_space ... (Maybe allow_grow is a bad name for the boolean, since effectively we want to affirmatively tell the system to ignore the actual files in the filesystem.. So perhaps reversing the boolean to: IMAGE_ROOTFS_USE_FILE_SIZES (not sure I like this name any better) but the overall change is MUCH smaller and easier to fit in and QA with the existing code (as well as document). > I think this would fit into the current model of how these size > variables work without needing to introduce a completely new variable > that does different math? > I agree, avoid additional math, just having a variable to determine if we do the 'max' logic or not makes more sense to me and would be a smaller patch set. --Mark >> >> >> I'm not an expert of all filesystems so maybe this suggestion can >> introduce problems. I don't see potential regressions for current users >> as well. >> >> What do you think? Another ideas? >> >> >> Thank you for your feedback. >> >> Regards, >> >> -- >> >> Charles-Antoine Couret >> >> Embedded Software Developer >> >> +32 488 27 56 40 >> Website <https://mind.be> • LinkedIn >> <https://www.linkedin.com/company/mind_essensium/> • Newsletter >> <http://eepurl.com/h-gy7v> >> >> <https://mind.be> logo >> >> >> >> >> >> -=-=-=-=-=-=-=-=-=-=-=- >> Links: You receive all messages sent to this group. >> View/Reply Online (#234250): https://lists.openembedded.org/g/openembedded-core/message/234250 >> Mute This Topic: https://lists.openembedded.org/mt/118563509/3616948 >> Group Owner: openembedded-core+owner@lists.openembedded.org >> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [mark.hatle@kernel.crashing.org] >> -=-=-=-=-=-=-=-=-=-=-=- >>
Le 30/03/26 à 18:21, Mark Hatle a écrit : > On 3/30/26 11:01 AM, Joshua Watt via lists.openembedded.org wrote: >> On Sun, Mar 29, 2026 at 11:12 AM Charles-Antoine Couret via >> lists.openembedded.org >> <charles-antoine.couret=mind.be@lists.openembedded.org> wrote: >>> What I can suggest is: >>> >>> * IMAGE_ROOTFS_MAXSIZE would be used against filesystem sizes, and not >>> against precomputed rootfs sizes as done today, it's likely not really >>> relevant to keep current behavior as it. I don't know if we would be >>> able to customize the value per filesystem type as done in my proposal >>> or if a global setting is good enough. >>> >>> * if IMAGE_ROOTFS_EXTRA_SPACE is set to 0 and IMAGE_OVERHEAD_FACTOR set >>> to 1.0, don't provide size parameter to mkfs to create a dynamic and >>> minimal filesystem, to avoid the need to always provide manually extra >>> margin just for filesystem overhead >>> >>> * Then introduce a new variable like IMAGE_ROOTFS_FIXED_SIZE which can >>> be provided (if set) to mkfs tool to have a fixed partition size >>> whatever are the other settings related to rootfs size. If unset (by >>> default), previous logic can be used instead. >> >> >> Perhaps a better option would be some sort of >> "IMAGE_ROOTFS_ALLOW_GROW" variable that controls if an image can >> actually grow beyond the IMAGE_ROOTFS_SIZE + overhead amount? The >> current code basically takes the max of IMAGE_ROOTFS_SIZE & the actual >> install size to determine the size, but this variable could supress >> that to make it only look at IMAGE_ROOTFS_SIZE. It could also set >> IMAGE_ROOTFS_MAXSIZE to the sum for additional checks? > > This makes more sense to me. > > Looking at the existing variables: > > > IMAGE_ROOTFS_SIZE - Defines the size in Kbytes for the generated image. > > Really this is the minimum size, computed with actual filesystem needs. > > > IMAGE_OVERHEAD_FACTOR - Defines a multiplier that the build system > applies to the initial image size for cases when the multiplier times > the returned disk usage value for the image is greater than the sum > of and IMAGE_ROOTFS_EXTRA_SPACE. > > > IMAGE_ROOTFS_EXTRA_SPACE - Defines additional free disk space created > in the image in Kbytes. By default, this variable is set to '0'. > > > IMAGE_ROOTFS_MAXSIZE - (not in the documentation.conf but...) This > value is a QA check to ensure that the filesystem does not exceed a > specific size after factoring in filesystem size, extra space and > overhead factor. > > (this should be added to the docs!) > > > Adding an additional boolean of "IMAGE_ROOTFS_ALLOW_GROW" would then > allow those variables to be used "as-is" without the automatic > growing. (I would assume existing behavior means ALLOW_GROW = 1 by > default.) > > As far as I can tell, in this patch set that would mean: > > IMAGE_ROOTFS_ALLOW_GROW = "0" > > IMAGE_ROOTFS_SIZE = _the_ size of filesystem you want > IMAGE_ROOTFS_EXTRA_SPACE = "0" > IMAGE_OVERHEAD_FACTOR = "1" > > Then a patch similar to: > > def get_rootfs_size(d): > import subprocess, oe.utils > > rootfs_alignment = int(d.getVar('IMAGE_ROOTFS_ALIGNMENT')) > overhead_factor = float(d.getVar('IMAGE_OVERHEAD_FACTOR')) > rootfs_req_size = int(d.getVar('IMAGE_ROOTFS_SIZE')) > rootfs_extra_space = eval(d.getVar('IMAGE_ROOTFS_EXTRA_SPACE')) > rootfs_maxsize = d.getVar('IMAGE_ROOTFS_MAXSIZE') > rootfs_allow_grow = d.getVar('IMAGE_ROOTFS_ALLOW_GROW') or "1" > ... > > size_kb = oe.utils.directory_size(d.getVar("IMAGE_ROOTFS")) / 1024 > > if rootfs_allow_grow: > base_size = size_kb * overhead_factor > bb.debug(1, '%f = %d * %f' % (base_size, size_kb, > overhead_factor)) > base_size2 = max(base_size, rootfs_req_size) + rootfs_extra_space > bb.debug(1, '%f = max(%f, %d)[%f] + %d' % (base_size2, base_size, > rootfs_req_size, max(base_size, rootfs_req_size), > rootfs_extra_space)) > else: > base_size2 = rootfs_req_size + rootfs_extra_space > > ... > > (Maybe allow_grow is a bad name for the boolean, since effectively we > want to affirmatively tell the system to ignore the actual files in > the filesystem.. So perhaps reversing the boolean to: > > IMAGE_ROOTFS_USE_FILE_SIZES (not sure I like this name any better) > > but the overall change is MUCH smaller and easier to fit in and QA > with the existing code (as well as document). Hi, I think we would go further because the naming and settings can be confusing and the end result is likely not optimal. As it today, to have the check of maximum rootfs size which works properly in all cases, you need to set IMAGE_ROOTFS_SIZE and IMAGE_ROOTFS_MAXSIZE to the same value (and IMAGE_OVERHEAD_FACTOR to 1, IMAGE_ROOTFS_EXTRA_SPACE is already set to 0 by default). From my point of view, IMAGE_ROOTFS_SIZE would not be mandatory set to have a working behaviour for this sanity check, its purpose is different. IMAGE_ROOTFS_MAXSIZE value alone would be enough. Then, the drawback of this approach is also that in any case your filesystem will have this maximum size even when it's not required. I know that my initial proposal has this issue as well but I think we can have a way to avoid it but this needs a more complex change as I suggested above. It would be better to check the file sizes after their creation instead of having a check before to be able to have this flexibility even if the change would be more important. Regards, Charles-Antoine Couret
From: Charles-Antoine Couret <charles-antoine.couret@mind.be> Details are described in the first patch. Difference from v3: * Don't use bashism which fails for Debian derived systems by default * Improve test checks Difference from v2: * Added working and failing tests in meta/lib/oeqa/selftest/cases/imagefeatures.py * Split commit to send documentation to right mailing list Difference from v1: * Added documentation for IMAGE_FILE_MAXSIZE variable * Added Python function to get the value of this variable from shell functions otherwise parsing issue can happen * Added an additional task to check the final result which works for all filesystems and not only those created with dd command. Charles-Antoine Couret (5): image_types: add python function to get the IMAGE_FILE_MAXSIZE:fstype value image_types: use IMAGE_FILE_MAXSIZE variable for ext2/3/4 image types image_types: use IMAGE_FILE_MAXSIZE variable for btrfs image types image_types: use IMAGE_FILE_MAXSIZE variable for f2fs image types image: add check_image_max_size as post function to check file size against IMAGE_FILE_MAXSIZE meta/classes-recipe/image.bbclass | 29 ++++++++++ meta/classes-recipe/image_types.bbclass | 43 ++++++++++++--- meta/lib/oeqa/selftest/cases/imagefeatures.py | 53 +++++++++++++++++++ 3 files changed, 117 insertions(+), 8 deletions(-)