Re: [Xen-devel] [PATCH] tools/hotplug: fix locking

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-06-21 at 12:42 +0100, Ian Campbell wrote:
> On Wed, 2012-06-20 at 18:53 +0100, Zhigang Wang wrote:
> > On 06/20/2012 11:34 AM, Ian Campbell wrote:
> > > On Wed, 2012-06-13 at 18:29 +0100, Zhigang Wang wrote:
> > >> # HG changeset patch
> > >> # User Zhigang Wang <zhigang.x.wang@xxxxxxxxxx>
> > >> # Date 1339608534 14400
> > >> # Node ID 650b03f412143c3057abbd0ae0e256e6b3fd2ba8
> > >> # Parent  32034d1914a607d7b6f1f060352b4cac973600f8
> > >> tools/hotplug: fix locking
> > > A better title would be "tools/hotplug: Switch to locking with flock"
> > > since "fix" is not very descriptive.
> > Agree.
> > >
> > > The commit message should also state why changing this scheme is
> > > preferable to fixing the current one.
> > I have two points:
> > 
> > 1. No timeout: in the old one, if one process holding the lock more than 100
> >    seconds, other processes could steal it.
> > 2. No leftovers: if a process holding this lock is dead, it will close the 
> > lock
> >    file, so it will not affect other processes.
> > 
> > >
> > > Is this flock tool widely available? Does it need to be added to the
> > > dependencies in the README?
> > It is widely distributed: it's part of the util-linux package. So I think no
> > need to document it.
> > >
> > >> The current locking implementation would allow two processes get the lock
> > >> simultaneously:
> > > [...snip shell trace...]
> > >
> > > Can you add a line or two of analysis to explain where in that shell
> > > spew things are actually going wrong and why?
> > I didn't spent much time on this complicated implement. But it fails for me 
> > and
> > also for others (from response).
> > >
> > >> This patch is ported from Red Hat Enterprise Linux 5.8.
> > > I think we need a Signed-off-by from the original patch author. (Unless
> > > DCO clause b somehow applies?)
> > I'm not sure how to handle this. There's no signed-off-by on the original 
> > Red
> > Hat patch. Could anyone in Red Hat help to get it signed off?
> 
> Perhaps Andrew Jones or Don Dutile can point us in the right direction
> (both CCd) if you identify the precise source of the patch (i.e. the
> name of the .src.rpm and the name of the patch within it)?
> 
> I looked in kernel-2.6.18-308.8.2.el5.src.rpm (which seems fairly
> recent) but maybe I'm looking in the wrong place.

Nevermind, I found it in xen-3.0.3-135.el5_8.2.src.rpm (I'd have sworn
that RH shipped kernel+xen in a single source package, oh well).

%changelog says:
        * Fri Sep 14 2007 Daniel P. Berrange <berrange@xxxxxxxxxx> - 
