geom labels as aliases

2021-09-29 Thread Andriy Gapon 



For an introduction, I never liked consequences of GEOM labels being implemented 
as geoms.  And I specifically do not mean explicitly created labels via glabel 
create / label.  I mean GEOM labels based on partition and filesystem labels, 
disk IDs, etc,  And I don't like things such as opening a device via one label 
spoiling other labels.


So, I've been reading through some recent-ish changes by Warner for GEOM aliases 
and I've got this (maybe not so) bright idea, what if those labels were 
implemented as aliases.  Would that work?  Would that require a lot of work?


I did a quick hack as a proof of concept.
The change is quite small.

diff --git a/sys/geom/label/g_label.c b/sys/geom/label/g_label.c
index 1df7e799b014..72b97d314de4 100644
--- a/sys/geom/label/g_label.c
+++ b/sys/geom/label/g_label.c
@@ -418,12 +418,15 @@ g_label_taste(struct g_class *mp, struct g_provider *pp, 
int flags __unused)

if (label[0] == '\0')
continue;
if (g_labels[i] != &g_label_generic) {
-   mediasize = pp->mediasize;
+   if (g_label_is_name_ok(label)) {
+   g_provider_add_alias(pp, "%s%s",
+   g_labels[i]->ld_dirprefix, label);
+   }
} else {
mediasize = pp->mediasize - pp->sectorsize;
+   g_label_create(NULL, mp, pp, label,
+   g_labels[i]->ld_dirprefix, mediasize);
}
-   g_label_create(NULL, mp, pp, label,
-   g_labels[i]->ld_dirprefix, mediasize);
}
g_access(cp, -1, 0, 0);
 end:

This seems to work in a basic smoke test of just booting up with the change.

root@freebsd:~ # cat /etc/fstab
# Custom /etc/fstab for FreeBSD VM images
/dev/gpt/rootfs   /   ufs rw  1   1
/dev/gpt/swapfs  noneswapsw  0   0
/dev/gpt/efiesp /boot/efi   msdosfs rw  2   2
root@freebsd:~ # mount
/dev/gpt/rootfs on / (ufs, local, soft-updates)
devfs on /dev (devfs)
/dev/gpt/efiesp on /boot/efi (msdosfs, local)

root@freebsd:~ # gpart show
=>   3  20971509  ada0  GPT  (10G)
 3   127 1  freebsd-boot  (64K)
   130 66584 2  efi  (33M)
 66714   2097152 3  freebsd-swap  (1.0G)
   2163866  18807646 4  freebsd-ufs  (9.0G)

=> 63  2097089  ada1  MBR  (1.0G)
   631- free -  (512B)
   64  2097088 1  freebsd  (1.0G)
root@freebsd:~ # gpart show -l
=>   3  20971509  ada0  GPT  (10G)
 3   127 1  bootfs  (64K)
   130 66584 2  efiesp  (33M)
 66714   2097152 3  swapfs  (1.0G)
   2163866  18807646 4  rootfs  (9.0G)

=> 63  2097089  ada1  MBR  (1.0G)
   631- free -  (512B)
   64  2097088 1  (null)  (1.0G)

root@freebsd:~ # find /dev -type l -ls | column -t
...
91   0  lrwxr-xr-x  1  root  wheel  7   Sep  29  08:00  /dev/diskid/DISK-QM00013 
->  ../ada0
95   0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/diskid/DISK-QM00013p1   ->  ../ada0p1
100  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/diskid/DISK-QM00013p2   ->  ../ada0p2
104  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/diskid/DISK-QM00013p3   ->  ../ada0p3
110  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/diskid/DISK-QM00013p4   ->  ../ada0p4
112  0  lrwxr-xr-x  1  root  wheel  7   Sep  29  08:00  /dev/diskid/DISK-QM00015 
->  ../ada1
114  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/diskid/DISK-QM00015s1   ->  ../ada1s1
93   0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/gptid/2313a319-9911-11eb-bf4c-002590ec5bf2  ->  ../ada0p1
98   0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/gptid/2313a323-9911-11eb-bf4c-002590ec5bf2  ->  ../ada0p2
102  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/gptid/2313a328-9911-11eb-bf4c-002590ec5bf2  ->  ../ada0p3
108  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00 
/dev/gptid/2313a32d-9911-11eb-bf4c-002590ec5bf2  ->  ../ada0p4
94   0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00  /dev/gpt/bootfs 
 ->  ../ada0p1
99   0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00  /dev/gpt/efiesp 
 ->  ../ada0p2
103  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00  /dev/gpt/swapfs 
 ->  ../ada0p3
109  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00  /dev/gpt/rootfs 
 ->  ../ada0p4
97   0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00  /dev/msdosfs/EFISYS 
 ->  ../ada0p2
106  0  lrwxr-xr-x  1  root  wheel  9   Sep  29  08:00  /dev/ufs/rootfs 
 ->  ../ada0p4
107  0  lrwxr-xr-x  1  root  wheel  9   Sep  29

Re: geom labels as aliases

2021-09-29 Thread Andriy Gapon 

On 29/09/2021 16:20, Warner Losh wrote:



On Wed, Sep 29, 2021 at 2:37 AM Andriy Gapon > wrote:



For an introduction, I never liked consequences of GEOM labels being
implemented
as geoms.  And I specifically do not mean explicitly created labels via 
glabel
create / label.  I mean GEOM labels based on partition and filesystem 
labels,
disk IDs, etc,  And I don't like things such as opening a device via one 
label
spoiling other labels.

So, I've been reading through some recent-ish changes by Warner for GEOM
aliases
and I've got this (maybe not so) bright idea, what if those labels were
implemented as aliases.  Would that work?  Would that require a lot of work?

I did a quick hack as a proof of concept.
The change is quite small.

diff --git a/sys/geom/label/g_label.c b/sys/geom/label/g_label.c
index 1df7e799b014..72b97d314de4 100644
--- a/sys/geom/label/g_label.c
+++ b/sys/geom/label/g_label.c
@@ -418,12 +418,15 @@ g_label_taste(struct g_class *mp, struct g_provider 
*pp,
int flags __unused)
                 if (label[0] == '\0')
                         continue;
                 if (g_labels[i] != &g_label_generic) {
-                       mediasize = pp->mediasize;
+                       if (g_label_is_name_ok(label)) {
+                               g_provider_add_alias(pp, "%s%s",
+                                   g_labels[i]->ld_dirprefix, label);
+                       }
                 } else {
                         mediasize = pp->mediasize - pp->sectorsize;
+                       g_label_create(NULL, mp, pp, label,
+                           g_labels[i]->ld_dirprefix, mediasize);
                 }
-               g_label_create(NULL, mp, pp, label,
-                   g_labels[i]->ld_dirprefix, mediasize);
         }
         g_access(cp, -1, 0, 0);
   end:

This seems to work in a basic smoke test of just booting up with the change.

[snip]


Of course, the change can break existing scripts / tools that count on 
labels
having their own geoms.

Also, it can be argued that it would be better to create such symbolic 
links in
userland.  It should be easy to port the label tasting code to userland and
hook
it to devd events.  That should cover all use cases except for the root
filesystem.

Finally, I haven't tested glabel create / label yet, so the change may need
some
more work in that area.

What is your opinion?
Is this something worth pursuing?


Conrad did something similar in 5b9b571cb and wound up reverting it. Too many
unforeseen issues, IIRC.


Whoosh, I completely missed that commit.
Unfortunately, the revert message it too terse:

Revert r361838

Reported by:delphij

I wonder if the reported problem was indeed too hard to overcome.

However, I already see a number of issues in my simplistic change.
First of all, removing obsolete aliases.

I am thinking that maybe each "alias label" should still have a geom attached to 
the corresponding provider but without a (downstream) provider of its own.  This 
way we can still receive events for spoiling, etc without introducing an 
alternative provider.  We then can manipulate aliases in response to the 
upstream changes.


Yeah, I see, a lot more work is needed.

--
Andriy Gapon

Re: geom labels as aliases

2021-09-29 Thread Conrad Meyer 
https://lists.freebsd.org/pipermail/freebsd-current/2020-June/076210.html


On Wed, Sep 29, 2021 at 07:20 Warner Losh  wrote:

