Re: [PATCH] tools/hotplug: fix locking

[Kouya Shimura <kouya@jp.fujitsu.com>]

Hi,

The owner support is introduced in c/s 8175, not by me.
Anyway, something is wrong with your execution trace.

> + '[' 6 -gt 5 ']'
> + sleep 1
<<< Why claim_lock() returns here???
> + do_or_die losetup -r /dev/loop27 /OVS/Repositories/0004fb00000300000aac184a9bbab7a9/VirtualDisks/0004fb00001200002b0ef651033c8381.img
> + losetup -r /dev/loop27 /OVS/Repositories/0004fb00000300000aac184a9bbab7a9/Vi

I don't know what is a problem and whether your patch resolves the issue or not.

It would be better to replace locking.sh with the RHEL5 implementation
which uses 'flock' rather than to fix it.

Thanks,
Kouya


Zhigang Wang writes:
> # HG changeset patch
> # User Zhigang Wang <zhigang.x.wang@oracle.com>
> # Date 1339421383 14400
> # Node ID eb72d7090b5956490b2f26c858cd9d896feca309
> # Parent  32034d1914a607d7b6f1f060352b4cac973600f8
> tools/hotplug: fix locking
> 
> The current locking implementation would allow two processes get the lock
> simultaneously:
> 
> ++ echo 16741: /etc/xen/scripts/block
> ++ cut -d : -f 1
> + local pid=16741
> + '[' -n 16741 -a 16741 '!=' unknown -a '!' -f /proc/16741/status ']'
> + '[' 5 -gt 5 ']'
> + sleep 0
> + retries=6
> + '[' 6 -lt 100 ']'
> + mkdir /var/run/xen-hotplug/block
> ++ _lock_owner /var/run/xen-hotplug/block
> ++ cat /var/run/xen-hotplug/block/owner
> + local 'new_owner=16741: /etc/xen/scripts/block'
> + '[' '16741: /etc/xen/scripts/block' '!=' '16741: /etc/xen/scripts/block' ']'

> ++ echo 16741: /etc/xen/scripts/block
> ++ cut -d : -f 1
> + local pid=16741
> + '[' -n 16741 -a 16741 '!=' unknown -a '!' -f /proc/16741/status ']'
> + '[' 6 -gt 5 ']'
> + sleep 1
> + do_or_die losetup -r /dev/loop27 /OVS/Repositories/0004fb00000300000aac184a9bbab7a9/VirtualDisks/0004fb00001200002b0ef651033c8381.img
> + losetup -r /dev/loop27 /OVS/Repositories/0004fb00000300000aac184a9bbab7a9/VirtualDisks/0004fb00001200002b0ef651033c8381.img
> + do_or_die losetup -r /dev/loop27 /OVS/Repositories/0004fb00000300000aac184a9bbab7a9/VirtualDisks/0004fb0000120000143173990458f2a7.img
> + losetup -r /dev/loop27 /OVS/Repositories/0004fb00000300000aac184a9bbab7a9/VirtualDisks/0004fb0000120000143173990458f2a7.img
> + xenstore_write backend/vbd/33/51920/node /dev/loop27
> + _xenstore_write backend/vbd/33/51920/node /dev/loop27
> + log debug 'Writing backend/vbd/33/51920/node' '/dev/loop27 to xenstore.'
> + local level=debug
> + shift
> + logger -p daemon.debug -- /etc/xen/scripts/block: 'Writing backend/vbd/33/51920/node' '/dev/loop27 to xenstore.'
> ioctl: LOOP_SET_FD: Device or resource busy
> + xenstore-write backend/vbd/33/51920/node /dev/loop27
> + fatal losetup -r /dev/loop27 '/OVS/Repositories/0004fb00000300000aac184a9bbab7a9/VirtualDisks/0004fb0000120000143173990458f2a7.img failed'
> 
> This patch removes the ownner support and fixed this issue per my test.
> 
> Kouya: would you please help to confirm this fix is correct?
> 
> Anyone has encountered this issue please help to test.
> 
> Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>
> Cc: Kouya Shimura <kouya@jp.fujitsu.com>
> 
> diff -r 32034d1914a6 -r eb72d7090b59 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	Mon Jun 11 09:29:43 2012 -0400
> @@ -48,32 +48,14 @@ sigerr() {
>  _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
> -
> +    mkdir "$lockdir" 2>/dev/null && trap "_release_lock $lockdir; sigerr" ERR && return
>      if [ $retries -gt $LOCK_SPINNING_RETRIES ]
>      then
>        sleep $LOCK_SLEEPTIME
> -    else
> -      sleep 0
>      fi
>      retries=$(($retries + 1))
>    done
> @@ -91,20 +73,7 @@ _release_lock()
>  _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!"
> +  log err "Forced to steal lock on $lockdir!"
>    _release_lock "$lockdir"
>    _claim_lock "$lockdir"
>  }
> -
> -
> -_lock_owner()
> -{
> -  cat "$1/owner" 2>/dev/null || echo "unknown"
> -}
> -
> -
> -_update_lock_info()
> -{
> -  echo "$$: $0" >"$1/owner"
> -}