| Message ID | cb530496edcec7755819be71f11336a7@crc.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > >> > > > >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. > > 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. > > The locktarget option is presented directly to the block-iscsi script and > not evaluated anywhere outside this script. > Sorry I dropped the ball. I think it would be helpful if you or Roger can send a patch to document all the knobs for block-iscsi. It doesn't have to be in that file, we can figure out where to add. FYI we are in the process of making 4.7 release, so the response here might take a bit longer. FAOD this patch is not targeting 4.7, right? I'm not too familiar with block script so I think I will defer this to Roger. I've also CC'ed Ian for you. To make a proper patch, please could you read: http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches Wei. > -- > Steven Haigh > > Email: netwiz@crc.id.au > 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=") > + 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. > + 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 > --- 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 > 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=") > + 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() > +{ > + ## Lock the iSCSI target as Exclusive Access. > + key=$(gethostip -x $(uname -n)) > + 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() > +{ > + ## 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@lists.xen.org > http://lists.xen.org/xen-devel
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@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. > > > > > > > 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@crc.id.au > 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 > ;; > *)
--- 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 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=") + 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() +{ + ## Lock the iSCSI target as Exclusive Access. + key=$(gethostip -x $(uname -n)) + 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() +{ + ## 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 ;; *)