Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH 0/3] Handling NFSv3 I/O errors in knfsd
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <ef9f2791ef395d7c968a386ce0a32ea503d6478f.camel@hammerspace.com>
Date:   Tue, 27 Aug 2019 09:59:25 -0400
Cc:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Bruce Fields <bfields@redhat.com>,
        Jeff Layton <jlayton@redhat.com>,
        Jeff Layton <jlayton@poochiereds.net>
Content-Transfer-Encoding: quoted-printable
Message-Id: <61F77AD6-BD02-4322-B944-0DC263EB9BD8@oracle.com>
References: <20190826165021.81075-1-trond.myklebust@hammerspace.com>
 <20190826205156.GA27834@fieldses.org>
 <ef9f2791ef395d7c968a386ce0a32ea503d6478f.camel@hammerspace.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Aug 26, 2019, at 5:02 PM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> On Mon, 2019-08-26 at 16:51 -0400, J. Bruce Fields wrote:
>> On Mon, Aug 26, 2019 at 12:50:18PM -0400, Trond Myklebust wrote:
>>> Recently, a number of changes went into the kernel to try to ensure
>>> that I/O errors (specifically write errors) are reported to the
>>> application once and only once. The vehicle for ensuring the errors
>>> are reported is the struct file, which uses the 'f_wb_err' field to
>>> track which errors have been reported.
>>>=20
>>> The problem is that errors are mainly intended to be reported
>>> through
>>> fsync(). If the client is doing synchronous writes, then all is
>>> well,
>>> but if it is doing unstable writes, then the errors may not be
>>> reported until the client calls COMMIT. If the file cache has
>>> thrown out the struct file, due to memory pressure, or just because
>>> the client took a long while between the last WRITE and the COMMIT,
>>> then the error report may be lost, and the client may just think
>>> its data is safely stored.
>>=20
>> These were lost before the file caching patches as well, right?  Or
>> is
>> there some regression?=20
>=20
> Correct. This is not a regression, but an attempt to fix a problem =
that
> has existed for some time now.
>=20
>>=20
>>> Note that the problem is compounded by the fact that NFSv3 is
>>> stateless,
>>> so the server never knows that the client may have rebooted, so
>>> there
>>> can be no guarantee that a COMMIT will ever be sent.
>>>=20
>>> The following patch set attempts to remedy the situation using 2
>>> strategies:
>>>=20
>>> 1) If the inode is dirty, then avoid garbage collecting the file
>>>   from the file cache.
>>> 2) If the file is closed, and we see that it would have reported
>>>   an error to COMMIT, then we bump the boot verifier in order to
>>>   ensure the client retransmits all its writes.
>>=20
>> Sounds sensible to me.
>>=20
>>> Note that if multiple clients were writing to the same file, then
>>> we probably want to bump the boot verifier anyway, since only one
>>> COMMIT will see the error report (because the cached file is also
>>> shared).
>>=20
>> I'm confused by the "probably should".  So that's future work?  I
>> guess
>> it'd mean some additional work to identify that case.  You can't
>> really
>> even distinguish clients in the NFSv3 case, but I suppose you could
>> use
>> IP address or TCP connection as an approximation.
>=20
> I'm suggesting we should do this too, but I haven't done so yet in
> these patches. I'd like to hear other opinions (particularly from you,
> Chuck and Jeff).

The strategy of handling these errors more carefully seems good.
Bumping the write/commit verifier so the client writes again to
retrieve the latent error is clever!

It's not clear to me though that the NFSv3 protocol can deal with
the multi-client write scenario, since it is stateless. We are now
making it stateful in some sense by preserving error state on the
server across NFS requests, without having any sense of an open
file in the protocol itself.

Would an "approximation" without open state be good enough? I
assume you are doing this to more fully support the FlexFiles
layout type. Do you have any analysis or thought about this next
step?

I also echo Bruce's concern about whether the client implementations
are up to snuff. There could be long-standing bugs or their protocol
implementation could be missing parts. This is more curiosity than
an objection, but maybe noting which client implementations you've
tested with would be good.


>> --b.
>>=20
>>> So in order to implement the above strategy, we first have to do
>>> the following: split up the file cache to act per net namespace,
>>> since the boot verifier is per net namespace. Then add a helper
>>> to update the boot verifier.
>>>=20
>>> Trond Myklebust (3):
>>>  nfsd: nfsd_file cache entries should be per net namespace
>>>  nfsd: Support the server resetting the boot verifier
>>>  nfsd: Don't garbage collect files that might contain write errors
>>>=20
>>> fs/nfsd/export.c    |  2 +-
>>> fs/nfsd/filecache.c | 76 +++++++++++++++++++++++++++++++++++++--
>>> ------
>>> fs/nfsd/filecache.h |  3 +-
>>> fs/nfsd/netns.h     |  4 +++
>>> fs/nfsd/nfs3xdr.c   | 13 +++++---
>>> fs/nfsd/nfs4proc.c  | 14 +++------
>>> fs/nfsd/nfsctl.c    |  1 +
>>> fs/nfsd/nfssvc.c    | 32 ++++++++++++++++++-
>>> 8 files changed, 115 insertions(+), 30 deletions(-)
>>>=20
>>> --=20
>>> 2.21.0
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever