Re: [PATCH v5 37/40] netfs: Optimise away reads above the point at which there can be no data

From: Nathan Chancellor
Date: Thu Dec 21 2023 - 18:02:18 EST

Next message: Martin Jücker: "[PATCH 1/2] ARM: dts: exynos: add accelerometer and gyro to p4note"
Previous message: Tejun Heo: "Re: [PATCHSET wq/for-6.8] workqueue: Implement system-wide max_active for unbound workqueues"
In reply to: David Howells: "[PATCH v5 37/40] netfs: Optimise away reads above the point at which there can be no data"
Next in thread: David Howells: "Re: [PATCH v5 37/40] netfs: Optimise away reads above the point at which there can be no data"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi David,

On Thu, Dec 21, 2023 at 01:23:32PM +0000, David Howells wrote:
> Track the file position above which the server is not expected to have any
> data (the "zero point") and preemptively assume that we can satisfy
> requests by filling them with zeroes locally rather than attempting to
> download them if they're over that line - even if we've written data back
> to the server. Assume that any data that was written back above that
> position is held in the local cache. Note that we have to split requests
> that straddle the line.
>
> Make use of this to optimise away some reads from the server. We need to
> set the zero point in the following circumstances:
>
> (1) When we see an extant remote inode and have no cache for it, we set
> the zero_point to i_size.
>
> (2) On local inode creation, we set zero_point to 0.
>
> (3) On local truncation down, we reduce zero_point to the new i_size if
> the new i_size is lower.
>
> (4) On local truncation up, we don't change zero_point.
>
> (5) On local modification, we don't change zero_point.
>
> (6) On remote invalidation, we set zero_point to the new i_size.
>
> (7) If stored data is discarded from the pagecache or culled from fscache,
> we must set zero_point above that if the data also got written to the
> server.
>
> (8) If dirty data is written back to the server, but not fscache, we must
> set zero_point above that.
>
> (9) If a direct I/O write is made, set zero_point above that.
>
> Assuming the above, any read from the server at or above the zero_point
> position will return all zeroes.
>
> The zero_point value can be stored in the cache, provided the above rules
> are applied to it by any code that culls part of the local cache.
>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Jeff Layton <jlayton@xxxxxxxxxx>
> cc: linux-cachefs@xxxxxxxxxx
> cc: linux-fsdevel@xxxxxxxxxxxxxxx
> cc: linux-mm@xxxxxxxxx
> ---

<snip>

> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index 8cde618cf6d9..a5374218efe4 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -136,6 +136,8 @@ struct netfs_inode {
> struct fscache_cookie *cache;
> #endif
> loff_t remote_i_size; /* Size of the remote file */
> + loff_t zero_point; /* Size after which we assume there's no data
> + * on the server */
> unsigned long flags;
> #define NETFS_ICTX_ODIRECT 0 /* The file has DIO in progress */
> #define NETFS_ICTX_UNBUFFERED 1 /* I/O should not use the pagecache */
> @@ -463,22 +465,30 @@ static inline void netfs_inode_init(struct netfs_inode *ctx,
> {
> ctx->ops = ops;
> ctx->remote_i_size = i_size_read(&ctx->inode);
> + ctx->zero_point = ctx->remote_i_size;
> ctx->flags = 0;
> #if IS_ENABLED(CONFIG_FSCACHE)
> ctx->cache = NULL;
> #endif
> + /* ->releasepage() drives zero_point */
> + mapping_set_release_always(ctx->inode.i_mapping);
> }

I bisected a crash that I see when trying to mount an NFS volume to this
change as commit 6e3c8451f624 ("netfs: Optimise away reads above the
point at which there can be no data") in next-20231221:

[ 45.964963] BUG: kernel NULL pointer dereference, address: 0000000000000078
[ 45.964975] #PF: supervisor write access in kernel mode
[ 45.964982] #PF: error_code(0x0002) - not-present page
[ 45.964987] PGD 0 P4D 0
[ 45.964996] Oops: 0002 [#1] PREEMPT SMP NOPTI
[ 45.965004] CPU: 2 PID: 2419 Comm: mount.nfs Not tainted 6.7.0-rc6-next-20231221-debug-09925-g857647efa9be #1 adbbe7bc5037c662bc8f9b8e78ccf16be15b5e58
[ 45.965014] Hardware name: HP HP Desktop M01-F1xxx/87D6, BIOS F.12 12/17/2020
[ 45.965019] RIP: 0010:nfs_alloc_inode+0xa2/0xc0 [nfs]
[ 45.965092] Code: 80 b0 01 00 00 00 00 00 00 48 c7 80 38 04 00 00 00 f7 1e c2 48 c7 80 58 04 00 00 00 00 00 00 48 c7 80 40 04 00 00 00 00 00 00 <f0> 80 0a 80 48 05 b8 01 00 00 e9 5f 2b 20 f5 66 66 2e 0f 1f 84 00
[ 45.965099] RSP: 0018:ffffc900058f7bc0 EFLAGS: 00010286
[ 45.965107] RAX: ffff8881958c7290 RBX: ffff888168f0f800 RCX: 0000000000000000
[ 45.965112] RDX: 0000000000000078 RSI: ffffffffc2140a71 RDI: ffff88817a12b880
[ 45.965118] RBP: ffff888168f0f800 R08: ffffc900058f7b70 R09: 88728c958188ffff
[ 45.965123] R10: 000000000003a5c0 R11: 0000000000000005 R12: ffffffffc22f1a80
[ 45.965128] R13: ffffc900058f7c30 R14: 0000000000000000 R15: 0000000000000002
[ 45.965134] FS: 00007ff78c318740(0000) GS:ffff8887ff280000(0000) knlGS:0000000000000000
[ 45.965140] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 45.965146] CR2: 0000000000000078 CR3: 000000018a514000 CR4: 0000000000350ef0
[ 45.965152] Call Trace:
[ 45.965160] <TASK>
[ 45.965167] ? __die+0x23/0x70
[ 45.965183] ? page_fault_oops+0x173/0x4e0
[ 45.965197] ? nfs_alloc_inode+0x21/0xc0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965279] ? exc_page_fault+0x7e/0x180
[ 45.965291] ? asm_exc_page_fault+0x26/0x30
[ 45.965308] ? nfs_alloc_inode+0x21/0xc0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965374] ? nfs_alloc_inode+0xa2/0xc0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965441] alloc_inode+0x1e/0xc0
[ 45.965452] ? __pfx_nfs_find_actor+0x10/0x10 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965517] iget5_locked+0x97/0xf0
[ 45.965525] ? __pfx_nfs_init_locked+0x10/0x10 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965593] nfs_fhget+0xe4/0x700 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965666] nfs_get_root+0xc6/0x4a0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965732] ? kernfs_rename_ns+0x85/0x210
[ 45.965754] nfs_get_tree_common+0xc7/0x520 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965826] vfs_get_tree+0x29/0xf0
[ 45.965836] fc_mount+0x12/0x40
[ 45.965846] do_nfs4_mount+0x12e/0x370 [nfsv4 9bac1f2bd94d7294fbbaf875b7b5cec5adc527f5]
[ 45.965946] nfs4_try_get_tree+0x48/0xd0 [nfsv4 9bac1f2bd94d7294fbbaf875b7b5cec5adc527f5]
[ 45.966034] vfs_get_tree+0x29/0xf0
[ 45.966041] ? srso_return_thunk+0x5/0x5f
[ 45.966051] path_mount+0x4ca/0xb10
[ 45.966063] __x64_sys_mount+0x11a/0x150
[ 45.966074] do_syscall_64+0x64/0xe0
[ 45.966083] ? do_syscall_64+0x70/0xe0
[ 45.966090] ? syscall_exit_to_user_mode+0x2b/0x40
[ 45.966098] ? srso_return_thunk+0x5/0x5f
[ 45.966106] ? do_syscall_64+0x70/0xe0
[ 45.966113] ? srso_return_thunk+0x5/0x5f
[ 45.966121] ? exc_page_fault+0x7e/0x180
[ 45.966130] entry_SYSCALL_64_after_hwframe+0x6c/0x74
[ 45.966138] RIP: 0033:0x7ff78c5f2a1e
...

It appears that ctx->inode.i_mapping is NULL in netfs_inode_init(). This
patch appears to cure the problem for me but I am not sure if it is
proper or not.

Cheers,
Nathan

diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index a5374218efe4..8daaba665421 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -471,7 +471,8 @@ static inline void netfs_inode_init(struct netfs_inode *ctx,
ctx->cache = NULL;
#endif
/* ->releasepage() drives zero_point */
- mapping_set_release_always(ctx->inode.i_mapping);
+ if (ctx->inode.i_mapping)
+ mapping_set_release_always(ctx->inode.i_mapping);
}

/**

Next message: Martin Jücker: "[PATCH 1/2] ARM: dts: exynos: add accelerometer and gyro to p4note"
Previous message: Tejun Heo: "Re: [PATCHSET wq/for-6.8] workqueue: Implement system-wide max_active for unbound workqueues"
In reply to: David Howells: "[PATCH v5 37/40] netfs: Optimise away reads above the point at which there can be no data"
Next in thread: David Howells: "Re: [PATCH v5 37/40] netfs: Optimise away reads above the point at which there can be no data"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]