Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     "anna@kernel.org" <anna@kernel.org>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: Problem with providing implementation id in NFSv4.1
In-reply-to: <9058936794e86fc3913046682ad35e38d89e053f.camel@hammerspace.com>
References: <165715642317.17141.14223480428236658557@noble.neil.brown.name>,
 <9058936794e86fc3913046682ad35e38d89e053f.camel@hammerspace.com>
Date:   Fri, 08 Jul 2022 17:01:48 +1000
Message-id: <165726370886.17141.15104062573305176746@noble.neil.brown.name>
Precedence: bulk

On Thu, 07 Jul 2022, Trond Myklebust wrote:
> On Thu, 2022-07-07 at 11:13 +1000, NeilBrown wrote:
> >=20
> > In NFSv4.1 when we EXCHANGE_ID to talk to a new server - possibly a
> > PNFS
> > Data Server that we haven't talked to before - we by default send an
> > implementation id.=C2=A0 This is created from several fields obtained from
> > utsname().
> > utsname() depends on current->nsproxy, and will crash if that is
> > NULL.
> >=20
> > When a process exits it calls, among other things,
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0exit_task_namespaces(tsk);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0exit_task_work(tsk);
> >=20
> > exit_task_namespaces() will set ->nsproxy to NULL
> > exit_task_work() will run delayed work items, including fput() on all
> > files that were still open when the process exited.=C2=A0 This will cause
> > any
> > pending writes to be flushed for NFS.
> >=20
> > So if a process writes to a file on a PNFS server, exits, and the MDS
> > tells the client to send the data to a DS which it hasn't established
> > a
> > connection with before, then it will crash in encode_exchange_id().
> >=20
> > That order of calls in do_exit() is deliberate so we cannot swap them
> > - see
> > Commit: 8aac62706ada ("move exit_task_namespaces() outside of
> > exit_notify()")
> >=20
> > The options that I can see are:
> > 1/ generate the implementation-id string at mount time and keep it
> > =C2=A0=C2=A0 around much like we do for cl_owner_id
> > 2/ Check current->nsproxy in encode_exchange_id() and skip the
> > =C2=A0=C2=A0 implementation id if ->nsproxy is not available.
> > =C2=A0=C2=A0 Note that there is no risk for a race with testing ->nxproxy.
> >=20
> > Doesn't anyone have a strong opinion of which is best.=C2=A0 I'm inclined
> > to
> > go with '2', but mostly because it is less coding.
> >=20
>=20
> I'd be fine with ignoring the implementation id in that case. The
> protocol doesn't require it, so servers are expecte to be able to deal
> with that case.

Thanks for the quick reply.  I agree with you.
However it turns out that this isn't a problem in mainline.

When you close a file the ->flush() happens earlier in do_exit() and the
name spaces are still around.  However if you have a file mapped and
have dirty pages, then mainline doesn't force a flush on final unmap, or
exit of the process that had it mapped.

As I explained in
https://lore.kernel.org/all/150304037195.30218.15740287358704674869.stgit@nob=
le/

I think this is wrong, so I have an fsync() call in nfs_file_release().
This *is* run after nsproxy has been cleared.

So mainline doesn't need this fix, but I do.

Thanks,
NeilBrown