Re: [RFC][PATCH] cifs: Fix cifs_writepages_region()

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On Fri, Feb 24, 2023 at 6:31 AM David Howells <dhowells@redhat.com> wrote:
>
> Here's the simplest fix for cifs_writepages_region() that gets it to work.

Hmm. The commit message for this is wrong.

> Fix the cifs_writepages_region() to just skip over members of the batch that
> have been cleaned up rather than retrying them.

It never retried them. The "skip_write" code did that same

                        start += folio_size(folio);
                        continue;

that your patch does, but it *also* had that

                        if (skips >= 5 || need_resched()) {

thing to just stop writing entirely.

> I'm not entirely sure why it fixes it, though.

Yes. Strange. Because it does the exact same thing as the "Oh, the
trylock worked, but it was still under writeback or fscache" did. I
just merged all the "skip write" cases.

But the code is clearly (a) not working and (b) the whole skip count
and need_resched() logic is a bit strange to begin with.

Can you humor me, and try if just removing that skip count thing
instead? IOW, this attached patch? Because that whole "let's stop
writing if we need to reschedule" sounds truly odd (we have a
cond_resched(), although it's per folio batch, not per-folio), and the
skip count logic doesn't make much sense to me either.

SteveF?

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 875 bytes --]

 fs/cifs/file.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 5365a3299088..7061d263315d 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2858,7 +2858,6 @@ static int cifs_writepages_region(struct address_space *mapping,
 				  loff_t start, loff_t end, loff_t *_next)
 {
 	struct folio_batch fbatch;
-	int skips = 0;
 
 	folio_batch_init(&fbatch);
 	do {
@@ -2927,17 +2926,6 @@ static int cifs_writepages_region(struct address_space *mapping,
 			return ret;
 
 skip_write:
-			/*
-			 * Too many skipped writes, or need to reschedule?
-			 * Treat it as a write error without an error code.
-			 */
-			if (skips >= 5 || need_resched()) {
-				ret = 0;
-				goto write_error;
-			}
-
-			/* Otherwise, just skip that folio and go on to the next */
-			skips++;
 			start += folio_size(folio);
 			continue;
 		}