Why keep var-lib-nfs-rpc_pipefs.mount around?

Why keep var-lib-nfs-rpc_pipefs.mount around?

[Andreas Hasenack]

Hi,

I was tracking down a Debian/Ubuntu bug with nfs-utils 2.6.1 where in
one case, after installing the packages, you would end up with
rpc_pipefs mounted at the same time in two locations: /run/rpc_pipefs
and /var/lib/nfs/rpc_pipefs. The /run location is what debian/ubuntu
default to.

After poking around a bit, I think I found out why that is
happening[1], but it led me to ask this question: why is
var-lib-nfs-rpc_pipefs.mount (and its corresponding rpc_pipefs.target
unit) still shipped, given that nfs-utils now has a generator?
Shouldn't the generator be enough for all cases, where rpc_pipefs is
mounted in the default compile-time location, or changed via a config
change to nfs.conf? I know currently it checks[2] whether the config
points at the default location, but that check could just be skipped
and then the generator would always produce the correct mount and
target units.


1. https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1971935/comments/22
2. https://salsa.debian.org/kernel-team/nfs-utils/-/blob/master/systemd/rpc-pipefs-generator.c#L138

Re: Why keep var-lib-nfs-rpc_pipefs.mount around?

[Benjamin Coddington]

On 8 Jul 2022, at 12:50, Andreas Hasenack wrote:

> Hi,
>
> I was tracking down a Debian/Ubuntu bug with nfs-utils 2.6.1 where in
> one case, after installing the packages, you would end up with
> rpc_pipefs mounted at the same time in two locations: /run/rpc_pipefs
> and /var/lib/nfs/rpc_pipefs. The /run location is what debian/ubuntu
> default to.
>
> After poking around a bit, I think I found out why that is
> happening[1], but it led me to ask this question: why is
> var-lib-nfs-rpc_pipefs.mount (and its corresponding rpc_pipefs.target
> unit) still shipped, given that nfs-utils now has a generator?

Could just be an oversight, or perhaps a better reason exists.  The
nfs-utils userspace has to handle a lot of different cases and legacy
setups.

Steve D, do you know?

Ben

> Shouldn't the generator be enough for all cases, where rpc_pipefs is
> mounted in the default compile-time location, or changed via a config
> change to nfs.conf? I know currently it checks[2] whether the config
> points at the default location, but that check could just be skipped
> and then the generator would always produce the correct mount and
> target units.
>
>
> 1. 
> https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1971935/comments/22
> 2. 
> https://salsa.debian.org/kernel-team/nfs-utils/-/blob/master/systemd/rpc-pipefs-generator.c#L138

Re: Why keep var-lib-nfs-rpc_pipefs.mount around?

[Steve Dickson]

My apologies delayed response... extended PTO

On 7/11/22 9:13 AM, Benjamin Coddington wrote:
> On 8 Jul 2022, at 12:50, Andreas Hasenack wrote:
> 
>> Hi,
>>
>> I was tracking down a Debian/Ubuntu bug with nfs-utils 2.6.1 where in
>> one case, after installing the packages, you would end up with
>> rpc_pipefs mounted at the same time in two locations: /run/rpc_pipefs
>> and /var/lib/nfs/rpc_pipefs. The /run location is what debian/ubuntu
>> default to.
>>
>> After poking around a bit, I think I found out why that is
>> happening[1], but it led me to ask this question: why is
>> var-lib-nfs-rpc_pipefs.mount (and its corresponding rpc_pipefs.target
>> unit) still shipped, given that nfs-utils now has a generator?
> 
> Could just be an oversight, or perhaps a better reason exists.  The
> nfs-utils userspace has to handle a lot of different cases and legacy
> setups.
> 
> Steve D, do you know?
Its not clear to me, if the read from nfs.conf does not
happen how changing the rpc_pipefs directory could happen.

When the read from nfs.conf happens and the rpc_pipefs directory
is not defined, the compiled in default rpc_pipefs directory
will be used and the generator will exit and not
generating the systemd files (using the installed ones).

If the rpc_pipefs directory is defined in nfs.conf, the
generator will set up that directory as the
rpc_pipefs directory and systemd files will be
generated.

So by taking out the nfs.conf read, the only way to change
the default rpc_pipefs directory is to recompile nfs-utils.

