Message ID | 1947793.1726494616@warthog.procyon.org.uk (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | cifs: Fix cifs readv callback merge resolution issue | expand |
On Mon, 16 Sept 2024 at 15:50, David Howells <dhowells@redhat.com> wrote: > > Fix this so that it is "false". The callback to netfslib in both SMB1 and > SMB2/3 now gets offloaded from the network message thread to a separate > worker thread and thus it's fine to do the slow work in this thread. Note that with this fixed, now *every* direct call of netfs_read_subreq_terminated() has that 'was_aync' as false. The exception ends up being the netfs_read_cache_to_pagecache() thing, which does that 'cres->ops->read()' with netfs_read_subreq_terminated() as a callback function. And that callback ends up being done with ki->was_async, which is actually set unconditionally to 'true' (except for the immediate failure case which then sets it to false after all). Could we please just remove that whole 'was_async' case entirely, and just make the cres->ops->read() path just do a workqueue (which seems to be what the true case does anyway)? So then the netfs_read_subreq_terminated() wouldn't need to take that pointless argument, with the only case of async use just fixing itself? Wouldn't that be cleaner? Linus
Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Could we please just remove that whole 'was_async' case entirely, and > just make the cres->ops->read() path just do a workqueue (which seems > to be what the true case does anyway)? > > So then the netfs_read_subreq_terminated() wouldn't need to take that > pointless argument, with the only case of async use just fixing > itself? Wouldn't that be cleaner? It's probably a good idea, but there's also erofs, which also goes through cachefiles_read() with it's own async callback which complicates things a little. David
On Mon, 16 Sept 2024 at 17:33, David Howells <dhowells@redhat.com> wrote: > > It's probably a good idea, but there's also erofs, which also goes through > cachefiles_read() with it's own async callback which complicates things a > little. So I was thinking that if cachefiles_read_complete() would just do the ->term_func() handling as a workqueue thing, that would make this all go away... Linus
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c index d81da161d3ed..7f3b37120a21 100644 --- a/fs/smb/client/cifssmb.c +++ b/fs/smb/client/cifssmb.c @@ -1266,7 +1266,7 @@ static void cifs_readv_worker(struct work_struct *work) struct cifs_io_subrequest *rdata = container_of(work, struct cifs_io_subrequest, subreq.work); - netfs_read_subreq_terminated(&rdata->subreq, rdata->result, true); + netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false); } static void
Fix an upstream merge resolution issue[1]. Prior to the netfs read healpers, the SMB1 asynchronous read callback, cifs_readv_worker() performed the cleanup for the operation in the network message processing loop, potentially slowing down the processing of incoming SMB messages. With commit a68c74865f51 ("cifs: Fix SMB1 readv/writev callback in the same way as SMB2/3"), this was moved to a worker thread (as is done in the SMB2/3 transport variant). However, the "was_async" argument to netfs_subreq_terminated (which was originally incorrectly "false" got flipped to "true" - which was then incorrect because, being in a kernel thread, it's not in an async context). This got corrected in the sample merge[2], but Linus, not unreasonably, switched it back to its previous value. Note that this value tells netfslib whether or not it can run sleepable stuff or stuff that takes a long time, such as retries and cleanups, in the calling thread, or whether it should offload to a worker thread. Fix this so that it is "false". The callback to netfslib in both SMB1 and SMB2/3 now gets offloaded from the network message thread to a separate worker thread and thus it's fine to do the slow work in this thread. Fixes: 35219bc5c71f ("Merge tag 'vfs-6.12.netfs' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs") Signed-off-by: David Howells <dhowells@redhat.com> cc: Linus Torvalds <torvalds@linux-foundation.org> cc: Steve French <stfrench@microsoft.com> cc: Paulo Alcantara <pc@manguebit.com> cc: Christian Brauner <brauner@kernel.org> cc: Jeff Layton <jlayton@kernel.org> cc: linux-cifs@vger.kernel.org cc: netfs@lists.linux.dev cc: linux-fsdevel@vger.kernel.org Link: https://lore.kernel.org/r/CAHk-=wjr8fxk20-wx=63mZruW1LTvBvAKya1GQ1EhyzXb-okMA@mail.gmail.com/ [1] Link: https://lore.kernel.org/linux-fsdevel/20240913-vfs-netfs-39ef6f974061@brauner/ [2] --- fs/smb/client/cifssmb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)