Bug 229929 - makefs(8) can underestimate image size
Summary: makefs(8) can underestimate image size
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Kirk McKusick
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-21 17:20 UTC by Alan Somers
Modified: 2022-06-18 14:29 UTC (History)
1 user (show)

See Also:
asomers: mfc-stable13+
asomers: mfc-stable12-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Somers freebsd_committer 2018-07-21 17:20:58 UTC
With no -M, -m, or -s options, makefs should autocalculate the image size.  However, sometimes it underestimates the size required, then fails when it's writing the image with an error like this:

Calculated size of `/tmp/kyua.ZQYFDj/2/work/test.img': 688128 bytes, 33 inodes
Extent size set to 32768
/tmp/kyua.ZQYFDj/2/work/test.img: 0.7MB (1344 sectors) block size 32768, fragment size 4096
        using 1 cylinder groups of 0.66MB, 21 blks, 256 inodes.
super-block backups (for fsck -b #) at:
 64,
Populating `/tmp/kyua.ZQYFDj/2/work/test.img'
makefs: Writing inode 22 (524288.file), bytes 491520 + 32768: No space left on device
Comment 1 commit-hook freebsd_committer 2018-07-21 17:25:00 UTC
A commit references this bug:

Author: asomers
Date: Sat Jul 21 17:24:15 UTC 2018
New revision: 336582
URL: https://svnweb.freebsd.org/changeset/base/336582

Log:
  makefs(8): add test case for PR 229929

  Fix two failing makefs test cases by adding "-M 1m", which was already used
  for every other FFS test case.  Add a new test case for the underlying
  issue: with no -M, -m, or -s options, makefs can underestimate image size.

  PR:		229929
  Reported by:	Jenkins
  MFC after:	2 weeks

Changes:
  head/usr.sbin/makefs/tests/makefs_ffs_tests.sh
Comment 2 Alan Somers freebsd_committer 2018-07-21 17:56:36 UTC
Additional info:

The smallest value of -M that makes this testcase pass is "-M 753665".  I think the problem probably lies in ffs_size_dir in usr.sbin/makefs/ffs.c.  That's the function that does most of the size estimation.  However, it relies on intimate knowledge of FFS's internals.  Assigning to the resident FFS expert.
Comment 3 commit-hook freebsd_committer 2018-10-01 15:41:01 UTC
A commit references this bug:

Author: asomers
Date: Mon Oct  1 15:40:07 UTC 2018
New revision: 339048
URL: https://svnweb.freebsd.org/changeset/base/339048

Log:
  MFC r336582:

  makefs(8): add test case for PR 229929

  Fix two failing makefs test cases by adding "-M 1m", which was already used
  for every other FFS test case.  Add a new test case for the underlying
  issue: with no -M, -m, or -s options, makefs can underestimate image size.

  PR:		229929
  Reported by:	Jenkins

Changes:
_U  stable/11/
  stable/11/usr.sbin/makefs/tests/makefs_ffs_tests.sh
