cifs: Fix cifs readv callback merge resolution issue

Commit Message

David Howells Sept. 16, 2024, 1:50 p.m. UTC

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 <[e-mail]>
cc: Linus Torvalds <[e-mail]>
cc: Steve French <[e-mail]>
cc: Paulo Alcantara <[e-mail]>
cc: Christian Brauner <[e-mail]>
cc: Jeff Layton <[e-mail]>
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
Link: https://lore.kernel.org/r/CAHk-=wjr8fxk20-wx=[e-mail]/ [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(-)

Comments

Linus Torvalds Sept. 16, 2024, 2:08 p.m. UTC | #1

On Mon, 16 Sept 2024 at 15:50, David Howells <[e-mail]> 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

David Howells Sept. 16, 2024, 3:33 p.m. UTC | #2

Linus Torvalds <[e-mail]> 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

Linus Torvalds Sept. 16, 2024, 4:49 p.m. UTC | #3

On Mon, 16 Sept 2024 at 17:33, David Howells <[e-mail]> 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

Patch

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