[pve-devel] [PATCH container] fix #1607: implement pct fstrim

[Thomas Lamprecht]

On 3/21/19 11:24 AM, Oguz Bektas wrote:
> hi,
> 
> On Thu, Mar 21, 2019 at 06:59:42AM +0100, Thomas Lamprecht wrote:
>> On 3/20/19 1:56 PM, Oguz Bektas wrote:
>>> runs fstrim on the rootfs and all mountpoints of a given container.
>>>
>>> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
>>> ---
>>>  src/PVE/CLI/pct.pm | 41 +++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 41 insertions(+)
>>>
>>> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
>>> index 794bc45..f89e183 100755
>>> --- a/src/PVE/CLI/pct.pm
>>> +++ b/src/PVE/CLI/pct.pm
>>> @@ -755,6 +755,45 @@ __PACKAGE__->register_method ({
>>>  	return undef;
>>>      }});
>>>  
>>> +__PACKAGE__->register_method ({
>>> +    name => 'fstrim',
>>> +    path => 'fstrim',
>>> +    method => 'POST',
>>> +    description => "Run fstrim on a chosen CT.",
>>> +    parameters => {
>>> +	additionalProperties => 0,
>>> +	properties => {
>>> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
>>> +	},
>>> +    },
>>> +    returns => { type => 'null' },
>>> +    code => sub {
>>> +
>>> +	my ($param) = @_;
>>> +	my $vmid = $param->{'vmid'};
>>> +
>>> +	my $rootdir = "/var/lib/lxc/$vmid/rootfs";
>>> +
>>> +	my $storecfg = PVE::Storage::config();
>>> +	PVE::LXC::Config->lock_config($vmid, sub {
>>
> re previous message about running cts: i've tested both on running and
> stopped cts, worked as expected


then write that.

>  
>> oh, and why lock? "lock_config" is for short running operations,
>> for longer you should use it only to place a "lock" entry to the
>> config, do your long operation and then remove the lock again in
>> a lock_config call.
>>
>> But for fstrim you do not need a lock at all, or?
> the operation itself takes about a few seconds usually, i put the

it may take that long on *your hardware*, but it can take much longer
elsewhere, this is an I/O operation send to the disks (controller), it
can take minutes to hours, there's no guarantee that it just needs a
few seconds. We simply do not do such operations in lock_config if
possible...

> lock_config there to make sure nothing funny happens while we're mounting the
> container in order to run fstrim on it and then unmounting. i figured
> it's fitting in this situation. what do you think?

see above, also what should happen? you get the config once, if there are
MPs removed during that time you get an error, which is catched and printed,
if one is added it did not receive a fstrim (but probably has nothing to
discard anyway) and the admin can always rerun this.

I mean he can already do:

# pct exec VMID fstrim -av

now - at least if the CT is running... so not sure about this.. also, any
modern distro has a fstrim timer/cron job anyway?

>>
>>> +	    my $conf = PVE::LXC::Config->load_config($vmid);
>>> +	    PVE::LXC::mount_all($vmid, $storecfg, $conf);
>>> +	    eval {
>>> +		my $path = "";
>>> +		PVE::LXC::Config->foreach_mountpoint($conf, sub {
>>> +		    my ($name, $mp) = @_;
>>> +		    $path = $mp->{mp};
>>> +		    my $cmd = ["fstrim", "-v", "$rootdir$path"];
>>> +		    PVE::Tools::run_command($cmd);
>>> +		});
>>> +	    };
>>> +
>>> +	    PVE::LXC::umount_all($vmid, $storecfg, $conf, 0);
>>> +	});
>>> +
>>> +	return undef;
>>> +    }});
>>> +
>>>  our $cmddef = {
>>>      list=> [ 'PVE::API2::LXC', 'vmlist', [], { node => $nodename }, sub {
>>>  	my $res = shift;
>>> @@ -841,6 +880,8 @@ our $cmddef = {
>>>  
>>>      cpusets => [ __PACKAGE__, 'cpusets', []],
>>>  
>>> +    fstrim => [ __PACKAGE__, 'fstrim', ['vmid']],
>>> +
>>>  };
>>>  
>>>  
>>>
>>