| Message ID | 20201214163623.2127-6-bouyer@netbsd.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NetBSD fixes | expand |
I think you want tot CC the tools dev on this one, specially Ian who knows how the Linux one is implemented and can likely give valuable input. On Mon, Dec 14, 2020 at 05:36:04PM +0100, Manuel Bouyer wrote: > --- > tools/hotplug/NetBSD/Makefile | 1 + > tools/hotplug/NetBSD/block | 5 ++- > tools/hotplug/NetBSD/locking.sh | 72 +++++++++++++++++++++++++++++++++ Seeing the file itself, I don't think there's any NetBSD specific stuff, so we might want to consider putting it in BSD/ instead, so it can be used by FreeBSD also? I'm also not sure if it would be useful to Linux folks? Thanks, Roger.
On Tue, Dec 29, 2020 at 12:29:09PM +0100, Roger Pau Monné wrote: > I think you want tot CC the tools dev on this one, specially Ian who > knows how the Linux one is implemented and can likely give valuable > input. > > On Mon, Dec 14, 2020 at 05:36:04PM +0100, Manuel Bouyer wrote: > > --- > > tools/hotplug/NetBSD/Makefile | 1 + > > tools/hotplug/NetBSD/block | 5 ++- > > tools/hotplug/NetBSD/locking.sh | 72 +++++++++++++++++++++++++++++++++ > > Seeing the file itself, I don't think there's any NetBSD specific > stuff, so we might want to consider putting it in BSD/ instead, so it > can be used by FreeBSD also? I'm not sure if FreeBSD needs the locking stuff. Also, there are certainly differences in block device handling between FreeBSD and NetBSD. Both OSes have diverged in this area.
Manuel Bouyer writes ("Re: [PATCH 05/24] Introduce locking functions for block device setup on NetBSD"):
> On Tue, Dec 29, 2020 at 12:29:09PM +0100, Roger Pau Monné wrote:
> > I think you want tot CC the tools dev on this one, specially Ian who
> > knows how the Linux one is implemented and can likely give valuable
> > input.
...
> > Seeing the file itself, I don't think there's any NetBSD specific
> > stuff, so we might want to consider putting it in BSD/ instead, so it
> > can be used by FreeBSD also?
>
> I'm not sure if FreeBSD needs the locking stuff.
> Also, there are certainly differences in block device handling between
> FreeBSD and NetBSD. Both OSes have diverged in this area.
I think most operating systems will want some kind of locking here.
I loooked at the code in the new tools/hotplug/NetBSD/locking.sh.
Unfortunately this area is complex and the available APIs and tools
are awkard, and the field is troubled by broken "traditional"
approaches involving O_EXCL or the moral equivalent, which cannot be
made reliable (if you think reliability implies never being broken due
to stale lock).
I doubt that the code in this patch is correct. It uses shlock(1)
which is based on link(2) and kill(2) and so on, which I think is
basically an O_EXCL-based approach as I discuss above. (I don't have
a formal proof of this contention.) The presence of an invocation of
the "trap" shell builtin in the new NetBSD script is a bad sign - a
reliable locking protocol would need that.
I see from https://man.netbsd.org that NetBSD has flock(1) and
stat(1). I think this means we could reuse the code in
tools/hotplug/Linux/locking.sh. Maybe it will need to be lightly
adapted, to NetBSD's flock(1) and stat(1). Perhaps via some kind of
substitution to avoid all the clone-and-hack.
Regards,
Ian.
On Wed, Jan 20, 2021 at 03:13:22PM +0000, Ian Jackson wrote: > Manuel Bouyer writes ("Re: [PATCH 05/24] Introduce locking functions for block device setup on NetBSD"): > > On Tue, Dec 29, 2020 at 12:29:09PM +0100, Roger Pau Monné wrote: > > > I think you want tot CC the tools dev on this one, specially Ian who > > > knows how the Linux one is implemented and can likely give valuable > > > input. > ... > > > Seeing the file itself, I don't think there's any NetBSD specific > > > stuff, so we might want to consider putting it in BSD/ instead, so it > > > can be used by FreeBSD also? > > > > I'm not sure if FreeBSD needs the locking stuff. > > Also, there are certainly differences in block device handling between > > FreeBSD and NetBSD. Both OSes have diverged in this area. > > I think most operating systems will want some kind of locking here. > > I loooked at the code in the new tools/hotplug/NetBSD/locking.sh. > Unfortunately this area is complex and the available APIs and tools > are awkard, and the field is troubled by broken "traditional" > approaches involving O_EXCL or the moral equivalent, which cannot be > made reliable (if you think reliability implies never being broken due > to stale lock). > > I doubt that the code in this patch is correct. It uses shlock(1) > which is based on link(2) and kill(2) and so on, which I think is > basically an O_EXCL-based approach as I discuss above. (I don't have > a formal proof of this contention.) The presence of an invocation of > the "trap" shell builtin in the new NetBSD script is a bad sign - a > reliable locking protocol would need that. Actually this patch is old - since Xen 4.8 at last. > > I see from https://man.netbsd.org that NetBSD has flock(1) and > stat(1). I think this means we could reuse the code in > tools/hotplug/Linux/locking.sh. Maybe it will need to be lightly > adapted, to NetBSD's flock(1) and stat(1). Perhaps via some kind of > substitution to avoid all the clone-and-hack. Yes, at last the stat call will need to be patched. But it seems to rely on a linux-specific behavoir, which is that /dev/stdin points to the real file on redirection: >ls -l /dev/stdin /proc/self/fd/0 < /etc/passwd lrwxrwxrwx 1 root root 15 Apr 30 2019 /dev/stdin -> /proc/self/fd/0 lr-x------ 1 bouyer ita-iatos 64 Jan 20 16:54 /proc/self/fd/0 -> /etc/passwd On NetBSD (and I guess other BSDs) this won't work, as /dev/stdin is a specific device: >ls -l /dev/stdin crw-rw-rw- 1 root wheel 22, 0 Nov 15 2007 /dev/stdin so stat -L will always return the same data. We can't use the same protocol.
Manuel Bouyer writes ("Re: [PATCH 05/24] Introduce locking functions for block device setup on NetBSD"):
> Yes, at last the stat call will need to be patched.
> But it seems to rely on a linux-specific behavoir, which is that
> /dev/stdin points to the real file on redirection:
> >ls -l /dev/stdin /proc/self/fd/0 < /etc/passwd
> lrwxrwxrwx 1 root root 15 Apr 30 2019 /dev/stdin -> /proc/self/fd/0
> lr-x------ 1 bouyer ita-iatos 64 Jan 20 16:54 /proc/self/fd/0 -> /etc/passwd
>
> On NetBSD (and I guess other BSDs) this won't work, as /dev/stdin is a
> specific device:
> >ls -l /dev/stdin
> crw-rw-rw- 1 root wheel 22, 0 Nov 15 2007 /dev/stdin
>
> so stat -L will always return the same data. We can't use the same
> protocol.
Ah.
The manpage I am looking at says:
If no argument is given, stat displays information about the
file descriptor for standard input.
Here NetBSD has a better command line API than Linux - Linux requires
pratting about with /dev/stdin and NetBSD doesn't. So I think
where on Linux we have
stat .... /dev/stdin
on NetBsd we can simply have
stat ....
with no filename argument.
I think NetBSD's stat(1) also takes different argumnts to specify the
format. NetBSD uses -f, whereas Linux uses -c. So the exact rune
will have to be different.
Ian.
On Wed, Jan 20, 2021 at 04:12:29PM +0000, Ian Jackson wrote: > Manuel Bouyer writes ("Re: [PATCH 05/24] Introduce locking functions for block device setup on NetBSD"): > > Yes, at last the stat call will need to be patched. > > But it seems to rely on a linux-specific behavoir, which is that > > /dev/stdin points to the real file on redirection: > > >ls -l /dev/stdin /proc/self/fd/0 < /etc/passwd > > lrwxrwxrwx 1 root root 15 Apr 30 2019 /dev/stdin -> /proc/self/fd/0 > > lr-x------ 1 bouyer ita-iatos 64 Jan 20 16:54 /proc/self/fd/0 -> /etc/passwd > > > > On NetBSD (and I guess other BSDs) this won't work, as /dev/stdin is a > > specific device: > > >ls -l /dev/stdin > > crw-rw-rw- 1 root wheel 22, 0 Nov 15 2007 /dev/stdin > > > > so stat -L will always return the same data. We can't use the same > > protocol. > > Ah. > > The manpage I am looking at says: > > If no argument is given, stat displays information about the > file descriptor for standard input. > > Here NetBSD has a better command line API than Linux - Linux requires > pratting about with /dev/stdin and NetBSD doesn't. So I think > where on Linux we have > stat .... /dev/stdin > on NetBsd we can simply have > stat .... > with no filename argument. Right, thanks. Then it would need to be done with 2 different calls but I don't think that's a problem (even with the linux version it would not be atomic anyway). > > I think NetBSD's stat(1) also takes different argumnts to specify the > format. NetBSD uses -f, whereas Linux uses -c. So the exact rune > will have to be different. Yes, and NetBSD doens't have %D (only %d)
Manuel Bouyer writes ("Re: [PATCH 05/24] Introduce locking functions for block device setup on NetBSD"):
> On Wed, Jan 20, 2021 at 04:12:29PM +0000, Ian Jackson wrote:
> > I think NetBSD's stat(1) also takes different argumnts to specify the
> > format. NetBSD uses -f, whereas Linux uses -c. So the exact rune
> > will have to be different.
>
> Yes, and NetBSD doens't have %D (only %d)
What's really important here is the inode number. But %d will do
nicely. I only used %D on Linux since device numbers are two-element
bitfields so displaying them in hex makes more sense to humans who
might be debugging this.
Ian.
Manuel Bouyer writes ("Re: [PATCH 05/24] Introduce locking functions for block device setup on NetBSD"):
> Right, thanks. Then it would need to be done with 2 different calls
> but I don't think that's a problem (even with the linux version it would
> not be atomic anyway).
Sorry, I forgot to reply to this. Yes, it would need to invocations
of stat(1). You are correct that the Linux version is not atomic in
that sense.
Ian.
diff --git a/tools/hotplug/NetBSD/Makefile b/tools/hotplug/NetBSD/Makefile index 6926885ab8..114b223207 100644 --- a/tools/hotplug/NetBSD/Makefile +++ b/tools/hotplug/NetBSD/Makefile @@ -3,6 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk # Xen script dir and scripts to go there. XEN_SCRIPTS = +XEN_SCRIPTS += locking.sh XEN_SCRIPTS += block XEN_SCRIPTS += vif-bridge XEN_SCRIPTS += vif-ip diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block index 32c20b6c89..23c8e38ebf 100644 --- a/tools/hotplug/NetBSD/block +++ b/tools/hotplug/NetBSD/block @@ -6,6 +6,7 @@ DIR=$(dirname "$0") . "${DIR}/hotplugpath.sh" +. "${DIR}/locking.sh" PATH=${bindir}:${sbindir}:${LIBEXEC_BIN}:/bin:/usr/bin:/sbin:/usr/sbin export PATH @@ -62,6 +63,7 @@ case $xstatus in available_disks="$available_disks $disk" eval $disk=free done + claim_lock block # Mark the used vnd(4) devices as ``used''. for disk in `sysctl hw.disknames`; do case $disk in @@ -77,6 +79,7 @@ case $xstatus in break fi done + release_lock block if [ x$device = x ] ; then error "no available vnd device" fi @@ -86,7 +89,7 @@ case $xstatus in device=$xparams ;; esac - physical_device=$(stat -f '%r' "$device") + physical_device=$(stat -L -f '%r' "$device") xenstore-write $xpath/physical-device $physical_device xenstore-write $xpath/hotplug-status connected exit 0 diff --git a/tools/hotplug/NetBSD/locking.sh b/tools/hotplug/NetBSD/locking.sh new file mode 100644 index 0000000000..88257f62b7 --- /dev/null +++ b/tools/hotplug/NetBSD/locking.sh @@ -0,0 +1,72 @@ +#!/bin/sh +# +# Copyright (c) 2016, Christoph Badura. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR(S) ``AS IS'' AND ANY EXPRESS +# OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL THE AUTHOR(S) BE LIABLE FOR ANY DIRECT, +# INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. +# + +LOCK_BASEDIR="$XEN_LOCK_DIR/xen-hotplug" + +_lockfd=9 +_have_lock=0 # lock not taken yet. + +SHLOCK="shlock ${_shlock_debug-}" + +_lock_set_vars() { + _lockfile="$LOCK_BASEDIR/$1.lock" + _lockfifo="$LOCK_BASEDIR/$1.fifo" +} + +_lock_init() { + mkdir -p "$LOCK_BASEDIR" 2>/dev/null || true + mkfifo $_lockfifo 2>/dev/null || true +} + +# +# use a named pipe as condition variable +# opening for read-only blocks when there's no writer. +# opening for read-write never blocks but unblocks any waiting readers. +# +_lock_wait_cv() { + eval "exec $_lockfd< $_lockfifo ; exec $_lockfd<&-" +} +_lock_signal_cv() { + eval "exec $_lockfd<> $_lockfifo ; exec $_lockfd<&-" +} + +claim_lock() { + _lock_set_vars $1 + _lock_init + until $SHLOCK -f $_lockfile -p $$; do + _lock_wait_cv + done + _have_lock=1 + # be sure to release the lock when the shell exits + trap "release_lock $1" 0 1 2 15 +} + +release_lock() { + _lock_set_vars $1 + [ "$_have_lock" != 0 -a -f $_lockfile ] && rm $_lockfile + _have_lock=0 + _lock_signal_cv; +}