(In reply to Martin Whitaker from comment #0)
> To reproduce, set up a GPT partitioned disk with one or more partitions
> defined. Run diskdrake, select that disk, click on the "Clear all" button,
> and then on the "Done" button.
> 
> This bug was introduced by commit 532fd1d6 in software/drakx. That commit
> causes c::disk_delete_all() to be called at the start of gpt::write(), which
> wipes the partition table (and causes the kernel to clear its view of the
> partition table). But $hd->{will_tell_kernel} contains a sequence of delete
> actions to remove the existing partitions, and it's executing the first one
> of these that leads to the error message.
> 
> I can't do anything to help fix this, because I don't know what bug lead to
> that commit. 

There's probably no bug report. Which additional information are you looking for that's lacking in commit 532fd1d6's log message?

    sync libparted view with diskdrake one
    
    in order to prevent overlapping errors when adding partitions:
    (b/c libparted doesn't know that diskdrake already removed the
    partitions in its memory view)
    "failed to add partition #1 on /dev/vda"
    "Error: Can't have overlapping partitions."
    "add_partition failed"

> We can't just skip the delete actions because some of them
> might come from user edits after the "Clear all" button was pressed.

CC: (none) => marja11, pterjan, thierry.vignaud
Assignee: bugsquad => mageiatools

(In reply to Marja van Waes from comment #1)
> There's probably no bug report. Which additional information are you looking
> for that's lacking in commit 532fd1d6's log message?

How to reproduce the original error. I've tried reverting the commit and retesting, and I see no errors. But it may just be that I'm not testing the specific sequence of changes that caused it to occur.

@Thierry, can you remember how to reproduce the bug that was fixed in the commit described above? (I know, it was a while ago...) I've tried reverting the commit and testing in diskdrake, but have never seen that error. Without understanding the previous bug, any change I make to fix this one risks reintroducing that one.

From reading http://gitweb.mageia.org/software/drakx/commit/?id=532fd1d60df306e204bae79c5158ca2302739966, it was running autoinst with autoclearing a disk which had a bunch of partitions.

You may want to try with eg ~ 10-20 partitions, taking a VM snapshot
then try autoinst with either:
       'partitioning' => { 
                           'auto_allocate' => 1,
                           'clearall' => 1,

or with:
       'interactiveSteps' => [ 
                               'doPartitionDisks',
                               'formatPartitions'
                               'summary',
                             ],

Or manually clear a GPT disk with lots of partitions while doing a manual installation (I tend to prefer autoinst due to faster testing, especially when trying different fixes).

BTW you may want to build your own boot.iso by rebuilding drakx-install-images with:
1) debug set to 1
http://svnweb.mageia.org/packages/cauldron/drakx-installer-images/current/SPECS/drakx-installer-images.spec?revision=1087779&view=markup#l28
2) set it to autoload stage2 from your own test server:
http://svnweb.mageia.org/packages/cauldron/drakx-installer-images/current/SPECS/drakx-installer-images.spec?revision=1087779&view=markup#l126


The original issue might well has been fixed by one of your patches

Aka choose "Erase and use entire disk" in the solutions suggested by the partitioning wizard:
http://gitweb.mageia.org/software/drakx/tree/perl-install/fs/partitioning_wizard.pm#n258

I've still not managed to reproduce the old bug, so I don't know how to solve this, other than by reverting the commit and waiting for the complaints...

Created attachment 9028 [details]
Replacement fix for original issue

The original issue only occurred in automatic installs, which is why I failed to reproduce it.

Created attachment 9029 [details]
Patch to ensure current partition table type is recorded when clearing a disk during automatic install

(fix for bug spotted whilst testing previous patch)

In an automatic install, the existing partition table can be cleared without first reading it. The test for whether a BIOS boot partition is required relies on the partition table type being recorded in $hd->{pt_table_type}, so we need to set this when we initialise a disk.

Created attachment 9030 [details]
Remove some code made redundant by the first patch

Now we force the kernel to reread the partition table when we initialise it, there's no need to also inform it that we've deleted all the existing partitions.

(this patch is not essential - the fix works with or without it)

Created attachment 9031 [details]
Patch to prevent unwanted udev events when writing a GPT partition table

Testing the previous patches on a Live system showed that it was possible for udevd to receive device change events from the kernel whilst a GPT partition table was being written, and hence for udevd to instruct the kernel to reread the partition table at an inopportune moment (just as was seen when writing a DOS partition table). It was always a bit of a mystery why I never observed this when writing a GPT partition table - it appears it was just chance.

As for DOS partition tables, causing the kernel to reread the partition table at the wrong time can cause it to get out of sync with the real state of the partition table.

The attached patch fixes the problem by ensuring all the changes to the partition table are written to disk with a single libparted commit. It also uses the libparted ped_disk_commit_to_os() function to inform the kernel of the changes before it closes the raw device, which should avoid any possible race with udevd.

Nice to see you get ideas from the Parted binding.
Looks good but for a couple peer reviewing remarks:

Though you need to rebase your patches over current origin/master :-)
Indeed first patch contains an unrelated change that is already in master

