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.

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

Superfluous use of partprobe #2211

Closed
codefiles opened this issue Nov 11, 2023 · 5 comments · Fixed by #2212
Closed

Superfluous use of partprobe #2211

codefiles opened this issue Nov 11, 2023 · 5 comments · Fixed by #2212

Comments

@codefiles
Copy link
Contributor

Pertinent partprobe manual information: [1]

partprobe is a program that informs the operating system kernel of partition table changes.

From pyparted, Disk.commit() also "informs the operating system [kernel] of the changes" to a partition table [2] therefore calling partprobe after DeviceHandler._perform_partitioning() is unnecessary.

disk.deletePartition(part_info.partition)
disk.commit()

disk.addPartition(partition=partition, constraint=disk.device.optimalAlignedConstraint)
disk.commit()

for part_mod in filtered_part:
# if the entire disk got nuked then we don't have to delete
# any existing partitions anymore because they're all gone already
requires_delete = modification.wipe is False
self._perform_partitioning(part_mod, modification.device, disk, requires_delete=requires_delete)
self.partprobe(modification.device.device_info.path)

A partprobe that is performed after formatting the partitions is unnecessary because formatting the partitions does not change the partition table.

if enc_conf is not None and part_mod in enc_conf.partitions:
self._perform_enc_formatting(
part_mod.safe_dev_path,
part_mod.mapper_name,
part_mod.safe_fs_type,
enc_conf
)
else:
self._perform_formatting(part_mod.safe_fs_type, part_mod.safe_dev_path)
lsblk_info = self._fetch_part_info(part_mod.safe_dev_path)

def _fetch_part_info(self, path: Path) -> LsblkInfo:
attempts = 3
lsblk_info: Optional[LsblkInfo] = None
self.partprobe(path)
for attempt_nr in range(attempts):
time.sleep(attempt_nr + 1)
lsblk_info = get_lsblk_info(path)
if lsblk_info.partn and lsblk_info.partuuid and lsblk_info.uuid:
break
self.partprobe(path)

Encrypting a partition does not change the partition table.

disk.device_handler.partprobe(self.luks_dev_path)

[1] https://man.archlinux.org/man/partprobe.8
[2] https://github.com/dcantrell/pyparted/blob/d5870bb7a5d512c17c92a4e4fe7e9a8e7c8c3528/src/parted/disk.py#L202-L205

@svartkanin
Copy link
Collaborator

Hmm I remember there was a reason we put the partprobe in place after partitioning, I can't fully remember now but I think there was an issue without it not loading the lsblk info correctly

@codefiles
Copy link
Contributor Author

Regardless of whatever the intent was in the past, no evidence has been provided to justify any of the uses of partprobe outlined in the issue.

@qlyoung
Copy link

qlyoung commented Dec 5, 2023

The patch for this issue reintroduced #1759. See #1759 (comment)

@codefiles
Copy link
Contributor Author

I doubt that but I could be wrong, logs would help. This type of issue appears to occur when partitions get created with invalid disk geometry (see #2210). I discovered another issue that will cause problems with partitioning and will raise an issue in a moment.

@qlyoung
Copy link

qlyoung commented Dec 5, 2023

I doubt that but I could be wrong

I readded a single call to partprobe() as part of the retry logic after calling lsblk fails and it worked - precisely the same as the original patch that resolved #1759.

logs would help

Getting logs for you would be rather inconvenient.

This type of issue appears to occur when partitions get created with invalid disk geometry (see #2210).

My geometry seems valid enough since the system installed with archinstall after my workaround subsequently booted and works perfectly. I checked manually and it seems fine. It's nothing complicated, just an ESP and a primary.

If I submit my patch, will you nack it? Just trying to gauge up front how much effort it's going to take for me to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants