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

Wrong properties after udev change event and missing entries in udev #425

Closed
pothos opened this issue Oct 16, 2017 · 12 comments
Closed

Wrong properties after udev change event and missing entries in udev #425

pothos opened this issue Oct 16, 2017 · 12 comments

Comments

@pothos
Copy link
Contributor

pothos commented Oct 16, 2017

Most ID_PART_* udev entries for an extended partition (i.e. the container itself) are lost when a new primary partition is created. But because UDisks still wants to set all values anew, it falls back to sysfs entries for only partition number, offset and size. An extended partition will therefore always have the IsContainer set to false (it is not queried in sysfs ) and Size set to 0 because that is the correct entry in sysfs. All contained partitons will therefore disappear in GNOME Disks because there is no container for them.

This behavior can be triggered by creating an empty extended partition and then an empty primary partition through UDisks. I could not get the same result by adding the primary partition with parted on the command line.

Maybe this can be forwarded to udev since the information loss happens there, but it could be that the way udev events are triggered is not allowed.
Initially the extended partiton udevadm info -q all -n /dev/loop0p2 has all ID_PART_* entries (_SIZE, _TYPE etc). But afterwards only ID_PART_TABLE_TYPE and ID_PART_TABLE_UUID remain.

@chinf
Copy link

chinf commented Oct 20, 2018

All contained partitons will therefore disappear in GNOME Disks because there is no container for them.

Relevant downstream bug: https://bugs.launchpad.net/ubuntu/+source/gnome-disk-utility/+bug/1641308

@chinf
Copy link

chinf commented Oct 20, 2018

This behavior can be triggered by creating an empty extended partition and then an empty primary partition through UDisks

Another trigger is just running GParted.

So udev is providing udisks2 with ID_PART_ENTRY_* for the extended partition, but it in turn is getting these from libblkid. If I run sudo blkid -po udev /dev/sda2 in my test VM I get the correct entries:

ID_PART_TABLE_TYPE=dos
ID_PART_ENTRY_SCHEME=dos
ID_PART_ENTRY_UUID=6511ebbf-02
ID_PART_ENTRY_TYPE=0x5
ID_PART_ENTRY_NUMBER=2
ID_PART_ENTRY_OFFSET=16779262
ID_PART_ENTRY_SIZE=4190210
ID_PART_ENTRY_DISK=8:0

...but after triggering a udev change event (by starting GParted) I get:
blkid: error: /dev/sda2: No such device or address

The problem may not be with udev, but in libblkid or beyond.

@vojtechtrefny
Copy link
Member

I was able to reproduce this on Ubuntu 18.04 , but not on Fedora 28 (these have same versions of libblockdev and udisks). I didn't try to reproduce this with GParted (I currently don't have Ubuntu VM with GUI), but if this also happens with GParted, it probably isn't a udisks related bug. On the other hand, this doesn't happen with parted and I also wasn't able to reproduce this with just libblockdev (manually without UDisks), so it looks like UDisks might be at least part of the problem (possibly scanning the disk/partition in a "wrong" time). The error message suggest that something opened the device with an exclusive lock and udev/blkid can't read the device because of that, but according to lsof there is no process holding the device. And stopping udisks doesn't help (stopping the service or killing the process should close all opened fds).

tl;dr I have no idea if udisks is part of the problem and I'll try to test this more.

I'll also try to find some better fallback for the size and partition type for extended partitions to properly set the IsContainer property. (But there is no such information in sysfs, so it might not be possible.)

@chinf
Copy link

chinf commented Oct 28, 2018

GParted isn't necessary to reproduce (and in case I was misunderstood, GParted isn't affected by the bug - it is only a trigger). Here is another test case (using a Ubuntu 18.04 desktop VM):

  1. Create & attach a new disk to the VM
  2. Run gnome-disk-utility
  3. Make MBR partition table on the new disk
  4. Create new extended partition (other settings left default)
  5. In a terminal, observe the output of sudo blkid -po udev /dev/sdb1 - this should be correct as per my comment above with "ID_PART_ENTRY_TYPE=0x5" etc.
  6. In gnome-disk-utility, create a new logical partition of any size
  7. Observe that the "volumes" table updates reflecting the bad data returned via udisks
  8. In the terminal, observe the new output of sudo blkid -po udev /dev/sdb1: "No such device or address"

On a 18.04 server VM, adapting the method above to use parted gives a different result (there are no problems with blkid). I see that parted results in a different TYPE in the output for blkid on the extended partition - 0xf rather than 0x5:

ID_PART_TABLE_TYPE=dos
ID_PART_ENTRY_SCHEME=dos
ID_PART_ENTRY_UUID=622f30d5-01
ID_PART_ENTRY_TYPE=0xf
ID_PART_ENTRY_NUMBER=1
ID_PART_ENTRY_OFFSET=2048
ID_PART_ENTRY_SIZE=7811072
ID_PART_ENTRY_DISK=8:16

[edit: ...these cases are consistent with pothos' original comment]

Is gnome-disk-utility using udisks to create the partitions? This method is different to using parted, but shares something in common with the startup (and only the startup - no edits are required) of GParted in terms of seemingly locking the extended partition device.

@chinf
Copy link

chinf commented Oct 28, 2018

After getting "No such device or address", I'm able to restore "normal operation" (up to the point of having an extended partition with correct blkid & udisks output, and ignoring data loss) by deleting & recreating the extended partition in gnome-disk-utility.

Is that consistent with something locking the extended partition device?

@pothos
Copy link
Contributor Author

pothos commented Apr 8, 2019

Hey,
thanks for investigating so far. I also got it working after running this in a separate process (where the extended partition is in sdb1):

fd = openat(AT_FDCWD, "/dev/sdb", O_RDWR);
close(fd);

Maybe this causes the kernel to trigger an event that needs to be triggered in libblockdev after the change (adding it there has no effect after releasing the lock in disk_commit).
Edit: also I notice the value of /sys/block/sdb/sdb1/size changing from 0 to 1.

@pothos
Copy link
Contributor Author

pothos commented Apr 8, 2019

The code which works when not run inside UDisks (e.g. directly libblockdev) uses _disk_sync_part_table in libparted where not a single BLKRRPART but many BLKPG ioctls tell the kernel what changed. It issues a resize for the extended partition when the logical is added, which insures the correct size (1 or 2 but not 0). Maybe the way UDisks interacts with udev while still keeping the lock causes problems. A workaround in libblockdev is calling the BLKRRPART ioctl in libblockdev's disk_commit (only if not resizing). (This is what udev anyway does later but can't due to the locks in libblockdev as well as UDisks.)

@pothos
Copy link
Contributor Author

pothos commented Apr 8, 2019

There are two ways to end up with a zero /sys/block/sdb/sdb1/size (extended partition) entry from the kernel. One is creating a logical partition in UDisks with then udev getting active while UDisks holds the flock. The other is resizing a partition with then udev getting active while maybe another utility accessing the created partition.

@pothos
Copy link
Contributor Author

pothos commented Apr 9, 2019

I believe it's a misbehavior in the BLKPG ioctl and I was able to reproduce in a single program (while keeping the fd open this problem can already be observed). A BLKRRPART ioctl fixes it when detected, so that after running, you don't need to fix it yourself.
But you could also remove this BLKRRPART call and, in addition, pause udevd with a STOP signal if you want to observe a zero size with e.g. cat /sys/block/sdb/sdb1/size after the program has run (closing the fd causes udevd to trigger the BLKRRPART).

This test also works for loop devices. I will reach out to the people who were involved when this syscall was added, and ask if they agree to have it fixed in the kernel.

#include <errno.h>
#include <fcntl.h>
#include <linux/blkpg.h>
#include <linux/fs.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

int resizepart(char *path, int start, int fd) {
  struct blkpg_partition part;
  struct blkpg_ioctl_arg ioctl_arg;
  memset(&part, 0, sizeof(part));
  part.start = start;
  part.length = 1;
  part.pno = 1;
  strncpy(part.devname, path, BLKPG_DEVNAMELTH);
  ioctl_arg.op = BLKPG_RESIZE_PARTITION;
  ioctl_arg.flags = 0;
  ioctl_arg.datalen = sizeof(struct blkpg_partition);
  ioctl_arg.data = (void *)&part;
  if (ioctl(fd, BLKPG, &ioctl_arg) == -1) {
    perror(strerror(errno));
    exit(errno);
  }
}

int addpart(char *path, int start, int fd) {
  struct blkpg_partition part;
  struct blkpg_ioctl_arg ioctl_arg;
  memset(&part, 0, sizeof(part));
  part.start = start + 2097152;
  part.length = 10485760; /* 10 MB */
  part.pno = 5;
  strncpy(part.devname, path, BLKPG_DEVNAMELTH);
  ioctl_arg.op = BLKPG_ADD_PARTITION;
  ioctl_arg.flags = 0;
  ioctl_arg.datalen = sizeof(struct blkpg_partition);
  ioctl_arg.data = (void *)&part;
  if (ioctl(fd, BLKPG, &ioctl_arg) == -1) {
    perror(strerror(errno));
    exit(errno);
  }
}

