Code review comment for ~raharper/curtin:fix/lvm-over-raid

On Wed, Aug 1, 2018 at 8:13 AM Scott Moser <email address hidden> wrote:
>
> > > is there a reason not to have lvm_scan do the udevadm_settle ?
> > > it seems like all callers are probably interested in having that happen.
> > >
> > > maybe we even have race conditions elsewhere as I don't see anywhere else
> > > that does use settle after calling this.
> >
> > In blockmeta, creating a volgroup does not create any device nodes, but
>
> But if that is the first time that 'lvm_scan' is run, I'm not sure
> how that would not possibly create device nodes.

A volume group is not directly accessible as a block device. You have
to create an LV on a VG to get an accessible block device.

>
> > creating the logical volumes will. We don't currently do a devsync()
> > (which does a settle using the --exit-if-exists=/path/to/device/expected
>
> I generally think that '--exit-if-exists=' is a bug.
> consider this:
> a.) /dev/sda has 1 partition on it, so '/dev/sda1' exists (from previous
> install).
> b.) we repartition /dev/sda, blockdev --rereadpt, and and then call
> 'udevadm settle --exit-if-exists=/dev/sda1'

We wipe the disk's partition table, and then force a reread of the
partition table
and ensure that the disk no longer has *any* partitions, whatever
their name was or the location.

The disk is clean before we create any new partitions.

> c.) udevadm checks to see if /dev/sda1 exists. It does (it always did)
> d.) udev events continue, and re-read the partition table, delete all
> the old partitions and re-create them.
> e.) while 'd' is happening our code thinks it is fine to use /dev/sda1
>
> I think we have this general fallacy in logic at places.

I disagree. The order of events that happen in curtin are as follows:

1) clear-holders calls wipe_superblock on /dev/sda (which has a /dev/sda1)
2) if the device being wiped has partitions (it does), then we wipe
the partition table
      and then reread the partition table, settle and query the disk
for any partitions, repeat
      until there are no partitions reported from the kernel, we fail
if we cannot wipe the partitions.
3) after all disks are clear (no partitions are left on any disk
specified which has wipe superblock
4) we create a partition table (empty) on /dev/sda
5) we create a first partition on /dev/sda, say /dev/sda1
6) we query the path to /dev/sda1 via the get_path_to_storage() which
calls devsync() which
    does a udevadm settle --exit-if-exists

There isn't a logic bug here because we know that *we* were the ones
to create the partition and we know what the expected /dev path is.

>
> /dev/sda1 above could just as well be /dev/vg0 or any other device.

It won't be a volume group, but yes, any other block device that we
create. The logic above holds for those as well. Where do you think
it fails after accounting for how we wipe devices and ensure they're
clear (including partitions)?

>
> > to have a fast exit path if the device node has been created) at the end
> > of lvm logical volume creation, but > the subsequent call of the user
> > of the lvm device uses get_path_to_storage() which will do a devsync().
>
> udevadm settle is *not* a slow operation if there are no events in the
> queue. It is a very important one if there *is* events in the queue.
>
> $ time udevadm settle
> real 0m0.013s
> user 0m0.004s
> sys 0m0.001s

At best, at worst, the default timeout for udevadm settle is 120
seconds; and without an --exit-if-exists, we wait on *all* events in
the queue at the time of the settle call.

>
> So... I'd much rather have any functions that may cause udevadm events
> to call settle by default and be safe than try to shave off an extra
> 1/100th of a second in a OS install.

The point isn't to shave off seconds, it's to not bother waiting on
events that are unrelated to the work we're performing. Any time we
avoid calls to settle, we help avoid potential 120 second slowdowns.

>
> --
> https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/351384
> You are the owner of ~raharper/curtin:fix/lvm-over-raid.