[pve-devel] [PATCH storage 1/3] Make the target cache works per target and portal

[Thomas Lamprecht]

On 9/25/19 10:28 AM, Daniel Berteaud wrote:
> When working with several ZFS over iSCSI / LIO storages, we might lookup
> between them with less than 15 sec interval.
> Previously, the cache of the previous storage was used, which was breaking
> disk move for example
> 

looks Ok from a quick glance, but I would like to avoid nested hash accesses,
e.g.: $foo->{$bar->{baz}}

As the access to @{$SETTINGS->{$scfg->{portal}.$scfg->{target}} is quite
frequent (five times) we could maybe factor it out in a private sub?

my $get_target_settings = sub {
    my ($scfg) = @_;

    my $id = "$scfg->{portal}.$scfg->{target}";
    return $SETTINGS->{$id};
};

then we could do:

my $target = $get_target_settings->($scfg);
for my $lun (@{$target->{luns}}) {
   ...

or 
$target->{luns} = $new;

(as like this, $target is a reference the $SETTINGS hash also sees the changes)

What do you think?

> Signed-off-by: Daniel Berteaud <daniel at firewall-services.com>
> ---
>  PVE/Storage/LunCmd/LIO.pm | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/PVE/Storage/LunCmd/LIO.pm b/PVE/Storage/LunCmd/LIO.pm
> index f246dbb..5f9794d 100644
> --- a/PVE/Storage/LunCmd/LIO.pm
> +++ b/PVE/Storage/LunCmd/LIO.pm
> @@ -145,7 +145,7 @@ my $parser = sub {
>  	# find correct TPG
>  	foreach my $tpg (@{$target->{tpgs}}) {
>  	    if ($tpg->{tag} == $tpg_tag) {
> -		$SETTINGS->{target} = $tpg;
> +		$SETTINGS->{$scfg->{portal}.$scfg->{target}} = $tpg;
>  		$haveTarget = 1;
>  		last;
>  	    }
> @@ -160,16 +160,16 @@ my $parser = sub {
>  
>  # removes the given lu_name from the local list of luns
>  my $free_lu_name = sub {
> -    my ($lu_name) = @_;
> +    my ($scfg, $lu_name) = @_;
>  
>      my $new = [];
> -    foreach my $lun (@{$SETTINGS->{target}->{luns}}) {
> +    foreach my $lun (@{$SETTINGS->{$scfg->{portal}.$scfg->{target}}->{luns}}) {
>  	if ($lun->{storage_object} ne "$BACKSTORE/$lu_name") {
>  	    push @$new, $lun;
>  	}
>      }
>  
> -    $SETTINGS->{target}->{luns} = $new;
> +    $SETTINGS->{$scfg->{portal}.$scfg->{target}}->{luns} = $new;
>  };
>  
>  # locally registers a new lun
> @@ -181,7 +181,7 @@ my $register_lun = sub {
>  	storage_object => "$BACKSTORE/$volname",
>  	is_new => 1,
>      };
> -    push @{$SETTINGS->{target}->{luns}}, $conf;
> +    push @{$SETTINGS->{$scfg->{portal}.$scfg->{target}}->{luns}}, $conf;
>  
>      return $conf;
>  };
> @@ -207,7 +207,7 @@ my $list_view = sub {
>      my $object = $params[0];
>      my $volname = $extract_volname->($scfg, $params[0]);
>  
> -    foreach my $lun (@{$SETTINGS->{target}->{luns}}) {
> +    foreach my $lun (@{$SETTINGS->{$scfg->{portal}.$scfg->{target}}->{luns}}) {
>  	if ($lun->{storage_object} eq "$BACKSTORE/$volname") {
>  	    return $lun->{index};
>  	}
> @@ -224,7 +224,7 @@ my $list_lun = sub {
>      my $object = $params[0];
>      my $volname = $extract_volname->($scfg, $params[0]);
>  
> -    foreach my $lun (@{$SETTINGS->{target}->{luns}}) {
> +    foreach my $lun (@{$SETTINGS->{$scfg->{portal}.$scfg->{target}}->{luns}}) {
>  	if ($lun->{storage_object} eq "$BACKSTORE/$volname") {
>  	    return $object;
>  	}
> @@ -289,7 +289,7 @@ my $delete_lun = sub {
>      my $path = $params[0];
>      my $volname = $extract_volname->($scfg, $params[0]);
>  
> -    foreach my $lun (@{$SETTINGS->{target}->{luns}}) {
> +    foreach my $lun (@{$SETTINGS->{$scfg->{portal}.$scfg->{target}}->{luns}}) {
>  	next if $lun->{storage_object} ne "$BACKSTORE/$volname";
>  
>  	# step 1: delete the lun
> @@ -310,7 +310,7 @@ my $delete_lun = sub {
>  	$execute_remote_command->($scfg, $timeout, $targetcli, 'saveconfig');
>  
>  	# update interal cache
> -	$free_lu_name->($volname);
> +	$free_lu_name->($scfg, $volname);
>  
>  	last;
>      }
> @@ -352,7 +352,7 @@ sub run_lun_command {
>  
>      # fetch configuration from target if we haven't yet or if it is stale
>      my $timediff = time - $SETTINGS_TIMESTAMP;
> -    if (!$SETTINGS || $timediff > $SETTINGS_MAXAGE) {
> +    if (!$SETTINGS || !$SETTINGS->{$scfg->{portal}.$scfg->{target}} || $timediff > $SETTINGS_MAXAGE) {
>  	$SETTINGS_TIMESTAMP = time;
>  	$parser->($scfg);
>      }
>