Comment 4 Kirk McKusick freebsd_committer 2018-10-02 04:37:33 UTC
(In reply to Alan Somers from comment #2)
Does your commit resolve this bug?
If so, please close it.
Comment 5 Alan Somers freebsd_committer 2018-10-02 14:12:50 UTC
No, my commit doesn't fix the bug.  I just added a test case.
Comment 6 Kornel Dulęba 2022-05-05 16:02:37 UTC
I've published two patches on phabricator that should resolve this:

1. https://reviews.freebsd.org/D35131
2. https://reviews.freebsd.org/D35132

With both of them applied the autocalculate_image_size test works just fine.
Comment 7 Kirk McKusick freebsd_committer 2022-05-14 19:13:45 UTC
(In reply to Kornel Dulęba from comment #6)
Thanks for your suggested fixes. I have approved your reviews as they are both accurate and a vast improvement over the current code.

Alan, would you like me to commit them?
Comment 8 commit-hook freebsd_committer 2022-05-16 22:36:22 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ecdc04d006de93eb343ce3b77208abd937d4f8ac

commit ecdc04d006de93eb343ce3b77208abd937d4f8ac
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2022-05-16 22:32:10 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2022-05-16 22:32:10 +0000

    makefs: fix calculation of file sizes

    When a new FS image is created we need to calculate how much space each
    file is going to consume.
    Fix two bugs in that logic:

    1) Count the space needed for indirect blocks for large files.
    1) Normally the trailing data of a file is written to a block of frag
       size, 4 kB by default.

    However for files that use indirect blocks a full block is allocated,
    32kB by default.  Take that into account.

    Adjust size calculations to match what is done in ffs_mkfs routine:

    * Depending on the UFS version the superblock is stored at a different
      offset. Take that into account.
    * Add the cylinder group block size.
    * All of the above has to be aligned to the block size.

    Finally, Remove "ncg" variable. It's always 1 and it was used to
    multiply stuff.

    PR:             229929
    Reviewed by:    mckusick
    MFC after:      2 weeks
    Sponsored by:   Semihalf
    Submitted by:   Kornel Dulęba <mindal@semihalf.com>
    Differential Revision:  https://reviews.freebsd.org/D35131
    Differential Revision:  https://reviews.freebsd.org/D35132

 usr.sbin/makefs/ffs.c                     | 50 ++++++++++++++++++-------------
 usr.sbin/makefs/tests/makefs_ffs_tests.sh |  1 -
 2 files changed, 30 insertions(+), 21 deletions(-)
Comment 9 Alan Somers freebsd_committer 2022-05-16 22:37:00 UTC
done.
Comment 10 Kirk McKusick freebsd_committer 2022-05-17 17:53:11 UTC
(In reply to Alan Somers from comment #9)
Great, thanks for the followup. I presume you will close this bug report once you have done the MFC.
Comment 11 Alan Somers freebsd_committer 2022-05-17 19:12:03 UTC
(In reply to Kirk McKusick from comment #10)
Yep, I'll take care of it.
Comment 12 Kornel Dulęba 2022-05-18 08:12:43 UTC
Thanks for committing the fix.
On a side note most makefs ffs tests use -M 1m to specify a minimum size of the fs image.
I don't think that's needed any more, i.e. I've run kyua with that flag removed from test cases and everything worked just fine.
Comment 13 commit-hook freebsd_committer 2022-06-18 14:18:34 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1a2920e96aacc253b4f65405640074bf595c431d

commit 1a2920e96aacc253b4f65405640074bf595c431d
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2022-05-16 22:32:10 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2022-06-18 14:17:23 +0000

    makefs: fix calculation of file sizes

    When a new FS image is created we need to calculate how much space each
    file is going to consume.
    Fix two bugs in that logic:

    1) Count the space needed for indirect blocks for large files.
    1) Normally the trailing data of a file is written to a block of frag
       size, 4 kB by default.

    However for files that use indirect blocks a full block is allocated,
    32kB by default.  Take that into account.

    Adjust size calculations to match what is done in ffs_mkfs routine:

    * Depending on the UFS version the superblock is stored at a different
      offset. Take that into account.
    * Add the cylinder group block size.
    * All of the above has to be aligned to the block size.

    Finally, Remove "ncg" variable. It's always 1 and it was used to
    multiply stuff.

    PR:             229929
    Reviewed by:    mckusick
    Sponsored by:   Semihalf
    Submitted by:   Kornel Dulęba <mindal@semihalf.com>
    Differential Revision:  https://reviews.freebsd.org/D35131
    Differential Revision:  https://reviews.freebsd.org/D35132

    (cherry picked from commit ecdc04d006de93eb343ce3b77208abd937d4f8ac)

 usr.sbin/makefs/ffs.c                     | 50 ++++++++++++++++++-------------
 usr.sbin/makefs/tests/makefs_ffs_tests.sh |  1 -
 2 files changed, 30 insertions(+), 21 deletions(-)