In fourth patch, get_partition_flag() is no more used for some time so it would be better IMHO to drop it in a separate commit in order to make 4th patch easier to read
Likewise for get_disk_type() simplification

Also it would be better to revert the unrelated reindent in get_disk_partitions()

Last but not least, keeping set_partition_flag() in its place would make the patch smaller and easier to read changes done to set_partition_flag()

Created attachment 9037 [details]
Replacement fix for original issue

Attachment 9028 is obsolete: 0 => 1

Created attachment 9038 [details]
Ensure current partition table type is recorded when clearing a disk during automatic install

Attachment 9029 is obsolete: 0 => 1

Created attachment 9039 [details]
Remove some code made redundant by the first patch

Attachment 9030 is obsolete: 0 => 1

Created attachment 9040 [details]
Prevent unwanted udev events when writing a GPT partition table

Attachment 9031 is obsolete: 0 => 1

I've rebased your patches on top of current master (thus removing the unrelated bits like the log change and droping an old unused function).
I've also added a little text at end of latest patch's description
I've also kept set_partition_flag() in place in order to see the actual changes

I wonder if 4th patch should be split between (for clarity):
1) enhancing the binding by keeping around the ped_disk object
2) do the commit at once

But that's you call.

One last thing: I think the ped_disk_probe() simplification in latest patch should be split in another commit as it's unrelated and in order to not make latest commit too big (and thus harder to reread back in eg 6 months)

Also you should maybe add to the commit chlog that you've overridden need_to_tell_kernel()?

Last but not least, you could get rid of the (PedDisk*) typing by setting proper types for parameters, which would need a typemap file

Created attachment 9041 [details]
simplify using ped_disk_probe()

Created attachment 9042 [details]
fix indentation

Created attachment 9043 [details]
Prevent unwanted udev events when writing a GPT partition table

Attachment 9040 is obsolete: 0 => 1

Created attachment 9047 [details]
simplify using typemap

only slightly tested.
To be squashed in latest patch.

@Martin: WDYT?

(In reply to Thierry Vignaud from comment #16)
> I've rebased your patches on top of current master (thus removing the
> unrelated bits like the log change and droping an old unused function).
> I've also added a little text at end of latest patch's description
> I've also kept set_partition_flag() in place in order to see the actual
> changes

I still think moving set_partition_flag() would make the actual code easier to follow, even if it does make it harder to check the patch.

> I wonder if 4th patch should be split between (for clarity):
> 1) enhancing the binding by keeping around the ped_disk object
> 2) do the commit at once
> 
> But that's you call.

I would always review the final code, not the patch, so adding an extra step doesn't add much value IMO.

> One last thing: I think the ped_disk_probe() simplification in latest patch
> should be split in another commit as it's unrelated and in order to not make
> latest commit too big (and thus harder to reread back in eg 6 months)

Actually that change was supposed to be part of the fix (as mentioned in the commit log). I read elsewhere that ped_disk_probe() opened the raw device read-only, which avoided unwanted change events reaching udevd. But having since looked at the libparted source code, this doesn't appear to be true :-(

> Also you should maybe add to the commit chlog that you've overridden
> need_to_tell_kernel()?

I was in two minds about whether to do that at all. In my tests, if it failed the first time, it always failed the second time. But that may be because I don't know what a "magic partition" is, so haven't tested that particular case.

> Last but not least, you could get rid of the (PedDisk*) typing by setting
> proper types for parameters, which would need a typemap file

Yes, I wanted to do that, but didn't know the mechanism. Apart from the typemap definition, your final patch is what I originally wrote.

