| Message ID | 20240913-vfs-netfs-39ef6f974061@brauner (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] vfs netfs | expand |
On Fri, 13 Sept 2024 at 18:57, Christian Brauner <brauner@kernel.org> wrote: > > /* Conflicts */ > > Merge conflicts with mainline Hmm. My conflict resolution is _similar_, but at the same time decidedly different. And I'm not sure why yours is different. > --- a/fs/smb/client/cifssmb.c > +++ b/fs/smb/client/cifssmb.c > @@@ -1261,16 -1261,6 +1261,14 @@@ openRetry > return rc; > } > > +static void cifs_readv_worker(struct work_struct *work) > +{ > + struct cifs_io_subrequest *rdata = > + container_of(work, struct cifs_io_subrequest, subreq.work); > + > - netfs_subreq_terminated(&rdata->subreq, > - (rdata->result == 0 || rdata->result == -EAGAIN) ? > - rdata->got_bytes : rdata->result, true); > ++ netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false); So here, I have ++ netfs_read_subreq_terminated(&rdata->subreq, rdata->result, true); with the third argument being 'true' instead of 'false' as in yours. The reason? That's what commit a68c74865f51 ("cifs: Fix SMB1 readv/writev callback in the same way as SMB2/3") did when it moved the (originally) netfs_subreq_terminated() into the worker, and it changed the 'was_async' argument from "false" to a "true". Now, that change makes little sense to me (a worker thread is not softirq context), but that's what commit a68c74865f51 does, and so that's logically what the merge should do. > + rdata->subreq.transferred += rdata->got_bytes; > - netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false); > ++ trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress); where did this trace_netfs_sreq() come from? > --- a/fs/smb/client/smb2pdu.c > +++ b/fs/smb/client/smb2pdu.c > @@@ -4614,6 -4613,10 +4613,8 @@@ smb2_readv_callback(struct mid_q_entry > server->credits, server->in_flight, > 0, cifs_trace_rw_credits_read_response_clear); > rdata->credits.value = 0; > + rdata->subreq.transferred += rdata->got_bytes; > - if (rdata->subreq.start + rdata->subreq.transferred >= rdata->subreq.rreq->i_size) > - __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags); > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress); And where did this conflict resolution come from? I'm not seeing why it removes that NETFS_SREQ_HIT_EOF bit logic.. Some searching gets me this: https://lore.kernel.org/all/1131388.1726141806@warthog.procyon.org.uk/ which seems to explain your merge resolution, and I'm even inclined to think that it might be right, but it's not *sensible*. That whole removal of the NETFS_SREQ_HIT_EOF bit ends up undoing part of commit ee4cdf7ba857 ("netfs: Speed up buffered reading"), and doesn't seem to be supported by the changes done on the other side of the conflict resolution. IOW, the changes may be *correct*, but they do not seem to be valid as a conflict resolution, and if they are fixes they should be done as such separately. Adding DavidH (and Steve French) to the participants, so that they can know about my confusion, and maybe send a patch to fix it up properly with actual explanations. Because I don't want to commit the merge as you suggest without explanations for why those changes were magically done independently of what seems to be happening in the development history. Linus
The pull request you sent on Fri, 13 Sep 2024 18:56:36 +0200:
> git@gitolite.kernel.org:pub/scm/linux/kernel/git/vfs/vfs tags/vfs-6.12.netfs
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/35219bc5c71f4197c8bd10297597de797c1eece5
Thank you!
Linus Torvalds <torvalds@linux-foundation.org> wrote: > > ++ netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false); > > So here, I have > > ++ netfs_read_subreq_terminated(&rdata->subreq, rdata->result, true); > > with the third argument being 'true' instead of 'false' as in yours. > > The reason? That's what commit a68c74865f51 ("cifs: Fix SMB1 > readv/writev callback in the same way as SMB2/3") did when it moved > the (originally) netfs_subreq_terminated() into the worker, and it > changed the 'was_async' argument from "false" to a "true". As part of these changes, the callback to netfslib from the SMB1 transport variant is now delegated to a separate worker thread by cifs_readv_callback() rather than being done in the cifs network processing thread (e.g. as is done by the SMB2/3 smb2_readv_worker() in smb2pdu.c), so it's better to pass "false" here. All that argument does is tell netfslib whether it can do cleanup processing and retrying in the calling thread (if "false") or whether it needs to offload it to another thread (if "true"). I should probably rename the argument from "was_async" to something more explanatory. By putting "true" here, it causes the already offloaded processing to further offload unnecessarily. It shouldn't break things though. > > + rdata->subreq.transferred += rdata->got_bytes; > > - netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false); > > ++ trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress); > > where did this trace_netfs_sreq() come from? It got copied across with other lines when sync'ing the code with smb2_readv_callback() whilst attempting the merge resolution. It's something that got missed out when porting the changes I'd made to SMB2/3 to SMB1. It should have been deferred to a follow up patch. > > --- a/fs/smb/client/smb2pdu.c > > +++ b/fs/smb/client/smb2pdu.c > > @@@ -4614,6 -4613,10 +4613,8 @@@ smb2_readv_callback(struct mid_q_entry > > server->credits, server->in_flight, > > 0, cifs_trace_rw_credits_read_response_clear); > > rdata->credits.value = 0; > > + rdata->subreq.transferred += rdata->got_bytes; > > - if (rdata->subreq.start + rdata->subreq.transferred >= rdata->subreq.rreq->i_size) > > - __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags); > > + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress); > > And where did this conflict resolution come from? I'm not seeing why > it removes that NETFS_SREQ_HIT_EOF bit logic.. A fix that went upstream via SteveF's tree rather than Christian's tree added NETFS_SREQ_HIT_EOF separately: 1da29f2c39b67b846b74205c81bf0ccd96d34727 netfs, cifs: Fix handling of short DIO read The code that added to twiddle NETFS_SREQ_HIT_EOF is in the source, just above the lines in the hunk above: if (rdata->result == -ENODATA) { __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags); rdata->result = 0; } else { size_t trans = rdata->subreq.transferred + rdata->got_bytes; if (trans < rdata->subreq.len && rdata->subreq.start + trans == ictx->remote_i_size) { __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags); rdata->result = 0; } } The two lines removed in the example resolution are therefore redundant and should have been removed, but weren't. David
On Fri, Sep 13, 2024 at 06:56:36PM +0200, Christian Brauner wrote: > Hey Linus, > > /* Summary */ > > This contains the work to improve read/write performance for the new > netfs library. > > The main performance enhancing changes are: > > - Define a structure, struct folio_queue, and a new iterator type, > ITER_FOLIOQ, to hold a buffer as a replacement for ITER_XARRAY. See > that patch for questions about naming and form. > > ITER_FOLIOQ is provided as a replacement for ITER_XARRAY. The > problem with an xarray is that accessing it requires the use of a > lock (typically the RCU read lock) - and this means that we can't > supply iterate_and_advance() with a step function that might sleep > (crypto for example) without having to drop the lock between > pages. ITER_FOLIOQ is the iterator for a chain of folio_queue > structs, where each folio_queue holds a small list of folios. A > folio_queue struct is a simpler structure than xarray and is not > subject to concurrent manipulation by the VM. folio_queue is used > rather than a bvec[] as it can form lists of indefinite size, > adding to one end and removing from the other on the fly. <...> > David Howells (24): > cachefiles: Fix non-taking of sb_writers around set/removexattr > netfs: Adjust labels in /proc/fs/netfs/stats > netfs: Record contention stats for writeback lock > netfs: Reduce number of conditional branches in netfs_perform_write() > netfs, cifs: Move CIFS_INO_MODIFIED_ATTR to netfs_inode > netfs: Move max_len/max_nr_segs from netfs_io_subrequest to netfs_io_stream > netfs: Reserve netfs_sreq_source 0 as unset/unknown > netfs: Remove NETFS_COPY_TO_CACHE > netfs: Set the request work function upon allocation > netfs: Use bh-disabling spinlocks for rreq->lock > mm: Define struct folio_queue and ITER_FOLIOQ to handle a sequence of folios > iov_iter: Provide copy_folio_from_iter() > cifs: Provide the capability to extract from ITER_FOLIOQ to RDMA SGEs > netfs: Use new folio_queue data type and iterator instead of xarray iter > netfs: Provide an iterator-reset function > netfs: Simplify the writeback code > afs: Make read subreqs async > netfs: Speed up buffered reading > netfs: Remove fs/netfs/io.c > cachefiles, netfs: Fix write to partial block at EOF > netfs: Cancel dirty folios that have no storage destination > cifs: Use iterate_and_advance*() routines directly for hashing > cifs: Switch crypto buffer to use a folio_queue rather than an xarray > cifs: Don't support ITER_XARRAY Christian, David, Do you have fixes for the following issues reported for series? https://lore.kernel.org/all/20240923183432.1876750-1-chantr4@gmail.com/ https://lore.kernel.org/all/4b5621958a758da830c1cf09c6f6893aed371f9d.camel@gmail.com/ https://lore.kernel.org/all/20240924094809.GA1182241@unreal/ In my case, I don't have any other workaround but simply revert these commits: "netfs: Use new folio_queue data type and iterator instead of xarray iter" "netfs: Provide an iterator-reset function" "netfs: Simplify the writeback code" "afs: Make read subreqs async" "netfs: Speed up buffered reading" "netfs: Remove fs/netfs/io.c" "cachefiles, netfs: Fix write to partial block at EOF" "netfs: Cancel dirty folios that have no storage destination" "cifs: Use iterate_and_advance*() routines directly for hashing" "cifs: Switch crypto buffer to use a folio_queue rather than an xarray" "cifs: Don't support ITER_XARRAY" "cifs: Fix signature miscalculation" "cifs: Fix cifs readv callback merge resolution issue" "cifs: Remove redundant setting of NETFS_SREQ_HIT_EOF" Thanks
Leon Romanovsky <leon@kernel.org> wrote: > Do you have fixes for the following issues reported for series? > https://lore.kernel.org/all/20240923183432.1876750-1-chantr4@gmail.com/ > https://lore.kernel.org/all/4b5621958a758da830c1cf09c6f6893aed371f9d.camel@gmail.com/ > https://lore.kernel.org/all/20240924094809.GA1182241@unreal/ I'm working on a fix for the third one at the moment, I think it's the read version of the write fix I posted previously: https://lore.kernel.org/linux-fsdevel/2050099.1727359110@warthog.procyon.org.uk/ I'm looking to see if I can make a general solution that abstracts out the buffer handling for both as we're early in the cycle. I haven't looked at the first two yet. I was in the middle of testing 9p when I hit the write-side bug that cifs was also seeing (ie. the url above). David
On Fri, Sep 27, 2024 at 09:01:19AM +0100, David Howells wrote: > Leon Romanovsky <leon@kernel.org> wrote: > > > Do you have fixes for the following issues reported for series? > > https://lore.kernel.org/all/20240923183432.1876750-1-chantr4@gmail.com/ > > https://lore.kernel.org/all/4b5621958a758da830c1cf09c6f6893aed371f9d.camel@gmail.com/ > > https://lore.kernel.org/all/20240924094809.GA1182241@unreal/ > > I'm working on a fix for the third one at the moment, I think it's the read > version of the write fix I posted previously: > > https://lore.kernel.org/linux-fsdevel/2050099.1727359110@warthog.procyon.org.uk/ > > I'm looking to see if I can make a general solution that abstracts out the > buffer handling for both as we're early in the cycle. I hope that you mean that we have plenty of time before the merge window ends. Otherwise, it will be very inconvenient to open official -next/-rc branches, based on -rc1, remembering to revert so many commits. It is better to have fast fixes and then work on improving them without worrying about time frames. Thanks
Leon Romanovsky <leon@kernel.org> wrote: > I hope that you mean that we have plenty of time before the merge window ends. > Otherwise, it will be very inconvenient to open official -next/-rc branches, > based on -rc1, remembering to revert so many commits. Yes, I'm aware of the time, thank you. Can you please try the branch at: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=netfs-fixes see if it fixes your problem? It runs through -g quick with 9p over TCP for me. David
On Fri, Sep 27, 2024 at 09:31:28PM +0100, David Howells wrote: > Leon Romanovsky <leon@kernel.org> wrote: > > > I hope that you mean that we have plenty of time before the merge window ends. > > Otherwise, it will be very inconvenient to open official -next/-rc branches, > > based on -rc1, remembering to revert so many commits. > > Yes, I'm aware of the time, thank you. > > Can you please try the branch at: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=netfs-fixes > > see if it fixes your problem? It runs through -g quick with 9p over TCP for > me. I tried it now and it didn't fix my issue. ➜ kernel git:(netfs-fixes) git l 8e18fe180b0a netfs: Abstract out a rolling folio buffer implementation [ 1.506633][ T1] Run /sbin/init as init process [ 1.506800][ T1] with arguments: [ 1.506895][ T1] /sbin/init [ 1.507004][ T1] with environment: [ 1.507104][ T1] HOME=/ [ 1.507201][ T1] TERM=linux [ 1.512981][ T1] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x6ce48 [ 1.513934][ T1] flags: 0x4000000000000000(zone=1) [ 1.514142][ T1] raw: 4000000000000000 ffffea0001b39248 ffffea00001583c8 0000000000000000 [ 1.514536][ T1] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000 [ 1.514796][ T1] page dumped because: VM_BUG_ON_FOLIO(((unsigned int) folio_ref_count(folio) + 127u <= 127u)) [ 1.515087][ T1] ------------[ cut here ]------------ [ 1.515231][ T1] kernel BUG at include/linux/mm.h:1444! [ 1.515375][ T1] Oops: invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN [ 1.515596][ T1] CPU: 4 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0+ #2546 [ 1.515823][ T1] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014 [ 1.516153][ T1] RIP: 0010:__iov_iter_get_pages_alloc+0x16d4/0x2210 [ 1.516345][ T1] Code: 84 f2 fa ff ff 48 89 ef e8 c9 20 98 ff e9 e5 fa ff ff 48 8d 48 ff e9 2c fe ff ff 48 c7 c6 00 eb 21 83 48 89 cf e8 fc 25 8a ff <0f> 0b 48 b8 00 00 00 00 00 fc ff df 4c 8b 74 24 68 44 8b 5c 24 30 [ 1.516874][ T1] RSP: 0000:ffff8880060f6e40 EFLAGS: 00010286 [ 1.517126][ T1] RAX: 000000000000005c RBX: ffffea0001b39234 RCX: 0000000000000000 [ 1.517434][ T1] RDX: 000000000000005c RSI: 0000000000000004 RDI: ffffed1000c1edbb [ 1.517676][ T1] RBP: dffffc0000000000 R08: 0000000000000000 R09: fffffbfff0718ce0 [ 1.517898][ T1] R10: 0000000000000003 R11: 0000000000000001 R12: ffff88800950f420 [ 1.518111][ T1] R13: ffff88800aba0c00 R14: 0000000000000002 R15: 0000000000001000 [ 1.518321][ T1] FS: 0000000000000000(0000) GS:ffff88806d000000(0000) knlGS:0000000000000000 [ 1.518587][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1.518804][ T1] CR2: 0000000000000000 CR3: 0000000003881001 CR4: 0000000000370eb0 [ 1.519058][ T1] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1.519302][ T1] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 1.519553][ T1] Call Trace: [ 1.519687][ T1] <TASK> [ 1.519790][ T1] ? __die+0x52/0x8f [ 1.519947][ T1] ? die+0x2a/0x50 [ 1.520107][ T1] ? do_trap+0x1d9/0x2c0 [ 1.520227][ T1] ? __iov_iter_get_pages_alloc+0x16d4/0x2210 [ 1.520421][ T1] ? do_error_trap+0xa3/0x160 [ 1.520599][ T1] ? __iov_iter_get_pages_alloc+0x16d4/0x2210 [ 1.520813][ T1] ? handle_invalid_op+0x2c/0x30 [ 1.520991][ T1] ? __iov_iter_get_pages_alloc+0x16d4/0x2210 [ 1.521245][ T1] ? exc_invalid_op+0x29/0x40 [ 1.521438][ T1] ? asm_exc_invalid_op+0x16/0x20 [ 1.521601][ T1] ? __iov_iter_get_pages_alloc+0x16d4/0x2210 [ 1.521799][ T1] ? iov_iter_extract_pages+0x1ee0/0x1ee0 [ 1.521954][ T1] ? radix_tree_node_alloc.constprop.0+0x16a/0x2c0 [ 1.522148][ T1] ? lock_acquire+0xe2/0x500 [ 1.522308][ T1] ? mark_lock+0xfc/0x2dc0 [ 1.522462][ T1] iov_iter_get_pages_alloc2+0x3d/0xe0 [ 1.522624][ T1] ? print_usage_bug.part.0+0x600/0x600 [ 1.522795][ T1] p9_get_mapped_pages.part.0.constprop.0+0x3bf/0x6c0 [ 1.523025][ T1] ? p9pdu_vwritef+0x320/0x1f20 [ 1.523239][ T1] ? p9_virtio_request+0x550/0x550 [ 1.523427][ T1] ? pdu_read+0xc0/0xc0 [ 1.523551][ T1] ? lock_release+0x220/0x780 [ 1.523714][ T1] ? pdu_read+0xc0/0xc0 [ 1.523824][ T1] p9_virtio_zc_request+0x728/0x1020 [ 1.523975][ T1] ? p9pdu_vwritef+0x320/0x1f20 [ 1.524119][ T1] ? p9_virtio_probe+0xa20/0xa20 [ 1.524271][ T1] ? netfs_read_to_pagecache+0x600/0xd90 [ 1.524424][ T1] ? mark_lock+0xfc/0x2dc0 [ 1.524586][ T1] ? p9pdu_finalize+0xdc/0x1d0 [ 1.524762][ T1] ? p9_client_prepare_req+0x235/0x360 [ 1.524918][ T1] ? p9_tag_alloc+0x6e0/0x6e0 [ 1.525084][ T1] ? lock_release+0x220/0x780 [ 1.525241][ T1] p9_client_zc_rpc.constprop.0+0x236/0x7d0 [ 1.525434][ T1] ? __create_object+0x5e/0x80 [ 1.525595][ T1] ? p9_client_flush.isra.0+0x390/0x390 [ 1.525746][ T1] ? lockdep_hardirqs_on_prepare+0x268/0x3e0 [ 1.525911][ T1] ? __call_rcu_common.constprop.0+0x475/0xc80 [ 1.526092][ T1] ? p9_req_put+0x17a/0x200 [ 1.526226][ T1] p9_client_read_once+0x343/0x840 [ 1.526361][ T1] ? p9_client_getlock_dotl+0x3c0/0x3c0 [ 1.526501][ T1] p9_client_read+0xf1/0x150 [ 1.526649][ T1] v9fs_issue_read+0x107/0x300 [ 1.526788][ T1] ? v9fs_issue_write+0x140/0x140 [ 1.526934][ T1] ? __rwlock_init+0x150/0x150 [ 1.527073][ T1] ? lockdep_hardirqs_on_prepare+0x268/0x3e0 [ 1.527249][ T1] netfs_read_to_pagecache+0x600/0xd90 [ 1.527396][ T1] netfs_readahead+0x47a/0x960 [ 1.527552][ T1] read_pages+0x17b/0xaf0 [ 1.527676][ T1] ? lru_move_tail+0x8f0/0x8f0 [ 1.527842][ T1] ? file_ra_state_init+0xd0/0xd0 [ 1.528004][ T1] page_cache_ra_unbounded+0x324/0x5f0 [ 1.528167][ T1] filemap_get_pages+0x597/0x16b0 [ 1.528324][ T1] ? filemap_add_folio+0x140/0x140 [ 1.528476][ T1] ? lock_is_held_type+0x81/0xe0 [ 1.528637][ T1] filemap_read+0x2ec/0xa90 [ 1.528799][ T1] ? filemap_get_pages+0x16b0/0x16b0 [ 1.528969][ T1] ? 0xffffffff81000000 [ 1.529090][ T1] ? find_held_lock+0x2d/0x110 [ 1.529286][ T1] ? lock_is_held_type+0x81/0xe0 [ 1.529437][ T1] ? down_read_interruptible+0x1f6/0x490 [ 1.529601][ T1] ? down_read+0x450/0x450 [ 1.529748][ T1] ? lockdep_hardirqs_on_prepare+0x3e0/0x3e0 [ 1.529953][ T1] ? find_held_lock+0x2d/0x110 [ 1.530098][ T1] netfs_buffered_read_iter+0xe2/0x130 [ 1.530254][ T1] ? netfs_file_read_iter+0xb2/0x130 [ 1.530398][ T1] __kernel_read+0x2db/0x8a0 [ 1.530554][ T1] ? __x64_sys_lseek+0x1d0/0x1d0 [ 1.530700][ T1] bprm_execve+0x548/0x1410 [ 1.530864][ T1] ? setup_arg_pages+0xb40/0xb40 [ 1.531007][ T1] ? __cond_resched+0x17/0x70 [ 1.531152][ T1] kernel_execve+0x26a/0x2f0 [ 1.531296][ T1] try_to_run_init_process+0xf/0x30 [ 1.531445][ T1] ? rest_init+0x1b0/0x1b0 [ 1.531595][ T1] kernel_init+0xe2/0x140 [ 1.531704][ T1] ? _raw_spin_unlock_irq+0x24/0x30 [ 1.531862][ T1] ret_from_fork+0x2d/0x70 [ 1.532013][ T1] ? rest_init+0x1b0/0x1b0 [ 1.532162][ T1] ret_from_fork_asm+0x11/0x20 [ 1.532342][ T1] </TASK> [ 1.532463][ T1] Modules linked in: [ 1.532679][ T1] ---[ end trace 0000000000000000 ]--- [ 1.532827][ T1] RIP: 0010:__iov_iter_get_pages_alloc+0x16d4/0x2210 [ 1.533109][ T1] Code: 84 f2 fa ff ff 48 89 ef e8 c9 20 98 ff e9 e5 fa ff ff 48 8d 48 ff e9 2c fe ff ff 48 c7 c6 00 eb 21 83 48 89 cf e8 fc 25 8a ff <0f> 0b 48 b8 00 00 00 00 00 fc ff df 4c 8b 74 24 68 44 8b 5c 24 30 [ 1.533633][ T1] RSP: 0000:ffff8880060f6e40 EFLAGS: 00010286 [ 1.533847][ T1] RAX: 000000000000005c RBX: ffffea0001b39234 RCX: 0000000000000000 [ 1.534054][ T1] RDX: 000000000000005c RSI: 0000000000000004 RDI: ffffed1000c1edbb [ 1.534261][ T1] RBP: dffffc0000000000 R08: 0000000000000000 R09: fffffbfff0718ce0 [ 1.534459][ T1] R10: 0000000000000003 R11: 0000000000000001 R12: ffff88800950f420 [ 1.534668][ T1] R13: ffff88800aba0c00 R14: 0000000000000002 R15: 0000000000001000 [ 1.534878][ T1] FS: 0000000000000000(0000) GS:ffff88806d000000(0000) knlGS:0000000000000000 [ 1.535129][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1.535309][ T1] CR2: 0000000000000000 CR3: 0000000003881001 CR4: 0000000000370eb0 [ 1.535517][ T1] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1.535734][ T1] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 1.535953][ T1] ------------[ cut here ]------------ > > David >
diff --cc fs/smb/client/cifssmb.c index cfae2e918209,04f2a5441a89..d0df0c17b18f --- a/fs/smb/client/cifssmb.c diff --cc fs/smb/client/smb2pdu.c index 88dc49d67037,95377bb91950..bb8ecbbe78af --- a/fs/smb/client/smb2pdu.c
Hey Linus, /* Summary */ This contains the work to improve read/write performance for the new netfs library. The main performance enhancing changes are: - Define a structure, struct folio_queue, and a new iterator type, ITER_FOLIOQ, to hold a buffer as a replacement for ITER_XARRAY. See that patch for questions about naming and form. ITER_FOLIOQ is provided as a replacement for ITER_XARRAY. The problem with an xarray is that accessing it requires the use of a lock (typically the RCU read lock) - and this means that we can't supply iterate_and_advance() with a step function that might sleep (crypto for example) without having to drop the lock between pages. ITER_FOLIOQ is the iterator for a chain of folio_queue structs, where each folio_queue holds a small list of folios. A folio_queue struct is a simpler structure than xarray and is not subject to concurrent manipulation by the VM. folio_queue is used rather than a bvec[] as it can form lists of indefinite size, adding to one end and removing from the other on the fly. - Provide a copy_folio_from_iter() wrapper. - Make cifs RDMA support ITER_FOLIOQ. - Use folio queues in the write-side helpers instead of xarrays. - Add a function to reset the iterator in a subrequest. - Simplify the write-side helpers to use sheaves to skip gaps rather than trying to work out where gaps are. - In afs, make the read subrequests asynchronous, putting them into work items to allow the next patch to do progressive unlocking/reading. - Overhaul the read-side helpers to improve performance. - Fix the caching of a partial block at the end of a file. - Allow a store to be cancelled. Then some changes for cifs to make it use folio queues instead of xarrays for crypto bufferage: - Use raw iteration functions rather than manually coding iteration when hashing data. - Switch to using folio_queue for crypto buffers. - Remove the xarray bits. Make some adjustments to the /proc/fs/netfs/stats file such that: - All the netfs stats lines begin 'Netfs:' but change this to something a bit more useful. - Add a couple of stats counters to track the numbers of skips and waits on the per-inode writeback serialisation lock to make it easier to check for this as a source of performance loss. Miscellaneous work: - Ensure that the sb_writers lock is taken around vfs_{set,remove}xattr() in the cachefiles code. - Reduce the number of conditional branches in netfs_perform_write(). - Move the CIFS_INO_MODIFIED_ATTR flag to the netfs_inode struct and remove cifs_post_modify(). - Move the max_len/max_nr_segs members from netfs_io_subrequest to netfs_io_request as they're only needed for one subreq at a time. - Add an 'unknown' source value for tracing purposes. - Remove NETFS_COPY_TO_CACHE as it's no longer used. - Set the request work function up front at allocation time. - Use bh-disabling spinlocks for rreq->lock as cachefiles completion may be run from block-filesystem DIO completion in softirq context. - Remove fs/netfs/io.c. /* Testing */ gcc version 14.2.0 (Debian 14.2.0-3) Debian clang version 16.0.6 (27+b1) All patches are based on the vfs-6.11-rc7.fixes merge to bring in prerequisite fixes in individual filesystems. All of this has been sitting in linux-next. No build failures or warnings were observed. /* Conflicts */ Merge conflicts with mainline ============================= No known merge conflicts. This has now a merge conflict with main due to some rather late cifs fixes. This can be resolved by: git rm fs/netfs/io.c and then: +++ b/fs/smb/client/cifssmb.c @@@ -1261,16 -1261,6 +1261,14 @@@ openRetry return rc; } +static void cifs_readv_worker(struct work_struct *work) +{ + struct cifs_io_subrequest *rdata = + container_of(work, struct cifs_io_subrequest, subreq.work); + - netfs_subreq_terminated(&rdata->subreq, - (rdata->result == 0 || rdata->result == -EAGAIN) ? - rdata->got_bytes : rdata->result, true); ++ netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false); +} + static void cifs_readv_callback(struct mid_q_entry *mid) { @@@ -1323,21 -1306,11 +1321,23 @@@ rdata->result = -EIO; } - if (rdata->result == 0 || rdata->result == -EAGAIN) - iov_iter_advance(&rdata->subreq.io_iter, rdata->got_bytes); + if (rdata->result == -ENODATA) { + __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags); + rdata->result = 0; + } else { - if (rdata->got_bytes < rdata->actual_len && - rdata->subreq.start + rdata->subreq.transferred + rdata->got_bytes == - ictx->remote_i_size) { ++ size_t trans = rdata->subreq.transferred + rdata->got_bytes; ++ if (trans < rdata->subreq.len && ++ rdata->subreq.start + trans == ictx->remote_i_size) { + __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags); + rdata->result = 0; + } + } + rdata->credits.value = 0; + rdata->subreq.transferred += rdata->got_bytes; - netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false); ++ trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress); + INIT_WORK(&rdata->subreq.work, cifs_readv_worker); + queue_work(cifsiod_wq, &rdata->subreq.work); release_mid(mid); add_credits(server, &credits, 0); } +++ b/fs/smb/client/smb2pdu.c @@@ -4614,6 -4613,10 +4613,8 @@@ smb2_readv_callback(struct mid_q_entry server->credits, server->in_flight, 0, cifs_trace_rw_credits_read_response_clear); rdata->credits.value = 0; + rdata->subreq.transferred += rdata->got_bytes; - if (rdata->subreq.start + rdata->subreq.transferred >= rdata->subreq.rreq->i_size) - __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags); + trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress); INIT_WORK(&rdata->subreq.work, smb2_readv_worker); queue_work(cifsiod_wq, &rdata->subreq.work); release_mid(mid); Merge conflicts with other trees ================================ No known merge conflicts. The following changes since commit 4356ab331c8f0dbed0f683abde345cd5503db1e4: Merge tag 'vfs-6.11-rc7.fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs (2024-09-04 09:33:57 -0700) are available in the Git repository at: git@gitolite.kernel.org:pub/scm/linux/kernel/git/vfs/vfs tags/vfs-6.12.netfs for you to fetch changes up to 4b40d43d9f951d87ae8dc414c2ef5ae50303a266: docs: filesystems: corrected grammar of netfs page (2024-09-12 12:20:43 +0200) Please consider pulling these changes from the signed vfs-6.12.netfs tag. Thanks! Christian ---------------------------------------------------------------- vfs-6.12.netfs ---------------------------------------------------------------- Christian Brauner (1): Merge branch 'netfs-writeback' of ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs into vfs.netfs David Howells (24): cachefiles: Fix non-taking of sb_writers around set/removexattr netfs: Adjust labels in /proc/fs/netfs/stats netfs: Record contention stats for writeback lock netfs: Reduce number of conditional branches in netfs_perform_write() netfs, cifs: Move CIFS_INO_MODIFIED_ATTR to netfs_inode netfs: Move max_len/max_nr_segs from netfs_io_subrequest to netfs_io_stream netfs: Reserve netfs_sreq_source 0 as unset/unknown netfs: Remove NETFS_COPY_TO_CACHE netfs: Set the request work function upon allocation netfs: Use bh-disabling spinlocks for rreq->lock mm: Define struct folio_queue and ITER_FOLIOQ to handle a sequence of folios iov_iter: Provide copy_folio_from_iter() cifs: Provide the capability to extract from ITER_FOLIOQ to RDMA SGEs netfs: Use new folio_queue data type and iterator instead of xarray iter netfs: Provide an iterator-reset function netfs: Simplify the writeback code afs: Make read subreqs async netfs: Speed up buffered reading netfs: Remove fs/netfs/io.c cachefiles, netfs: Fix write to partial block at EOF netfs: Cancel dirty folios that have no storage destination cifs: Use iterate_and_advance*() routines directly for hashing cifs: Switch crypto buffer to use a folio_queue rather than an xarray cifs: Don't support ITER_XARRAY Dennis Lam (1): docs: filesystems: corrected grammar of netfs page Documentation/filesystems/netfs_library.rst | 2 +- fs/9p/vfs_addr.c | 11 +- fs/afs/file.c | 30 +- fs/afs/fsclient.c | 9 +- fs/afs/write.c | 4 +- fs/afs/yfsclient.c | 9 +- fs/cachefiles/io.c | 19 +- fs/cachefiles/xattr.c | 34 +- fs/ceph/addr.c | 76 +-- fs/netfs/Makefile | 4 +- fs/netfs/buffered_read.c | 766 ++++++++++++++++---------- fs/netfs/buffered_write.c | 309 +++++------ fs/netfs/direct_read.c | 147 ++++- fs/netfs/internal.h | 43 +- fs/netfs/io.c | 804 ---------------------------- fs/netfs/iterator.c | 50 ++ fs/netfs/main.c | 7 +- fs/netfs/misc.c | 94 ++++ fs/netfs/objects.c | 16 +- fs/netfs/read_collect.c | 544 +++++++++++++++++++ fs/netfs/read_pgpriv2.c | 264 +++++++++ fs/netfs/read_retry.c | 256 +++++++++ fs/netfs/stats.c | 27 +- fs/netfs/write_collect.c | 246 +++------ fs/netfs/write_issue.c | 93 ++-- fs/nfs/fscache.c | 19 +- fs/nfs/fscache.h | 7 +- fs/smb/client/cifsencrypt.c | 144 +---- fs/smb/client/cifsglob.h | 4 +- fs/smb/client/cifssmb.c | 6 +- fs/smb/client/file.c | 96 ++-- fs/smb/client/smb2ops.c | 219 ++++---- fs/smb/client/smb2pdu.c | 27 +- fs/smb/client/smbdirect.c | 82 +-- include/linux/folio_queue.h | 156 ++++++ include/linux/iov_iter.h | 104 ++++ include/linux/netfs.h | 46 +- include/linux/uio.h | 18 + include/trace/events/netfs.h | 144 +++-- lib/iov_iter.c | 240 ++++++++- lib/kunit_iov_iter.c | 259 +++++++++ lib/scatterlist.c | 69 ++- 42 files changed, 3520 insertions(+), 1984 deletions(-) delete mode 100644 fs/netfs/io.c create mode 100644 fs/netfs/read_collect.c create mode 100644 fs/netfs/read_pgpriv2.c create mode 100644 fs/netfs/read_retry.c create mode 100644 include/linux/folio_queue.h