On Wed, Aug 8, 2018 at 9:58 AM Scott Moser <email address hidden> wrote:
>
> > > > 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.
>
> non-sense.
> have you ever seen 'udevadm settle' take 120 seconds?  Can you give
> an example of when it *could* do that?  The only thing I can reasonably

https://ask.fedoraproject.org/en/question/81006/slow-boot-time-udev-settle/
https://askubuntu.com/questions/888010/about-systemd-udev-settle-service-and-slow-booting
https://bugzilla.redhat.com/show_bug.cgi?id=735866
https://bbs.archlinux.org/viewtopic.php?id=189369
https://forums.opensuse.org/showthread.php/506828-Udev-3-minute-delay-on-boot
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=754987
https://lists.debian.org/debian-user/2015/04/msg01592.html

Settle can block up to 120 seconds any time one or more events
that are in the queue when settle is called that do not get processed.
The event in queue may be unrelated to storage; it can be any event
in the system (net, block, usb); settle doesn't care.

> come up with are disk based operations on large md or something.
> Those are exactly the reasons we want to settle.  Anything else that
> was seemingly unrelated but is taking significant time honestly
> probably something important to an installer.  I'd much rather
> see a log entry that showed a timeout failed on a settle than
> try to diagnose some related failure.

Recall this discussion:
  https://bugs.launchpad.net/cloud-init/+bug/1766287
  https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/344198

>
> If you use --exit-if-exists without having settle'd and checked that
> the thing didn't exist before you perform an operation that would
> cause it to it is a logic flaw.  and its one you're going to pull
> your hair out trying to find.

I've already showed that  we ensure that disks have no partitions
and constructed devices are destroy. We only call --exit-if-exists
after we've issued commands which create new devices;

>
> Your only argument against calling udevadm settle is to not waste time.
> And my argument is that it is not a waste of time both because
> a.) it is a very insignificant amount of time except for when it is
>    important.

It may be short, it may be up to 120 seconds.  It introduces variability
that we don't need and if you don't know that it's needed it cannot
provide "safety" w.r.t devices.

> b.) it is important in most cases.

My argument is that we should only call settle when we *need* it.
Extra calls to settle just because at best waste a few milliseconds,
at worse large amounts of latency to the install path.

Calling settle when no events are in the queue doesn't
ensure safety.  One needs to know when you've queued events (ie you've
created a new device)
and then calling settle is effective, and we use exit-if-exists to
keep us from waiting
for events that are not related to the device we created.

We know when we need to call it.

>
> I'd much prefer that any function that makes changes to disks
> settle by default.  If the caller wants to *not* settle (because

Note that lvm_scan does *not* create any devices.  It *may* activate
lvs if some are present.

> they have a better way or more knowledge) then we can do that.
> The function is just more caller friendly if it doesn't have race
> conditions waiting to explode in the user's face.

The assumption that we always need to settle is the core issue.
Callers always need to know whether they need to call settle,
and in curtin, we have that knowledge.

Also where is the race?  You keep pointing to the --exist-if-exists scenario
but that's unrelated to an lvm scan were we might activate an LV
of whom's path we don't know (and wouldn't)