steved.
> 
> Ben
> 
>> Shouldn't the generator be enough for all cases, where rpc_pipefs is
>> mounted in the default compile-time location, or changed via a config
>> change to nfs.conf? I know currently it checks[2] whether the config
>> points at the default location, but that check could just be skipped
>> and then the generator would always produce the correct mount and
>> target units.
>>
>>
>> 1. 
>> https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1971935/comments/22 
>>
>> 2. 
>> https://salsa.debian.org/kernel-team/nfs-utils/-/blob/master/systemd/rpc-pipefs-generator.c#L138 
>>
> 

Re: Why keep var-lib-nfs-rpc_pipefs.mount around?

[Andreas Hasenack]

Hi,

On Sat, Jul 23, 2022 at 2:29 PM Steve Dickson <steved@redhat.com> wrote:
>
> My apologies delayed response... extended PTO
>
> On 7/11/22 9:13 AM, Benjamin Coddington wrote:
> > On 8 Jul 2022, at 12:50, Andreas Hasenack wrote:
> >
> >> Hi,
> >>
> >> I was tracking down a Debian/Ubuntu bug with nfs-utils 2.6.1 where in
> >> one case, after installing the packages, you would end up with
> >> rpc_pipefs mounted at the same time in two locations: /run/rpc_pipefs
> >> and /var/lib/nfs/rpc_pipefs. The /run location is what debian/ubuntu
> >> default to.
> >>
> >> After poking around a bit, I think I found out why that is
> >> happening[1], but it led me to ask this question: why is
> >> var-lib-nfs-rpc_pipefs.mount (and its corresponding rpc_pipefs.target
> >> unit) still shipped, given that nfs-utils now has a generator?
> >
> > Could just be an oversight, or perhaps a better reason exists.  The
> > nfs-utils userspace has to handle a lot of different cases and legacy
> > setups.
> >
> > Steve D, do you know?
> Its not clear to me, if the read from nfs.conf does not
> happen how changing the rpc_pipefs directory could happen.

The read happens, it's just that in that particular bug scenario, the
/etc/nfs.conf file isn't there yet.

In the debian case, two things are triggering this bug[1]:
- the /etc/nfs.conf file is not shipped by the package in that
location. Instead, it's put in place by the postinst script (like
rpm's %postinst)[2].
- autofs recommends[3], not depends, nfs-common. This means that
autofs can be setup before nfs-common is, and if that happens,
/etc/nfs.conf doesn't exist yet. But the nfs-common systemd unit files
do exist, and the generator is run when autofs calls systemctl
daemon-reload in its postinst. Since there is no /etc/nfs.conf, the
generator exits silently.

> When the read from nfs.conf happens and the rpc_pipefs directory
> is not defined, the compiled in default rpc_pipefs directory

Or if /etc/nfs.conf isn't there, the generator exits silently.

> will be used and the generator will exit and not
> generating the systemd files (using the installed ones).
>
> If the rpc_pipefs directory is defined in nfs.conf, the
> generator will set up that directory as the
> rpc_pipefs directory and systemd files will be
> generated.
>
> So by taking out the nfs.conf read, the only way to change
> the default rpc_pipefs directory is to recompile nfs-utils.

Actually, I'm doing two things:
- taking out the var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target units
- taking out the bit from the generator that compares the configured
pipefs directory with the compile-time default:
--- a/systemd/rpc-pipefs-generator.c
+++ b/systemd/rpc-pipefs-generator.c
@@ -139,9 +139,6 @@ int main(int argc, char *argv[])
     s = conf_get_str("general", "pipefs-directory");
     if (!s)
         exit(0);
-    if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
-            strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
-        exit(0);

     if (is_non_pipefs_mountpoint(s))
         exit(1);

Therefore I'm fully relying on the generator all the time, whatever
the pipefs directory is. And my question is wouldn't this be a sane
default behavior for all users of nfs-utils, instead of having the
extra complication of having two units for each of rpc_pipefs mount
and target? Did I miss something?

Unfortunately I haven't heard back yet from the debian maintainer
about this.[4] Maybe there is a "debian packaging" way to fix this. I
also thought about systemd conditionals on /etc/nfs.conf, but then I
would probably have to add them to a bunch of units (all of them?).

1. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1014429
2. https://salsa.debian.org/kernel-team/nfs-utils/-/blob/master/debian/nfs-common.postinst#L9
3. https://salsa.debian.org/debian/autofs/-/blob/master/debian/control#L35
4. https://salsa.debian.org/kernel-team/nfs-utils/-/merge_requests/18

Always run rpc-pipefs-generator generator (was: Re: Why keep var-lib-nfs-rpc_pipefs.mount around?)

[Salvatore Bonaccorso]

[-- Attachment #1: Type: text/plain, Size: 5417 bytes --]

Hi Steve, 

On Mon, Jul 25, 2022 at 09:38:40AM -0300, Andreas Hasenack wrote:
> Hi,
> 
> On Sat, Jul 23, 2022 at 2:29 PM Steve Dickson <steved@redhat.com> wrote:
> >
> > My apologies delayed response... extended PTO
> >
> > On 7/11/22 9:13 AM, Benjamin Coddington wrote:
> > > On 8 Jul 2022, at 12:50, Andreas Hasenack wrote:
> > >
> > >> Hi,
> > >>
> > >> I was tracking down a Debian/Ubuntu bug with nfs-utils 2.6.1 where in
> > >> one case, after installing the packages, you would end up with
> > >> rpc_pipefs mounted at the same time in two locations: /run/rpc_pipefs
> > >> and /var/lib/nfs/rpc_pipefs. The /run location is what debian/ubuntu
> > >> default to.
> > >>
> > >> After poking around a bit, I think I found out why that is
> > >> happening[1], but it led me to ask this question: why is
> > >> var-lib-nfs-rpc_pipefs.mount (and its corresponding rpc_pipefs.target
> > >> unit) still shipped, given that nfs-utils now has a generator?
> > >
> > > Could just be an oversight, or perhaps a better reason exists.  The
> > > nfs-utils userspace has to handle a lot of different cases and legacy
> > > setups.
> > >
> > > Steve D, do you know?
> > Its not clear to me, if the read from nfs.conf does not
> > happen how changing the rpc_pipefs directory could happen.
> 
> The read happens, it's just that in that particular bug scenario, the
> /etc/nfs.conf file isn't there yet.
> 
> In the debian case, two things are triggering this bug[1]:
> - the /etc/nfs.conf file is not shipped by the package in that
> location. Instead, it's put in place by the postinst script (like
> rpm's %postinst)[2].
> - autofs recommends[3], not depends, nfs-common. This means that
> autofs can be setup before nfs-common is, and if that happens,
> /etc/nfs.conf doesn't exist yet. But the nfs-common systemd unit files
> do exist, and the generator is run when autofs calls systemctl
> daemon-reload in its postinst. Since there is no /etc/nfs.conf, the
> generator exits silently.
> 
> > When the read from nfs.conf happens and the rpc_pipefs directory
> > is not defined, the compiled in default rpc_pipefs directory
> 
> Or if /etc/nfs.conf isn't there, the generator exits silently.
> 
> > will be used and the generator will exit and not
> > generating the systemd files (using the installed ones).
> >
> > If the rpc_pipefs directory is defined in nfs.conf, the
> > generator will set up that directory as the
> > rpc_pipefs directory and systemd files will be
> > generated.
> >
> > So by taking out the nfs.conf read, the only way to change
> > the default rpc_pipefs directory is to recompile nfs-utils.
> 
> Actually, I'm doing two things:
> - taking out the var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target units
> - taking out the bit from the generator that compares the configured
> pipefs directory with the compile-time default:
> --- a/systemd/rpc-pipefs-generator.c
> +++ b/systemd/rpc-pipefs-generator.c
> @@ -139,9 +139,6 @@ int main(int argc, char *argv[])
>      s = conf_get_str("general", "pipefs-directory");
>      if (!s)
>          exit(0);
> -    if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
> -            strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
> -        exit(0);
> 
>      if (is_non_pipefs_mountpoint(s))
>          exit(1);
> 
> Therefore I'm fully relying on the generator all the time, whatever
> the pipefs directory is. And my question is wouldn't this be a sane
> default behavior for all users of nfs-utils, instead of having the
> extra complication of having two units for each of rpc_pipefs mount
> and target? Did I miss something?
> 
> Unfortunately I haven't heard back yet from the debian maintainer
> about this.[4] Maybe there is a "debian packaging" way to fix this. I
> also thought about systemd conditionals on /etc/nfs.conf, but then I
> would probably have to add them to a bunch of units (all of them?).
> 
> 1. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1014429
> 2. https://salsa.debian.org/kernel-team/nfs-utils/-/blob/master/debian/nfs-common.postinst#L9
> 3. https://salsa.debian.org/debian/autofs/-/blob/master/debian/control#L35
> 4. https://salsa.debian.org/kernel-team/nfs-utils/-/merge_requests/18

FWIW, in Debian we have applied the respective change. The idea would
be to only depend on a single mechanism for setting up the mounts
rather than a combination of the two (the generator and the static
mount unit). For this reason we have applied the attached patch, and
are not installing the units that we will let the generator produce,
that is var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target

We in Debian for long have diverged too much from you upstream,
causing that we lacked behind several new upstream version and stuck
with old versions in stable releases. We want to avoid running into
that again in future. So if this make sense to you, would you apply
the same (or as you prefer similar) change to you upstream?

On one side so you could apply Andreas Hasenack patch, secondly
installing the var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target
could be dropped (note no changes to the other units needed as the
repsective needed dependencies are generated by the systemd
generator).

Ben, Andreas, please add what else is needed from your point of view
please!

Thanks a lot for considering this. If you have any suggestion further
how we can unify the Debian downstream to you upstream, let us know
please.

Regards,
Salvatore

[-- Attachment #2: always-run-generator.patch --]
[-- Type: text/x-diff, Size: 932 bytes --]

Description: Always run the generator
 Run the generator even if the pipefs-directory setting is the default one.
Author: Andreas Hasenack <andreas@canonical.com>
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1971935
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1014429
Forwarded: https://lore.kernel.org/linux-nfs/EE39279C-4E40-48C8-ABC9-707EB1AD6D79@redhat.com/
Last-Update: 2022-07-12
---
This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
diff --git a/systemd/rpc-pipefs-generator.c b/systemd/rpc-pipefs-generator.c
index c24db567..7c42431f 100644
--- a/systemd/rpc-pipefs-generator.c
+++ b/systemd/rpc-pipefs-generator.c
@@ -139,9 +139,6 @@ int main(int argc, char *argv[])
 	s = conf_get_str("general", "pipefs-directory");
 	if (!s)
 		exit(0);
-	if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
-			strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
-		exit(0);
 
 	if (is_non_pipefs_mountpoint(s))
 		exit(1);

Re: Always run rpc-pipefs-generator generator (was: Re: Why keep var-lib-nfs-rpc_pipefs.mount around?)

[Benjamin Coddington]

On 9 Jul 2023, at 3:38, Salvatore Bonaccorso wrote:

> Hi Steve,

...

> FWIW, in Debian we have applied the respective change. The idea would
> be to only depend on a single mechanism for setting up the mounts
> rather than a combination of the two (the generator and the static
> mount unit). For this reason we have applied the attached patch, and
> are not installing the units that we will let the generator produce,
> that is var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target
>
> We in Debian for long have diverged too much from you upstream,
> causing that we lacked behind several new upstream version and stuck
> with old versions in stable releases. We want to avoid running into
> that again in future. So if this make sense to you, would you apply
> the same (or as you prefer similar) change to you upstream?
>
> On one side so you could apply Andreas Hasenack patch, secondly
> installing the var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target
> could be dropped (note no changes to the other units needed as the
> repsective needed dependencies are generated by the systemd
> generator).
>
> Ben, Andreas, please add what else is needed from your point of view
> please!

I don't think I've seen the PATCH land on the list addressed to nfs-utils
maintainer yet, but I could have missed it.

Otherwise it looks sane to me, but I could be missing some upstream case.

> Thanks a lot for considering this. If you have any suggestion further
> how we can unify the Debian downstream to you upstream, let us know
> please.

At Red Hat, we use "upstream first" as a leading principle.  If this change
makes sense for upstream, send Adreas' patch along and I am sure Steve D will
consider it or let us know why its not acceptible for upstream.

Ben

Re: Always run rpc-pipefs-generator generator (was: Re: Why keep var-lib-nfs-rpc_pipefs.mount around?)

[Salvatore Bonaccorso]

Hi,

On Mon, Jul 10, 2023 at 10:39:43AM -0400, Benjamin Coddington wrote:
> On 9 Jul 2023, at 3:38, Salvatore Bonaccorso wrote:
> 
> > Hi Steve,
> 
> ...
> 
> > FWIW, in Debian we have applied the respective change. The idea would
> > be to only depend on a single mechanism for setting up the mounts
> > rather than a combination of the two (the generator and the static
> > mount unit). For this reason we have applied the attached patch, and
> > are not installing the units that we will let the generator produce,
> > that is var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target
> >
> > We in Debian for long have diverged too much from you upstream,
> > causing that we lacked behind several new upstream version and stuck
> > with old versions in stable releases. We want to avoid running into
> > that again in future. So if this make sense to you, would you apply
> > the same (or as you prefer similar) change to you upstream?
> >
> > On one side so you could apply Andreas Hasenack patch, secondly
> > installing the var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target
> > could be dropped (note no changes to the other units needed as the
> > repsective needed dependencies are generated by the systemd
> > generator).
> >
> > Ben, Andreas, please add what else is needed from your point of view
> > please!
> 
> I don't think I've seen the PATCH land on the list addressed to nfs-utils
> maintainer yet, but I could have missed it.
> 
> Otherwise it looks sane to me, but I could be missing some upstream case.
> 
> > Thanks a lot for considering this. If you have any suggestion further
> > how we can unify the Debian downstream to you upstream, let us know
> > please.
> 
> At Red Hat, we use "upstream first" as a leading principle.  If this change
> makes sense for upstream, send Adreas' patch along and I am sure Steve D will
> consider it or let us know why its not acceptible for upstream.

Andreas, could you sent a proper patchset please, so upstream can have
a look at it for inclusion? 

Regards,
Salvatore

Re: Always run rpc-pipefs-generator generator (was: Re: Why keep var-lib-nfs-rpc_pipefs.mount around?)

[Andreas Hasenack]

On it, need to refresh some knowledge and think with an upstream hat on now :)

On Mon, Jul 24, 2023 at 6:12 AM Salvatore Bonaccorso <carnil@debian.org> wrote:
>
> Hi,
>
> On Mon, Jul 10, 2023 at 10:39:43AM -0400, Benjamin Coddington wrote:
> > On 9 Jul 2023, at 3:38, Salvatore Bonaccorso wrote:
> >
> > > Hi Steve,
> >
> > ...
> >
> > > FWIW, in Debian we have applied the respective change. The idea would
> > > be to only depend on a single mechanism for setting up the mounts
> > > rather than a combination of the two (the generator and the static
> > > mount unit). For this reason we have applied the attached patch, and
> > > are not installing the units that we will let the generator produce,
> > > that is var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target
> > >
> > > We in Debian for long have diverged too much from you upstream,
> > > causing that we lacked behind several new upstream version and stuck
> > > with old versions in stable releases. We want to avoid running into
> > > that again in future. So if this make sense to you, would you apply
> > > the same (or as you prefer similar) change to you upstream?
> > >
> > > On one side so you could apply Andreas Hasenack patch, secondly
> > > installing the var-lib-nfs-rpc_pipefs.mount and rpc_pipefs.target
> > > could be dropped (note no changes to the other units needed as the
> > > repsective needed dependencies are generated by the systemd
> > > generator).
> > >
> > > Ben, Andreas, please add what else is needed from your point of view
> > > please!
> >
> > I don't think I've seen the PATCH land on the list addressed to nfs-utils
> > maintainer yet, but I could have missed it.
> >
> > Otherwise it looks sane to me, but I could be missing some upstream case.
> >
> > > Thanks a lot for considering this. If you have any suggestion further
> > > how we can unify the Debian downstream to you upstream, let us know
> > > please.
> >
> > At Red Hat, we use "upstream first" as a leading principle.  If this change
> > makes sense for upstream, send Adreas' patch along and I am sure Steve D will
> > consider it or let us know why its not acceptible for upstream.
>
> Andreas, could you sent a proper patchset please, so upstream can have
> a look at it for inclusion?
>
> Regards,
> Salvatore