Example of case (1) LVM on top of DMRaid, on Utuntu 12.04 LTS


GParted built with --enable-libparted-dmraid.  LVM2 PV partition created
on a DMRaid array.  The partition is shows with this warning:

    Unable to read the contents of this file system!
    Because of this some operations may be unavailable.
    The cuase might be a missing software package.
    The following list of software packages is required for lvm2 pv
    file system support: lvm2.


GParted is using /dev/mapper/isw_cgahfjhcdb_MyArray0p2 as the partition
block device name.

# ls -l /dev/mapper
total 0
crw------- 1 root root 10,236 Jun 12 11:59 control
lrwxrwxrwx 1 root root      7 Jun 12 12:17 isw_cgahfjhcdb_MyArray0 -> ../dm-0
lrwxrwxrwx 1 root root      7 Jun 12 12:17 isw_cgahfjhcdb_MyArray0p1 -> ../dm-1
lrwxrwxrwx 1 root root      7 Jun 12 12:17 isw_cgahfjhcdb_MyArray0p2 -> ../dm-2


GParted created the LVM2 PV using using this command:

# lvm pvcreate -M 2 /dev/mapper/isw_cgahfjhcdb_MyArray0p2
Physical volume "/dev/mapper/isw_cgahfjhcdb_MyArray0p2" successfully created


However when querying LVM, it reports an alternate block device name.

# lvm pvs
  PV         VG   Fmt  Attr PSize PFree
  /dev/dm-2       lvm2 a-   1.00g 1.00g


/dev/mapper/isw_cgahfjhcdb_MyArray0p2 is a symlink to /dev/dm-2.

lvm pvs command reports a different block device name to that which
GParted is using so it doesn't find the usage information in the cache
of LVM2 information.  LVM2_PV_Info::get_pv_cache_entry_by_name() is only
performing a string compare to match block device names.

Example of case (1) LVM on top of DMRaid, on Ubuntu 14.04 LTS


GParted built with --enable-libparted-dmraid.  LVM2 PV partition created
on a DMRaid array.  Details are as the same as for Ubuntu 12.04 LTS in
comment 1, except for the following differences.


DMRaid array name differs.  GParted is using this as the partition name:

# cd /dev/mapper
# ls -l isw_bacdehijbd_MyArray0p2
brw-rw---- 1 root disk 252,   2 Jun 12 13:16 isw_bacdehihbd_MyArray0p2


This time LVM is reporting a different name again.

# lvm pvs
  PV                                                VG   Fmt  Attr PSize PFree
  /dev/disk/by-id/dm-name-isw_bacdehijbd_MyArray0p2 VG   lvm2 a--  1.00g 1.00g

# cd /dev/disk/by-id
# ls -l dm-name-isw_bacdehijbd_MyArray0p2
lrwxrwxrwx 10 Jun 12 13:16 dm-name-isw_bacdehihbd_MyArray0p2 -> ../../dm-2
# ls -l /dev/dm-2
brw-rw---- 1 root disk 252, 2 Jun 12 13:24 /dev/dm-2


GParted is using this name:
    /dev/mapper/isw_bacdehihbd_MyArray0p2    blk 252, 2

and LVM is reporting:
    /dev/disk/by-id/dm-name-isw_bacdehijbd_MyArray0p2  symlink to ->
    /dev/dm-2                                blk 252, 2

Therefore when comparing block device name for equality, it is not
enough to follow symlinks and see if the resolve to the same name.  Need
to check that they refer to the same block device; that the major, minor
pair match.

Example case (2) multi-btrfs on top of LUKS, on CentOS 6


Multi-btrfs created on LUKS encrypted partitions.

# cryptsetup luksFormat /dev/sdb7
# cryptsetup luksOpen /dev/sdb7 sdb7_crypt
# cryptsetup luksFormat /dev/sdb8
# cryptsetup luksOpen /dev/sdb8 sdb8_crypt

# ls -l /dev/mapper
total 0
crw-rw----. 1 root root 10, 58 Jun  4 20:36 control
lrwxrwxrwx. 1 root root      7 Jun 19 16:40 sdb7_crypt -> ../dm-0
lrwxrwxrwx. 1 root root      7 Jun 19 16:40 sdb8_crypt -> ../dm-1

# mkfs.btrfs -L encrypted-btrfs /dev/mapper/sdb7_crypt /dev/mapper/sdb8_crypt


Btrfs command doesn't even recognise /dev/mapper/sdb7_crypt as
containing any file system.  Instead it only works with dm-* names and
reports them too.

# btrfs filesystem show /dev/mapper/sdb7_crypt
Btrfs Btrfs v0.20-rc1
# btrfs filesystem show /dev/dm-0
Label: 'encrypted-btrfs'  uuid: 88c3f50d-4860-48ae-8a52-0db7a49461a8
	Total devices 2 FS bytes used 28.00KB
	devid    2 size 510.00MB used 0.00 path /dev/dm-1
	devid    1 size 510.00MB used 12.00MB path /dev/dm-0

Btrfs Btrfs v0.20-rc1


GParted is working with /dev/mapper/sdb[78]_crypt canonical Device
Mapper names, but btrfs is only working with and reporting /dev/dm-*
names.

Reviewing GParted code I find these uses of caches which store block
device names and are matched by string compare in one way or another.
There may well be other code in GParted which is also using string
compare to match block devices.


Proc_Partitions_Info
*   Cache loaded from /proc/partitions
*   ::device_paths_cache
    *   std::vector of whole disk block device names.
        E.g. ["/dev/sda", "/dev/sdb", ...]
    *   returned by ::get_device_paths()
*   ::alternate_paths_cache
    *   std::map indexed by block device names.
        E.g. x["/dev/sda1"] -> ["/dev/sda1"]
    *   returned by ::get_alternate_paths()

FS_Info
*   Cache loaded from command "blkid"
*   ::fs_info_cache
    *   Glib::ustring of blkid output
*   ::get_device_entry()
    *   perform string match on block device name

DMRaid
*   Cache loaded from "dmraid -sa -c"
*   ::dmraid_devices
    *   std::vector of whole DMRaid array block device names.
        E.g. ["isw_bbaecedgch_MyArray0"]
    *   Linearly search and string compared
    *   ::get_devices() returns names prefixed with "/dev/mapper"
        E.g. ["/dev/mapper/isw_bbaecedgch_MyArray0"]

LVM2_PV_Info
*   Cache loaded from "lvm pvs ..."
*   ::lvm2_pv_cache
    *   std::vector of struct LVM2_PV, including disk block device names
        E.g. [{"/dev/sda10", ...}, {"/dev/sda11", ...}, ...]
*   ::get_pv_cache_entry_name_name()
    *    linearly search and string compared

btrfs
*   cache incrementally loaded from "btrfs filesystem show /dev/PTN"
*   ::btrfs_device_cache
    *   std::map indexed by block device names.
        E.g. x["/dev/sdb1"] -> {devid=1, members=["/dev/sdd1",
                                              "/dev/sdc1", "/dev/sdb1"]}
    *   ::get_cache_entry()
        *   lookup in std::map using block device name.
    *   ::get_members() list of btrfs member devices, used for display
    *   ::get_mount_device() also linear search and string compares
        members in vector

SWRaid_Info
*   Cache loaded from command "mdadm -Esv" and file "/proc/mdstat"
*   ::swraid_info_cache
    *   std::vector of struct SWRaid_Member
        E.g. [{member="/dev/sda1", array="/dev/md1", ...}, ...]
*   ::get_cache_entry_by_member()
    *   linear search and string compare

LUKS_Info
*   Cache loaded from "dmsetup table --target crypt"
*   ::luks_mapping_cache
    *   std::vector of struct LUKS_Mapping
        E.g. [{name="sdb6_crypt", major=8, minor=22, path="/dev/sdb6", ...}, ...]
*   ::get_cache_entry()
    *   linear search string matching block device name, falling back to
        major, minor match using stat() call when required

GParted_Core::mount_info
*   Cache loaded from files "/proc/mounts" and "/proc/swaps"
*   Also loaded from command "mount" if required
*   ::mount_info
    *   std::map of Glib::ustring to vector of Glib::ustring
    *   Maps block device names to list of mounting points
        E.g. x["/dev/sda1"] -> ["/boot"]
*   Accessed using .find() and loop from .begin() to .end()

GParted_Core::fstab_info
*   Cache loaded from file "/etc/fstab"
*   ::fstab_info
    *   std::map of Glib::ustring to vector of Glib::ustring
    *   Maps fstab configured mounts
        E.g. x["/dev/sda1"] -> ["/boot"]
*   Assessed using .find()

Hi Curtis,


So GParted is written assuming string compare can be used to match block
devices.  It is used in lots of places in the code.  The question is
should GParted continue to use string compare in all these locations to
match devices?


Option (1): Stick with string comparing block device names
  Then there needs to be a way to canonicalise all block devices to
  their preferred names for all names read from files and command output
  when populating the caches.  Then continue to use string compare to
  access the caches.

  Call Utils::canonical_path() as required.


Option (2): Use class with major, minor comparison
  Change to major, minor block device numbers everywhere.  Looking ahead
  it would seem simplest to implement a Block_Device class which
  represents the name and major, minor numbers of block devices.
  Objects of type Block_Device would be used everywhere, replacing path
  names.

  Construct Block_Device("/dev/sda1") or Block_Device(252, 3) as
  required.


Option (3): Stick with block device names, but use major, minor
            comparison
  Half way house of block device names continuing to be used everywhere
  but using major, minor comparison.  This option gets a bit
  complicated.

  Open coded linear searches of caches can easily be replaced with
  Utils::same_block_device() when required.  Just need to make sure it's
  not missed anywhere.

  However those caches which use std::map with the path as the key are
  harder to handle.  The key of a std::map needs to be orderable; that
  is operator<() is implemented [1][2][3].

  There are several options for this:
  3.1) Stop using std::map altogether.  Instead use a vector and linear
       search to find entries using comparison method of choice.
  3.2) Write a class which implements operator<() for block device names
       which takes major, minor number into account and is passed as the
       3rd argument to std::map.
  3.3) Write a Block_Device class which represents name and major, minor
       and implements operator=() and operator<() allowing standard
       comparison and std::map to be use.
       => This is option (2).

[1] std::map
    http://www.cplusplus.com/reference/map/map/
[2] std::map::key_comp
    http://www.cplusplus.com/reference/map/map/key_comp/
[3] create an own comparator for map
http://stackoverflow.com/questions/5733254/create-an-own-comparator-for-map


No matter which option is chosen, there is going to be a lot of block
device comparison, even for option (1) of just canonicalising path
names.  Therefore another cache containing block device names, major &
minor numbers will almost certainly going to be needed.


I am inclined to use option (2) Block_Device class with major, minor
name comparison.
Pluses:
1) It makes a strong statement about intent that a class is used which
   implements custom (block device relevant) comparison using major,
   minor comparison;
2) It encapsulates things;
3) It is very obvious strings containing paths have been converted to
   Block_Device objects;
Minuses:
1) Seems it bit heavyweight;
2) More change.


So I've changed my mind from our earlier email chat after further
investigation.  Thoughts welcome.


thanks,
Mike

Hi Mike,

Your investigation appears to have flushed out a few issues.

One permutation that caught me by surprise was your example in comment #2 with three string paths for the same device:

  GParted is using this name:
    /dev/mapper/isw_bacdehihbd_MyArray0p2    blk 252, 2

  and LVM is reporting:
    /dev/disk/by-id/dm-name-isw_bacdehijbd_MyArray0p2  symlink to ->
    /dev/dm-2                                blk 252, 2

Because there are even more string options available
   e.g., /dev/disk/[by-id|by-label|by-path|by-uuid] 
in addition to the /dev/mapper/* and /dev/dm-#, the string comparison option gets quite messy.  Further from comment #3 it appears the btrfs show command gives different results depending on which string name is used -- this might be a separate issue but still makes things messy.

It seems that we need to be sure to use a descriptive string name so that users know which device/partition is being referred to, and further that we also need to use the appropriate string name for commands such as btrfs show.

From reading your comments, the three things that we appear to need are as follows:

  A)  Descriptive device/partition name string for user
  B)  Appropriate device/partition name for commands (e.g., btrfs show)
  C)  Comparison method that works works for many device/partitions

I think the complexity of these issues warrants a separate class to track block devices and to hide the details of the comparison method.

In simpler words, I agree that "option (2): Use class with major, minor comparison" may be the best path forward.

Thanks,
Curtis

Hi Curtis,

Thanks for the feedback.  Agreed, I'll implement option (2): Use class
with major, minor comparison.  I think it's going to take a little
while.


On the needed feature (B) appropriate device name for commands (e.g.
btrfs show) ...

Not yet sure if I want to add this.  The btrfs failure from comment #3
is also a proxy for another subtle issue found on Fedora 23, also
documented in bug 760080 comment 1, in which btrfs show does work with
alternate names.  Btrfs on CentOS 6 is ancient and unsupported.

However from the recent ZFS related partition identification question,
bug 766989, ZFS document recommends using /dev/disk/by-id/ names [1] so
it might become necessary to better support ZFS in the future.

[1] Ubuntu 16.04 Root on ZFS
    zpool create ... /dev/disk/by-id/...
https://github.com/zfsonlinux/zfs/wiki/Ubuntu%2016.04%20Root%20on%20ZFS


Thanks,
Mike

Example case (2) multi-btrfs on top of LUKS, on CentOS 7


Create btrfs on LUKS encrypted partition.

# cryptsetup luksFormat --force-password /dev/sda11
# cryptsetup luksOpen /dev/sda11 sda11_crypt
# ls -l /dev/mapper
total 0
crw-------. 1 root root 10, 236 Jun 27 10:30 control
lrwxrwxrwx. 1 root root       7 Jun 27 11:16 sda11_crypt -> ../dm-0

# mkfs.btrfs -L encrypted-btrfs /dev/mapper/sda11_crypt
# btrfs filesystem show /dev/mapper/sda11_crypt
Label: 'encrypted-btrfs'  uuid: 11cb152f-e6b5-4677-8947-09737a98268a
	Total devices 1 FS bytes used 28.00KiB
	devid    1 size 1022.00MiB used 12.00MiB path /dev/mapper/sda11_crypt

btrfs-progs v3.19.1


Mount the file system

# mount /dev/mapper/sda11_crypt /mnt/1
# fgrep btrfs /proc/mounts
/dev/mapper/sda11_crypt /mnt/1 btrfs rw,seclabel,relatime,ssd,space_cache 0 0


Add second device into the file system

# cryptsetup luksFormat --force-password /dev/sda12
# cryptsetup luksOpen /dev/sda12 sda12_crypt
# ls -l /dev/mapper
total 0
crw-------. 1 root root 10, 236 Jun 27 10:30 control
lrwxrwxrwx. 1 root root       7 Jun 27 11:30 sda11_crypt -> ../dm-0
lrwxrwxrwx. 1 root root       7 Jun 27 11:36 sda12_crypt -> ../dm-1

# btrfs device add /dev/mapper/sda12_crypt /mnt/1
# btrfs filesystem show /dev/mapper/sda12_crypt 
Label: 'encrypted-btrfs'  uuid: 11cb152f-e6b5-4677-8947-09737a98268a
	Total devices 2 FS bytes used 32.00KiB
	devid    1 size 1022.00MiB used 12.00MiB path /dev/mapper/sda11_crypt
	devid    2 size 1022.00MiB used 0.00B path /dev/mapper/sda12_crypt

btrfs-progs v3.19.1


Remove first and mounting device from the file system

# btrfs device delete /dev/mapper/sda11_crypt /mnt/1
# btrfs filesystem show /dev/mapper/sda12_crypt 
Label: 'encrypted-btrfs'  uuid: 11cb152f-e6b5-4677-8947-09737a98268a
	Total devices 1 FS bytes used 96.00KiB
	devid    2 size 1022.00MiB used 144.00MiB path /dev/mapper/sda12_crypt

btrfs-progs v3.19.1


Now the kernel reports the mounting device as /dev/dm-1 rather than the
canonical Device Mapper name of /dev/mapper/sda12_crypt.

# fgrep btrfs /proc/mounts
/dev/dm-1 /mnt/1 btrfs rw,seclabel,relatime,ssd,space_cache 0 0


This causes GParted to no longer be able to detect the file system is
mounted.  This is because GParted reads /proc/mounts to know which file
systems are mounted, but LUKS support in GParted is using the canonical
Device Mapper name /dev/mapper/sda12_crypt.

Hi Curtis,

Can you do some testing / investigation please.  Can you build GParted
with --enable-libparted-dmraid and test it on a partitioned DMRaid
array.  Particularly interested in cases where GParted breaks.

When I tried viewing a partitioned DMRaid array, creating a partition
and then deleting a partition, I find it works:

Distro             View      Create      Delete
------             -----     ------      ------
CentOS 5           Works      Works      Works
Ubuntu 12.04 LTS   Works      Works      Works
Debian 8           Works      Works      Works

I get breakages on Debian 6, but then that is a no longer supported
distribution.

NOTES:
1) I'm actually building GParted from my git HEAD with a stack of
   patches for this bug report.
2) I'm wondering if DMRaid.cc module can be greatly simplified to the
   equivalent of what is used with --enable-libparted-dmraid.  I.e. just
   reporting DMRaid array names without any calls to "dmraid" command to
   stop and start arrays, etc.

Thanks,
Mike

Created attachment 331156 [details] [review]
Block special comparison by major, minor number (Draft 1)

Hi Curtis,

Here is draft 1 of the patchset.  So far it:
1) Removes alternate paths;
2) Add BlockSpecial() objects into all the caches for matching devices
   by major, minor numbers instead of by name;
   except DMRaid ...
I've been trying to understand the DMRaid module enough to work out if
there are cases of device matching which is being performed by string
compare and should be changed to by major, minor number instead.  So far
I haven't been able to work that out. :-(

Help wanted,
Mike

Hi Mike,

My preliminary testing of patch D1 in comment #10 has gone well.  So far I have not observed any regressions.  There are some typos and grammatical errors in the commit comments, but I suspect you are not concerned about these at this time.

With respect to the DMRaid module, it should not require any changes as it should not require comparisons by major, minor number.  The module automatically creates and removes partition paths affected by GParted operations using it's own regular expressions on the /dev/mapper entries.  This should continue to work as expected with the caveat mentioned in this bug report, and has done so in my testing on kubuntu 16.04 both with and without --enable-libparted-dmraid.

Since I recently upgraded to a SSD with kubuntu 16.04, I no longer have my old Debian installation.  When I get around to re-installing Debian then I can test this patch set further.

Curtis

Hi Mike,

I installed Debian Testing (Stretch 9), installed the *dmraid* package and performed some testing.

On boot-up, the FAKE RAID is set up as */dev/mapper/isw_efjbbijhh_Vol0*.
The first partition has the entry      */dev/mapper/isw_efjbbijhh_Vol01*.
Note the missing "p" between the "0" and the "1".