3.0.3-40.el5
        - Rewrite locking in hotplug scripts to fix timeouts (rhbz #267861)

Daniel CCd.

I wonder if there were any followup fixes or changes, especially wrt to
your point 1 above (the lack of a timeout now) requiring xend (or now
libxl) changes?

> 
> Ian.
> > 
> > Thanks,
> > 
> > Zhigang
> > >
> > >
> > > Thanks,
> > > Ian.
> > >
> > >> Signed-off-by: Zhigang Wang <zhigang.x.wang@xxxxxxxxxx>
> > >> Cc: Kouya Shimura <kouya@xxxxxxxxxxxxxx>
> > >>
> > >> diff -r 32034d1914a6 -r 650b03f41214 tools/hotplug/Linux/locking.sh
> > >> --- a/tools/hotplug/Linux/locking.sh     Thu Jun 07 19:46:57 2012 +0100
> > >> +++ b/tools/hotplug/Linux/locking.sh     Wed Jun 13 13:28:54 2012 -0400
> > >> @@ -1,5 +1,6 @@
> > >>  #
> > >>  # Copyright (c) 2005 XenSource Ltd.
> > >> +# Copyright (c) 2007 Red Hat
> > >>  #
> > >>  # This library is free software; you can redistribute it and/or
> > >>  # modify it under the terms of version 2.1 of the GNU Lesser General 
> > >> Public
> > >> @@ -19,92 +20,30 @@
> > >>  # Serialisation
> > >>  #
> > >>  
> > >> -LOCK_SLEEPTIME=1
> > >> -LOCK_SPINNING_RETRIES=5
> > >> -LOCK_RETRIES=100
> > >>  LOCK_BASEDIR=/var/run/xen-hotplug
> > >>  
> > >> +_setlockfd()
> > >> +{
> > >> +    local i
> > >> +    for ((i = 0; i < ${#_lockdict}; i++))
> > >> +    do [ -z "${_lockdict[$i]}" -o "${_lockdict[$i]}" = "$1" ] && break
> > >> +    done
> > >> +    _lockdict[$i]="$1"
> > >> +    let _lockfd=200+i
> > >> +}
> > >> +
> > >>  
> > >>  claim_lock()
> > >>  {
> > >> -  local lockdir="$LOCK_BASEDIR/$1"
> > >> -  mkdir -p "$LOCK_BASEDIR"
> > >> -  _claim_lock "$lockdir"
> > >> +    mkdir -p "$LOCK_BASEDIR"
> > >> +    _setlockfd $1
> > >> +    eval "exec $_lockfd>>$LOCK_BASEDIR/$1"
> > >> +    flock -x $_lockfd
> > >>  }
> > >>  
> > >>
> > >>  release_lock()
> > >>  {
> > >> -  _release_lock "$LOCK_BASEDIR/$1"
> > >> +    _setlockfd $1
> > >> +    flock -u $_lockfd
> > >>  }
> > >> -
> > >> -
> > >> -# This function will be redefined in xen-hotplug-common.sh.
> > >> -sigerr() {
> > >> -  exit 1
> > >> -}
> > >> -
> > >> -
> > >> -_claim_lock()
> > >> -{
> > >> -  local lockdir="$1"
> > >> -  local owner=$(_lock_owner "$lockdir")
> > >> -  local retries=0
> > >> -
> > >> -  while [ $retries -lt $LOCK_RETRIES ]
> > >> -  do
> > >> -    mkdir "$lockdir" 2>/dev/null && trap "_release_lock $lockdir; 
> > >> sigerr" ERR &&
> > >> -      _update_lock_info "$lockdir" && return
> > >> -
> > >> -    local new_owner=$(_lock_owner "$lockdir")
> > >> -    if [ "$new_owner" != "$owner" ]
> > >> -    then
> > >> -      owner="$new_owner"
> > >> -      retries=0
> > >> -    else
> > >> -      local pid=$(echo $owner | cut -d : -f 1)
> > >> -      if [ -n "$pid" -a "$pid" != "unknown" -a ! -f "/proc/$pid/status" 
> > >> ]
> > >> -      then
> > >> -        _release_lock $lockdir
> > >> -      fi
> > >> -    fi
> > >> -
> > >> -    if [ $retries -gt $LOCK_SPINNING_RETRIES ]
> > >> -    then
> > >> -      sleep $LOCK_SLEEPTIME
> > >> -    else
> > >> -      sleep 0
> > >> -    fi
> > >> -    retries=$(($retries + 1))
> > >> -  done
> > >> -  _steal_lock "$lockdir"
> > >> -}
> > >> -
> > >> -
> > >> -_release_lock()
> > >> -{
> > >> -  trap sigerr ERR
> > >> -  rm -rf "$1" 2>/dev/null || true
> > >> -}
> > >> -
> > >> -
> > >> -_steal_lock()
> > >> -{
> > >> -  local lockdir="$1"
> > >> -  local owner=$(cat "$lockdir/owner" 2>/dev/null || echo "unknown")
> > >> -  log err "Forced to steal lock on $lockdir from $owner!"
> > >> -  _release_lock "$lockdir"
> > >> -  _claim_lock "$lockdir"
> > >> -}
> > >> -
> > >> -
> > >> -_lock_owner()
> > >> -{
> > >> -  cat "$1/owner" 2>/dev/null || echo "unknown"
> > >> -}
> > >> -
> > >> -
> > >> -_update_lock_info()
> > >> -{
> > >> -  echo "$$: $0" >"$1/owner"
> > >> -}
> > >>
> > >> _______________________________________________
> > >> Xen-devel mailing list
> > >> Xen-devel@xxxxxxxxxxxxx
> > >> http://lists.xen.org/xen-devel
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@xxxxxxxxxxxxx
> > > http://lists.xen.org/xen-devel
> > 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel