Bug 498137 - _disk_alloc_freespace not handling correct starting sectors
_disk_alloc_freespace not handling correct starting sectors
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: parted (Show other bugs)
rawhide
All Linux
urgent Severity urgent
: ---
: ---
Assigned To: Joel Andres Granados
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 496760
  Show dependency treegraph
 
Reported: 2009-04-28 23:08 EDT by David Cantrell
Modified: 2013-01-10 00:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-11 16:22:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
parted-1.8.8-alloc-freespace.patch (10.90 KB, patch)
2009-04-28 23:10 EDT, David Cantrell
no flags Details | Diff
msdos metadata creation patch. (Against Upstream) (4.46 KB, patch)
2009-05-07 11:18 EDT, Joel Andres Granados
no flags Details | Diff

  None (edit)
Description David Cantrell 2009-04-28 23:08:54 EDT
dlehman brought this to my attention.  The reproducer is simple using pyparted and ipython.  Conditions:

1) msdos labeled disk with some partitions on it, but NOT starting at the beginning of the disk.  I started around cylinder 100 and created two partitions.

2) Make filesystems on the partitions, just for sanity.

3) Start ipython and run the following:


In [1]: import parted

In [2]: device = parted.getDevice("/dev/sdb")

In [3]: disk = parted.Disk(device)

In [4]: freespace = disk.getFreeSpaceRegions()

In [5]: freespace
Out[5]: [<parted.geometry.Geometry object at 0x1c019d0>]

In [6]: freespace[0].start
Out[6]: 63L

In [7]: disk.deleteAllPartitions()
Out[7]: True

In [8]: disk.commit()
Out[7]: True

In [9]: freespace = disk.getFreeSpaceRegions()

In [10]: freespace
Out[10]: [<parted.geometry.Geometry object at 0x1c01c10>]

In [11]: freespace[0].start
Out[11]: 0L


Problem:
The disk still has an msdos label, but when we delete all partitions, libparted internally updates the part_list with new freespace regions.  After all partitions are removed, the new freespace region starts at sector 0.  However, if we have any partitions on the disk but some room at the beginning free, libparted will report sector 63 as the first available sector for free space.

dlehman and I agree that regardless of the number of partitions on the disk, libparted should be consistently reporting the correct starting sector for free space.  This is disk label dependent.  msdos and pc98 (maybe?) should start at sector 63 since everything before that is reserved for the MBR.  Other disk labels will have other conditions.

What I have done is created a patch for parted that creates a new PedDiskOps function that is per disk label.  It's called 'get_start_sector'.  For msdos and pc98, I'm returning 63.  For all other disk labels, I am returning 0, which is consistent with what _disk_alloc_freespace() was doing before.

Now, how come libparted is doing the correct thing when there is actually a partition or two on the disk (the correct thing being returning sector 63 for the first available free space)?  The problem is buried in the msdos_alloc_metadata() function in libparted/labels/dos.c.  It seems that metadata allocation actually happens once all partitions are removed, but it's not set up correctly when part_list contains FREESPACE partitions.

So, I see two ways to correct this problem.  Hack around it using the patch I've provided.  Or figure out why msdos_alloc_metadata() is not setting up placeholders correctly with no partitions on the disk.
Comment 1 David Cantrell 2009-04-28 23:10:49 EDT
Created attachment 341690 [details]
parted-1.8.8-alloc-freespace.patch
Comment 2 Joel Andres Granados 2009-05-06 07:37:39 EDT
The following script show the same behavior in parted UI.
<snip>
#!/bin/bash
parted="/usr/local/sbin/parted"
dd if=/dev/zero of=test bs=1M count=100
$parted test mklabel msdos
$parted test unit s print free
$parted test mkpart primary 50M 100M
$parted test unit s print free
rm test
</snip>

Notice that the first time the script prints the free space, it starts
in sector 0.  And the second time it prints the free space it begins in
sector 32. (This is with parted from git HEAD.)
Comment 3 Joel Andres Granados 2009-05-07 11:18:32 EDT
Created attachment 342857 [details]
msdos metadata creation patch.  (Against Upstream)

I followed this to the add_startend_metadata function in parted.  Most of the explanation of the patch is in the patch itself.

I ran all the parted tests and everything seems ok.  I'm a little worried that it might break some package that depends on parted (It does not change the API though).  Comments in this respect appreciated.

I will create a parted package for f11 shortly so we can test this further.
Comment 4 Joel Andres Granados 2009-05-07 12:32:00 EDT
Here is the package with the patch:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1340814
Comment 5 David Cantrell 2009-05-07 15:15:28 EDT
(In reply to comment #3)
> Created an attachment (id=342857) [details]
> msdos metadata creation patch.  (Against Upstream)
> 
> I followed this to the add_startend_metadata function in parted.  Most of the
> explanation of the patch is in the patch itself.
> 
> I ran all the parted tests and everything seems ok.  I'm a little worried that
> it might break some package that depends on parted (It does not change the API
> though).  Comments in this respect appreciated.
> 
> I will create a parted package for f11 shortly so we can test this further.  

The approach looks good to me.  Better than the fix I threw together.  I don't think this will introduce any regressions in DOS label support.  However, I do have some small comments:

1) Is the PC98 label also affected?  If so, should probably also fix that code too.

2) parted uses GNU-style code, so check your indentation on some of those wrapping statements.  Also, the // for comments may be rejected.

Other than that, it looks good to me.
Comment 6 Joel Andres Granados 2009-05-08 07:37:21 EDT
1) I just made a test with pc98 and it behaves correctly with the test in comment #2.  The test was performed after applying the patch.

2) Thx for the extra comments :).  I already posted the patch upstream but will include your observations if it commes back to me.
Comment 7 Joel Andres Granados 2009-05-08 11:35:49 EDT
This should be present in parted-1_8_8-17_fc11
Comment 8 Jesse Keating 2009-05-11 16:22:18 EDT
That parted is in the freeze, closing this bug.

Note You need to log in before you can comment on or make changes to this bug.