Date: Mon, 7 Aug 2017 15:20:15 -0700 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r322198 - in head: share/man/man9 sys/geom Message-ID: <cc7bb1f5-a5b7-334f-a3e2-2dd6735fbcc1@freebsd.org> In-Reply-To: <CANCZdfpOKZ0TnZF6g1ABMhtFAA8KwBv0mMWQ=4Tkm0P3XY-fKQ@mail.gmail.com> References: <201708072112.v77LCcXm001489@repo.freebsd.org> <ff9f9256-bd4e-0096-ed24-742ef8aed24a@freebsd.org> <CANCZdfpOKZ0TnZF6g1ABMhtFAA8KwBv0mMWQ=4Tkm0P3XY-fKQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 08/07/17 14:32, Warner Losh wrote: > > > On Mon, Aug 7, 2017 at 3:19 PM, Nathan Whitehorn > <nwhitehorn@freebsd.org <mailto:nwhitehorn@freebsd.org>> wrote: > > It would be really nice to let gpart provide aliases correct to > partition labels, which would fix the existing racy and unreliable > glabel code. Do you see any obstacles to using this code for that? > > > I'm not sure I understand well enough the issue here to have an opinion. We get /dev/gpt/foo (etc.) right now by parsing the GPT labels with a completely parallel piece of code to gpart and then create an extra geom provider based on the label. This falls down in four ways: 1. The code is racy. It is perfectly possible to get a GPT label set by gpart but not have it picked up by glabel for some period of time that may be infinite (until a retaste), since the events don't propagate. 2. The resulting /dev entry is unpredictable from the label, since glabel internally does some fixups related to spaces, etc. You need to copy and paste glabel's munging code. 3. It isn't implemented for all of the schemes gpart supports since it has a reimplementation of the partition table parser. 4. Because it uses an extra provider, mounting, say, /dev/adaXpY causes /dev/gpt/Z to vanish. This combination of things is why we don't currently use labels in the installer and never have. Having gpart internally create symlinks would fix all of this at a stroke. I will take a look at this some more and see how hard it would be to implement; at the very least, I think you would also need a disk_remove_alias() or the like. -Nathan > Warner > > On 08/07/17 14:12, Warner Losh wrote: > > Author: imp > Date: Mon Aug 7 21:12:38 2017 > New Revision: 322198 > URL: https://svnweb.freebsd.org/changeset/base/322198 > <https://svnweb.freebsd.org/changeset/base/322198>; > > Log: > Expose API to allow disks to ask for alias names in devfs. > Implement disk_add_alias to allow aliases to be added to > disks. All > disk have a primary name (say "foo") can also have > secondary names > (say "bar") such that all instances of "foo" also have a "bar" > alias. So if you have foo0, foo0p1, foo1, foo1s1 and > foo1s1a nodes > created by the foo driver and gpart, device nodes bar0, > bar0p1, bar1, > bar1s1 and bar1s1a will appear as symlinks back to the > original nodes. > This generalizes to multiple aliases. However, since the > unit number > follows the primary name, multiple device drivers can't > create the > same aliases unless those drives coorinate the unit number > space (eg > you couldn't add an alias 'disk' to both 'da' and 'ada' > because it's > possible to have da0 and ada0, because 'disk0' is ambiguous). > Differential Revision: > https://reviews.freebsd.org/D11873 > <https://reviews.freebsd.org/D11873>; > > Modified: > head/share/man/man9/disk.9 > head/sys/geom/geom_disk.c > head/sys/geom/geom_disk.h > > Modified: head/share/man/man9/disk.9 > ============================================================================== > --- head/share/man/man9/disk.9 Mon Aug 7 21:12:33 2017 > (r322197) > +++ head/share/man/man9/disk.9 Mon Aug 7 21:12:38 2017 > (r322198) > @@ -27,7 +27,7 @@ > .\" > .\" $FreeBSD$ > .\" > -.Dd October 30, 2012 > +.Dd August 3, 2017 > .Dt DISK 9 > .Os > .Sh NAME > @@ -45,6 +45,8 @@ > .Fn disk_destroy "struct disk *disk" > .Ft int > .Fn disk_resize "struct disk *disk" "int flags" > +.Ft void > +.Fn disk_add_alias "struct disk *disk" "const char *alias" > .Sh DESCRIPTION > The disk storage API permits kernel device drivers providing > access to > disk-like storage devices to advertise the device to other > kernel > @@ -69,6 +71,20 @@ function, > fill in the fields and call > .Fn disk_create > when the device is ready to service requests. > +.Fn disk_add_alias > +adds an alias for the disk and must be called before > +.Fn disk_create , > +but may be called multiple times. > +For each alias added, a device node will be created with > +.Xr make_dev_alias 9 > +in the same way primary device nodes are created with > +.Xr make_dev 9 > +for > +.Va d_name > +and > +.Va d_unit . > +Care should be taken to ensure that only one driver creates > aliases > +for any given name. > .Fn disk_resize > can be called by the driver after modifying > .Va d_mediasize > @@ -227,7 +243,13 @@ structure for this disk device. > .El > .Sh SEE ALSO > .Xr GEOM 4 , > -.Xr devfs 5 > +.Xr devfs 5 , > +.Xr MAKE_DEV 9 > .Sh AUTHORS > This manual page was written by > .An Robert Watson . > +.Sh BUGS > +Disk aliases are not a general purpose aliasing mechanism, > but are > +intended only to ease the transition from one name to another. > +They can be used to ensure that nvd0 and nda0 are the same thing. > +They cannot be used to implement the diskX concept from macOS. > > Modified: head/sys/geom/geom_disk.c > ============================================================================== > --- head/sys/geom/geom_disk.c Mon Aug 7 21:12:33 2017 > (r322197) > +++ head/sys/geom/geom_disk.c Mon Aug 7 21:12:38 2017 > (r322198) > @@ -676,6 +676,7 @@ g_disk_create(void *arg, int flag) > struct g_provider *pp; > struct disk *dp; > struct g_disk_softc *sc; > + struct disk_alias *dap; > char tmpstr[80]; > if (flag == EV_CANCEL) > @@ -704,6 +705,10 @@ g_disk_create(void *arg, int flag) > sc->dp = dp; > gp = g_new_geomf(&g_disk_class, "%s%d", dp->d_name, > dp->d_unit); > gp->softc = sc; > + LIST_FOREACH(dap, &dp->d_aliases, da_next) { > + snprintf(tmpstr, sizeof(tmpstr), "%s%d", > dap->da_alias, dp->d_unit); > + g_geom_add_alias(gp, tmpstr); > + } > pp = g_new_providerf(gp, "%s", gp->name); > devstat_remove_entry(pp->stat); > pp->stat = NULL; > @@ -791,6 +796,7 @@ g_disk_destroy(void *ptr, int flag) > struct disk *dp; > struct g_geom *gp; > struct g_disk_softc *sc; > + struct disk_alias *dap, *daptmp; > g_topology_assert(); > dp = ptr; > @@ -802,6 +808,8 @@ g_disk_destroy(void *ptr, int flag) > dp->d_geom = NULL; > g_wither_geom(gp, ENXIO); > } > + LIST_FOREACH_SAFE(dap, &dp->d_aliases, da_next, daptmp) > + g_free(dap); > g_free(dp); > } > @@ -834,8 +842,11 @@ g_disk_ident_adjust(char *ident, size_t size) > struct disk * > disk_alloc(void) > { > + struct disk *dp; > - return (g_malloc(sizeof(struct disk), M_WAITOK | M_ZERO)); > + dp = g_malloc(sizeof(struct disk), M_WAITOK | M_ZERO); > + LIST_INIT(&dp->d_aliases); > + return (dp); > } > void > @@ -882,6 +893,18 @@ disk_destroy(struct disk *dp) > if (dp->d_devstat != NULL) > devstat_remove_entry(dp->d_devstat); > g_post_event(g_disk_destroy, dp, M_WAITOK, NULL); > +} > + > +void > +disk_add_alias(struct disk *dp, const char *name) > +{ > + struct disk_alias *dap; > + > + dap = (struct disk_alias *)g_malloc( > + sizeof(struct disk_alias) + strlen(name) + 1, > M_WAITOK); > + strcpy((char *)(dap + 1), name); > + dap->da_alias = (const char *)(dap + 1); > + LIST_INSERT_HEAD(&dp->d_aliases, dap, da_next); > } > void > > Modified: head/sys/geom/geom_disk.h > ============================================================================== > --- head/sys/geom/geom_disk.h Mon Aug 7 21:12:33 2017 > (r322197) > +++ head/sys/geom/geom_disk.h Mon Aug 7 21:12:38 2017 > (r322198) > @@ -66,6 +66,11 @@ typedef enum { > DISK_INIT_DONE > } disk_init_level; > +struct disk_alias { > + LIST_ENTRY(disk_alias) da_next; > + const char *da_alias; > +}; > + > struct disk { > /* Fields which are private to geom_disk */ > struct g_geom *d_geom; > @@ -109,6 +114,9 @@ struct disk { > /* Fields private to the driver */ > void *d_drv1; > + > + /* Fields private to geom_disk, to be moved on next > version bump */ > + LIST_HEAD(,disk_alias) d_aliases; > }; > #define DISKFLAG_RESERVED 0x1 /* Was NEEDSGIANT */ > @@ -132,6 +140,7 @@ void disk_attr_changed(struct disk *dp, > const char *at > void disk_media_changed(struct disk *dp, int flag); > void disk_media_gone(struct disk *dp, int flag); > int disk_resize(struct disk *dp, int flag); > +void disk_add_alias(struct disk *disk, const char *); > #define DISK_VERSION_00 0x58561059 > #define DISK_VERSION_01 0x5856105a > > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?cc7bb1f5-a5b7-334f-a3e2-2dd6735fbcc1>