MIME-Version: 1.0
References: <2009825.1677229488@warthog.procyon.org.uk> <CAHk-=whAAOVBrzwb2uMjCmdRrtudGesYj0tuqdUgi8X_gbw1jw@mail.gmail.com>
 <20230220135225.91b0f28344c01d5306c31230@linux-foundation.org>
 <2134430.1677240738@warthog.procyon.org.uk> <2213409.1677249075@warthog.procyon.org.uk>
In-Reply-To: <2213409.1677249075@warthog.procyon.org.uk>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri, 24 Feb 2023 08:06:16 -0800
Message-ID: <CAHk-=whFKL4VuFBWvenG8fAgfvbf36PDgouUSx47rZDWr9BkJw@mail.gmail.com>
Subject: Re: [RFC][PATCH] cifs: Fix cifs_writepages_region()
To:     David Howells <dhowells@redhat.com>
Cc:     Steve French <stfrench@microsoft.com>,
        Vishal Moola <vishal.moola@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Jan Kara <jack@suse.cz>, Paulo Alcantara <pc@cjr.nz>,
        Matthew Wilcox <willy@infradead.org>,
        Huang Ying <ying.huang@intel.com>,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Xin Hao <xhao@linux.alibaba.com>, linux-mm@kvack.org,
        mm-commits@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: multipart/mixed; boundary="00000000000025890805f57450f3"
Precedence: bulk

--00000000000025890805f57450f3
Content-Type: text/plain; charset="UTF-8"

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

--00000000000025890805f57450f3
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_leiq6lh00>
X-Attachment-Id: f_leiq6lh00

IGZzL2NpZnMvZmlsZS5jIHwgMTIgLS0tLS0tLS0tLS0tCiAxIGZpbGUgY2hhbmdlZCwgMTIgZGVs
ZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvZnMvY2lmcy9maWxlLmMgYi9mcy9jaWZzL2ZpbGUuYwpp
bmRleCA1MzY1YTMyOTkwODguLjcwNjFkMjYzMzE1ZCAxMDA2NDQKLS0tIGEvZnMvY2lmcy9maWxl
LmMKKysrIGIvZnMvY2lmcy9maWxlLmMKQEAgLTI4NTgsNyArMjg1OCw2IEBAIHN0YXRpYyBpbnQg
Y2lmc193cml0ZXBhZ2VzX3JlZ2lvbihzdHJ1Y3QgYWRkcmVzc19zcGFjZSAqbWFwcGluZywKIAkJ
CQkgIGxvZmZfdCBzdGFydCwgbG9mZl90IGVuZCwgbG9mZl90ICpfbmV4dCkKIHsKIAlzdHJ1Y3Qg
Zm9saW9fYmF0Y2ggZmJhdGNoOwotCWludCBza2lwcyA9IDA7CiAKIAlmb2xpb19iYXRjaF9pbml0
KCZmYmF0Y2gpOwogCWRvIHsKQEAgLTI5MjcsMTcgKzI5MjYsNiBAQCBzdGF0aWMgaW50IGNpZnNf
d3JpdGVwYWdlc19yZWdpb24oc3RydWN0IGFkZHJlc3Nfc3BhY2UgKm1hcHBpbmcsCiAJCQlyZXR1
cm4gcmV0OwogCiBza2lwX3dyaXRlOgotCQkJLyoKLQkJCSAqIFRvbyBtYW55IHNraXBwZWQgd3Jp
dGVzLCBvciBuZWVkIHRvIHJlc2NoZWR1bGU/Ci0JCQkgKiBUcmVhdCBpdCBhcyBhIHdyaXRlIGVy
cm9yIHdpdGhvdXQgYW4gZXJyb3IgY29kZS4KLQkJCSAqLwotCQkJaWYgKHNraXBzID49IDUgfHwg
bmVlZF9yZXNjaGVkKCkpIHsKLQkJCQlyZXQgPSAwOwotCQkJCWdvdG8gd3JpdGVfZXJyb3I7Ci0J
CQl9Ci0KLQkJCS8qIE90aGVyd2lzZSwganVzdCBza2lwIHRoYXQgZm9saW8gYW5kIGdvIG9uIHRv
IHRoZSBuZXh0ICovCi0JCQlza2lwcysrOwogCQkJc3RhcnQgKz0gZm9saW9fc2l6ZShmb2xpbyk7
CiAJCQljb250aW51ZTsKIAkJfQo=
--00000000000025890805f57450f3--