From: David Howells <dhowells@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
Vishal Moola <vishal.moola@gmail.com>,
Steve French <stfrench@microsoft.com>
Cc: dhowells@redhat.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
Subject: Re: [GIT PULL] MM updates for 6.3-rc1
Date: Fri, 24 Feb 2023 09:04:48 +0000 [thread overview]
Message-ID: <2009825.1677229488@warthog.procyon.org.uk> (raw)
In-Reply-To: <CAHk-=whAAOVBrzwb2uMjCmdRrtudGesYj0tuqdUgi8X_gbw1jw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Yes, I saw David Howells resolution suggestion. I think that one
> was buggy. It would wait for a page under writeback, and then go on to
> the *next* one without writing it back. I don't thin kthat was right.
You're right. Vishal's patch introduced it into afs and I copied it across
and didn't notice it either then or on review of Vishal's patch. He inserted
the extra for-loop as he's now extracting a batch, but kept the continue that
used to repeat the extraction - except now it continues the wrong loop.
So afs will need fixing too. The simplest ways I think are to just decrement
the loop counter before continuing or to stick a goto in back to the beginning
of the loop (which is what you did in cifs). But I'm not sure that's the
correct thing to do. The previous code dropped the found folio and then
repeated the search in case the folio got truncated, migrated or punched. I
suspect that's probably what we should do.
Also, thinking about it again, I'm not sure whether fetching a batch with
filemap_get_folios_tag() like this in {afs,cifs}_writepages_region() is
necessarily the right thing to do. There are three cases I'm thinking of:
(1) A single folio is returned. This is trivial.
(2) A run of contiguous folios are returned - {afs,cifs}_extend_writeback()
is likely to write them back, in which case the batch is probably not
useful. Note that *_extend_writeback() walks the xarray directly itself
as it wants contiguous folios and doesn't want to extract any folio it's
not going to use.
(3) A list of scattered folios is returned. Granted this is more efficient
if nothing else interferes - but there could be other writes in the gaps
that we then skip over, other flushes that render some of our list clean
or page invalidations. This is a change in behaviour, but I'm not sure
that matters too much since a flush/sync can only be expected to write
back what's modified at the time it is initiated.
Further, processing each entry in the list is potentially very slow
because we're doing a write across the network for each one (cifs might
bump this into the background, but it might also have to (re)open a file
handle on the server and wait for credits first to even begin the
transaction).
Which means all of the folios in the batch may then get pinned for a long
period of time - up to 14x for the last folio in the batch - which could
prevent things like page migration.
Further, we might not get to write out all the folios in the batch as
*_extend_writeback() might hit the wbc limit first.
> That said, I'm not at all convinced my version is right either. I
> can't test it, and that means I probably messed up. It looked sane to
> me when I did it, and it builds cleanly, but I honestly doubt myself.
It doesn't seem to work. A write seems to end in lots of:
CIFS: VFS: No writable handle in writepages rc=-9
being emitted. I'll poke further into it - there's always the possibility
that some other patch is interfering.
David
next prev parent reply other threads:[~2023-02-24 9:05 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-20 21:52 [GIT PULL] MM updates for 6.3-rc1 Andrew Morton
2023-02-21 23:36 ` Andrew Morton
2023-02-24 1:33 ` pr-tracker-bot
2023-02-24 1:33 ` Linus Torvalds
2023-02-24 1:56 ` Andrew Morton
2023-02-24 3:01 ` Huang, Ying
2023-02-24 9:04 ` David Howells [this message]
2023-02-24 12:12 ` David Howells
2023-02-24 14:31 ` [RFC][PATCH] cifs: Fix cifs_writepages_region() David Howells
2023-02-24 16:06 ` Linus Torvalds
2023-02-24 16:11 ` Matthew Wilcox
2023-02-24 17:15 ` David Howells
2023-02-24 18:44 ` Matthew Wilcox
2023-02-24 20:13 ` David Howells
2023-02-24 20:16 ` Linus Torvalds
2023-02-24 20:45 ` Matthew Wilcox
2023-02-27 13:20 ` David Howells
2023-02-24 20:58 ` David Howells
2023-02-24 17:19 ` David Howells
2023-02-24 18:58 ` Linus Torvalds
2023-02-24 19:05 ` Linus Torvalds
2023-03-01 18:32 ` [EXTERNAL] " Steven French
2023-02-24 14:48 ` [RFC][PATCH] cifs, afs: Revert changes to {cifs,afs}_writepages_region() David Howells
2023-02-24 16:13 ` Linus Torvalds
2023-02-24 15:13 ` [RFC][PATCH] cifs: Improve use of filemap_get_folios_tag() David Howells
2023-02-24 16:22 ` Linus Torvalds
2023-02-24 17:22 ` David Howells
2023-02-26 2:43 ` [GIT PULL] MM updates for 6.3-rc1 Linus Torvalds
2023-02-26 3:27 ` Linus Torvalds
2023-02-26 3:53 ` Linus Torvalds
2023-02-26 3:57 ` Andrew Morton
2023-02-26 4:03 ` Linus Torvalds
2023-02-26 4:12 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2009825.1677229488@warthog.procyon.org.uk \
--to=dhowells@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=baolin.wang@linux.alibaba.com \
--cc=jack@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mm-commits@vger.kernel.org \
--cc=pc@cjr.nz \
--cc=stfrench@microsoft.com \
--cc=torvalds@linux-foundation.org \
--cc=vishal.moola@gmail.com \
--cc=willy@infradead.org \
--cc=xhao@linux.alibaba.com \
--cc=ying.huang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).