int main(int argc, char **argv) {
  char *disk;
  char *existing_extended_part;
  char *new_logical_part;
  int fd;
  int read_fd;
  char path[100];
  char buf[100];
  int start;
  const char *loopback = "";
  if (argc != 2) {
    printf("Usage: %s sdb \nAnnounces a logical sdb5 partition to the kernel "
           "on MBR drive /dev/sdb, which must only have a single extended "
           "partition (sdb1) and no logical partitons present yet.\nThis is "
           "done with BLKPG calls to issue a resize call for sdb1 and an add "
           "call for sdb5.\nThe observed problem is that the kernel then has a "
           "zero size for the extended partition (thus, nothing can access it "
           "anymore). Only a BLKRRPART can fix it again, as will be done here "
           "when a zero size is detected.\n\nEven when the BLKRRPART is "
           "removed here,"
           "after this program ends, udev will anyway trigger a BLKRRPART,"
           "therefore, you may have to pause udevd if it is"
           "interfering with a command you want to run afterwards:\n"
           "sudo kill -s STOP `pgrep udevd`\nThen, after running all you "
           "commands, "
           "you can continue udevd again:\nsudo kill -s CONT `pgrep udevd`",
           argv[0]);
    return -1;
  }
  disk = argv[1];
  if (strncmp(disk, "loop", 4) == 0)
    loopback = "p";
  memset(path, 0, sizeof(path));
  snprintf(path, sizeof(path), "/sys/block/%s/%s%s1/start", disk, disk,
           loopback);
  read_fd = open(path, O_RDONLY);
  if (read_fd == -1) {
    perror(strerror(errno));
    exit(errno);
  }
  memset(buf, 0, sizeof(buf));
  if (read(read_fd, buf, sizeof(buf)) == -1)
    return -1;
  close(read_fd);
  start = atoi(buf) * 512;
  memset(path, 0, sizeof(path));
  snprintf(path, sizeof(path), "/dev/%s", disk);
  fd = open(path, O_RDWR);
  if (fd == -1) {
    perror(strerror(errno));
    exit(errno);
  }
  /* order can be reversed to add/resize and the result is still the same:
   * /sys/block/sdb/sdb1/size containing a 0 instead of 1 or 2 */

  memset(path, 0, sizeof(path));
  snprintf(path, sizeof(path), "/dev/%s%s1", disk, loopback);
  resizepart(path, start, fd);

  memset(path, 0, sizeof(path));
  snprintf(path, sizeof(path), "/dev/%s%s5", disk, loopback);
  addpart(path, start, fd);

  /* even adding this again as last step does not help */
  memset(path, 0, sizeof(path));
  snprintf(path, sizeof(path), "/dev/%s%s1", disk, loopback);
  resizepart(path, start, fd);

  /* fetch value now for convenience and also because no interfering BLKRRPART
   * was done yet by other programs */
  memset(path, 0, sizeof(path));
  snprintf(path, sizeof(path), "/sys/block/%s/%s%s1/size", disk, disk,
           loopback);
  while (1) {
    memset(buf, 0, sizeof(buf));
    read_fd = open(path, O_RDONLY);
    if (read_fd == -1 || read(read_fd, buf, sizeof(buf)) == -1) {
      printf("could not fetch %s\n", path);
      if (read_fd != -1)
        close(read_fd);
      perror(strerror(errno));
      exit(errno);
    }
    close(read_fd);
    printf("new size in %s: %s\n", path, buf);
    if (atoi(buf) != 0) {
      break;
    }
    ioctl(fd, BLKRRPART, 0);
    printf("issuing BLKRRPART\n");
  }
  close(fd);
  return 0;
}

@pothos
Copy link
Contributor Author

pothos commented Apr 9, 2019

Ah, well, there is a linux_part.length *= disk->dev->sector_size; missing in line 2551 of parted's _blkpg_resize_partition code. That's why I observed the wrong numbers in strace and added them here. It should be part.length = 512; and then everything works.

@pothos
Copy link
Contributor Author

pothos commented Apr 9, 2019

Oh, this has been fixed in libparted master since commit c6dc6e5d in June 2015. However, since there was no release since 2014, the distributions seem to only pick a few fixes. At least for Debian, it does not include the fix for this issue… so, we need to urge the Debian maintainers to include the fix. That explains the failures with Debian and Ubuntu but not on Fedora.
Edit: Reported to https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=926735

@pothos
Copy link
Contributor Author

pothos commented Apr 14, 2019

Closing this here as it is a distro issue.

@pothos pothos closed this as completed Apr 14, 2019
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

No branches or pull requests

3 participants