Re: [make-initrd] [PATCH v6 08/22] bootchain-waitdev: introduces an optional waitdev_timeout

[Alexey Gladkov <gladkov.alexey@gmail.com>]

On Tue, Oct 26, 2021 at 02:25:55PM +0300, Leonid Krivoshein wrote:
> 
> 26.10.2021 13:55, Alexey Gladkov пишет:
> > On Sun, Oct 24, 2021 at 08:21:08PM +0300, Leonid Krivoshein wrote:
> > > "waitdev_timeout" describes a common timeout for all "waitdev" steps
> > > in the "bootchain". Defining a timeout allows to use a fallback if
> > > the specified devices are not ready yet. By default is not set, which
> > > makes to wait forever.
> > > 
> > > Signed-off-by: Leonid Krivoshein <klark.devel@gmail.com>
> > > ---
> > >   features/bootchain-waitdev/README.md           |  3 +++
> > >   .../etc/initrd/cmdline.d/bootchain-waitdev     |  1 +
> > >   .../data/lib/bootchain/waitdev                 | 18 ++++++++++++++++++
> > >   .../data/lib/initrd/pre/bootchain/300-waitdev  |  3 +++
> > >   4 files changed, 25 insertions(+)
> > > 
> > > diff --git a/features/bootchain-waitdev/README.md b/features/bootchain-waitdev/README.md
> > > index 126a904..d6bae63 100644
> > > --- a/features/bootchain-waitdev/README.md
> > > +++ b/features/bootchain-waitdev/README.md
> > > @@ -13,6 +13,9 @@ feature. It allows to wait a specified block or character special devices.
> > >      the same as `root=`, but with optional `CDROM:` prefix. This parameter can be
> > >      specified more than once depending on how many times a corresponding element
> > >      is mentioned in the `bootchain`.
> > > +- `waitdev_timeout` describes a common timeout for all `waitdev` steps in the
> > > +  `bootchain`. Defining a timeout allows to use a fallback if the specified
> > > +  devices are not ready yet. By default is not set, which makes to wait forever.
> > Почему он общий между всеми waitdev ???
> 
> Логика в этом железная: если для сборки корня шагам, следующим за
> wiatdev'ами, нужны все указанные устройства, то не имеет значения, какого
> устройства в цепочке не хватает. Это очень похоже на общий rootdelay и схему
> "И". Общий таймаут нужен для того, чтобы не ждать вечно и дать возможность
> отработать некоему fallback'у, следующим за этими шагами, или выдать общий
> на все отказы диалог, и вернуться в начало.

Аналогия с rootdelay= тут не корректна т.к. rootdelay= распространяется на
весь процесс загрузки. Я бы понял, если бы timeout распространялся на весь
bootchain=, тогда было бы логично.

Таймаут логичен если после него будет либо ошибка, либо предполагаются
какие-то действия.

Я предложил добавить таймаут как параметр конкретного waitdev.

> > Получается, если первый waitdev прождал весь wait_timeout, то остальные
> > даже пробовать не будут, но это же неправильно так как они другой девайс
> > ожидают.
> 
> Можно было бы им дать попробовать без дополнительных sleep'ов, но есть два
> контраргумента:
> 
> 1) Цепочка могла быть построена исходя из знания, что к устройству "Б"
> нельзя обращаться, пока не будет подключено устройство "А". Обращение к "Б"
> без готовности "А" может привести к нехорошим последствиям, типа зависания.
> 
> 2) Чтобы отработал fallback, по всей видимости, последний waitdev не должен
> быть готов.

А как происходит этот fallback ?

В твоей реализации если достигнут таймаут, то последующие waitdev просто
exit 0 делают и невозможно понять дождались они чего-то или нет.

Получается, что следующий шаг может только гадать о результате waitdev.

Потому что мне сейчас приходит в голову сделать параметр (или шаг)
onfail, но это явно не то чем пользуешься ты для failback.

> Если игнорировать эти моменты, можно дать хотя бы попробовать проверить
> готовность следующих устройств без дополнительных sleep'ов, благо асинхронно
> работающая конструкция это позволяет сделать.
> 
> 
> > Например, мы ждём сначала cdrom, а потом usb-флешку с ключом.
> > 
> > >   ## Example
> > > diff --git a/features/bootchain-waitdev/data/etc/initrd/cmdline.d/bootchain-waitdev b/features/bootchain-waitdev/data/etc/initrd/cmdline.d/bootchain-waitdev
> > > index 3544c25..6929a86 100644
> > > --- a/features/bootchain-waitdev/data/etc/initrd/cmdline.d/bootchain-waitdev
> > > +++ b/features/bootchain-waitdev/data/etc/initrd/cmdline.d/bootchain-waitdev
> > > @@ -1 +1,2 @@
> > >   register_array string WAITDEV
> > > +register_parameter number WAITDEV_TIMEOUT
> > > diff --git a/features/bootchain-waitdev/data/lib/bootchain/waitdev b/features/bootchain-waitdev/data/lib/bootchain/waitdev
> > > index 60464d9..fa99c45 100755
> > > --- a/features/bootchain-waitdev/data/lib/bootchain/waitdev
> > > +++ b/features/bootchain-waitdev/data/lib/bootchain/waitdev
> > > @@ -3,6 +3,14 @@
> > >   . bootchain-sh-functions
> > >   check_parameter WAITDEV
> > > +
> > > +timeout=
> > > +timecnt=/.initrd/bootchain/waitdev/TIMECNT
> > > +
> > > +[ ! -s "$timecnt" ] ||
> > > +	read -r timeout < "$timecnt" ||:
> > > +[ "$timeout" != 0 ] ||
> > > +	exit 0
> > >   devspec="$(get_parameter WAITDEV)"
> > >   while [ -n "$devspec" ]; do
> > > @@ -25,4 +33,14 @@ while [ -n "$devspec" ]; do
> > >   	fi
> > >   	sleep 1
> > > +	[ -n "$timeout" ] ||
> > > +		continue
> > > +	timeout=$(($timeout - 1))
> > > +
> > > +	if [ "$timeout" = 0 ]; then
> > > +		message "device waiting timeout exceedded"
> > > +		break
> > > +	fi
> > >   done
> > > +
> > > +[ -z "$timeout" ] || printf '%s\n' "$timeout" > "$timecnt"
> > > diff --git a/features/bootchain-waitdev/data/lib/initrd/pre/bootchain/300-waitdev b/features/bootchain-waitdev/data/lib/initrd/pre/bootchain/300-waitdev
> > > index 3642722..5e0f040 100755
> > > --- a/features/bootchain-waitdev/data/lib/initrd/pre/bootchain/300-waitdev
> > > +++ b/features/bootchain-waitdev/data/lib/initrd/pre/bootchain/300-waitdev
> > > @@ -5,6 +5,9 @@
> > >   dir=/.initrd/bootchain/waitdev
> > >   mkdir -p -- "$dir"
> > > +[ -z "${WAITDEV_TIMEOUT-}" ] ||
> > > +	printf '%s\n' "$WAITDEV_TIMEOUT" >"$dir"/TIMECNT
> > > +
> > >   i=0
> > >   while [ "$i" -lt "${WAITDEV:-0}" ]; do
> > >   	touch "$dir/$i"
> > > -- 
> > > 2.24.1
> > > 
> > > _______________________________________________
> > > Make-initrd mailing list
> > > Make-initrd@lists.altlinux.org
> > > https://lists.altlinux.org/mailman/listinfo/make-initrd
> > > 
> 
> -- 
> Best regards,
> Leonid Krivoshein.
> 
> _______________________________________________
> Make-initrd mailing list
> Make-initrd@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/make-initrd

-- 
Rgrds, legion