Re: [PATCH] tools/hotplug: fix locking

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2012-06-13 at 18:29 +0100, Zhigang Wang wrote:
> # HG changeset patch
> # User Zhigang Wang <zhigang.x.wang@oracle.com>
> # 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.

The commit message should also state why changing this scheme is
preferable to fixing the current one.

Is this flock tool widely available? Does it need to be added to the
dependencies in the README?

> 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?

> 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?)


Thanks,
Ian.

> Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>
> Cc: Kouya Shimura <kouya@jp.fujitsu.com>
> 
> 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@lists.xen.org
> http://lists.xen.org/xen-devel