[pve-devel] [PATCH v7 pve-storage 05/10] Refactor method to find next lun number.

[Fabian Grünbichler]

On Tue, Jun 20, 2017 at 10:39:57PM +0200, mir at datanom.net wrote:
> From: Michael Rasmussen <mir at datanom.net>
> 
> Signed-off-by: Michael Rasmussen <mir at datanom.net>
> ---
>  PVE/Storage/FreeNASPlugin.pm | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/Storage/FreeNASPlugin.pm b/PVE/Storage/FreeNASPlugin.pm
> index 3a18a31..91a1b0c 100644
> --- a/PVE/Storage/FreeNASPlugin.pm
> +++ b/PVE/Storage/FreeNASPlugin.pm
> @@ -534,24 +534,37 @@ my $freenas_get_lun_number = sub {
>          $lunid = $2 - 1;
>      } elsif ($volname =~ /^vm-(\d+)-state/) {
>          # Find id for temporary LUN
> +        my %used_luns;
>          my $target = $freenas_get_target->($scfg, $1);
> -        my $id = $max_luns;
>          my $t2extents = $freenas_request->($scfg, 'GET', "services/iscsi/targettoextent/");
>  
> +        # Looking for existing LUN
> +        my $eid = $freenas_get_extent->($scfg, $volname);
> +
>          foreach my $t2extent (@$t2extents) {
> +            die "Max snapshots ($active_snaps) is reached\n"
> +            unless keys %used_luns < $active_snaps;

unless & wrongly indented

> +
>              next unless $t2extent->{iscsi_target} == $target &&
>                          $t2extent->{iscsi_lunid} + 1 > $max_luns &&
>                          $t2extent->{iscsi_lunid} < $max_luns + $active_snaps;

unless

> -            my $eid = $freenas_get_extent->($scfg, $volname);
> -            if ($eid) {
> -                my $extent = $freenas_request->($scfg, 'GET', "services/iscsi/extent/$eid/");
> -                # Request to get lunid for an existing lun
> -                last if $t2extent->{iscsi_extent} eq $eid;
> +
> +            if (defined $eid && $t2extent->{iscsi_extent} == $eid) {
> +                $lunid = $t2extent->{iscsi_lunid};
> +                last;
>              }

return $t2extent->{iscsi_lunid}
  if ...;

?

> -            $id++;
> +
> +            $used_luns{$t2extent->{iscsi_lunid}} = $t2extent;

you only use the key, so why not set to 1? or even use a list, since I
guess you won't get a LUN ID back twice?

>          }
> -        die "Max snapshots ($active_snaps) is reached\n" unless ($id - $max_luns) < $active_snaps;
> -        $lunid = $id;
> +
> +        do {
> +            if (%used_luns) {
> +                my @ids = sort { $a <=> $b } keys %used_luns;
> +                $lunid = $ids[-1] + 1;

what if @ids is [20, 21, 23] (because lun 22 got deactivated/deleted/...)?
then $lunid could now be outside the allowed range? wouldn't taking the
first free slot make more sense?

> +            } else {
> +                $lunid = $max_luns;
> +            }
> +        } unless $lunid;

syntax! also, if you return above instead of last + setting $lunid, you
don't even need it :)

>      } elsif ($volname =~ /^(vm|base)-\d+-disk-\d+\@vzdump$/) {
>          # Required to be able to exposed read-only LUNs for snapshot backup CT
>          $lunid = $max_luns + $active_snaps;
> -- 
> 2.11.0
> 
> 
> ----
> 
> This mail was virus scanned and spam checked before delivery.
> This mail is also DKIM signed. See header dkim-signature.
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel