Re: [PATCH cluster v2 2/2] cfs lock: unlock when encountering signal

[Fiona Ebner <f.ebner@proxmox.com>]

Am 18.02.26 um 7:33 PM schrieb Thomas Lamprecht:
> Am 18.02.26 um 16:45 schrieb Fiona Ebner:
>> If the lock directory is not removed after failing because of a
>> signal, it won't be possible to acquire the lock anymore before the
>> 120 second timeout imposed on the lock by pmxcfs. This can easily
>> happen by a second, unrelated task in production and is quite
>> surprising. Install a signal handler that releases the lock if it was
>> already acquired. If an old handler is defined, it is invoked,
>> otherwise the signal is raised again. Just using 'die' would change
>> the execution flow compared to before the change.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>  src/PVE/Cluster.pm | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
>> index bdb465f..7165d1c 100644
>> --- a/src/PVE/Cluster.pm
>> +++ b/src/PVE/Cluster.pm
>> @@ -615,6 +615,22 @@ my $cfs_lock = sub {
>>  
>>      my $is_code_err = 0;
>>      eval {
>> +        # catch signals to release the lock - further defer to old handler if one was set
>> +        my $old_sig;
>> +        $old_sig->{$_} = $SIG{$_} for qw(INT TERM QUIT HUP PIPE);
> 
> really a non-issue in practice and basically the same thing under the hood, but
> this could probably just a map, something like (untested):
> 
> my $old_sig = { map { $_ => $SIG{$_} qw(INT TERM QUIT HUP PIPE) };

Will do!

>> +
>> +        local $SIG{INT} = local $SIG{TERM} = local $SIG{QUIT} = local $SIG{HUP} =
>> +            local $SIG{PIPE} = sub {
>> +                my $signame = $_[0];
>> +                rmdir $filename if $got_lock; # if we held the lock always unlock again
> 
> Could be nice to output a warning if above rmdir fails?

Good point! Will also add it to the original line I copied this from.

>> +                if ($old_sig->{$signame}) {
>> +                    $old_sig->{$signame}->(@_);
>> +                } else {
>> +                    $SIG{$signame} = 'DEFAULT';
>> +                    POSIX::raise($signame);
> 
> hmm, this reads alright, but then I'm wondering if it should be added elsewhere?
> As I found not a single "POSIX::raise" or "raise\(" instance in our perl code
> inside the /usr/share/perl5/{PVE,Proxmox} directories on a recent PVE 9 system, but
> we have quite a few signal overrides, and while I did not checked those, I do believe
> to remember that some of those fallback to the handler defined by the calling site.

The only ones I found that do invoke the previous handler are in
PVE::Daemon. They also do not use raise, but terminate the server.

For some other ones it's most likely intentional to convert the signal
to a simple die. For example PVE:VZDump::QemuServer, where it makes
sense to just catch the signal and proceed with aborting the backup
rather than raise it again.

Compared to those, cfs_lock() is quite low in the call chains and there
are callers that just warn about an error from cfs_lock(). So while it
is essential to not convert a signal to a simple die in cfs_lock(), it
might not be for other current signal overrides.

> Describing how exactly the code flow changes would be nice in any case.

Do you mean expanding on the sentence mentioning "code flow" in the
commit message or something else?

>> +                }
>> +                die "interrupted by signal\n";
>> +            };
>>  
>>          mkdir $lockdir;
>>