Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On June 24, 2021 11:10 am, Thomas Lamprecht wrote:
> On 24.06.21 09:56, Fabian Grünbichler wrote:
>> On June 24, 2021 9:29 am, Wolfgang Bumiller wrote:
>>> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
>>> ---
>>>  PVE/Storage/BTRFSPlugin.pm | 30 ++++++++++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>>> index 133edc6..0e111a0 100644
>>> --- a/PVE/Storage/BTRFSPlugin.pm
>>> +++ b/PVE/Storage/BTRFSPlugin.pm
>>> @@ -20,6 +20,7 @@ use constant {
>>>      FS_NOCOW_FL => 0x00800000,
>>>      FS_IOC_GETFLAGS => 0x40086602,
>>>      FS_IOC_SETFLAGS => 0x80086601,
>>> +    BTRFS_MAGIC => 0x9123683e,
>>>  };
>>>  
>>>  # Configuration (similar to DirPlugin)
>>> @@ -89,8 +90,29 @@ sub check_config {
>>>      return PVE::Storage::DirPlugin::check_config($self, $sectionId, $config, $create, $skipSchemaCheck);
>>>  }
>>>  
>>> +my sub getfsmagic($) {
>>> +    my ($path) = @_;
>>> +    # The field type sizes in `struct statfs` are defined in a rather annoying way, and we only
>>> +    # need the first field, which is a `long` for our supported platforms.
>>> +    # Should be moved to pve-rs, so this can be the problem of the `libc` crate ;-)
>>> +    # Just round up and extract what we need:
>>> +    my $buf = pack('x160');
>>> +    if (0 != syscall(&PVE::Syscall::SYS_statfs, $path, $buf)) {
>>> +	die "statfs on '$path' failed - $!\n";
>>> +    }
>>> +
>>> +    return unpack('L!', $buf);
>>> +}
>>> +
>>> +my sub assert_btrfs($) {
>>> +    my ($path) = @_;
>>> +    die "'$path' is not a btrfs file system\n"
>>> +	if getfsmagic($path) != BTRFS_MAGIC;
>>> +}
>>> +
>>>  sub activate_storage {
>>>      my ($class, $storeid, $scfg, $cache) = @_;
>>> +    assert_btrfs($scfg->{path});
>>>      return PVE::Storage::DirPlugin::activate_storage($class, $storeid, $scfg, $cache);
>> shouldn't this be the other way round? first check for things like 
>> is_mountpoint, then whether btrfs is there.. makes for less confusing 
>> error message at least..
>> 
> 
> But then we create already the sub-directories in DirPlugin's SUPER->activate_storage call
> to the base plugin one and leave that stuff over when the assert fails?
> 

true. but OTOH, we do support dir storages where $path does not exist 
yet before the first activation..

maybe

if is_mountpoint check that mountpoint // path with DirPlugin::path_is_mounted && btrfs

then call activate_storage from dir plugin

then check $path is btrfs

most setups should have is_mountpoint set (except maybe / on btrfs with 
no separate "data" filesystem..), so this should handle most of it. if 
we pull in the mkdir $path handling into the BTRFSPlugin, then 
everything would be handled (and only the subdir creation is delegate to 
the DirPlugin..)