Re: [PATCH] v2 - Add exclusive locking option to block-iscsi

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, May 05, 2016 at 03:52:30PM +1000, Steven Haigh wrote:
> On 2016-05-05 12:32, Steven Haigh wrote:
> > Overview
> > 
> > If you're using iSCSI, you can mount a target by multiple Dom0
> > machines on the same target. For non-cluster aware filesystems, this
> > can lead to disk corruption and general bad times by all. The iSCSI
> > protocol allows the use of persistent reservations as per the SCSI
> > disk spec. Low level SCSI commands for locking are handled by the
> > sg_persist program (bundled with sg3_utils package in EL).
> > 
> > The aim of this patch is to create a 'locktarget=y' option specified
> > within the disk 'target' command for iSCSI to lock the target in
> > exclusive mode on VM start with a key generated from the local systems
> > IP, and release this lock on the shutdown of the DomU.
> > 
> > Example Config:
> > disk            =
> > ['script=block-iscsi,vdev=xvda,target=iqn=iqn.1986-03.com.sun:02:mytarget,portal=iscsi.example.com,locktarget=y']
> > 
> > In writing this, I have also re-factored parts of the script to put
> > some things in what I believe to be a better place to make expansion
> > easier. This is mainly in removing functions that purely call other
> > functions with no actual code execution.
> > 
> > Signed-off-by: Steven Haigh <netwiz@crc.id.au>
> > 
> > (on a side note, first time I've submitted a patch to the list and I'm
> > currently stuck on a webmail client, so apologies in advance if this
> > all goes wrong ;)
> 
> Changes in v2:
> Bugfix: Call find_device to locate the /dev/sdX component of the iSCSI
> target before trying to run unlock_device().
> 
> Apologies for this oversight.

Thanks for the patch! A couple of comments below.

> -- 
> Steven Haigh
> 
> Email: netwiz@crc.id.au
> Web: https://www.crc.id.au
> Phone: (03) 9001 6090 - 0412 935 897

> --- block-iscsi 2016-02-10 01:44:19.000000000 +1100
> +++ block-iscsi-lock    2016-05-05 15:42:09.557191235 +1000
> @@ -31,33 +31,37 @@
>      echo $1 | sed "s/^\("$2"\)//"
>  }
>  
> -check_tools()
> -{
> -    if ! command -v iscsiadm > /dev/null 2>&1; then
> -        fatal "Unable to find iscsiadm tool"
> -    fi
> -    if [ "$multipath" = "y" ] && ! command -v multipath > /dev/null 2>&1; then
> -        fatal "Unable to find multipath"
> -    fi
> -}
> -
>  # Sets the following global variables based on the params field passed in as
>  # a parameter: iqn, portal, auth_method, user, multipath, password
>  parse_target()
>  {
>      # set multipath default value
>      multipath="n"
> -    for param in $(echo "$1" | tr "," "\n")
> -    do
> +    for param in $(echo "$1" | tr "," "\n"); do
>          case $param in
>          iqn=*)
>              iqn=$(remove_label $param "iqn=")
> +            if ! command -v iscsiadm > /dev/null 2>&1; then
> +                fatal "Could not find iscsiadm tool."
> +            fi
>              ;;
>          portal=*)
>              portal=$(remove_label $param "portal=")
>              ;;
>          multipath=*)
>              multipath=$(remove_label $param "multipath=")
> +            if ! command -v multipath > /dev/null 2>&1; then
> +                fatal "Multipath selected, but no multipath tools found"
> +            fi
> +            ;;
> +        locktarget=*)
> +            locktarget=$(remove_label $param "locktarget=")
> +            if ! command -v sg_persist > /dev/null 2>&1; then
> +                fatal "Locking requested but no sg_persist found"
> +            fi
> +            if ! command -v gethostip > /dev/null 2>&1; then
> +                fatal "Locking requested but no gethostip found for key generation"
> +            fi

Why don't you just add this to check_tools? In any case, if you want to fold 
check_tools functionality into parse_target I think it should be done in a 
separate patch in order for it to be easier to review.

IMHO, I prefer to have both functions separated, because it's quite clear 
what parse_target and check_tools do. 

>              ;;
>          esac
>      done
> @@ -96,38 +100,29 @@
>      fi
>  }
>  
> -# Attaches the target $iqn in $portal and sets $dev to point to the
> -# multipath device
> -attach()
> -{
> -    do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null
> -    find_device
> -}
>  
> -# Discovers targets in $portal and checks that $iqn is one of those targets
> -# Also sets the auth parameters to attach the device
> -prepare()
> +lock_device()
>  {
> -    # Check if target is already opened
> -    iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened"
> -    # Discover portal targets
> -    iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \
> -        fatal "No matching target iqn found"
> -}
> -
> -# Attaches the device and writes xenstore backend entries to connect
> -# the device
> -add()
> -{
> -    attach
> -    write_dev $dev
> +    ## Lock the iSCSI target as Exclusive Access.
> +    key=$(gethostip -x $(uname -n))

Isn't there anything else that can be used as a key? What would happen if 
the host IP changes while the VM is running? Couldn't the iqn (or a hash 
of it) be used as the key?

> +    if ! sg_persist -d ${dev} -o -G -S ${key} > /dev/null; then
> +        unlock_device
> +        iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null
> +        fatal "iSCSI LOCK: Failed to register with target"
> +    fi
> +    if ! sg_persist -d ${dev} -o -R -K ${key} -T 6 > /dev/null; then
> +        unlock_device
> +        iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null
> +        fatal "iSCSI LOCK: Failed to set persistent reservation"
> +    fi
>  }
>  
> -# Disconnects the device
> -remove()
> +unlock_device()
>  {
> -    find_device
> -    do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null
> +    ## Unlock the iSCSI target.
> +    key=$(gethostip -x $(uname -n))
> +    sg_persist -d ${dev} -o -L -K ${key} -T 6 > /dev/null || true
> +    sg_persist -d ${dev} -o -G -K ${key} -S 0 > /dev/null || true

I'm not sure whether the return code of those functions shouldn't be 
checked. I know this is the teardown path, so there's not much that can 
be done here on failure, but it seems like at least the script should check 
for the return code '3', and then retry the command. According to 
sg3_utils(8), a '3' means:

"the DEVICE reports that it is not ready for the operation requested. The 
device may be in the process of becoming ready (e.g. spinning up but not at 
speed) so the utility may work after a wait."

>  }
> 
>  command=$1
> @@ -138,15 +133,28 @@
> 
>  parse_target "$target"
> 
> -check_tools || exit 1
> -
>  case $command in
>  add)
> -    prepare
> -    add
> +    # Check if target is already opened
> +    iscsiadm -m session 2>&1 | grep -q "$iqn" && fatal "Device already opened"
> +    # Discover portal targets
> +    iscsiadm -m discovery -t st -p $portal 2>&1 | grep -q "$iqn" || \
> +        fatal "No matching target iqn found"
> +
> +    ## Login to the iSCSI target.
> +    do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > /dev/null
> +    find_device
> +    if [ "$locktarget" = "y" ]; then
> +        lock_device
> +    fi
> +    write_dev $dev
>      ;;
>  remove)
> -    remove
> +    find_device
> +    if [ "$locktarget" = "y" ]; then
> +        unlock_device
> +    fi
> +    do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > /dev/null

You are doing quite of a rework of the script here (by removing/merging a 
bunch of functions). I think this should be sent as a separate patch, in 
order to ease the review (mixing new features with code cleanup makes it 
quite hard to review).

Roger.

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