Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PARTITION_STYLE="msdos" broken on storage with 4k blocks #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

PARTITION_STYLE="msdos" broken on storage with 4k blocks #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions common.sh.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,9 @@ format_disk0() {
if sfdisk --help | fgrep -q -- '--no-reread'; then
ARGS="--no-reread $ARGS"
fi

ARGS="--label dos $ARGS"
sfdisk $ARGS --quiet "$1" <<EOF
${PARTITION_ALIGNMENT},,L,*
1M,,L,*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sfdisk man page says:

The default start offset for the first partition  is  1MiB.

What do you think about leaving the value entirely out? And when/if sfdisk defaults change again, this code won't need change too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If sfdisk changes default, we would need to change at last re-paritition offset (2048), so I would prefer to have exact values here to be on safe side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. I'm talking about removing the value, i.e. changing this into:

sfdisk $ARGS --quiet "$1" <<EOF
,,,L,*

Which makes sfdisk enter its value. We wouldn't have any PARTITION_OFFSET value anymore.

EOF
}

Expand All @@ -127,6 +127,18 @@ map_disk0() {

unmap_disk0() {
kpartx -d -p- $1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, why do you reformat the disk in the umap function? This should just unmap any partitions, but not touch the disk.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that it's right place. I don't want to touch disk until kernel has view of partitions via kpartx, so I opted to put it in unmap. I considered pushing it into CLEANUP, but losetup puts losetup -d in CLEANUP in create and using CLEANUP would involve modifying create also, so it seemed to me like cleaner and smaller patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, sorry to ask, but do you understand how the sequence of actions happen here? In unmap the OS is already installed. Basically this happens:

  1. format disk
  2. map partitions
  3. install OS
  4. run cleanup, which includes unmap.

Why do you want to repartition in cleanup/unmap?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This re-partition after host has finished with filesystem is the fix for 4k problems.

4k drives need 256 as aligment to create first partition at 1M which turns info offset 2048 with 512b blocks which makes them work in kvm.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a bit of documentation about it at https://github.com/ffzg/gnt-info/blob/master/doc/deboostrap-4k-msdos.txt

Does this explains it better then my attempts so far?

if sfdisk --help | fgrep -q -- '--no-reread'; then
ARGS="--no-reread $ARGS"
fi

# loopback device has correct 512b blocks
blockdev=$(losetup --show -f $1)
sfdisk $ARGS --delete "$blockdev"
sfdisk $ARGS --label dos --quiet $blockdev <<EOF
2048,,L,*
EOF
losetup -d $blockdev
}

cleanup() {
Expand Down Expand Up @@ -161,7 +173,6 @@ fi
: ${VARIANTS_DIR:="@sysconfdir@/ganeti/instance-debootstrap/variants"}
: ${GENERATE_CACHE:="yes"}
: ${CLEAN_CACHE:="14"} # number of days to keep a cache file
: ${PARTITION_ALIGNMENT:=2048} # sectors to align partition (1Mib default)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you could just change here PARTITION_ALIGNMENT to 1M, and not need any changes - plus it'd remain configurable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though about this, but in my setup, I had PARTITION_ALIGMENT set to 2048, so we would also need to somehow update existing configuration or convert number (2048) to 1M (or any value which user specified). This seems to me more fragile, and since nobody noticed that it doesn't work, my assumption is that it's not heavily used, and having only one option to configure instead of two seems like simpler solution. Other than rbd which has 4Mb logical IO, I can't think of other storage which would create optimal layout with hard-coded 1M value, and to be honest, I prefer to have partition at 1M offset even on rbd.

Having said all this, I'm willing to write conversion from number to xM notation and keep PARTITION_ALIGMENT in code if you prefer that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal is to make PARTITION_ALIGNMENT default to (string value) "1m", instead of hardcoding it as 1m in the sfdisk invocation. Why would it need conversion at all?

if [ -z "$OS_API_VERSION" -o "$OS_API_VERSION" = "5" ]; then
DEFAULT_PARTITION_STYLE="none"
else
Expand Down
6 changes: 0 additions & 6 deletions defaults
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,3 @@ CLEAN_CACHE="14"
# Ganeti 1.2 (os api version 5)
# PARTITION_STYLE="none"

# PARTITION_ALIGNMENT: the alignment of the partitions in sectors
# (512B); this defaults to 1MiB to give grub enough space for
# embedding and for better alignment with modern media (HDDs and
# SSDs), feel free to increase it if your media has even bigger
# allocation blocks
# PARTITION_ALIGNMENT=2048