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

Jump to bottom

homework: resize to maximum disk space if disk size is not specified #25066

Merged
merged 1 commit into from Jun 19, 2023
Merged

homework: resize to maximum disk space if disk size is not specified #25066

merged 1 commit into from Jun 19, 2023

Conversation

gibeom-gwon
Copy link
Contributor

If the backing storage is LUKS2 on a block device, auto resize mode is enabled, and disk size is not specified, resize the partition to the maximum expandable size.

Fixes: #22255, #23967

#23740 may also be related.

bluca
bluca reviewed Oct 19, 2022
View reviewed changes
src/home/homework-luks.c Outdated Show resolved Hide resolved
@bluca

This comment was marked as resolved.

Sign in to view
@bluca bluca added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR homed homed, homectl, pam_homed labels Oct 19, 2022
@bluca bluca removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Oct 19, 2022
@bluca bluca requested a review from poettering October 20, 2022 18:11
@gibeom-gwon
Copy link
Contributor Author

Removed the dead code. Round up is not performed.

poettering
poettering reviewed Oct 24, 2022
View reviewed changes
src/home/homework-luks.c
int r;

assert(fd >= 0);
assert(p);
Copy link
Member

Choose a reason for hiding this comment

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

so this is actually not correct to assume. if a naked block device is used (i.e. no partition table) this will be NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added partition != NULL condition. This will make it the same as before if it's a naked block device.

poettering
poettering reviewed Oct 24, 2022
View reviewed changes
src/home/homework-luks.c

last_lba = fdisk_get_last_lba(c); /* One sector before boundary where usable space ends */
assert(last_lba < UINT64_MAX/512);
end = DISK_SIZE_ROUND_DOWN((last_lba + 1) * 512); /* Round down to multiple of 4K */
Copy link
Member

Choose a reason for hiding this comment

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

so, this is problematic if there are partitions behind this one. I guess to to be correct we'd have to iterate through all partitions on the disk, and look for the one whose start offset is smallest of those larger than our partition. And only if no such partition exist use the last LBA value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

systemd/src/home/homework-luks.c

Lines 2713 to 2715 in 4856f63

} else if (fdisk_partition_get_end(p) > partition_offset / 512U)
return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Can't extend, not last partition in image.");
}

prepare_resize_partition() returns error if the partition is not the last. Therefore, this ensures that no more partitions exist.

@poettering
Copy link
Member

mm, so how come auto-resize is on on such block-device backed home dirs?
it appears to me we shouldn#t enable it on them.

@gibeom-gwon
Copy link
Contributor Author

mm, so how come auto-resize is on on such block-device backed home dirs? it appears to me we shouldn#t enable it on them.

systemd/src/shared/user-record.c

Line 1995 in 4856f63

return user_record_storage(h) == USER_LUKS ? AUTO_RESIZE_SHRINK_AND_GROW : AUTO_RESIZE_OFF;

How about set auto_resize_mode = AUTO_RESIZE_OFF when homectl create if block device used? This will keep the settings of the home already created. But I'm not sure where to set it, homectl or homed.

@keszybz keszybz added the please-review PR is ready for (re-)review by a maintainer label Dec 8, 2022
@keszybz keszybz added please-review PR is ready for (re-)review by a maintainer and removed please-review PR is ready for (re-)review by a maintainer labels Jan 25, 2023
@mrc0mmand mrc0mmand mentioned this pull request May 30, 2023
Closed
@mrc0mmand mrc0mmand linked an issue May 30, 2023 that may be closed by this pull request
Systemd-homed fails since v250 #23740
Closed
@mrc0mmand mrc0mmand mentioned this pull request May 30, 2023
Closed
@mrc0mmand mrc0mmand linked an issue May 30, 2023 that may be closed by this pull request
Auto-resize of a LUKS + btrfs homed partition doesn't work #23967
Closed
@mrc0mmand mrc0mmand added this to the v254 milestone May 30, 2023
@mrc0mmand
Copy link
Member

Setting the milestone, since two of the three issues this PR should address are in the milestone as well.

poettering
poettering reviewed Jun 13, 2023
View reviewed changes
src/home/homework-luks.c
last_lba = fdisk_get_last_lba(c); /* One sector before boundary where usable space ends */
assert(last_lba < UINT64_MAX/512);
end = DISK_SIZE_ROUND_DOWN((last_lba + 1) * 512); /* Round down to multiple of 4K */

Copy link
Member

Choose a reason for hiding this comment

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

before we subtract start from end we shuld really check for overflows, and generate some error

Copy link
Member

Choose a reason for hiding this comment

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

i.e.

if (start > end)
        return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Last LBA is before partition end.");

or so

poettering
poettering requested changes Jun 13, 2023
View reviewed changes
Copy link
Member

@poettering poettering left a comment

Choose a reason for hiding this comment

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

sorry for dropping the ball on this one, and being so slow with the review.

looks pretty good, some minor comment.s

src/home/homework-luks.c
last_lba = fdisk_get_last_lba(c); /* One sector before boundary where usable space ends */
assert(last_lba < UINT64_MAX/512);
end = DISK_SIZE_ROUND_DOWN((last_lba + 1) * 512); /* Round down to multiple of 4K */

Copy link
Member

Choose a reason for hiding this comment

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

i.e.

if (start > end)
        return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Last LBA is before partition end.");

or so

src/home/homework-luks.c Outdated Show resolved Hide resolved
src/home/homework-luks.c Outdated
assert(last_lba < UINT64_MAX/512);
end = DISK_SIZE_ROUND_DOWN((last_lba + 1) * 512); /* Round down to multiple of 4K */

*ret_maximum_partition_size = end - start;
Copy link
Member

Choose a reason for hiding this comment

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

maybe also apply DISK_SIZE_ROUND_DOWN() just in case

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jun 13, 2023
@gibeom-gwon
homework: resize to maximum disk space if disk size is not specified
17a8346 
If the backing storage is LUKS2 on a block device, auto resize mode
is enabled, and disk size is not specified, resize the partition to
the maximum expandable size.

Fixes: systemd#22255, systemd#23967
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jun 18, 2023
@poettering poettering merged commit 5bfc4de into systemd:main Jun 19, 2023
47 of 48 checks passed
@github-actions github-actions bot removed the please-review PR is ready for (re-)review by a maintainer label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluca bluca bluca left review comments

@poettering poettering poettering requested changes

Assignees
No one assigned
Labels
homed homed, homectl, pam_homed
Milestone
v254
5 participants
@gibeom-gwon @bluca @poettering @mrc0mmand @keszybz