From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Trond Myklebust <trondmy@kernel.org>,
        Rick Macklem <rick.macklem@gmail.com>,
        Matthew Wilcox <willy@infradead.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 3/3] nfsd: simplify write verifier handling
Thread-Topic: [PATCH 3/3] nfsd: simplify write verifier handling
Thread-Index: AQHZP/AVtkfMbaZe202q+VCt515DH67NnHMAgAAskoCAAK6mgIAAEiWA
Date:   Tue, 14 Feb 2023 14:58:49 +0000
Message-ID: <2EB94DDA-0894-40C7-925B-C0068DEA577C@oracle.com>
References: <20230213211345.385005-1-jlayton@kernel.org>
 <20230213211345.385005-4-jlayton@kernel.org>
 <CAM5tNy56at7gvUBbc1T7ay=NZQ08qOaAxdux2ZB70sZdb3L0xw@mail.gmail.com>
 <5e19458b1eba1dc4c187d14ec0c74547acb6a2a2.camel@kernel.org>
 <bd23b795b30c5640a8c7ebbe98cee048cc4022be.camel@kernel.org>
In-Reply-To: <bd23b795b30c5640a8c7ebbe98cee048cc4022be.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?n38G8M5L6eua9e1nWpCKQC/ooZa8jnHo7gy+gr+nlgOwQFUYwLs/9ea4wcAV?=
 =?us-ascii?Q?0gkXq9NOe55nzG2relxYtbrcT6S/UXTq58IHuaa53kKIvUF98iWzpCri8cNO?=
 =?us-ascii?Q?jnazYxno6tsp0jvvc68eKKVlcX2GM61og8yi6b5dP+jmP6sL94G0wO8K+VsE?=
 =?us-ascii?Q?dtCr2gnWcId11vDhrceAIEBZQIWO8MXXpX2TfN/nv4ohXaW8VXcCqyqxW8eB?=
 =?us-ascii?Q?qcPpG8XTeLat685yj1FIZpfVqYzony2IIllBuzdnZkI6h1qnBddWsDCXKEIP?=
 =?us-ascii?Q?iZfi0Ncw8k8h6fEj7GAmK1mzilOvNQ9tfomuaTtEA0Gh78Qd2t+GVgDIT6V7?=
 =?us-ascii?Q?67OId/TrZw0gUj0+kwYuvhn+N0FDPfaQrPhdPmUOgfKAR46iU7NQMJAz3U8h?=
 =?us-ascii?Q?DMYFLBzdLHnoiJa2Q06QW2lNUOfL20OEwlMoJIJyRhmYymSpaSQgfvd1Z5C+?=
 =?us-ascii?Q?ooODlMsWXgUTjmExiiSW8VZjXShIplzqpxMQ8CFqc5hyN48XoX1P1CLkK6zl?=
 =?us-ascii?Q?OsiAiwbITFYX27f3dbwbYBSj946AT7aWYx31yYt+0S43tCfHBOrGetgDR/xW?=
 =?us-ascii?Q?j5OaS+S7GpgwYKStw1C8nifb3Qp+l0mrPP6Qm2Scpv6rblFu4/wutUgYopC4?=
 =?us-ascii?Q?fNay3pS/ErcHgHpuTAnihw2uXbXs4PkoNgChEBuoHToPUG556ijgYIMY2ubG?=
 =?us-ascii?Q?lQeJkwQNJhTkbByzwgo0kZ0tpbPei2nAHLpap8JyalHxaEj6kkjZiKpqyi5+?=
 =?us-ascii?Q?gQ3G4G9YTJgWDUtxev9GHOBO/mtzfnYKRrvNBvdC0qBXl6fuIQ5AEQqYcWC9?=
 =?us-ascii?Q?DFnU/wG2sRH+e93BjGb2vRheXy4ZF/4oNoRt9+VO7upodQL7/RuDCiZnLdVR?=
 =?us-ascii?Q?cEKqSf6nESFxlr9prjeQyPJY7P0Mm196mZSu+FggeVYkB3YlzcE1M7rHgqsQ?=
 =?us-ascii?Q?o4eJoCBsnIkKqGFcge4QTa90iDOWoBXog3WUfJKtiJCKRRBtz7LAkSpFhPXZ?=
 =?us-ascii?Q?FJy45D7dipgXGXo5jfXKvVUBIZ0Yr4xFcNJISBO+Y4vUgCIkMcVPNdi5wXw2?=
 =?us-ascii?Q?AHH4i/2MQxFlCg42SI8f2cNKEoOQrtsHhTO+4a0cXrn8VT40XzpMpU1VeN5Z?=
 =?us-ascii?Q?1YVvTYMgDx3tF8q/5HWSekA0Dclc0gSxPoLjVSYci1liKqjZKGqEfLCpZ/eP?=
 =?us-ascii?Q?r7w5VbM0GAg1qGPAv5281hH0FuJW/U8yAVO7itp8iPv2zhb+EBd6hKx9u2oK?=
 =?us-ascii?Q?HaDijRQpG009ropK8Jj1jOrl5NahbjC2zzEsFk9PjndrZQ2B7RuyS9CFp8Mj?=
 =?us-ascii?Q?F3jirA4ETOIvuRtOiJnb6OX+9UxVxysWyakoQE/OCtrv9iZbUVNGXdgZWit3?=
 =?us-ascii?Q?+L7rZz5mKmgd+1UahmP+lEAmzAqjOgXe2MqZqFoHUhvpq6yjRJm89gNAAiJo?=
 =?us-ascii?Q?ilIn5v9WkT8nz0xWi1VCf+tTOgO95OrKJOYOZOV8GzcE7bdNhbZaSJft1L4R?=
 =?us-ascii?Q?g0+OrQRkx+YX4pgFG8dbJoPQgwld4YwLsJ8ADz15rReF9Jy9Y3J6vQ6JmdO2?=
 =?us-ascii?Q?QKoDxvZJbMczJP6VguJbdoR7v+WPbAQ/sHijJZKSAfVEx9Y8r9FQDkOZ8Gjh?=
 =?us-ascii?Q?dw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4E9C5ABE76F9A549AF74A3A21DDA307A@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 8ERAOaR86/mJ2vsAsBPTh4pP7jQWcGdVGnzxfIOvHjO696CokJFO6uw+P0g4jXuw91rXf1Pdv1b+vZUfEzdvJkjsqpnwprrAWn60ic9b1qvol3hx64evaRhc0M4nxVL3BtCLKDET4H2c0ySWpEnEMfSADMMJgD2smNvxHClm2Dqs+UuXD4fBkdiabmzR4Tzp3hzPIiKMUd5aJ6u8L56EWOigqul9hJAxuM1C8Mj435LQgtTr+akrn5oc5MyBlSht26YKaJbAIjDxqI8QuWkFZ3EVXH5VOHAFKSbh7kh7BVRr4O9ptSSJJm9EgwWASGBNDh1IYSeoPvRH7mQPy6eKbuodFW6c4WTPcffkYciDw7QtjH46k6F6BHz2RB7BYlqikYtgeOMEi5gxFYCiiaSkO5645+bpZ2tiwor+Mwrhe4H0Ga8gHIstQVMsd98AsMcjrFW1DRZS59CezDjCBQeM0Xj4JC91o87Wx8fSiCUiwMFYfIa5akuFGDo+NT7zcA9wlamcy3OSAFhyLYy4FX9NXK3pgp7KCq78QAmNl8ywoq8//qCtRhkbM0uA8MaPe9pazjI9KmmgHum4UGXIebJ1xpJVlr76C9AwzhEr4LZrdTHoWhmJgDqnav2G2COT9lae25j2jPmarQvrK25MirwlKRIzcfspvt9XdW6imcw+tWvw/seC6FbVn94rnuJWmxi+YHwczRJZo7xpWcLgcCqXh6IX2C1TGyy8txpC8Psr/cjEzpkOTRaVuG00egOJvaJ+DCIMe1BXsciikzlZtXOxVgPo4TbOq2iNLsE3KZOjiOJGJlwIxTXwYPBI1RSIWiLEIdc9EN012uv2v7k1zaOTDptgX4h2OCRYu4hj4iNM70E=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4d490f2c-6236-43a2-85ca-08db0e9bfb27
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Feb 2023 14:58:49.9292
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Zg82wtyAR3SCcOEtmaB0Qj9qKKH4rM+ErnCq23TPWMQ1u1IqVl4bGN9CxiIwXOX2HpG+hXPyPKJUmnHaD69/HA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4547
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.170.22
 definitions=2023-02-14_10,2023-02-14_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 adultscore=0
 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2302140129
