Re: mounting root from NFS via ROOTDEVNAME

On Mon, 2013-06-03 at 00:06 -0700, Craig Rodrigues wrote:
> On Tue, May 28, 2013 at 8:13 AM, Eggert, Lars <lars_at_netapp.com> wrote:
> 
> > Hi,
> >
> > to conclude this thread, the patch below allows one to specify an nfs
> > rootfs via the ROOTDEVNAME kernel option, which will be mounted when BOOTP
> > does not return a root-path option.
> >
> > Lars
> >
> >
> > diff --git a/sys/nfs/bootp_subr.c b/sys/nfs/bootp_subr.c
> > index 2c57a91..972fb12 100644
> > --- a/sys/nfs/bootp_subr.c
> > +++ b/sys/nfs/bootp_subr.c
> > _at__at_ -45,6 +45,7 _at__at_ __FBSDID("$FreeBSD$");
> >
> >  #include "opt_bootp.h"
> >  #include "opt_nfs.h"
> > +#include "opt_rootdevname.h"
> >
> >  #include <sys/param.h>
> >  #include <sys/systm.h>
> > _at__at_ -870,8 +871,20 _at__at_ bootpc_call(struct bootpc_globalcontext *gctx, struct
> > thread *td)
> >                                         rtimo = time_second +
> >                                                 BOOTP_SETTLE_DELAY;
> >                                         printf(" (got root path)");
> > -                               } else
> > +                               } else {
> >                                         printf(" (no root path)");
> > +#ifdef ROOTDEVNAME
> > +                                       /*
> > +                                        * If we'll mount rootfs from
> > +                                        * ROOTDEVNAME, we can accept
> > +                                        * offers without root paths.
> > +                                        */
> > +                                       gotrootpath = 1;
> > +                                       rtimo = time_second +
> > +                                               BOOTP_SETTLE_DELAY;
> > +                                       printf(" (ROOTDEVNAME)");
> > +#endif
> > +                               }
> >                                 printf("\n");
> >                         }
> >                 } /* while secs */
> > _at__at_ -1440,6 +1453,16 _at__at_ bootpc_decode_reply(struct nfsv3_diskless *nd,
> > struct bootpc_ifcontext *ifctx,
> >
> >         p = bootpc_tag(&gctx->tag, &ifctx->reply, ifctx->replylen,
> >                        TAG_ROOT);
> > +#ifdef ROOTDEVNAME
> > +       /*
> > +        * If there was no root path in BOOTP, use the one in ROOTDEVNAME.
> > +        */
> > +       if (p == NULL) {
> > +               p = strdup(ROOTDEVNAME, M_TEMP);
> > +               if (strcmp(strsep(&p, ":"), "nfs") != 0)
> > +                       panic("ROOTDEVNAME is not an NFS mount point");
> > +       }
> > +#endif
> >         if (p != NULL) {
> >                 if (gctx->setrootfs != NULL) {
> >                         printf("rootfs %s (ignored) ", p);
> >
> 
> 
> Sorry for not responding, I've been busy for the past few days.
> 
> Thank you for persisting with this, and trying to clean this up.
> This is  relatively old code that hasn't been touched in about 15 years,
> so not that many people have worked on it.
> 
> I don't like things like ROOTDEVNAME which need to be specified in
> the kernel config and are not part of the GENERIC kernel.
> This usually ends up where code which is behind #ifdef ROOTDEVNAME
> will bitrot because not everyone uses it.
> 
> I would like to see the ROOTDEVNAME kernel option go away,
> in favor of a variable that can be specified in loader.conf.
> 
> If I search the code via Opengrok, I with this:
> http://bxr.su/s?n=25&start=25&sort=relevancy&q=ROOTDEVNAME&project=FreeBSD
> 
> there already seems to be a variable "rootdev" that is checked in a bunch
> of places in the loader, so it would be nice if we could use that.
> 
> My personal preference  would be to delete ROOTDEVNAME from all the kernel
> configs
> and deprecate the kernel option, and instead use a tunable variable
> which could be set in loader.conf.
> 
> This may not be practical.  Do you think it would be doable if
> we can have something like this in the kernel code:
> 
> const char *rootdevname =
> #ifdef ROOTDEVNAME
> rootdevname = ROOTDEVNAME;
> #else
> rootdevname = NULL
> #endif
> 
> if (rootdevname == NULL) {
>         rootdevname = getenv("rootdev");
> }
> 
> 
> or something like that?

I'm strongly opposed to removing the ROOTDEVNAME config option, for the
simple reason that loader(8) is optional so there has to be a way to set
certain basic runtime options at compile time, and ROOTDEVNAME is one of
them.

There is already a loader tunable for the root dev, vfs.root.mountfrom.
Also, there is already a mechanism similar to your proposal above (but a
bit more complex) in kern/vfs_mountroot.c.  It sets up a list of
potential roots based on the flags passed in from the boot(8) stage;
ROOTDEVNAME will be near the beginning of the list of the RB_DFLTROOT
option (-r at the boot: prompt) is used, and near the end of the list if
it isn't.  I vaguely remember that this mechanism doesn't work for nfs
mounts, but I forget why (hopefully I'll rediscover it as I test Lars'
patch this morning).

-- Ian