After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 779339 - enforce at least 1 MiB "free space following"
enforce at least 1 MiB "free space following"
Status: RESOLVED FIXED
Product: gparted
Classification: Other
Component: application
0.28.1
Other Linux
: Normal normal
: ---
Assigned To: Mike Fleetwood
gparted maintainers alias
Depends on:
Blocks:
 
 
Reported: 2017-02-27 22:06 UTC by Liv
Modified: 2017-08-07 17:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix operations for snap-to-alignment (patch - Draft 1 -- Work-In-Progress) (2.69 KB, patch)
2017-04-19 20:05 UTC, Curtis Gedak
none Details | Review
Fix snap-to-alignment of operations creating partitions (v1) (6.46 KB, patch)
2017-04-21 09:13 UTC, Mike Fleetwood
none Details | Review

Description Liv 2017-02-27 22:06:33 UTC
This might be related to Bug 684034. 

The other day I attempted to create an 15 GiB ext3 partition on the free space between between another ext3 partition of the same size and a swap partition (10 GiB), all located on the same Extended partition. However GParted ended in a (generic) error, unable to create the partition. When I allowed however 1 MiB of "free space after", then and only then GParted managed to create the requested partition. 

Which makes me think that it might be a good idea to always ensure at least 1 MiB between partitions, both before (as seems to be enforced currently) and after. The error message was too generic to point to this specific issue (it was, essentially, "GParted failed to create the partition"), and other users might not be lucky enough to stumble upon the right way to go about this. 

I used the latest LiveCD when this issue occurred.
Comment 1 Curtis Gedak 2017-02-28 16:27:01 UTC
Please save and post the gparted_details.html log file, because it will help diagnose the issue.

How were the partitions originally aligned (Cylinder, MiB, other)?

What type of partition were you resizing (Primary, Extended, Logical)?

There is a known problem when switching from Cylinder to MiB alignment in some specific situations.  See Bug 741061 - Error when resizing partition.
Comment 2 Mike Fleetwood 2017-02-28 21:43:06 UTC
Hi All,

I have reproduced this error.  It is really easy.  All the below steps
use the default MiB alignment.

1) Setup:
On an MSDOS partitioned drive create 2 logical partitions with a some
space between them.

2) Test:
Create a 3rd partition between the other two.

Fails with the following error dialog:

    (-) <b>An error occurred while applying the operations</b>
        See the details for more information.
        <b>IMPORTANT</b>
        If you want support, you need to provide the saved details!
        See http://gparted.org/save-details.htm for more information.
                                                               [ OK ]

And these operation details:
    + libparted messages
      - Unable to satisfy all constraints on the partition.
Comment 3 Mike Fleetwood 2017-02-28 21:47:40 UTC
The 3rd partition attempted to be created correctly has an enforced
minimum of 1 MiB free space preceding for it's own EBR.  However it
defaults to 0 MiB free space following.  I therefore assume libparted
rejects this because it would create the partition over the top of the
EBR for the following logical partition.
Comment 4 Liv 2017-02-28 21:50:26 UTC
This is precisely what I've seen. Including using the default MiB alignment and the exact setup.
Comment 5 Curtis Gedak 2017-03-01 16:16:45 UTC
Thank you Liv for reporting the issue and Mike for detailing the steps to reproduce the problem.

I have an idea about how to work-around the issue from the GUI, but won't get a chance to investigate until next week.

Curtis
Comment 6 Curtis Gedak 2017-04-19 20:05:39 UTC
Created attachment 350091 [details] [review]
Fix operations for snap-to-alignment (patch - Draft 1 -- Work-In-Progress)

From troubleshooting this bug, I learned that it was accidentally introduced with the refactoring included with the GParted 0.23.0 release.

The core issue is that the GParted_Core::snap_to_alignment code that previously ensured space for the EBR after a new logical partition creation was now unable to perform it's task.  This was due to an empty copy of the partitions in the Device object contained within the Operations object.

Attached is a draft patch that restores the snap_to_alignment functionality.  This patch should not be applied yet.


I think that ideally the extra space (when required) for the following EBR should be reserved and not displayed in the GUI when creating a new partition.

I plan to investigate this option to see what is involved.

Curtis
Comment 7 Mike Fleetwood 2017-04-20 08:02:11 UTC
Hi Curtis,

I'm pretty sure you have already done this, but anyway I bisected this
bug to this commit between GParted 0.22.0 and 0.23.0:

https://git.gnome.org/browse/gparted/commit/?id=90e3ed68fc60c1395c2f3c20ed0b4493037753fb
Shallow copy Device object into Operation object (#750168)

The commit message includes this section:
> These additional deep copied Partition objects in the Operation object
> are never accessed.  Therefore don't copy the contained Partition
> objects when copying the Device object into the Operation object.
Evidently this can't be true.  It would be good to understand how the
Partitions within a Device within an Operation ends up being required
and get accessed when composing new partitions, rather than using the
primary Devices array and the contained Partition vectors.  And fixing
this.

I am all for your current approach of investigating what is happening,
rather than just reverting this commit.

Thanks,
Mike
Comment 8 Curtis Gedak 2017-04-20 16:57:06 UTC
Hi Mike,

Thanks for confirming the issue by bisecting the code.  I actually
discovered the problem by placing print messages in the code, which is
time consuming, but permitted me to better understand the situation.
The commit is referenced in the patch.

>> These additional deep copied Partition objects in the Operation
>> object are never accessed.  Therefore don't copy the contained
>> Partition objects when copying the Device object into the Operation
>> object.
>
> Evidently this can't be true.

Correct.

The additional deep copied Partition objects are accessed by the
snap_to_alignment methods.  These methods include checks to ensure
that partitions reserve unallocated space for the MBR, EBR, and
GPT/backup, in addition to enforcing correct partition boundaries.

The snap_to_alignment methods are only called for operations that
change partition boundaries, such as Create, Copy, and Resize_Move.

In order to be valid, the checks must work with the partition layout
as it exists in the operation queue.  The existing partition layout
is not sufficient.

The calling structure is as follows:

1) An action is initiated from the GUI.  For example
   Partition -> Create which calls Win_GParted::activate_new().