X-Proofpoint-GUID: kA-CqMo-kiVQL6KY84VM04C-64pj_H18
X-Proofpoint-ORIG-GUID: kA-CqMo-kiVQL6KY84VM04C-64pj_H18
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 14, 2023, at 8:53 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2023-02-13 at 22:28 -0500, Trond Myklebust wrote:
>> On Mon, 2023-02-13 at 16:49 -0800, Rick Macklem wrote:
>>> On Mon, Feb 13, 2023 at 1:14 PM Jeff Layton <jlayton@kernel.org>
>>> wrote:
>>>>=20
>>>> CAUTION: This email originated from outside of the University of
>>>> Guelph. Do not click links or open attachments unless you recognize
>>>> the sender and know the content is safe. If in doubt, forward
>>>> suspicious emails to IThelp@uoguelph.ca
>>>>=20
>>>>=20
>>>> The write verifier exists to tell the client when the server may
>>>> have
>>>> forgotten some unstable writes. The typical way that this happens
>>>> is if
>>>> the server crashes, but we've also extended nfsd to change it when
>>>> there
>>>> are writeback errors as well.
>>>>=20
>>>> The way it works today though, we call something like vfs_fsync
>>>> (e.g.
>>>> for a COMMIT call) and if we get back an error, we'll reset the
>>>> write
>>>> verifier.
>>>>=20
>>>> This is non-optimal for a couple of reasons:
>>>>=20
>>>> 1/ There could be significant delay between an error being
>>>> recorded and the reset. It would be ideal if the write verifier
>>>> were to
>>>> change as soon as the error was recorded.
>>>>=20
>>>> 2/ It's a bit of a waste, in that if we get a writeback error on a
>>>> single inode, we'll end up resetting the write verifier for
>>>> everything,
>>>> even on inodes that may be fine (e.g. on a completely separate fs).
>>>>=20
>>> Here's the snippet from RFC8881:
>>>    The final portion of the result is the field writeverf.  This
>>> field
>>>    is the write verifier and is a cookie that the client can use to
>>>    determine whether a server has changed instance state (e.g.,
>>> server
>>>    restart) between a call to WRITE and a subsequent call to either
>>>    WRITE or COMMIT.  This cookie MUST be unchanged during a single
>>>    instance of the NFSv4.1 server and MUST be unique between
>>> instances
>>>    of the NFSv4.1 server.  If the cookie changes, then the client
>>> MUST
>>>    assume that any data written with an UNSTABLE4 value for committed
>>>    and an old writeverf in the reply has been lost and will need to
>>> be
>>>    recovered.
>>>=20
>>> I've always interpreted the writeverf as "per-server" and not  "per-
>>> file".
>>> Although I'll admit the above does not make that crystal clear, it
>>> does make
>>> it clear that the writeverf applies to a "server instance" and not a
>>> file or
>>> file system on the server.
>>>=20
>>> The FreeBSD client assumes it is "per-server" and re-writes all
>>> uncommitted
>>> writes for the server, not just ones for the file (or file system)
>>> the
>>> writeverf is
>>> replied with.  (I vaguely recall Solaris does the same?)
>>>=20
>>> At the very least, I think you should run this past the IETF working
>>> group
>>> (nfsv4@ietf.org) to see what they say w.r.t. the writeverf being
>>> "per-file" vs
>>> "per-server".
>>>=20
>>=20
>> As I recall, we've already had this discussion on the IETF NFSv4
>> working group mailing list:
>> https://mailarchive.ietf.org/arch/msg/nfsv4/99Ow2muMylXKWd9lzi9_BX2LJDY/
>>=20
>>=20
>> That's why I kept it a global in the first place.
>>=20
>> Now note that RFC8881 does also clarify in Section 18.3.3 that:
>>=20
>>=20
>>   The server must vary the value of the write
>>   verifier at each server event or instantiation that may lead to a
>>   loss of uncommitted data.  Most commonly this occurs when the server
>>   is restarted; however, other events at the server may result in
>>   uncommitted data loss as well.
>>=20
>> So I feel it is quite OK to use the verifier the way we do now in order
>> to signify that a fatal write error has occurred and that clients must
>> resend any data that was uncommitted.
>>=20
>=20
> Thanks, I missed that discussion. I think you guys have convinced me
> that we have to keep this per-server. I won't bother starting a new
> thread on it.
>=20
> It's a pity. It would have been a lot more elegant as a per-inode thing!
>=20
> Chuck, I think that means we'll just want to keep patch #1 in this=20
> series?

Regarding patch 1/3:

"sizeof(verf)" works as well as "sizeof(*verf) * 2" and is a little
more clear to boot. You can redrive a v2 of your patch or I can make
one. Up to you.


--
Chuck Lever