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;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Dave Chinner <david@fromorbit.com>
CC:     Anna Schumaker <anna@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v3 6/6] NFSD: Repeal and replace the READ_PLUS
 implementation
Thread-Topic: [PATCH v3 6/6] NFSD: Repeal and replace the READ_PLUS
 implementation
Thread-Index: AQHYmHrzYsqP79DW8UWdtY79IK5gxq1/y34AgAOLZACAAtl+AIAAIOUAgAAtKQA=
Date:   Wed, 20 Jul 2022 01:26:13 +0000
Message-ID: <CF981532-ADC0-43F9-A304-9760244A53D5@oracle.com>
References: <20220715184433.838521-1-anna@kernel.org>
 <20220715184433.838521-7-anna@kernel.org>
 <EC97C20D-A317-49F9-8280-062D1AAEE49A@oracle.com>
 <20220718011552.GK3600936@dread.disaster.area>
 <CAFX2Jf=FrXHMxioWLHFkRHxBNDRe-9SBUmCcco9gkaY8EQOSZg@mail.gmail.com>
 <20220719224434.GL3600936@dread.disaster.area>
In-Reply-To: <20220719224434.GL3600936@dread.disaster.area>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?D02Fy0Y/X3bThhSNe6vkqcvquMAY15nWq5wPIMpRJgev1PZU/eaBmHbGPvH1?=
 =?us-ascii?Q?dfaOKFtGSXhh47cR+V3rVyhiNb5VblxsgE0uMBq2MBWxm06yiHIZXL3lS0Or?=
 =?us-ascii?Q?Q/KLuhEpqPAYaAqr901iSHxkNmZRrYhWGOSlE2n+jvkCaXhooxU2EJ6ZGjkv?=
 =?us-ascii?Q?pUk6OhL1K5Y7IdIQ6Xk7dtMUtqOpCk+iRxXC2r4/NZw97K5BGmqvdn7++A1r?=
 =?us-ascii?Q?PsVSkrxu/OjwygZTxxGRCu+aSuQq9slY+o9g0xjRgu5sDUCirYtUVSDhSEiG?=
 =?us-ascii?Q?H6O8KlAGQbHtyhkrmGaHOeVricTjfiO11IrGUxAA9PIUFg3GJz+FC1ur96y5?=
 =?us-ascii?Q?atJE26uWGVzpBwUfaFIu/X3mwZKe0CCgP2rEA3j0ipQayBiLkLR7ZZCECDkQ?=
 =?us-ascii?Q?2xpP8q5bW58+UEukAe6V4yUhdELHPXRDJpefT9QUYN/v04OrefJrq3HgOsOU?=
 =?us-ascii?Q?ZRXirFSQF8B0cv0rZtlxKSyBUweX7RjoHVdcczc1Z/ytmPyybeMdIJCCLGRN?=
 =?us-ascii?Q?ut/spsacUIqPOzS88kGiDkZm8J+2v6UYYuFRFU+EliWbMTdaJQBz/DMeZ/KO?=
 =?us-ascii?Q?Nis6X5uhHcQlo0fiOcYIYBxrSnOmxv7Xd3p+PoEMKVllqgCqgvaKv87XUP7c?=
 =?us-ascii?Q?NPUO2gglHSlR/E/mdVRQzrfQXQl7h7eWlUVCvsTssRgFFXOkx39SiSVABx+/?=
 =?us-ascii?Q?CZTJjV6NPlTchLqPeZ7pWvR+Ofa5hlZllvu32x7Ci423kZiugM/K+PItPXQ7?=
 =?us-ascii?Q?I1nSds4buTkm4oZ9dltLc+0QU3kOmr/oLwuVFp2GOFFnS8fDXJIgYflqFZ/p?=
 =?us-ascii?Q?oYTopVIGPx5P79lxhGRJROrI418pkZ10lx/ASXxLLmfopmqu4EIFA+kD+T13?=
 =?us-ascii?Q?KBoo6kcKvxKkIiOjHra3kF98ld4qllX6oxmy2sj0ENz+HfaKu4xWAe2z21iP?=
 =?us-ascii?Q?G5yIaYpn5urQVyq+eIegzCYY3KPdatur07gtAYkQFI97fcB71EqZa8Mozk6b?=
 =?us-ascii?Q?Cd3gqKRncwDGohp7p4d3+VIRpDGaRo66nnFPlWiY6UKiL7o8MCZ4TvKNNoQZ?=
 =?us-ascii?Q?dm5npBf9ZFowudCjGNbRc3I9NqauFOkQM9gsjjPj5GfJQuIyIJxXRv5JHCkJ?=
 =?us-ascii?Q?uHXdirXoS2xFECyGK20loWzdiWBKLeCbSsN/GzL2d9MAd6EavNC9AFESTrz2?=
 =?us-ascii?Q?BaJc9qywSfbjE+UtlPt5e3w17YaR0siUYtQm2Oy7VH/vTcOLSBzwOHztwA04?=
 =?us-ascii?Q?qZnBrdFdAUo4UYn5EQ0QoHtT1fpeTXN+npyIyj0funYjQU7NqN81Euve0FgJ?=
 =?us-ascii?Q?lkcyD6KO5PbwEcwE1ZFfRAok8sW9Tm2cruncZWTyVZsSN/EvRJQ/4NyynU2a?=
 =?us-ascii?Q?unT93PJpK3h6KQl8OgEa42XGPMRKSd/5bSjB6dKxRKwb65UgGjLg8ckuGOyl?=
 =?us-ascii?Q?XCOEIw6wLOgSooIbmNdrV1RKFGpQ/vtpD3vIXERQ5wta5o24L5d2+isWLrn2?=
 =?us-ascii?Q?73myKG5cpxdXCgE0oKhWNw5EieW9GL8zwUACphltWRGnHn9imZy/GmAZwGZ9?=
 =?us-ascii?Q?NIM30Du00T4chUVRFw2RDdnHyq5G14ngCgDKA/wjZTa4K3pIi3AdPxdhHMNn?=
 =?us-ascii?Q?rg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <66CDAB30CC8BE841983C69CD8A659C24@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
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: d4eef12a-38a2-4cd3-569e-08da69eed579
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jul 2022 01:26:13.0509
 (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: ETRYqtV12h1fcv4WCu2CFpAi75n872dR5KJutgJWoGy1KN+xNrcDRS5xEzq1AHw0oLHeZINH2UKVRnK292JCyg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB6302
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-19_10,2022-07-19_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0 phishscore=0
 suspectscore=0 mlxlogscore=999 adultscore=0 spamscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207200003
X-Proofpoint-ORIG-GUID: qcQoEMLjrK8LaU1nXiU3vBVy-ZhH2FfD
X-Proofpoint-GUID: qcQoEMLjrK8LaU1nXiU3vBVy-ZhH2FfD
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 19, 2022, at 6:44 PM, Dave Chinner <david@fromorbit.com> wrote:
>=20
> On Tue, Jul 19, 2022 at 04:46:50PM -0400, Anna Schumaker wrote:
>> On Sun, Jul 17, 2022 at 9:16 PM Dave Chinner <david@fromorbit.com> wrote=
:
>>>=20
>>> On Fri, Jul 15, 2022 at 07:08:13PM +0000, Chuck Lever III wrote:
>>>>> On Jul 15, 2022, at 2:44 PM, Anna Schumaker <anna@kernel.org> wrote:
>>>>>=20
>>>>> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>>>>=20
>>>>> Rather than relying on the underlying filesystem to tell us where hol=
e
>>>>> and data segments are through vfs_llseek(), let's instead do the hole
>>>>> compression ourselves. This has a few advantages over the old
>>>>> implementation:
>>>>>=20
>>>>> 1) A single call to the underlying filesystem through nfsd_readv() me=
ans
>>>>>  the file can't change from underneath us in the middle of encoding.
>>>=20
>>> Hi Anna,
>>>=20
>>> I'm assuming you mean the vfs_llseek(SEEK_HOLE) call at the start
>>> of nfsd4_encode_read_plus_data() that is used to trim the data that
>>> has already been read out of the file?
>>=20
>> There is also the vfs_llseek(SEEK_DATA) call at the start of
>> nfsd4_encode_read_plus_hole(). They are used to determine the length
>> of the current hole or data segment.
>>=20
>>>=20
>>> What's the problem with racing with a hole punch here? All it does
>>> is shorten the read data returned to match the new hole, so all it's
>>> doing is making the returned data "more correct".
>>=20
>> The problem is we call vfs_llseek() potentially many times when
>> encoding a single reply to READ_PLUS. nfsd4_encode_read_plus() has a
>> loop where we alternate between hole and data segments until we've
>> encoded the requested number of bytes. My attempts at locking the file
>> have resulted in a deadlock since vfs_llseek() also locks the file, so
>> the file could change from underneath us during each iteration of the
>> loop.
>=20
> So the problem being solved is that the current encoding is not
> atomic, rather than trying to avoid any computational overhead of
> multiple vfs_llseek calls (which are largely just the same extent
> lookups as we do during the read call)?

Reviewing [1] and [2] I don't find any remarks about atomicity
guarantees. If a client needs an uncontended view of a file's
data, it's expected to fence other accessors via a OPEN(deny)
or LOCK operation, or serialize the requests itself.


> The implementation just seems backwards to me - rather than reading
> data and then trying to work out where the holes are, I suspect it
> should be working out where the holes are and then reading the data.
> This is how the IO path in filesystems work, so it would seem like a
> no-brainer to try to leverage the infrastructure we already have to
> do that.
>=20
> The information is there and we have infrastructure that exposes it
> to the IO path, it's just *underneath* the page cache and the page
> cache destroys the information that it used to build the data it
> returns to the NFSD.
>=20
> IOWs, it seems to me that what READ_PLUS really wants is a "sparse
> read operation" from the filesystem rather than the current "read
> that fills holes with zeroes". i.e. a read operation that sets an
> iocb flag like RWF_SPARSE_READ to tell the filesystem to trim the
> read to just the ranges that contain data.
>=20
> That way the read populates the page cache over a single contiguous
> range of data and returns with the {offset, len} that spans the
> range that is read and mapped. The caller can then read that region
> out of the page cache and mark all the non-data regions as holes in
> whatever manner they need to.
>=20
> The iomap infrastructure that XFS and other filesystems use provide
> this exact "map only what contains data" capability - an iomap tells
> the page cache exactly what underlies the data range (hole, data,
> unwritten extents, etc) in an efficient manner, so it wouldn't be a
> huge stretch just to limit read IO ranges to those that contain only
> DATA extents.
>=20
> At this point READ_PLUS then just needs to iterate doing sparse
> reads and recording the ranges that return data as vector of some
> kind that is then passes to the encoding function to encode it as
> a sparse READ_PLUS data range....

The iomap approach seems sensible to me and covers the two basic
usage scenarios:

- Large sparse files, where we want to conserve both network
  bandwidth and client (and intermediate) cache occupancy.
  These are best served by exposing data and holes.

- Everyday files that are relatively small and generally will
  continue few, if any, holes. These are best served by using
  a splice read (where possible) -- that is, READ_PLUS in this
  case should work exactly like READ.

My impression of client implementations is that, a priori,
a client does not know whether a file contains holes or not,
but will probably always use READ_PLUS and let the server
make the choice for it.

Now how does the server make that choice? Is there an attribute
bit that indicates when a file should be treated as sparse? Can
we assume that immutable files (or compressed files) should
always be treated as sparse? Alternately, the server might use
the file's data : hole ratio.


--
Chuck Lever

[1] https://datatracker.ietf.org/doc/html/rfc7862#section-6

[2] https://datatracker.ietf.org/doc/html/rfc5661#section-18.22