https://git.gnome.org/browse/gparted/tree/src/Win_GParted.cc?h=GPARTED_0_28_1#n2072

2) After the user has completed the dialog actions,
   Win_GParted::Add_Operation is called.

https://git.gnome.org/browse/gparted/tree/src/Win_GParted.cc?h=GPARTED_0_28_1#n714

   For operations that might create new or adjust existing partition
   boundaries, there is a call to snap_to_alignment.

3) In GParted_Core::snap_to_alignment, there is a call to the
   appropriate alignment method (MiB or Cylinder).

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?h=GPARTED_0_28_1#n499

4) Many checks are performed in GParted_Core::snap_to_mebibyte.

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?h=GPARTED_0_28_1#n341

   A: Currently the GUI reserves 1 MiB of unallocated space for:
    - Primary/Extended partitions that begin at the start of the disk (MBR/GPT)
    - Logical partitions (EBR in front of partition)

   B: However, the GUI does not reserve 1 MiB of unallocated space for:
     - End of GPT disk (GPT backup)
     - Logical partitions (EBR after logical if another logical following)

   C: Further snap_to_mebibyte ensures things like:
     - Logical partition does not extend beyond end of Extended.
     - No overlaps in partitions.

   The snap_to_mebibyte method covers all cases in A and B and C.
   Because the GUI also covers A, the routines in snap_to_mebibyte for
   A are merely a fail-safe and should not change boundaries.

   However, since the GUI does not cover B or C, snap_to_mebibyte is
   the only place where the B and C checks are enforced.

   Several of the checks require an accurate image of the currently
   queued partition layout, which is provided by the deep copied
   Partition objects.  These checks are:

   i. Extended needs to allow space for EBR in front of logical.

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?h=GPARTED_0_28_1#n379

   ii. Allow space for EBR after logical if following logical exists.

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?h=GPARTED_0_28_1#n412

   iii. Ensure logical end does not extend beyond extended.

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?h=GPARTED_0_28_1#n435

   iv. Ensure no primary or extended overlaps.

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?h=GPARTED_0_28_1#n443

   v. Ensure extend fully encompasses logicals.

https://git.gnome.org/browse/gparted/tree/src/GParted_Core.cc?h=GPARTED_0_28_1#n443


I am still leaning towards restoring the deep copy Partition objects
in the Operation object.  The question in my mind is whether to do
this only for the Create, Copy, and Resize_Move actions, OR should we
do it for all actions in an effort to avoid future problems.


From a user perspective I think it would provide a better experience
if we could account for reserving extra space after logical partitions
in the GUI.

We do this for the MBR/EBR in front of partitions.  This logic
currently complicates the GUI.

The challenge as I see it is that adding logic for reserving extra
space after logical partitions will further complicate the GUI code,
which is a bit of a mess to start with.

Curtis
Comment 9 Mike Fleetwood 2017-04-21 09:13:38 UTC
Created attachment 350190 [details] [review]
Fix snap-to-alignment of operations creating partitions (v1)

Hi Curtis,

How does this simple fix for this bug look?

Rather than take deep copies of the Device and Partitions in all
Operation objects (reverting the causing commit) just make
Add_Operation() pass the current Device, as displayed in the UI, to
snap_to_alignment().

Thanks,
Mike


P.S.
Long term I think all the accounting for reserved space and alignment
should be abstracted into a common set of methods or classes with a
simple interface and be used in the UI classes which create and move
partitions so that all constraints are applied while the operation is
composed.
Comment 10 Curtis Gedak 2017-04-23 15:12:56 UTC
Hi Mike,

Your proposed patch handles the need to work with the current virtual representation of the disk partitions and works for me in my tests.

Changing the GUI behaviour is a much bigger endeavour and one that I'm not ready to tackle right now.

If you are okay with patch v1 from comment #9, then I will commit it in the coming days.

Curtis
Comment 11 Mike Fleetwood 2017-04-24 12:09:52 UTC
Hi Curtis,

Push the commit to master when ready.

Mike
Comment 12 Curtis Gedak 2017-04-24 16:48:55 UTC
Patch v1 from comment #9 has been committed to the git repository for inclusion in the next release of GParted.

The relevant git commit can be viewed at the following link:

Fix snap-to-alignment of operations creating partitions (#779339)
https://git.gnome.org/browse/gparted/commit/?id=75131d85a5f5a1832e55d57039c63a2807fbbae6

Curtis
Comment 13 Curtis Gedak 2017-08-07 17:18:47 UTC
This enhancement was included in the GParted 0.29.0 release on August 7, 2017.