* [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate
@ 2021-06-24 7:29 Wolfgang Bumiller
2021-06-24 7:56 ` Fabian Grünbichler
0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2021-06-24 7:29 UTC (permalink / raw)
To: pve-devel
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);
}
@@ -179,6 +201,14 @@ sub btrfs_cmd {
return $msg;
}
+sub on_add_hook {
+ my ($class, $storeid, $scfg, %param) = @_;
+
+ assert_btrfs($scfg->{path});
+
+ return;
+}
+
sub btrfs_get_subvol_id {
my ($class, $path) = @_;
my $info = $class->btrfs_cmd(['subvolume', 'show', '--', $path]);
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate
2021-06-24 7:29 [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate Wolfgang Bumiller
@ 2021-06-24 7:56 ` Fabian Grünbichler
2021-06-24 8:43 ` Wolfgang Bumiller
2021-06-24 9:10 ` Thomas Lamprecht
0 siblings, 2 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2021-06-24 7:56 UTC (permalink / raw)
To: Proxmox VE development discussion; +Cc: Wolfgang Bumiller
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..
> }
>
> @@ -179,6 +201,14 @@ sub btrfs_cmd {
> return $msg;
> }
>
> +sub on_add_hook {
> + my ($class, $storeid, $scfg, %param) = @_;
> +
> + assert_btrfs($scfg->{path});
not needed - adding a new storage calls activate_storage if the storage
is enabled..
> +
> + return;
> +}
> +
> sub btrfs_get_subvol_id {
> my ($class, $path) = @_;
> my $info = $class->btrfs_cmd(['subvolume', 'show', '--', $path]);
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate
2021-06-24 7:56 ` Fabian Grünbichler
@ 2021-06-24 8:43 ` Wolfgang Bumiller
2021-06-24 9:10 ` Thomas Lamprecht
1 sibling, 0 replies; 6+ messages in thread
From: Wolfgang Bumiller @ 2021-06-24 8:43 UTC (permalink / raw)
To: Fabian Grünbichler; +Cc: Proxmox VE development discussion
On Thu, Jun 24, 2021 at 09:56:58AM +0200, 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..
ack, sending v2
>
> > }
> >
> > @@ -179,6 +201,14 @@ sub btrfs_cmd {
> > return $msg;
> > }
> >
> > +sub on_add_hook {
> > + my ($class, $storeid, $scfg, %param) = @_;
> > +
> > + assert_btrfs($scfg->{path});
>
> not needed - adding a new storage calls activate_storage if the storage
> is enabled..
removing in v2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate
2021-06-24 7:56 ` Fabian Grünbichler
2021-06-24 8:43 ` Wolfgang Bumiller
@ 2021-06-24 9:10 ` Thomas Lamprecht
2021-06-24 9:23 ` Fabian Grünbichler
1 sibling, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2021-06-24 9:10 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
Cc: Wolfgang Bumiller
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?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate
2021-06-24 9:10 ` Thomas Lamprecht
@ 2021-06-24 9:23 ` Fabian Grünbichler
2021-06-24 9:27 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Fabian Grünbichler @ 2021-06-24 9:23 UTC (permalink / raw)
To: Proxmox VE development discussion, Thomas Lamprecht; +Cc: Wolfgang Bumiller
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..)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate
2021-06-24 9:23 ` Fabian Grünbichler
@ 2021-06-24 9:27 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2021-06-24 9:27 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
Cc: Wolfgang Bumiller
On 24.06.21 11:23, Fabian Grünbichler wrote:
> 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:
>>>> 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..)
>
I just duplicated the DirPlugin activate storage, could be factored out maybe but
for now I prefer it as is, using actual plugin methods from other plugins feels
always a bit weird and risky, as on the use-site one is seldom aware of that when
changing things there, risking breakage - so in the longer term I'd like that the
more generic stuff moves to a "static" helper module, not having a export-base.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-24 9:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-24 7:29 [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate Wolfgang Bumiller
2021-06-24 7:56 ` Fabian Grünbichler
2021-06-24 8:43 ` Wolfgang Bumiller
2021-06-24 9:10 ` Thomas Lamprecht
2021-06-24 9:23 ` Fabian Grünbichler
2021-06-24 9:27 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox