Date: Thu, 30 Mar 2017 09:05:03 -0400
From: Scott Mayhew <smayhew@redhat.com>
To: NeilBrown <neilb@suse.com>
Cc: Steve Dickson <SteveD@RedHat.com>, linux-nfs@vger.kernel.org
Subject: Re: [nfs-utils PATCH] systemd: add generators for the rpc_pipefs
 mountpoint
Message-ID: <20170330130503.mc43qnglsptdczgd@tonberry.usersys.redhat.com>
References: <20170328133703.19325-1-smayhew@redhat.com>
 <2aace10d-1b9b-b8e8-4f85-caf31f5f51bf@RedHat.com>
 <20170329180207.xkcxepra442j6vv6@tonberry.usersys.redhat.com>
 <87a8835l17.fsf@notabene.neil.brown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
In-Reply-To: <87a8835l17.fsf@notabene.neil.brown.name>
Sender: linux-nfs-owner@vger.kernel.org

On Thu, 30 Mar 2017, NeilBrown wrote:

> On Wed, Mar 29 2017, Scott Mayhew wrote:
> 
> > On Wed, 29 Mar 2017, Steve Dickson wrote:
> >
> >> Hello,
> >> 
> >> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
> >> > The nfs.conf and idmapd.conf files both have config options for the
> >> > pipefs mountpoint.  Currently, changing these from the defaults also
> >> > requires manually overriding the systemd unit files that are hard-coded
> >> > to mount the filesystem on /var/lib/nfs/rpc_pipefs.
> >> The Pipefs-Directory config variable is not documented in either
> >> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
> >> to know about it as to read the code. I would not call that
> >> a supported interface and can easily be removed. IMHO.
> >> The same thing goes for the Cache-Expiration variable. 
> >
> > So you're saying to document the Pipefs-Directory and Cache-Expiration
> > variables, not remove them... right?  I think they should be documented
> > in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
> >> 
> >> > 
> >> > This patch adds two generators to create drop-in config files to
> >> > override the dependencies for the systemd units that require the pipefs.
> >> > There are two because rpc.idmapd uses a separate configuration file.  If
> >> > idmapd's configurations are ever folded into the nfs.conf, then the
> >> > idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
> >> > specified for idmapd in rpc-pipefs-generator.c.
> >> So I'm thinking we just read Pipefs-Directory out of 
> >> one spot that would be /etc/nfs.conf.
> >
> > I agree but then rpc.idmapd and nfsidmap should be modified to read
> > /etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
> > confusing unless some of the section names and/or variable names from
> > /etc/idmapd.conf were changed up a bit.  That's quite a bit more than
> > I'm trying to accomplish here.
> >
> >> That way we would only 
> >> have to support one of these generators.
> >
> > If your issue is that there are two generators then I can fold them into
> > single one... then if the idmapd.conf ever get folded into nfs.conf it's
> > simply a matter of deleting a few lines of code.  The only reason I made
> > two generators is becuase it made more sense to me that way.  I'm fine
> > either way.
> >
> >> 
> >> Also please document the Pipefs-Directory variable in both
> >> the nfs.conf man page and in the example nfs.conf file
> >> in the git tree. 
> >
> > It's actually already documented in both, under the gssd section.
> >
> >> 
> >> > 
> >> > This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
> >> > since that is the most logical alternate location for the pipefs
> >> > filesystem.  Users wanting to use some other location besides
> >> > /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
> >> > own systemd mount unit file, but the generators would take care of the
> >> > rest.
> >> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
> >> is needed, especially since it is not being install (aka no updates
> >> to the Makefile.am files).
> >
> > I forgot to update the Makefile.am by mistake.  I definitely want the
> > new unit file.  The idea is to give users a choice between
> > /var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
> > manually messing around with systemd configs.
> 
> Could the new unit file be generated by the generator???
> i.e. the generator creates the foo.mount based on the nfs.conf file
> contents, as well as create the dependencies between services and this
> file.

Good idea, I'll try it.

-Scott
> 
> ??
> 
> NeilBrown