[parted-devel] [BUG] _disk_sync_part_table() is broken

[Petr Uzel]

Hi list!

There's a bug in current development version of parted which causes that the
partition table, as seen by the kernel, is not updated correctly after deleting
a partition. This bug was introduced in commit
f6bd20573e3ecfb63f62d88c52a0870fb8851b59 (fix computation of largest partition
number). Yes, I know - this patch was proposed by me - sorry for that :)

Steps to reproduce:
-------------------

Start with the empty partition table, create primary partition and then delete this
partition. The on-disk partition table representation is correct, but the kernel is
not correctly informed about the change. This might be fixed by running
'blockdev --rereadpt /dev/hdd' command.

# ./parted/parted /dev/hdd
GNU Parted 1.8.8.1.90-cc1f
Using /dev/hdd
Welcome to GNU Parted! Type 'help' to view a list of commands.
(parted) print
Model: WDC WD153BA (ide)
Disk /dev/hdd: 15.4GB
Sector size (logical/physical): 512B/512B
Partition Table: msdos

Number  Start  End  Size  Type  File system  Flags

(parted) mkpart primary ext2 0 2G
(parted) rm 1
(parted) print
Model: WDC WD153BA (ide)
Disk /dev/hdd: 15.4GB
Sector size (logical/physical): 512B/512B
Partition Table: msdos

Number  Start  End  Size  Type  File system  Flags
<SNIP>

# cat /proc/partitions
<SNIP>
22    64   15021720 hdd
22    65    1951866 hdd1
# blockdev --rereadpt /dev/hdd
# cat /proc/partitions
<SNIP>
22    64   15021720 hdd

Analysis: 
---------

When the partition is deleted, parted first updates its in-memory
representation of partition table [1], then it writes the partition table a
then it commits the changes to OS. On newer kernels, the disk commit is done by
_disk_sync_part_table() called from linux_disk_commit().

This function needs to inform the kernel that partition #1 was deleted. To find
out what partitions it should iterate over, it uses MIN(16, ped_disk_get_last_partition_num(disk)). 
But the problem is that ped_disk_get_last_partition_num() uses in-memory
representation of partition table, in which the partition table has no
partitions (see [1]), and thus it returns -1.

The result is that parted does not inform the kernel that partition #1 was deleted.

Previous state:
---------------

Before the broken commit was checked in, parted used PED_MAX(16, last_partition_num())
to determine which partitions might be changed. But this also is not correct, because : 
1. for <= 16 partitions on the disk, the last_partition_num() call is useless 
2. if there's >16 partitions, the last_partition_num() might return inconsistent
result (in case the last partition was just deleted)

I know that there may be situations where there are more than 16 partitions on
the disk, but does parted want to support this? If not, the PED_MAX(16, last_partition_num())
might be changed to be just 16 and everything should work fine.

Another solution might be introducing something like
'last_sensible_partition_num()', which would return the largest partition
number that would make sense, depending on disk label type, OS, ...

Note: _dm_reread_partition_table() probably suffers the same - I have not tested
it personally, though.

What do you guys think?

And once again - sorry for introducing that bug to parted.


-- 
Best regards / s pozdravem

Petr Uzel, Packages maintainer
---------------------------------------------------------------------
SUSE LINUX, s.r.o.                          e-mail: puzel at suse.cz
Lihovarská 1060/12                          tel: +420 284 028 964
190 00 Prague 9                             fax: +420 284 028 951
Czech Republic                              http://www.suse.cz