* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal