Re: [PATCH] tools/hotplug: fix locking

[Zhigang Wang <zhigang.x.wang@oracle.com>]

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

Thanks,

Zhigang
>
>
> 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
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel