#1885 PR `merged`: #1884 - Partition information recorded is unexpected when disk has 4K block size¶

Labels: bug, cleanup, fixed / solved / done

rmetrich opened issue at 2018-08-01 08:50:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Please fill in the following items before submitting a new pull request:

Pull Request Details:¶

Type: Bug Fix
Impact: Normal
Reference to related issue (URL): https://github.com/rear/rear/issues/1884
How was this pull request tested? Tested on AIX/LPAR/PPC64le
Brief description of the changes in this pull request:
- Use blockdev to retrieve the size of the disk and block size (unused)
- Compute partition start using 512 bytes blocks (this is hardcoded in the Linux kernel)

pcahyna commented at 2018-08-02 18:39:¶

@rmetrich are you sure that other users of get_block_size() actually want the logical block size and not simply 512 bytes? I suspect it might be more correct to simply make it return 512 always. E.g. in usr/share/rear/layout/save/default/445_guess_bootloader.sh:

    # Get all strings in the first 512*4=2048 bytes on the disk:
    bootloader_area_strings_file="$TMP_DIR/bootloader_area_strings"
    block_size=$( get_block_size ${disk_device##*/} )
    dd if=$disk_device bs=$block_size count=4 | strings >$bootloader_area_strings_file

Not sure about usr/share/rear/layout/prepare/GNU/Linux/100_include_partition_code.sh. There it is used e.g. in

            block_size=$( get_block_size "$sysfs_name" )
            device_size=$( get_disk_size  "$sysfs_name" )

            ### GPT disks need 33 LBA blocks at the end of the disk
            # For the SUSE specific gpt_sync_mbr partitioning scheme
            # see https://github.com/rear/rear/issues/544
            if [[ "$label" == "gpt" || "$label" == "gpt_sync_mbr" ]] ; then
                device_size=$( mathlib_calculate "$device_size - 33*$block_size" )

rmetrich commented at 2018-08-03 07:18:¶

@pcahyna I don't know, that's why I left the code unchanged.
If get_block_size is always supposed to be set to 512 on Linux, then we can just get rid off that function.

jsmeix commented at 2018-08-03 07:57:¶

Regarding usr/share/rear/layout/save/default/445_guess_bootloader.sh

It should not matter if it gets the strings in the first 512 * 4 = 2048 bytes on the disk
or if it gets the strings in the first 4096 * 4 = 16384 bytes on the disk because
it is just some reasonable looking limit what the "first bytes on the disk"
should be that are examined to guess the used bootloader.
If that "first bytes on the disk" are 16384 nothing should go wrong.
Furthermore this part of the code is already where ReaR guesses
which means this code can never work 100% right and it is not meant
to work 100% right. If ReaR's guess is wrong the user must explicitly
specify his actually used bootloader via the BOOTLOADER variable.

Regarding usr/share/rear/layout/prepare/GNU/Linux/100_include_partition_code.sh

I am really not at all an expert in GPT partitioning but I would assume
nothing goes wrong if more space is left at the end of the disk
(i.e. 33 * 4096 = 135168 with 4K block size instead of usually 16896).
I think all what happens is that there is a bit more possibly unused
space at the end of the disk.
The 33 * 512 space at the end of the disk is for the "Secondary GPT", see
https://en.wikipedia.org/wiki/GUID_Partition_Table
but I think a GPT is not hard limited to 33 * 512 = 16896 bytes.
I think 33 * 512 = 16896 bytes is only the default space that should be reserved
as minimum for the GPT because
https://en.wikipedia.org/wiki/GUID_Partition_Table
reads

The UEFI specification stipulates that a minimum of 16,384 bytes,
regardless of sector size, be allocated for the Partition Entry Array.

where 16384 = 128 * 128 which matches 128 GPT partition entries.
But as far as I know GPT is not hard limited to 128 partitions.
I think GPT would support more than 128 partitions.

By the way:
Currently ReaR fails badly if there are more than 128 partitions,
see usr/share/rear/lib/layout-functions.sh

get_partition_number() {
...
    # Catch if $number is too big, report it as a bug
    (( $number <= 128 ))
    StopIfError "Partition $partition is numbered '$number'. More than 128 partitions is not supported."

I know I know - the whole layout code gets older and older
and needs more and more to be completely overhauled,
cf. My "vague private todo list" in
https://github.com/rear/rear/issues/791#issuecomment-406513969

jsmeix commented at 2018-08-06 10:14:¶

@schabrolles
could you have a look here an review it?
If you don't have time I would "just merge" it tomorrow
because it was tested by @rmetrich

schabrolles commented at 2018-08-06 10:24:¶

I don't have 4K block disk available (SAN). but I can run a simple backup/restore on several system to validate the current code is still working with 512.

jsmeix commented at 2018-08-06 11:04:¶

@schabrolles
a test that there are no regressions on your systems
would be much appreciated - many thanks in advance!

schabrolles commented at 2018-08-06 11:22:¶

I got an error when trying to recover. I got a dvd with no-image on my VM (like a lot of people) and I got an error when trying to read the size of the DVD (with no ISO)... => recovery aborted.

Comparing disks
ERROR: Reading the size of disk sr0 failed
Some latest log messages since the last called script 250_compare_disks.sh:
  2018-08-06 13:14:40.594453611 Including layout/prepare/default/250_compare_disks.sh
  2018-08-06 13:14:40.596503777 Comparing disks
  blockdev: cannot open /dev/sr0: No medium found
Aborting due to an error, check /var/log/rear/rear-rear-rhel-142.log for details
Exiting rear recover (PID 2979) and its descendant processes
Running exit tasks
Terminated

pcahyna commented at 2018-08-06 11:43:¶

Is it true that recovery on a non-512B-sector disk has been completely non-working until now, so there is no risk of regressions even if the code is not 100% correct?

pcahyna commented at 2018-08-06 11:47:¶

dvd with no-image

What does no-image mean? I would like to reproduce the problem (which does not seem to be directly related to the PR, does it?).

jsmeix commented at 2018-08-06 11:59:¶

@pcahyna
the failure in https://github.com/rear/rear/pull/1885#issuecomment-410676283
is related to the changes in this pull request because
layout/prepare/default/250_compare_disks.sh
calls the get_disk_size function which is changed by this pull request that fails now at

    if has_binary blockdev; then
        blockdev --getsize64 /dev/${disk_name##*/}  # sda/sda1 -> sda1 ; sda -> sda
        StopIfError "Reading the size of disk ${disk_name##*/} failed"
        return
    fi

cf. https://github.com/rear/rear/pull/1885/files

rmetrich commented at 2018-08-06 12:31:¶

Thanks for pointing the issue. I don't know either what is no-image, but for sure blockdev /dev/sr0 fails because there is no disk inserted.
So we need to do fall-through to old method instead.

rmetrich commented at 2018-08-06 12:33:¶

Still, I'm wondering what is the reason for getting size of /dev/sr0 if there is no disk. Very likely 0 should be returned instead of 2GB.

jsmeix commented at 2018-08-06 12:40:¶

@rmetrich
the reason for getting size of /dev/sr0 even if there is no disk is that code in
usr/share/rear/layout/prepare/default/250_compare_disks.sh

    for current_device_path in /sys/block/* ; do
        # Continue with next block device if the device is a multipath device slave
        is_multipath_path ${current_device_path#/sys/block/} && continue
        # Continue with next block device if the current one has no queue directory:
        test -d $current_device_path/queue || continue
        # Continue with next block device if no size can be read for the current one:
        test -r $current_device_path/size || continue
        current_disk_name="${current_device_path#/sys/block/}"
        current_size=$( get_disk_size $current_disk_name )
        test "$current_size" -gt '0' && replacement_hardware_disk_sizes=( "${replacement_hardware_disk_sizes[@]}" "$current_size" )
    done

For this particular use case it would be right when the get_disk_size function
results 0 as fallback value when blockdev fails - but I don't know if this
fallback value is the right one also for all other get_disk_size function calls.

jsmeix commented at 2018-08-06 12:52:¶

I think I can reproduce it on a KVM/QEMU virtual machine
with a second virtual IDE CDROM drive where no medium is
(i.e. where no ISO image is connected to) because there I get:

# blockdev --getsize64 /dev/sr1
blockdev: cannot open /dev/sr1: No medium found

# cat /sys/block/sr1/size
2097151

# echo '2097151 * 512' | bc -l
1073741312

in contrast to /dev/sr0 with a medium/ISO image

# blockdev --getsize64 /dev/sr0
69345280

# cat /sys/block/sr0/size
135440

# echo '135440 * 512' | bc -l
69345280

schabrolles commented at 2018-08-06 13:01:¶

@jsmeix , that's it ! https://github.com/rear/rear/pull/1885#pullrequestreview-143579905.
When I got the error when the virtual dvd is empty (no ISO image attached).

jsmeix commented at 2018-08-06 13:11:¶

@rmetrich
I propose to change the blockdev calls to

    if has_binary blockdev ; then
        # sda/sda1 -> sda1 ; sda -> sda
        blockdev --getsize64 /dev/${disk_name##*/} && return
    fi

and

    if has_binary blockdev ; then
        blockdev --getss /dev/$disk_name && return
    fi

i.e. a simple try and fall through if it fails.
A possible blockdev error message appears automatically in the log.

rmetrich commented at 2018-08-06 13:19:¶

+1, submitted

jsmeix commented at 2018-08-06 13:25:¶

@schabrolles
could you re-test if now the failure is avoided on your systems?

By the way:
This shows how important it is that various different people test something
because I would never ever had the idea to set up a testing KVM system
with an additional CDROM where no ISO is attached to...

pcahyna commented at 2018-08-06 13:28:¶

Still, I'm wondering what is the reason for getting size of /dev/sr0 if there is no disk. Very likely 0 should be returned instead of 2GB.

Why? No disk is something different than a disk with zero size.

The question rather is, why does ReaR need to know the size of the DVD?

jsmeix commented at 2018-08-06 13:59:¶

That get_disk_size function seems to be an endless cause of pain
because I found https://github.com/rear/rear/issues/1370
which was avoided by @gozora at that time by adding
the retry_command function so that in

retry_command test -r /sys/block/$disk_name/size || Error "..."

it retries 5 times and waits 1 second between each retry
if /sys/block/$disk_name/size appears.

But now if blockdev is available we just run it once and fall through if it fails
without any waiting if something what it needs may become available a bit later.

This may lead to a wrong disk size output when blockdev would output
the right disk size but fails because something what it needs is not yet available
and the old fall-through code results a wrong disk size output.

Therefore I wonder if that

retry_command test -r /sys/block/$disk_name/size || Error "..."

waiting should not be better also before the blockdev call like (excerpt):

    retry_command test -r /sys/block/$disk_name/size || Error "Could not determine size of disk $disk_name"

    if has_binary blockdev; then
        blockdev --getsize64 /dev/$disk_name && return
    fi

    # Linux always considers sectors to be 512 bytes long. See the note in the
    # kernel source, specifically, include/linux/types.h regarding the sector_t
    # type for details.
    local block_size=512
    local nr_blocks=$( < /sys/block/$disk_name/size)
    local disk_size=$(( nr_blocks * block_size ))

rmetrich commented at 2018-08-06 14:04:¶

Hmm, we miss the real root cause here. I suspect this is due to some partprobe just before that and it took some time for udev to create the device names.
Also found such issue when loading the usb-storage module: it took some seconds for device nodes to show up.

jsmeix commented at 2018-08-06 14:23:¶

Good grief!

I see now that we have had already "much fun" in 2017
with that little piece of code and how to avoid issues with it,
see https://github.com/rear/rear/pull/1418

It seems up to now I had successfully managed to forget about it
but now it seems that little monster touches me again - how scaring!

@rmetrich
if you are really brave you may dare to read through
https://github.com/rear/rear/pull/1418
It even contains partprobe ;-)

In general about waiting for device like thingies see
https://github.com/rear/rear/issues/791

I know - I know - the whole layout code gets older and older
and needs more and more to be completely overhauled, cf.
https://github.com/rear/rear/pull/1885#issuecomment-410176541

schabrolles commented at 2018-08-06 14:27:¶

@jsmeix tested with modification provided by @rmetrich .... it is working now :-)

jsmeix commented at 2018-08-06 14:46:¶

The latest code also works for me on my test system
as in https://github.com/rear/rear/pull/1885#issuecomment-410697398
with a second virtual IDE CDROM drive where no medium/ISO is.
FYI:
During "rear -D recover" I get in the log (excerpts)

+ source /usr/share/rear/layout/prepare/default/250_compare_disks.sh
...
+++ blockdev --getsize64 /dev/sr0
+++ return
++ current_size=69390336
...
+++ blockdev --getsize64 /dev/sr1
blockdev: cannot open /dev/sr1: No medium found
+++ local block_size=512
+++ retry_command test -r /sys/block/sr1/size
+++ local retry=0
++++ eval test -r /sys/block/sr1/size
+++++ test -r /sys/block/sr1/size
+++ command_stdout=
+++ echo -n ''
+++ local nr_blocks=2097151
+++ local disk_size=1073741312
+++ echo 1073741312
++ current_size=1073741312

The size of /sev/sr0 is a bit different here compared to
https://github.com/rear/rear/pull/1885#issuecomment-410697398
because there is a new ReaR recovery system ISO with the latest code.

jsmeix commented at 2018-08-07 10:13:¶

I will merge it soon today unless there are objections right now
because it is currently a good step forward and it should be
sufficiently backward compatible so that further regressions
should be (hopefully) mostly avoided.

jsmeix commented at 2018-08-07 10:32:¶

@rmetrich
via https://github.com/rear/rear/pull/1885/commits/6e390a0b269ce471997d6d6713343560e3e86898
I dared to add explanatory comments directly to your code here
so that at any time later it is still clear from plain reading the code
why the code is as it is even for users who cannot compare or reproduce
how the code behaves under different conditions on different systems
cf. https://github.com/rear/rear/wiki/Coding-Style

rmetrich commented at 2018-08-07 11:22:¶

@jsmeix Thanks a lot!

jsmeix commented at 2018-08-07 11:22:¶

@rmetrich @schabrolles
many thanks for all your work here!

[Export of Github issue for rear/rear.]

#1885 PR merged: #1884 - Partition information recorded is unexpected when disk has 4K block size¶