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 <dan...@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);
> }
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