When using patch D1 from comment #10, and gparted compiled with no configure options, the LVM2 PV and other partition types file system space usages worked correctly on FAKE RAID.

When using patch D1 from comment #10, and gparted compiled with --enable-libparted-dmraid, the LVM2 PV and other partition types file system space usages FAILED to work correctly on FAKE RAID.  This is to be expected because Debian Testing DMRAID does not create partitions with the same naming standard as libparted.

In conclusion, patch set D1 is an improvement for file system usage when used on Debian Testing both with and without FAKE RAID.

Thanks,
Curtis

Created attachment 331583 [details] [review]
Block special comparison by major, minor number (v1)

Hi Curtis,

Here's patchset v1.

It uses BlockSpecial objects in all the caches, except for DMRaid which
you identified as not requiring change, as per comment 11.  I think use
of BlockSpecial objects in the rest of the code, such as in Device and
Partition* objects is not required because they represent a view of
disks and partitions using preferred user disk names.  E.g. sd*, md*,
mapper/*.

It does:
1) Removes alternate paths from Device and Partition objects;
2) Adds BlockSpecial object usage into the various caches.
[This far is the minimum required features]
3) Add another cache, of major, minor numbers looked up via stat() into
   BlockSpecial class to reduce explicit calls to stat().  Also further
   reduces calls to stat() by pre-populating major, minor numbers found
   in the already being read /proc/partitions.
4) Various refactoring / tidyups including:
4.1) Use static class interfaces for remaining caches;
4.2) Move mount_info and fstab_info maps into separate module.

Thanks,
Mike

Hi Mike,

From reviewing this latest patch set (v1) I can see that this took
significant effort to develop.  Thank you for your work in this area
to improve GParted, and thank you for your patience.

Following are my preliminary testing results and code review comments.
Please feel free to question any of my statements.


PRELIMINARY TESTING RESULTS
---------------------------

I've discovered a problem on kubuntu 16.04 on my Intel FAKE RAID
whereby the usage information is not found for an LVM2 PV partition.

The problem occurs if gparted is compiled with
--enable-libparted-dmraid.  This seems strange to me because kubuntu
creates DMRAID device names in the same form that libparted uses in
this distro (e.g. isw_efjbbijhh_Vol0p6).

I suspect the problem is related to the fact that the /dev/mapper/isw*
device name is a block special file whereas the /dev/mapper/isw*
partition entries are symbolic links to ../dm-* block special files.


Information from my setup follows:

  - Kubuntu 16.04
  - dmraid package installed
  - mdadm  package not installed
  - LVM2 PV partition 6 on Intel FAKE RAID

$ ls -l /dev/mapper
total 0
crw------- 1 root root  10, 236 Aug  2 10:38 control
brw-rw---- 1 root disk 252,   0 Aug  2 10:38 isw_efjbbijhh_Vol0
lrwxrwxrwx 1 root root        7 Aug  2 10:38 isw_efjbbijhh_Vol0p1 -> ../dm-1
lrwxrwxrwx 1 root root        7 Aug  2 10:38 isw_efjbbijhh_Vol0p5 -> ../dm-2
lrwxrwxrwx 1 root root        7 Aug  2 10:38 isw_efjbbijhh_Vol0p6 -> ../dm-3
$ ls -l /dev/dm-*
brw-rw---- 1 root disk 252, 0 Aug  2 10:38 /dev/dm-0
brw-rw---- 1 root disk 252, 1 Aug  2 10:38 /dev/dm-1
brw-rw---- 1 root disk 252, 2 Aug  2 10:38 /dev/dm-2
brw-rw---- 1 root disk 252, 3 Aug  2 10:38 /dev/dm-3


The problem does not occur if gparted is compiled with no extra
configure options.  This is likely due to the fact that GParted
directly calls dmraid to create the /dev/mapper partition entries and
these would be block special files (not symbolic links).


I can investigate further if required.


CODE REVIEW COMMENTS
--------------------

Following are my comments from reviewing the code in patch set v1 from
comment # 13.


[01/23] Simplify Device object to a single path (#767842)

Confusing sentence.  Suggest change.

FROM:
GParted stored a list of paths for Device and Partition objects.  It
sorted this list [1][2] and treated the first specially as that it what
                                                           ^^^^^^^
get_path() returned and was used almost everywhere; with the file system
specific tools, looked up in various *_Info caches, etc.

TO:
GParted stored a list of paths for Device and Partition objects.  It
sorted this list [1][2] and treated the first specially as what
get_path() returned and was used almost everywhere; with the file system
specific tools, looked up in various *_Info caches, etc.


[02/23] Rename Device::add_path() to set_path() (#767842)
  - no change

[03/23] Simplify Partition object to a single path (#767842)
  - no change

[04/23] Update calibrate_partition() for single partition path (#767842)
  - no change

[05/23] Rename Partition::add_path() to set_path() (#767842)
  - no change

[06/23] Remove now unused Proc_Partitions_Info::get_alternate_paths() (#767842)
  - no change

[07/23] Create BlockSpecial class and use in LVM2_PV_Info (#767842)

Confusing sentence.  Suggest change.

FROM:
Implement class BlockSpecial which encapsulates the name and major,
minor numbers for a block special device.  Also performs comparison as
needed.  For bug 767842 comment 4 and 5 for further investigation and
         ^^^                 ^^^
decision for choosing to implement a class.

TO:
Implement class BlockSpecial which encapsulates the name and major,
minor numbers for a block special device.  Also performs comparison as
needed.  See bug 767842 comments 4 and 5 for further investigation and
decision for choosing to implement a class.


[08/23] Add BlockSpecial into mount_info and fstab_info (#767842)
  - no change

[09/23] Use BlockSpecial objects in btrfs member cache (#767842)

Extremely minor grammatical change.  I'm being way too picky.  :)

FROM:
There are no known errors which effect the remaining caches in GParted.
                                ^^^^^^
TO:
There are no known errors which affect the remaining caches in GParted.


[10/23] Overload is_dev_mounted() function (#767842)
  - no change

[11/23] Use BlockSpecial in LUKS_Info module cache (#767842)
  - no change

[12/23] Use BlockSpecial in SWRaid_Info module cache (#767842)
  - no change

[13/23] Remove use of retired vol_id from FS_Info module (#767842)

We should also remove vol_id from the following files:
  README
  src/hfs.cc
  src/hfsplus.cc


[14/23] Parse FS_Info cache into fields while loading (#767842)
  - no change

[15/23] Use BlockSpecial in FS_Info module cache (#767842)
  - no change

[16/23] Cache looked up major, minor numbers in BlockSpecial class (#767842)

Minor sentence problem.  Suggest change.

FROM:
Creation of every BlockSpecial object use to result in a stat() OS call.
                                      ^^^^
On one of my test VMs debugging with 4 disks and a few partitions on
each, GParted refresh generated 541 calls to stat() in the
BlockSpecial(name) constructor.  However there were only 45 unique
names.  So on average each name was stat()ed approximately 12 time.
                                                              ^^^^^
TO:
Creation of every BlockSpecial object used to result in a stat() OS call.
On one of my test VMs debugging with 4 disks and a few partitions on
each, GParted refresh generated 541 calls to stat() in the
BlockSpecial(name) constructor.  However there were only 45 unique
names.  So on average each name was stat()ed approximately 12 times.


[17/23] Pre-populate BlockSpecial cache while reading /proc/partitions (#767842)

Minor spelling mistake.  Suggest change:

FROM:
For plain disks (sd*) and Linux Software RAID arrays (md*) the kernel
name is the common users pace name too, therefore matches what GParted
                   ^^^^^^^^^^
uses and pre-populating does avoid calling stat() at all for these
names.

TO:
For plain disks (sd*) and Linux Software RAID arrays (md*) the kernel
name is the common userspace name too, therefore matches what GParted
uses and pre-populating does avoid calling stat() at all for these
names.


[18/23] Switch to a static interface for Proc_Partitions_Info
  - no change

[19/23] Simplify whole device matching in load_proc_partitions_Info_cache()
  - no change

[20/23] Switch to static class interface for FS_Info
  - no change

[21/23] Remove unimplemented prototype DMRaid::partition_is_mounted_by_path()
  - no change

[22/23] Split mount_info and fstab_info maps into separate Mount_Info module

Minor spelling mistake.  Suggest change.

FROM:
GParted_Core slighly.
             ^^^^^^^
TO:
GParted_Core slightly.


[23/23] Remove no longer needed includes from GParted_Core.cc
  - no change


Thanks,
Curtis

Hi Curtis,

I've tracked down the issue with LVM2 PV on top of DMRaid on
Ubuntu 16.04 LTS you reported.


My ISW array details:  (Not especially relevant)
# dmraid -r
/dev/sdc: isw, "isw_jjdaidaaj", GROUP, ok, 16777214 sectors, data@ 0
/dev/sdb: isw, "isw_jjdaidaaj", GROUP, ok, 16777214 sectors, data@ 0
# dmraid -n | head -3
/dev/sdc (isw):
0x000 sig: "  Intel Raid ISM Cfg Sig. 1.1.00"
...
0x138 isw_dev[0].volume: "        MyArray0"
...
/dev/sdb (isw):
0x000 sig: "  Intel Raid ISM Cfg Sig. 1.1.00"
...
# ls -l /dev/mapper
total 0
crw------- 1 root root  10, 236 Aug  3 15:21 control
brw-rw---- 1 root disk 252,   0 Aug  3 15:37 isw_jjdaidaaj_MyArray0
lrwxrwxrwx 1 root root        7 Aug  3 15:37 isw_jjdaidaaj_MyArray0p1 -> ../dm-1
lrwxrwxrwx 1 root root        7 Aug  3 15:37 isw_jjdaidaaj_MyArray0p2 -> ../dm-2
lrwxrwxrwx 1 root root        7 Aug  3 15:37 isw_jjdaidaaj_MyArray0p3 -> ../dm-3
# ls -l /dev/dm*
brw-rw---- 1 root disk 252, 0 Aug  3 15:21 /dev/dm-0
brw-rw---- 1 root disk 252, 1 Aug  3 15:37 /dev/dm-1
brw-rw---- 1 root disk 252, 2 Aug  3 15:37 /dev/dm-2
brw-rw---- 1 root disk 252, 3 Aug  3 15:37 /dev/dm-3


Create LVM2 PV on top of an ISW DMRaid array using GParted.
This runs command:
  lvm pvcreate -M 2 /dev/mapper/isw_jjdaidaaj_MyArray0p2

Refresh after applying this command reports the values correctly.


However manually querying the LVM2 PV details reports this:
(The important bit)
# lvm pvs
  PV         VG   Fmt  Attr PSize PFree
  /dev/sdc2       lvm2 ---  1.00g 1.00g
# blkid | fgrep LVM
/dev/sdb2: UUID="voKYhB-Dj81-ZVzc-w3m5-1uTB-Oaxd-O7JUbn" TYPE="LVM2_member" PARTUUID="6147c0f7-02"
/dev/sdc2: UUID="voKYhB-Dj81-ZVzc-w3m5-1uTB-Oaxd-O7JUbn" TYPE="LVM2_member" PARTUUID="6147c0f7-02"
/dev/mapper/isw_jjdaidaaj_MyArray0p2: UUID="voKYhB-Dj81-ZVzc-w3m5-1uTB-Oaxd-O7JUbn" TYPE="LVM2_member" PARTUUID="6147c0f7-02"

  Cause
  -----
  So the OS has block devices for /dev/mapper/isw*p2, /dev/sdb2 and
  /dev/sdc2 which because I am using mirroring all contain the same
  data.  LVM2 has decided that the PV is actually on /dev/sdc2 instead
  of /dev/mapper/isw*p2.

Refreshing GParted now gets a warning message and no usage details
against /dev/mapper/isw*p2 and /dev/sdb2.  And correct usage details
for /dev/sdc2.

  Previous case
  -------------
  This is the same cause as the failure I documented in
  bug 715191 comment 16.  Multiple separate block devices all pointing
  to / containing the same bytes and signatures.


The hard question is what to do about it.  Options are:
1) Nothing.  Having LVM2 on top of fake/BIOS RAID is rare and being
   broken by incorrectly configured distros.
2) Add filters to LVM2 configuration so that disks within fake/BIOS RAID
   arrays aren't examined for signatures.
3) Configure DMRaid some how to make it not create partition block
   devices for on disks within fake/BIOS RAID arrays.  (In this case
   don't create sdb[123] and sdc[123]).  I know that I have seen this
   happen on some distro when I was testing and seen a flag on the
   dmraid command.  But might involve adjusting udev rules.
4) Something else.

So far I am thinking options (1) do nothing.


Mike

Thank you Mike for investigating this issue further.

I too think that having LVM2 on top of fake/BIOS RAID is rare and would recommend against this practice.  If there were a simple solution then I'd go with that.  Since I don't see a simple solution I think our efforts are better spent elsewhere.

I agree with your suggestion to proceed with option (1) "do nothing".

Curtis

Created attachment 332748 [details] [review]
Block special comparison by major, minor number (v2)

Hi Curtis,

Here's patchset v2 with your requested corrections, unless noted here as
doing something different.

[01/24] Simplify Device object to a single path (#767842)
"sorted this list [1][2] and treated the first specially as that it what
 [changed to]                                                     *
"sorted this list [1][2] and treated the first specially as that is what

[13/24] Remove use of retired vol_id from FS_Info module (#767842)
  Created additional commit to remove vol_id from README and
  src/hfs*.cc.
  [18/24] Remove remaining use of retired vol_id

[17/24] Pre-populate BlockSpecial cache while reading /proc/partitions (#767842)
  "users pace" -> "user space"

[24/24] Remove no longer needed includes from GParted_Core.cc
  Removed less headers after identified use for these:
  #include <sys/types.h>  keep for size_t
  #include <stdlib.h>     keep for free(), malloc() and possibly others
  #include <unistd.h>     keep for sleep()
  Documented commit removing the need for the still removed headers.

Thanks,
Mike

Hi Mike,

Thank you for the detailed description of the recent changes.

Patch set v2 from comment #17 looks good to me.  It worked as expected in all my tests, with the minor exception of the rare situation of LVM2 on top of fake RAID which we already know about and is an issue in the current code base.

I successfully ran brief tests on:

debian    8.0  (Jessie)
fedora   23
kubuntu  16.04
opensuse 13.2

If there are no objections then I will commit patch set v2 to the git master tomorrow.

Curtis

Thank you Mike for your work to improve handling of partition
recognition and comparison using major and minor numbers.

Patch set v2 from comment #17 has been committed to the git master
branch for inclusion in the next release of GParted.

The relevant git commits can be viewed at the following links:

Simplify Device object to a single path (#767842)
https://git.gnome.org/browse/gparted/commit/?id=902afaa010489d748927da1d4e779018d932d439

Rename Device::add_path() to set_path() (#767842)
https://git.gnome.org/browse/gparted/commit/?id=1dc8a0c628ecc4585b519bc16a1f110703b77d31

Simplify Partition object to a single path (#767842)
https://git.gnome.org/browse/gparted/commit/?id=214255eda3160bdee2eab985451f9231b75cd783

Update calibrate_partition() for single partition path (#767842)
https://git.gnome.org/browse/gparted/commit/?id=54652b0d4e117ab76c7b4dcc79e57045379d0ce8

Rename Partition::add_path() to set_path() (#767842)
https://git.gnome.org/browse/gparted/commit/?id=40f39bdbe27cb390b86de7ef79c6b9f8aab5623e

Remove now unused Proc_Partitions_Info::get_alternate_paths() (#767842)
https://git.gnome.org/browse/gparted/commit/?id=0f4df8dfd171ccdf076f562c4371fa237525a674

Create BlockSpecial class and use in LVM2_PV_Info (#767842)
https://git.gnome.org/browse/gparted/commit/?id=ab2d4f5ee6a1e139df3f4340e4247954e0f2a6f2

Add BlockSpecial into mount_info and fstab_info (#767842)
https://git.gnome.org/browse/gparted/commit/?id=a800ca8b68d60251101ee27e569dfa2a97b67886

Use BlockSpecial objects in btrfs member cache (#767842)
https://git.gnome.org/browse/gparted/commit/?id=abee66484fb355a1bac5b03f361bab7435df867b

Overload is_dev_mounted() function (#767842)
https://git.gnome.org/browse/gparted/commit/?id=e4a8530b1428bbd991d4e5b626377efc287e12b3

Use BlockSpecial in LUKS_Info module cache (#767842)
https://git.gnome.org/browse/gparted/commit/?id=7cd574cac5f7d076ebd94795f5c6f41b333fdee7

Use BlockSpecial in SWRaid_Info module cache (#767842)
https://git.gnome.org/browse/gparted/commit/?id=2b013e494c1580c139eaa342be659e46d5d24077

Remove use of retired vol_id from FS_Info module (#767842)
https://git.gnome.org/browse/gparted/commit/?id=8aa34f7baaac5a70e8901275619f7587b2ed21b8

Parse FS_Info cache into fields while loading (#767842)
https://git.gnome.org/browse/gparted/commit/?id=23da3ab9a83979ae97aa634f60834930a1c56569

Use BlockSpecial in FS_Info module cache (#767842)
https://git.gnome.org/browse/gparted/commit/?id=ce8fb1dd91c045991d196fde300a419f00356c84

Cache looked up major, minor numbers in BlockSpecial class (#767842)
https://git.gnome.org/browse/gparted/commit/?id=003d6eff943b3ce37e90100f660eaceda8519421

Pre-populate BlockSpecial cache while reading /proc/partitions (#767842)
https://git.gnome.org/browse/gparted/commit/?id=571304d2c662f8c35e9529b71ae48ffe263c038f

Remove remaining use of retired vol_id
https://git.gnome.org/browse/gparted/commit/?id=91e5a0960eb57b9b877d47e070a0578de4a313bf

Switch to a static interface for Proc_Partitions_Info
https://git.gnome.org/browse/gparted/commit/?id=912766c2e11aab2cecd916bdc2027cc14087c4a0

Simplify whole device matching in load_proc_partitions_Info_cache()
https://git.gnome.org/browse/gparted/commit/?id=5055c0b638ca236ea9457eaf151c3bc54a872065

Switch to static class interface for FS_Info
https://git.gnome.org/browse/gparted/commit/?id=a94be2da05c65b36c290d993c89f5580e85042ca

Remove unimplemented prototype DMRaid::partition_is_mounted_by_path()
https://git.gnome.org/browse/gparted/commit/?id=0785c5d7ef044799de2c61000a3bcf0ae5ba72b2

Split mount_info and fstab_info maps into separate Mount_Info module
https://git.gnome.org/browse/gparted/commit/?id=63ec73dfdac70fde8fabb543d0a7dc5808e168d5

Remove no longer needed includes from GParted_Core.cc
https://git.gnome.org/browse/gparted/commit/?id=e6f4b1c30505b0d4f0ce4da7e258b325e2875d27

Curtis

This enhancement was included in the GParted 0.27.0 release on October 19, 2016.