Date: Mon, 7 Aug 2017 16:27:26 -0600 From: Warner Losh <imp@bsdimp.com> To: Nathan Whitehorn <nwhitehorn@freebsd.org> 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: <CANCZdfq2=%2BNUUVFZNoDyY0%2B0s5eadSzQqJ%2B8yefAuJ76CdfK1Q@mail.gmail.com> In-Reply-To: <cc7bb1f5-a5b7-334f-a3e2-2dd6735fbcc1@freebsd.org> References: <201708072112.v77LCcXm001489@repo.freebsd.org> <ff9f9256-bd4e-0096-ed24-742ef8aed24a@freebsd.org> <CANCZdfpOKZ0TnZF6g1ABMhtFAA8KwBv0mMWQ=4Tkm0P3XY-fKQ@mail.gmail.com> <cc7bb1f5-a5b7-334f-a3e2-2dd6735fbcc1@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Aug 7, 2017 at 4:20 PM, Nathan Whitehorn <nwhitehorn@freebsd.org> wrote: > > > On 08/07/17 14:32, Warner Losh wrote: > > > > On Mon, Aug 7, 2017 at 3:19 PM, Nathan Whitehorn <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. > I've experienced all but #1 and #2. I've run systems for about a decade with root mounted on /dev/gpt/HOST-root, usr on /dev/gpt/HOST-usr, etc. Must have gotten lucky, or not hit the use case that you've seen. Not sure what's up with #3, but it sounds orthogonal and an incomplete gpart/glabel integration. #4 has always bothered me. While device aliases like I've done here would solve that problem, I'm not sure what other issues there'd be demoting glabel devices to mere /dev/ nodes from first class geom objects. The main issue I see is needing to have the aliases in place when tasting time comes for the geom being tasted. I haven't thought through geom's sequence enough to know if that would be more robust or not. Warner -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 >>> >>> 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 >>> >>> 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?CANCZdfq2=%2BNUUVFZNoDyY0%2B0s5eadSzQqJ%2B8yefAuJ76CdfK1Q>