(In reply to Martin Whitaker from comment #21)
> I still think moving set_partition_flag() would make the actual code easier
> to follow, even if it does make it harder to check the patch.

I think it should be then moved as a separate commit.
Else it's impossible to tell what has changed from just looking at the commit diff.

> > But that's you call.

> I would always review the final code, not the patch, so adding an extra step
> doesn't add much value IMO.

As you wish. I usually I advertise small readable incremental changes.

> > One last thing: I think the ped_disk_probe() simplification in latest patch
> > should be split in another commit as it's unrelated and in order to not make
> > latest commit too big (and thus harder to reread back in eg 6 months)
> 
> Actually that change was supposed to be part of the fix (as mentioned in the
> commit log). I read elsewhere that ped_disk_probe() opened the raw device
> read-only, which avoided unwanted change events reaching udevd. But having
> since looked at the libparted source code, this doesn't appear to be true :-(

:-(
Well we can either keep the separate simplification commit as it or adjust its  chlog to mention that potentially libparted could do it RO.
The only udev aware interesting change I was aware was using ped_disk_commit() as in the parted bindings I sent you
 
> > Also you should maybe add to the commit chlog that you've overridden
> > need_to_tell_kernel()?
> 
> I was in two minds about whether to do that at all. In my tests, if it
> failed the first time, it always failed the second time. But that may be
> because I don't know what a "magic partition" is, so haven't tested that
> particular case.

magic as in partition_table::tell_kernel() ?
That was introduced when we switched from "tell the kernel to just reread the whole partition table" to "smartly tell it which parts got deleted/added".
This was helping when some partitions were in used (eg: mounted, ...) else one would had to reboot else there would have been a discrepancy between the  physical layout and the kernel layout, which could lead to interesting stuff if mkfs /dev/sda7 actually format sda6 or sda8...

So we try to umount them before telling the kernel to reead the partition table...
If some are still in use, then the kernel would abort the reread and we would have to reboot in order to be in a sane state

I later improved the process by using libparted to tell the kernel to reread the part table:
http://gitweb.mageia.org/software/drakx/commit/?id=63942995

See http://gitweb.mageia.org/software/drakx/commit/?id=a0092a3d667863341339645f5d0bc9bc4b4b5108
and http://gitweb.mageia.org/software/drakx/commit/?id=03116801 (previous commit)

> > Last but not least, you could get rid of the (PedDisk*) typing by setting
> > proper types for parameters, which would need a typemap file
> 
> Yes, I wanted to do that, but didn't know the mechanism. Apart from the
> typemap definition, your final patch is what I originally wrote.

So if your tests are as good as mines, I suggests:
- squash the typemap WIP commit in the last patch
- push that in master
- do a release

I can do that if you want, or I can leave it to you as you wish.

Basically I think that even if some changes eventually fix the same bug (aka they're all needed in order to actually fix it) but they're independent, they should be committed as independent separate commits, thus easier to reread and understand.

I'm talking from experience, b/c when you track down why something was done and you end on a 10 or 18 years old big commit, that's not funny.

Especially when the chlog is "See_The_Changelog", "*** empty log message ***", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "no_comment", "update", "0.05-1mdk", ...
All of which are real cases of very old chlog messages :-(
And yes, I really had to hit such commits when digging a bug sometimes :-(

Thus this is why I favor clean, short & readable commits :-)

I hope you see my point (made of historical pain :-) )


So if we eliminate the risk of udev events at different points, I think they should be different commits. Even if we still have only one BZ.
Also cleanups should be split (eg: indentation, unlogged stuff, ...)

Created attachment 9054 [details]
Prevent unwanted udev events when writing a GPT partition table

This updates last patch with the typemap improvement and also makes the commit chlog a little bit more verbose.

I've two more things to add though:

1) you do both commit_to_dev & to_os in disk_commit().
Ideally this should be safely replaced by ped_disk_commit()... but for the partition_table::write() logic trying harder in case a partition is busy (users don't like having to reboot -- we're not windows)

Though, the unified ped_disk_commit() prevents udev events between both operations. See:
http://git.savannah.gnu.org/cgit/parted.git/tree/libparted/disk.c#n521

Maybe should we bind ped_device_open() & close() so that we keep a fd opened on the raw device to prevent such udev events between both calls?
(that's not a defect in your patch, we already have this issue before, the only safe one is the test program I send you along the WIP parted binding)

2) we now tell the kernel about the new partition flags in partition_table::gpt::write() when calling disk_commit() while previously we were deferring that to partition_table::write() which after having called the gpt's write() calls tell_kernel() -> c::tell_kernel_to_reread_partition_table() thus your need_to_tell_kernel()

I added a comment about why you introduced it (and also enhanced the chlog message)

It's more understandable that way for other devs :-)

Now we only have clean all of this in mga7 by unifying dos/mbr/... partition layout formats by relying on libparted for everything & using a proper Parted binding instead of only having gpt being "modern" :-)

BTW using a proper parted would solve an issue, here we'll leak info about one PedDisk object per disk as we never call ped_disk_destroy() when the perl object is unreferenced but that's a minor issue...

Attachment 9043 is obsolete: 0 => 1
Attachment 9047 is obsolete: 0 => 1

Created attachment 9055 [details]
Prevent unwanted udev events when writing a GPT partition table

And I forgot to call again git format-patch... :-(

Attachment 9054 is obsolete: 0 => 1

To be more clear, if we had have a separate Ped::Disk module in c::, we would move the destroy call from disk_commit() into DESTROY()

(In reply to Thierry Vignaud from comment #22)
> (In reply to Martin Whitaker from comment #21)
> > I was in two minds about whether to do that at all. In my tests, if it
> > failed the first time, it always failed the second time. But that may be
> > because I don't know what a "magic partition" is, so haven't tested that
> > particular case.
> 
> magic as in partition_table::tell_kernel() ?
> That was introduced when we switched from "tell the kernel to just reread
> the whole partition table" to "smartly tell it which parts got
> deleted/added".
> This was helping when some partitions were in used (eg: mounted, ...) else
> one would had to reboot else there would have been a discrepancy between the
> physical layout and the kernel layout, which could lead to interesting stuff
> if mkfs /dev/sda7 actually format sda6 or sda8...
> 
> So we try to umount them before telling the kernel to reead the partition
> table...

What I did during testing was to create and mount a FAT32 partition before running diskdrake. diskdrake was aware it was mounted, and wouldn't let me delete that partition without unmounting it, but it did let me clear all partitions. At that point the commit to os in gpt::write() failed (as expected), and partition_table::tell_kernel() was called. That didn't unmount the partition (because $hd->{real_mntpoint} wasn't set), so its commit to os failed too.

I was surprised that diskdrake let me clear all partitions when one was mounted ... and I think that means we need a better test for whether any partitions are mounted in dos::need_to_tell_kernel().


(In reply to Thierry Vignaud from comment #24)
> I've two more things to add though:
> 
> 1) you do both commit_to_dev & to_os in disk_commit().
> Ideally this should be safely replaced by ped_disk_commit()... but for the
> partition_table::write() logic trying harder in case a partition is busy
> (users don't like having to reboot -- we're not windows)

Also ped_disk_commit() isn't actually documented as part of the libparted API.

> Though, the unified ped_disk_commit() prevents udev events between both
> operations. See:
> http://git.savannah.gnu.org/cgit/parted.git/tree/libparted/disk.c#n521
> Maybe should we bind ped_device_open() & close() so that we keep a fd opened
> on the raw device to prevent such udev events between both calls?

Yes, I had already looked at that. I had assumed that ped_disk_new() opened the raw device and ped_disk_destroy() closed it, but again from browsing the source code, that's not so. So yes, we should make this safe. Revised patch to follow.


> Now we only have clean all of this in mga7 by unifying dos/mbr/... partition
> layout formats by relying on libparted for everything & using a proper
> Parted binding instead of only having gpt being "modern" :-)

I was going to suggest that, but not until mga6 is done.


> BTW using a proper parted would solve an issue, here we'll leak info about
> one PedDisk object per disk as we never call ped_disk_destroy() when the
> perl object is unreferenced but that's a minor issue...

ped_disk_destroy() is called in c::disk_commit(), so it's only if there's a fatal error in gpt::write() that it won't be called. So I thought we could live with that for now.

Created attachment 9060 [details]
Prevent unwanted udev events when writing a GPT partition table

Updated to keep raw disk fd open between ped_disk_commit_to_dev() and ped_disk_commit_to_os(). Not yet tested.

Also updated commit log to remove text describing a change that has moved to another patch.

Attachment 9055 is obsolete: 0 => 1

Created attachment 9082 [details]
Ensure kernel is informed when a DOS partition table is cleared

As mentioned in comment 27, diskdrake allows the user to clear all partitions even when one or more of those partitions are mounted. dos::need_to_tell_kernel() was only testing whether any of the new partitions was mounted. This patch ensures it also takes the old partitions into account.

I've done some testing and found no problems. If you have no further objections, Thierry, I'll rebase again and push these patches.

Yeah go ahead

commit ca515b773e51493aee36d67a5893af33101493b1
Author: Martin Whitaker <mageia@...>
Date:   Sat Mar 4 12:02:55 2017 +0000

    Revised fix for clearing GPT partitions during automatic install.
    
    This reverts commit 532fd1d60df306e204bae79c5158ca2302739966, which
    introduced a new bug when clearing GPT partitions in an interactive
    session (mga#20264), and replaces it with a new solution.
    
    When a partition table is initialised, we now add an 'init' action to
    the $hd->{will_tell_kernel} list. This is used both by gpt::write()
    (to clear the partition table) and by partition_table::tell_kernel()
    (to force the kernel to reread the partition table). Previous changes
    stored in $hd->{will_tell_kernel} are discarded, as they are no longer
    of interest.
    
    This also removes support for the will_tell_kernel 'force_reboot'
    action, as nothing uses that any more.
---
 Commit Link:
   http://gitweb.mageia.org/software/drakx/commit/?id=ca515b773e51493aee36d67a5893af33101493b1

Fix released, so closing.

Status: NEW => RESOLVED
Resolution: (none) => FIXED

Removing IN_ERRATA6 keyword, since the bug is fixed. It looks like the entry had been removed from the wiki already.

Keywords: IN_ERRATA6 => (none)