Re: [Xen-devel] Resend: [PATCH] v3 - Add exclusive locking option to block-iscsi

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 19, 2016 at 11:29:32AM +1000, Steven Haigh wrote:
> On 2016-05-09 14:22, Steven Haigh wrote:
> > On 2016-05-05 15:52, 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@xxxxxxxxx>
> > > > 
> > > > (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.
> > > 
> > 
> > Changes in v3:
> > * Split the block-iscsi cleanup into a seperate patch
> > (block-iscsi-locking-v3_01_simplify_block-iscsi.patch).
> > * Add locking in second patch file
> > (block-iscsi-locking-v3_02_add_locking.patch)
> 
> Resend of patches.

IMHO, if those patches are going to be committed to Xen they need to be 
sent using git, they are (at least) missing a proper changelog.

> There was a mention of having to add further documentation to
> xl-disk-configuration.txt - however there are no mentions of block-iscsi
> script within the documentation to add. As such, it probably would be out of
> place to add things here.

Hm, I don't think we have ever added specific block-scripts configuration 
options to xl-disk-configuration.txt. What I did was to instead add this 
information at the top of the script file itself, and the locktarget option 
should be documented there together with the other options. Sadly I see that 
the 'multipath' option did not add the documentation.

> The locktarget option is presented directly to the block-iscsi script and
> not evaluated anywhere outside this script.
> 
> -- 
> Steven Haigh
> 
> Email: netwiz@xxxxxxxxx
> Web: https://www.crc.id.au
> Phone: (03) 9001 6090 - 0412 935 897

> --- block-iscsi.orig    2016-05-09 15:12:02.489495212 +1000
> +++ block-iscsi 2016-05-09 15:16:35.447480532 +1000
> @@ -31,16 +31,6 @@
>      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()
> @@ -52,12 +42,18 @@
>          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=")

This is wrong, multipath can have the values 'y' or 'n' only, and there's a 
check below that makes sure of that. Here you would be checking if 
'multipath' is available even if multipath=n has been set.

IMHO, I think that having a separation between the option parser and the 
tools availability check makes sense, and avoids mistakes like this.

> +            if ! command -v multipath > /dev/null 2>&1; then
> +                fatal "Multipath selected, but no multipath tools found"
> +            fi
>              ;;
>          esac
>      done
> @@ -96,40 +92,6 @@
>      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()
> -{
> -    # 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
> -}
> -
> -# Disconnects the device
> -remove()
> -{
> -    find_device
> -    do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > 
> /dev/null
> -}
> -
>  command=$1
>  target=$(xenstore-read $XENBUS_PATH/params || true)
>  if [ -z "$target" ]; then
> @@ -138,15 +100,21 @@
> 
>  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.

Why the double #?

> +    do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --login > 
> /dev/null
> +
> +    write_dev $dev
>      ;;
>  remove)
> -    remove
> +    do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > 
> /dev/null
>      ;;
>  *)
>      exit 1 

IMHO, I prefer the script as it is now, probably because I've written it 
myself. I think the functions are clearly documented, and I'm still not sure 
why the locktarget option cannot be added without the refactoring.

> --- block-iscsi.orig 2016-05-09 15:16:35.447480532 +1000
> +++ block-iscsi    2016-05-05 15:43:58.222159351 +1000
> @@ -37,8 +37,7 @@
>  {
>      # set multipath default value
>      multipath="n"
> -    for param in $(echo "$1" | tr "," "\n")
> -    do
> +    for param in $(echo "$1" | tr "," "\n"); do

Spurious change? There doesn't seem to be any need for it.

>          case $param in
>          iqn=*)
>              iqn=$(remove_label $param "iqn=")
> @@ -55,6 +54,15 @@
>                  fatal "Multipath selected, but no multipath tools found"
>              fi
>              ;;
> +        locktarget=*)
> +            locktarget=$(remove_label $param "locktarget=")

Since this is a boolean option, you would have to check that locktarget is 
either 'y' or 'n' only, and bail out on other values. For example see how 
the multipath option does it a couple of lines below.

> +            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
> +            ;;
>          esac
>      done
>      if [ -z "$iqn" ] || [ -z "$portal" ]; then
> @@ -92,6 +100,31 @@
>      fi
>  }
> 
> +
> +lock_device()

This needs a comment describing what the function is expected to do.

> +{
> +    ## Lock the iSCSI target as Exclusive Access.
> +    key=$(gethostip -x $(uname -n))

I still don't like using the hostip as a key, isn't there anything else more 
suitable? Can't you use something like a host UUID?

> +    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
> +}
> +
> +unlock_device()

Same here, a small comment would make sense IMHO.

> +{
> +    ## 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
> +}
> +
>  command=$1
>  target=$(xenstore-read $XENBUS_PATH/params || true)
>  if [ -z "$target" ]; then
> @@ -110,10 +143,17 @@
> 
>      ## 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)
> +    find_device
> +    if [ "$locktarget" = "y" ]; then
> +        unlock_device
> +    fi
>      do_or_die iscsiadm -m node --targetname "$iqn" -p "$portal" --logout > 
> /dev/null
>      ;;
>  *) 


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