>
>
> On Wed, Sep 29, 2021 at 7:36 AM Andriy Gapon  wrote:
>
>> On 29/09/2021 16:20, Warner Losh wrote:
>> >
>> >
>> > On Wed, Sep 29, 2021 at 2:37 AM Andriy Gapon > > > wrote:
>> >
>> >
>> > For an introduction, I never liked consequences of GEOM labels being
>> > implemented
>> > as geoms.  And I specifically do not mean explicitly created labels
>> via glabel
>> > create / label.  I mean GEOM labels based on partition and
>> filesystem labels,
>> > disk IDs, etc,  And I don't like things such as opening a device
>> via one label
>> > spoiling other labels.
>> >
>> > So, I've been reading through some recent-ish changes by Warner for
>> GEOM
>> > aliases
>> > and I've got this (maybe not so) bright idea, what if those labels
>> were
>> > implemented as aliases.  Would that work?  Would that require a lot
>> of work?
>> >
>> > I did a quick hack as a proof of concept.
>> > The change is quite small.
>> >
>> > diff --git a/sys/geom/label/g_label.c b/sys/geom/label/g_label.c
>> > index 1df7e799b014..72b97d314de4 100644
>> > --- a/sys/geom/label/g_label.c
>> > +++ b/sys/geom/label/g_label.c
>> > @@ -418,12 +418,15 @@ g_label_taste(struct g_class *mp, struct
>> g_provider *pp,
>> > int flags __unused)
>> >  if (label[0] == '\0')
>> >  continue;
>> >  if (g_labels[i] != &g_label_generic) {
>> > -   mediasize = pp->mediasize;
>> > +   if (g_label_is_name_ok(label)) {
>> > +   g_provider_add_alias(pp, "%s%s",
>> > +   g_labels[i]->ld_dirprefix,
>> label);
>> > +   }
>> >  } else {
>> >  mediasize = pp->mediasize - pp->sectorsize;
>> > +   g_label_create(NULL, mp, pp, label,
>> > +   g_labels[i]->ld_dirprefix, mediasize);
>> >  }
>> > -   g_label_create(NULL, mp, pp, label,
>> > -   g_labels[i]->ld_dirprefix, mediasize);
>> >  }
>> >  g_access(cp, -1, 0, 0);
>> >end:
>> >
>> > This seems to work in a basic smoke test of just booting up with
>> the change.
>> [snip]
>> >
>> > Of course, the change can break existing scripts / tools that count
>> on labels
>> > having their own geoms.
>> >
>> > Also, it can be argued that it would be better to create such
>> symbolic links in
>> > userland.  It should be easy to port the label tasting code to
>> userland and
>> > hook
>> > it to devd events.  That should cover all use cases except for the
>> root
>> > filesystem.
>> >
>> > Finally, I haven't tested glabel create / label yet, so the change
>> may need
>> > some
>> > more work in that area.
>> >
>> > What is your opinion?
>> > Is this something worth pursuing?
>> >
>> >
>> > Conrad did something similar in 5b9b571cb and wound up reverting it.
>> Too many
>> > unforeseen issues, IIRC.
>>
>> Whoosh, I completely missed that commit.
>> Unfortunately, the revert message it too terse:
>>
>>  Revert r361838
>>
>>  Reported by:delphij
>>
>> I wonder if the reported problem was indeed too hard to overcome.
>>
>> However, I already see a number of issues in my simplistic change.
>> First of all, removing obsolete aliases.
>>
>> I am thinking that maybe each "alias label" should still have a geom
>> attached to
>> the corresponding provider but without a (downstream) provider of its
>> own.  This
>> way we can still receive events for spoiling, etc without introducing an
>> alternative provider.  We then can manipulate aliases in response to the
>> upstream changes.
>>
>
> Aliases are automatically removed when the providing geom goes away,
> which as part of the tasting process automatically happens.
>
> I too wish the commit log was more complete, or had a pointer
> to the exact details.
>
> Warner
>
>
>

Re: geom labels as aliases

2021-09-29 Thread Andriy Gapon 

On 29/09/2021 20:28, Xin LI wrote:
For context, the discussion was at 
https://lists.freebsd.org/pipermail/freebsd-current/2020-June/thread.html#76210 .


My takeaway[*], perhaps incorrect, from reading that is that the problem is with 
this check in g_label_taste():


/* Skip providers that are already open for writing. */
if (pp->acw > 0)
return (NULL);

On the one hand this looks reasonable, on the other hand if a disk is open for 
writing it does not mean that its can change (it cannot).  If a partition is 
open for writing it does not mean that its label can change (it cannot unless 
its container is open for writing as well).  It does not necessarily mean that a 
filesystem label will change (although it can).


We probably need a better mechanism to communicate label changes than the access 
bits alone.


As a quick solution, perhaps we could add a flag that allows a label class to 
declare that it can safely test a provider even when it's open for writing?


[*]
I made my conclusion based on this snippet:

ZFS gets an exclusive hold of 'ada1p2' despite the pool is carefully created
to use /dev/diskid/p2 instead of ada1p2.

...
However, this will prevent GEOM from properly creating 
/dev/diskid/.



--
Andriy Gapon

[Bug 258787] geli leaks memory

2021-09-29 Thread bugzilla-noreply 
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=258787

Mark Linimon  changed:

   What|Removed |Added

   Assignee|b...@freebsd.org|g...@freebsd.org

-- 
You are receiving this mail because:
You are the assignee for the bug.