Regression: Unable to add a partition to a disk that has another partition in use

[Phillip Susi]

Binary package hint: parted

If there is free space on a disk but one or more partitions on the disk are in use and you try to add a new partition using that free space, parted fails to update the kernel partition table with the new partition in Lucid Beta 1. This works fine in Karmic. We switched from 1.8.8 in Karmic to 2.x in lucid, so this regression is likely from upstream.

Parted ships with the diskprobe utility that seems to be responsible for rescanning the partition table and updating the kernel. It appears that in Karmic it makes use of the new partition manipulation ioctls that partx uses rather than the old BLKPRT ioctl that fdisk uses, which does not work if any partitions on the disk are in use. For some reason it appears that in lucid it has gone back to using BLKPRT.

[Phillip Susi (psusi)]

Confirmed from inspecting the sources that the BLKPG code was removed upstream. There is a ChangeLog entry explaining the change. Will take the issue upstream.

[Phillip Susi (psusi)]

Seems I'll be working on a patch for this.

[Phillip Susi (psusi)]

Attaching debdiff of my first attempt to resolve this.

[Phillip Susi (psusi)]

Second attempt. This time it only throws the warning if you are modifying the partition that was in use. If you only changed other partitions, you should be fine. Will do some testing and if it looks good will send the patches upstream for review.

[Phillip Susi (psusi)]

Tested and this version seems to be working well for me. Sending upstream for approval.

[Colin Watson (cjwatson)]

Thanks for looking at this! I definitely want to take something like this for Lucid, and this looks fairly close to being usable. A few comments:

Could you use 'quilt refresh -pab' so that you don't change the format of all the patches, please?

Try to pay attention to the style of the surrounding code. Your error-check-BLKPG.patch is formatted completely differently: you have spaces inside parentheses, different brace indentation, tab/space differences, and so on. I know it seems picky but it really makes the patch easier to read when it's in line with what's around it, and you'll need to sync this up for upstream acceptance anyway.

Your removal of the _has_partitions call in linux_disk_commit seems risky; I recall this being added to fix a bug. I would be more comfortable if this were left in, even if it turns out that it isn't strictly necessary now for some reason; it seems to me that it's at least needed for the _kernel_reread_part_table fallback, so it seems that it would be best to check it at the top level.

Is the length the only change that might necessitate telling the kernel to re-read a partition? Are there any circumstances in which a type change might matter too? I'm hoping not but was wondering if you'd thought about this.

[Colin Watson (cjwatson)]

In fact, use 'quilt refresh --no-timestamps -pab'. (You can set this up in ~/.quiltrc if you like.)

[Neil Perry (nperry)]

@ Phillip Susi - I have reviewed you're patch, which seems to build fine and fix the issue. Could you please post this upstream.

Thanks

[Colin Watson (cjwatson)]

On Fri, Mar 26, 2010 at 01:40:37PM -0000, Neil Perry wrote:
> @ Phillip Susi - I have reviewed you're patch, which seems to build fine
> and fix the issue. Could you please post this upstream.

I commented separately on issues with the patch, with a more detailed
review (does the Ubuntu Review Team review code, or just the form? The
latter seems more useful). I agree that it should be forwarded upstream
once those issues are resolved, but I intend to apply it or a variant of
it to Ubuntu without waiting for it to get through upstream review,
since this is critical for Lucid.

[Phillip Susi (psusi)]

I am emailing this upstream now for them to comment on and will work on the cleanups you suggested tomorrow ( going to see Lewis Black live tonight ).

[Phillip Susi (psusi)]

This should be the final cut. I've done quite a bit of testing and an pleased with the results. The last try did not warn if it failed to remove a partition and you did not try to replace it. This one collects a list of all partitions that it could not remove, discards any that are supposed to be added back exactly as they were, then if there are any left, throws a warning listing all the partitions left on the list. Also cleaned up the unintentional whitespace changes.

[Colin Watson (cjwatson)]

I'm still missing the _has_partitions call (see comment 6), and there's
still a moderate amount of whitespace damage. I'll sync this up myself
and see how it looks ...

[Colin Watson (cjwatson)]

I fixed up the problems I mentioned, and did some basic testing. In the process, I ran into and fixed what I believe is a bug in extended partition geometry handling that broke automatic partitioning in the installer (I forwarded this fix upstream). Other than that, things seem to be working well, so I've uploaded this. Thanks!

Phillip, could you please review the patch files in parted 2.2-1ubuntu4 and sync them up with what you're sending upstream?

[Launchpad Janitor (janitor)]

This bug was fixed in the package parted - 2.2-1ubuntu4

---------------
parted (2.2-1ubuntu4) lucid; urgency=low

  [ Phillip Susi ]
  * put-back-BLKPG.patch: Upstream removed the code using the new
    BLKPG ioctls instead of the old BLKRRPART ioctl to update the
    in-kernel partition table. This patch reverses that change.
  * error-check-BLKPG.patch: Add error checking to the BLKPG code to
    fix up the old BLKPG code to satisfy upstream. (LP: #540940)
  * Refresh quilt patches.

  [ Colin Watson ]
  * extended-geom.patch: Always allow at least two sectors for extended boot
    record.
 -- Colin Watson <email address hidden> Mon, 29 Mar 2010 14:45:28 +0100

[Phillip Susi (psusi)]

I think I ran into that same bug... when putting parted into sector mode it allowed me to create a logical partition starting on the sector immediately following the EBR of the extended partition, but the second sector is also consumed by the device node that is created for the extended partition itself. I can find no reason for this second sector to be reserved. It seems like it is a bug in the kernel partition table code that parted picked up trying to do the same thing that BLKRRPART does. Subsequent logical partitions appear to only have the single EBR sector, then immediately begin the logical partition just fine.

I'll take a look at the version you uploaded and see about making a few minor changes already suggested by upstream.

[Colin Watson (cjwatson)]

Even if that is a bug (which I'm not convinced about - I haven't spent
the time digging into LILO to verify), to be honest I'd much rather
preserve it and deal with its consequences (by means of my extended-geom
patch or similar) than try to change it. That goes double a month
before Lucid's release. :-)

[Phillip Susi (psusi)]

True, your patch should be used for now, but I'm going to push upstream to fix the kernel. If that one sector is not there, then the partition should not use it. It seems the code was put there to force the size to 2 since LILO wants 2 sectors, but it seems to me that when the space for the second sector is not there, it should not try to use it, and if there are more spare sectors there, they should be usable too, if you wanted to install a boot loader there that uses more than 2 sectors, like grub.

[Mario Limonciello (superm1)]

I can confirm this fix resolves the problems from the duplicate bug (532961). Thanks!