Hi all,
Today's linux-next merge of the mm-stable tree got a conflict in:
fs/cifs/file.c
between commit:
c8859bc0c129 ("cifs: Remove unused code")
from the cifs tree and commits:
4cda80f3a7a5 ("cifs: convert wdata_alloc_and_fillpages() to use filemap_get_folios_tag()")
d585bdbeb79a ("fs: convert writepage_t callback to pass a folio")
from the mm-stable tree.
This is a real mess :-(
I have no idea how to fix this up, so I have used the cifs tree from
next-20230217 for today.
--
Cheers,
Stephen Rothwell
On Mon, Feb 20, 2023 at 03:29:33PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the mm-stable tree got a conflict in:
>
> fs/cifs/file.c
>
> between commit:
>
> c8859bc0c129 ("cifs: Remove unused code")
>
> from the cifs tree and commits:
>
> 4cda80f3a7a5 ("cifs: convert wdata_alloc_and_fillpages() to use filemap_get_folios_tag()")
> d585bdbeb79a ("fs: convert writepage_t callback to pass a folio")
>
> from the mm-stable tree.
>
> This is a real mess :-(
Doesn't look too bad to me. Dave's commit is just removing the
functions, so it doesn't matter how they're being changed.
The real question in my mind is why for-next is being updated two days
before the merge window with new patches. What's the point in -next
if patches are being added at this late point?
Matthew Wilcox <[email protected]> wrote:
> Doesn't look too bad to me. Dave's commit is just removing the
> functions, so it doesn't matter how they're being changed.
>
> The real question in my mind is why for-next is being updated two days
> before the merge window with new patches. What's the point in -next
> if patches are being added at this late point?
It's more of a transfer of a subset of my iov/splice patches from the
linux-block tree to the cifs tree. I thought Jens would've dropped my branch
from his tree for the moment.
David
On Mon, Feb 20, 2023 at 02:29:41PM +0000, David Howells wrote:
> Matthew Wilcox <[email protected]> wrote:
>
> > Doesn't look too bad to me. Dave's commit is just removing the
> > functions, so it doesn't matter how they're being changed.
> >
> > The real question in my mind is why for-next is being updated two days
> > before the merge window with new patches. What's the point in -next
> > if patches are being added at this late point?
>
> It's more of a transfer of a subset of my iov/splice patches from the
> linux-block tree to the cifs tree. I thought Jens would've dropped my branch
> from his tree for the moment.
Your iov/splice patches don't conflict. The part that you snipped says
it's c8859bc0c129 ("cifs: Remove unused code")
Hi Matthew,
On Mon, 20 Feb 2023 13:58:29 +0000 Matthew Wilcox <[email protected]> wrote:
>
> On Mon, Feb 20, 2023 at 03:29:33PM +1100, Stephen Rothwell wrote:
> >
> > Today's linux-next merge of the mm-stable tree got a conflict in:
> >
> > fs/cifs/file.c
> >
> > between commit:
> >
> > c8859bc0c129 ("cifs: Remove unused code")
> >
> > from the cifs tree and commits:
> >
> > 4cda80f3a7a5 ("cifs: convert wdata_alloc_and_fillpages() to use filemap_get_folios_tag()")
> > d585bdbeb79a ("fs: convert writepage_t callback to pass a folio")
> >
> > from the mm-stable tree.
> >
> > This is a real mess :-(
>
> Doesn't look too bad to me. Dave's commit is just removing the
> functions, so it doesn't matter how they're being changed.
The problem I see is that an earlier commit in the cifs tree moves the
use of find_get_pages_range_tag() to another function and 4cda80f3a7a5
then removes find_get_pages_range_tag().
> The real question in my mind is why for-next is being updated two days
> before the merge window with new patches. What's the point in -next
> if patches are being added at this late point?
Indeed :-(
--
Cheers,
Stephen Rothwell
Hi Matthew,
On Mon, 20 Feb 2023 15:12:58 +0000 Matthew Wilcox <[email protected]> wrote:
>
> On Mon, Feb 20, 2023 at 02:29:41PM +0000, David Howells wrote:
> > Matthew Wilcox <[email protected]> wrote:
> >
> > > Doesn't look too bad to me. Dave's commit is just removing the
> > > functions, so it doesn't matter how they're being changed.
> > >
> > > The real question in my mind is why for-next is being updated two days
> > > before the merge window with new patches. What's the point in -next
> > > if patches are being added at this late point?
> >
> > It's more of a transfer of a subset of my iov/splice patches from the
> > linux-block tree to the cifs tree. I thought Jens would've dropped my branch
> > from his tree for the moment.
>
> Your iov/splice patches don't conflict. The part that you snipped says
> it's c8859bc0c129 ("cifs: Remove unused code")
That is just the immediate conflict. See my other reply.
--
Cheers,
Stephen Rothwell
On Mon, Feb 20, 2023 at 07:01:57PM +1100, Stephen Rothwell wrote:
> Hi Matthew,
>
> On Mon, 20 Feb 2023 13:58:29 +0000 Matthew Wilcox <[email protected]> wrote:
> >
> > On Mon, Feb 20, 2023 at 03:29:33PM +1100, Stephen Rothwell wrote:
> > >
> > > Today's linux-next merge of the mm-stable tree got a conflict in:
> > >
> > > fs/cifs/file.c
> > >
> > > between commit:
> > >
> > > c8859bc0c129 ("cifs: Remove unused code")
> > >
> > > from the cifs tree and commits:
> > >
> > > 4cda80f3a7a5 ("cifs: convert wdata_alloc_and_fillpages() to use filemap_get_folios_tag()")
> > > d585bdbeb79a ("fs: convert writepage_t callback to pass a folio")
> > >
> > > from the mm-stable tree.
> > >
> > > This is a real mess :-(
> >
> > Doesn't look too bad to me. Dave's commit is just removing the
> > functions, so it doesn't matter how they're being changed.
>
> The problem I see is that an earlier commit in the cifs tree moves the
> use of find_get_pages_range_tag() to another function and 4cda80f3a7a5
> then removes find_get_pages_range_tag().
Ah. Just removing all traces of it should be fine. As long as there
are no remaining callers of find_get_pages_range_tag() after the merge,
it's good from my point of view.
On Mon, Feb 20, 2023 at 2:55 PM Stephen Rothwell <[email protected]> wrote:
>
> Hi Matthew,
>
> On Mon, 20 Feb 2023 13:58:29 +0000 Matthew Wilcox <[email protected]> wrote:
> >
> > On Mon, Feb 20, 2023 at 03:29:33PM +1100, Stephen Rothwell wrote:
> > >
> > > Today's linux-next merge of the mm-stable tree got a conflict in:
> > >
> > > fs/cifs/file.c
> > >
> > > between commit:
> > >
> > > c8859bc0c129 ("cifs: Remove unused code")
> > >
> > > from the cifs tree and commits:
> > >
> > > 4cda80f3a7a5 ("cifs: convert wdata_alloc_and_fillpages() to use filemap_get_folios_tag()")
> > > d585bdbeb79a ("fs: convert writepage_t callback to pass a folio")
> > >
> > > from the mm-stable tree.
> > >
> > > This is a real mess :-(
> >
> > Doesn't look too bad to me. Dave's commit is just removing the
> > functions, so it doesn't matter how they're being changed.
>
> The problem I see is that an earlier commit in the cifs tree moves the
> use of find_get_pages_range_tag() to another function and 4cda80f3a7a5
> then removes find_get_pages_range_tag().
>
> > The real question in my mind is why for-next is being updated two days
> > before the merge window with new patches. What's the point in -next
> > if patches are being added at this late point?
>
> Indeed :-(
I don't think it was so much that they were added late (most were
reviewed over multiple week period) - just moved trees to make it
easier a week ago. The changes David etc. have been making recently
to the series seemed fairly small. And I am hoping that his series
allows removal of more dead code as well. Also FYI Paulo caught a
minor bug in one of Dave's patches while testing today, and I noticed
a merge conflict with a small unrelated patch that went into
mm/filemap.c for rc8 so I am rebasing my cifs-2.6.git for-next on 6.2
now (to avoid that merge conflict) and will update later this
afternoon with the trivial fix for the problem Paulo pointed out.
--
Thanks,
Steve
Hi Matthew,
On Mon, 20 Feb 2023 20:58:03 +0000 Matthew Wilcox <[email protected]> wrote:
>
> On Mon, Feb 20, 2023 at 07:01:57PM +1100, Stephen Rothwell wrote:
> > Hi Matthew,
> >
> > On Mon, 20 Feb 2023 13:58:29 +0000 Matthew Wilcox <[email protected]> wrote:
> > >
> > > On Mon, Feb 20, 2023 at 03:29:33PM +1100, Stephen Rothwell wrote:
> > > >
> > > > Today's linux-next merge of the mm-stable tree got a conflict in:
> > > >
> > > > fs/cifs/file.c
> > > >
> > > > between commit:
> > > >
> > > > c8859bc0c129 ("cifs: Remove unused code")
> > > >
> > > > from the cifs tree and commits:
> > > >
> > > > 4cda80f3a7a5 ("cifs: convert wdata_alloc_and_fillpages() to use filemap_get_folios_tag()")
> > > > d585bdbeb79a ("fs: convert writepage_t callback to pass a folio")
> > > >
> > > > from the mm-stable tree.
> > > >
> > > > This is a real mess :-(
> > >
> > > Doesn't look too bad to me. Dave's commit is just removing the
> > > functions, so it doesn't matter how they're being changed.
> >
> > The problem I see is that an earlier commit in the cifs tree moves the
> > use of find_get_pages_range_tag() to another function and 4cda80f3a7a5
> > then removes find_get_pages_range_tag().
>
> Ah. Just removing all traces of it should be fine. As long as there
> are no remaining callers of find_get_pages_range_tag() after the merge,
> it's good from my point of view.
But I can't do that since commit
d08089f649a0 ("cifs: Change the I/O paths to use an iterator rather than a page list")
in the cifs tree introduces a new usage of it in code that is used in
the cifs code ... so someone has to figure out what the merge
resolution is between the 2 trees (how to replace that new usage) and
let me know and then we need to test that combination for a while
before asking Linus to take it.
--
Cheers,
Stephen Rothwell
Hi Steve,
On Mon, 20 Feb 2023 15:00:50 -0600 Steve French <[email protected]> wrote:
>
> I don't think it was so much that they were added late (most were
> reviewed over multiple week period) - just moved trees to make it
> easier a week ago. The changes David etc. have been making recently
> to the series seemed fairly small.
But they only turned up in linux-next over the weekend :-( So we are
only now finding out how they conflict badly with code that has been
published in the mm tree for weeks. So the combination has not been
tested at all. So it is really not ready to go into Linus' tree, right?
I am still using the cifs tree from next-20230217 - head commit
e18f461adf10 ("cifs: Fix uninitialized memory reads for oparms.mode")
because I don't know enough about the folio changes and the cifs
changes to merge them.
--
Cheers,
Stephen Rothwell
Stephen Rothwell <[email protected]> wrote:
> in the cifs tree introduces a new usage of it in code that is used in
> the cifs code ... so someone has to figure out what the merge
> resolution is between the 2 trees (how to replace that new usage) and
> let me know and then we need to test that combination for a while
> before asking Linus to take it.
It seems I can't fix my patch and give Steve a replacement patch because the
new filemap_get_folios_tag() that I would need to use hasn't been added
upstream yet. Do we have any idea when the mm tree might land?
David
Hi David,
On Tue, 21 Feb 2023 07:19:17 +0000 David Howells <[email protected]> wrote:
>
> Stephen Rothwell <[email protected]> wrote:
>
> > in the cifs tree introduces a new usage of it in code that is used in
> > the cifs code ... so someone has to figure out what the merge
> > resolution is between the 2 trees (how to replace that new usage) and
> > let me know and then we need to test that combination for a while
> > before asking Linus to take it.
>
> It seems I can't fix my patch and give Steve a replacement patch because the
> new filemap_get_folios_tag() that I would need to use hasn't been added
> upstream yet. Do we have any idea when the mm tree might land?
Andrew has already asked for it to be merged, so its up to Linus.
You could fetch it yourself and do a trial merge and send me your
resolution ..
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2023-02-20-13-37
--
Cheers,
Stephen Rothwell
Stephen Rothwell <[email protected]> wrote:
> Andrew has already asked for it to be merged, so its up to Linus.
>
> You could fetch it yourself and do a trial merge and send me your
> resolution ..
>
> git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2023-02-20-13-37
Okay, did that. See attached. Also here:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=iov-cifs-mm
David
---
commit 71ad4f67439e60fe04bbf7aed8870e6f83a5d15e
Author: David Howells <[email protected]>
Date: Tue Feb 21 13:23:05 2023 +0000
cifs: Handle transition to filemap_get_folios_tag()
filemap_get_folios_tag() is being added and find_get_pages_range_tag() is
being removed in effectively a single event. This causes a problem for
the:
cifs: Change the I/O paths to use an iterator rather than a page list
patch[1] on the cifs/for-next branch as it's adding a new user of the
latter (which is going away), but can't yet be converted to using the
former (which doesn't yet exist upstream).
Here's a conversion patch that could be applied at merge time to deal with
this. The new cifs_writepages_region() is based directly on
afs_writepages_region() and the AFS changes in the mm tree[2]:
commit acc8d8588cb7e3e64b0d2fa611dad06574cd67b1
Author: Vishal Moola (Oracle) <[email protected]>
afs: convert afs_writepages_region() to use filemap_get_folios_tag()
can be replicated in cifs almost exactly.
Signed-off-by: David Howells <[email protected]>
cc: Stephen Rothwell <[email protected]>
cc: Steve French <[email protected]>
cc: Shyam Prasad N <[email protected]>
cc: Rohith Surabattula <[email protected]>
cc: Tom Talpey <[email protected]>
cc: Paulo Alcantara <[email protected]>
cc: Jeff Layton <[email protected]>
cc: [email protected]
cc: Vishal Moola (Oracle) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/ [1]
Link: https://lore.kernel.org/r/[email protected]/ [2]
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 58801d39213a..52af9cf93c65 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2856,78 +2856,85 @@ static int cifs_writepages_region(struct address_space *mapping,
struct writeback_control *wbc,
loff_t start, loff_t end, loff_t *_next)
{
+ struct folio_batch fbatch;
struct folio *folio;
- struct page *head_page;
+ unsigned int i;
ssize_t ret;
int n, skips = 0;
+ folio_batch_init(&fbatch);
+
do {
pgoff_t index = start / PAGE_SIZE;
- n = find_get_pages_range_tag(mapping, &index, end / PAGE_SIZE,
- PAGECACHE_TAG_DIRTY, 1, &head_page);
+ n = filemap_get_folios_tag(mapping, &index, end / PAGE_SIZE,
+ PAGECACHE_TAG_DIRTY, &fbatch);
if (!n)
break;
- folio = page_folio(head_page);
- start = folio_pos(folio); /* May regress with THPs */
+ for (i = 0; i < n; i++) {
+ folio = fbatch.folios[i];
+ start = folio_pos(folio); /* May regress with THPs */
- /* At this point we hold neither the i_pages lock nor the
- * page lock: the page may be truncated or invalidated
- * (changing page->mapping to NULL), or even swizzled
- * back from swapper_space to tmpfs file mapping
- */
- if (wbc->sync_mode != WB_SYNC_NONE) {
- ret = folio_lock_killable(folio);
- if (ret < 0) {
- folio_put(folio);
- return ret;
- }
- } else {
- if (!folio_trylock(folio)) {
- folio_put(folio);
- return 0;
+ /* At this point we hold neither the i_pages lock nor the
+ * page lock: the page may be truncated or invalidated
+ * (changing page->mapping to NULL), or even swizzled
+ * back from swapper_space to tmpfs file mapping
+ */
+ if (wbc->sync_mode != WB_SYNC_NONE) {
+ ret = folio_lock_killable(folio);
+ if (ret < 0) {
+ folio_batch_release(&fbatch);
+ return ret;
+ }
+ } else {
+ if (!folio_trylock(folio))
+ continue;
}
- }
- if (folio_mapping(folio) != mapping ||
- !folio_test_dirty(folio)) {
- start += folio_size(folio);
- folio_unlock(folio);
- folio_put(folio);
- continue;
- }
+ if (folio->mapping != mapping ||
+ !folio_test_dirty(folio)) {
+ start += folio_size(folio);
+ folio_unlock(folio);
+ continue;
+ }
- if (folio_test_writeback(folio) ||
- folio_test_fscache(folio)) {
- folio_unlock(folio);
- if (wbc->sync_mode != WB_SYNC_NONE) {
- folio_wait_writeback(folio);
+ if (folio_test_writeback(folio) ||
+ folio_test_fscache(folio)) {
+ folio_unlock(folio);
+ if (wbc->sync_mode != WB_SYNC_NONE) {
+ folio_wait_writeback(folio);
#ifdef CONFIG_CIFS_FSCACHE
- folio_wait_fscache(folio);
+ folio_wait_fscache(folio);
#endif
- } else {
- start += folio_size(folio);
- }
- folio_put(folio);
- if (wbc->sync_mode == WB_SYNC_NONE) {
- if (skips >= 5 || need_resched())
- break;
- skips++;
+ } else {
+ start += folio_size(folio);
+ }
+ if (wbc->sync_mode == WB_SYNC_NONE) {
+ if (skips >= 5 || need_resched()) {
+ *_next = start;
+ return 0;
+ }
+ skips++;
+ }
+ continue;
}
- continue;
- }
- if (!folio_clear_dirty_for_io(folio))
- /* We hold the page lock - it should've been dirty. */
- WARN_ON(1);
+ if (!folio_clear_dirty_for_io(folio))
+ /* We hold the page lock - it should've been dirty. */
+ WARN_ON(1);
- ret = cifs_write_back_from_locked_folio(mapping, wbc, folio, start, end);
- folio_put(folio);
- if (ret < 0)
- return ret;
+ ret = cifs_write_back_from_locked_folio(mapping, wbc,
+ folio, start, end);
+ if (ret < 0) {
+ folio_batch_release(&fbatch);
+ return ret;
+ }
+
+ start += ret;
+ }
- start += ret;
+ folio_batch_release(&fbatch);
cond_resched();
} while (wbc->nr_to_write > 0);
On Tue, Feb 21, 2023 at 02:39:24PM +0000, David Howells wrote:
> - folio = page_folio(head_page);
> - start = folio_pos(folio); /* May regress with THPs */
> + for (i = 0; i < n; i++) {
> + folio = fbatch.folios[i];
> + start = folio_pos(folio); /* May regress with THPs */
What does this comment mean?
> + /* At this point we hold neither the i_pages lock nor the
> + * page lock: the page may be truncated or invalidated
> + * (changing page->mapping to NULL), or even swizzled
> + * back from swapper_space to tmpfs file mapping
Where does this comment come from? This is cifs, not tmpfs. You'll
never be asked to writeback a page from the swap cache. Dirty pages
can be truncated, so the first half of the comment is still accurate.
I'd rather it moved down to below the folio lock, and was rephrased
so it described why we're checking everything again.
The actual code looks right.
Matthew Wilcox <[email protected]> wrote:
> > + start = folio_pos(folio); /* May regress with THPs */
>
> What does this comment mean?
"start" may end up going backwards if it's pointing to the middle of a folio.
> > + /* At this point we hold neither the i_pages lock nor the
> > + * page lock: the page may be truncated or invalidated
> > + * (changing page->mapping to NULL), or even swizzled
> > + * back from swapper_space to tmpfs file mapping
>
> Where does this comment come from? This is cifs, not tmpfs. You'll
> never be asked to writeback a page from the swap cache. Dirty pages
> can be truncated, so the first half of the comment is still accurate.
> I'd rather it moved down to below the folio lock, and was rephrased
> so it described why we're checking everything again.
I picked it up into afs from somewhere - nfs maybe? The same comment is in
fs/btrfs/extent_io.c. grep for 'swizzled' in fs/. You modified the comment
in b93b016313b3ba8003c3b8bb71f569af91f19fc7 in 2018, so it's been around a
while.
David
Matthew Wilcox <[email protected]> wrote:
> > + /* At this point we hold neither the i_pages lock nor the
> > + * page lock: the page may be truncated or invalidated
> > + * (changing page->mapping to NULL), or even swizzled
> > + * back from swapper_space to tmpfs file mapping
>
> Where does this comment come from? This is cifs, not tmpfs. You'll
> never be asked to writeback a page from the swap cache. Dirty pages
> can be truncated, so the first half of the comment is still accurate.
> I'd rather it moved down to below the folio lock, and was rephrased
> so it described why we're checking everything again.
Actually, it's in v6.2 cifs and I just move it in the patch where I copy the
afs writepages implementation into cifs. afs got it in 2007 when I added
write support[1] and I suspect I copied it from cifs. cifs got it in 2005
when Steve added writepages support[2]. I think he must've got it from
fs/mpage.c and the comment there is prehistoric.
David
31143d5d515ece617ffccb7df5ff75e4d1dfa120 [1]
37c0eb4677f733a773df6287b0f73f00274402e3 [2]
On Tue, Feb 21, 2023 at 03:05:26PM +0000, David Howells wrote:
> Matthew Wilcox <[email protected]> wrote:
>
> > > + start = folio_pos(folio); /* May regress with THPs */
> >
> > What does this comment mean?
>
> "start" may end up going backwards if it's pointing to the middle of a folio.
So that's "regress" in the sense of "May point earlier in the file",
rather than "May cause a bug" (which was how I read it)?
> > > + /* At this point we hold neither the i_pages lock nor the
> > > + * page lock: the page may be truncated or invalidated
> > > + * (changing page->mapping to NULL), or even swizzled
> > > + * back from swapper_space to tmpfs file mapping
> >
> > Where does this comment come from? This is cifs, not tmpfs. You'll
> > never be asked to writeback a page from the swap cache. Dirty pages
> > can be truncated, so the first half of the comment is still accurate.
> > I'd rather it moved down to below the folio lock, and was rephrased
> > so it described why we're checking everything again.
>
> I picked it up into afs from somewhere - nfs maybe? The same comment is in
> fs/btrfs/extent_io.c. grep for 'swizzled' in fs/. You modified the comment
> in b93b016313b3ba8003c3b8bb71f569af91f19fc7 in 2018, so it's been around a
> while.
I was just removing references to ->tree_lock ;-)
David Howells <[email protected]> wrote:
> > > + /* At this point we hold neither the i_pages lock nor the
> > > + * page lock: the page may be truncated or invalidated
> > > + * (changing page->mapping to NULL), or even swizzled
> > > + * back from swapper_space to tmpfs file mapping
> >
> > Where does this comment come from? This is cifs, not tmpfs. You'll
> > never be asked to writeback a page from the swap cache. Dirty pages
> > can be truncated, so the first half of the comment is still accurate.
> > I'd rather it moved down to below the folio lock, and was rephrased
> > so it described why we're checking everything again.
>
> Actually, it's in v6.2 cifs and I just move it in the patch where I copy the
> afs writepages implementation into cifs. afs got it in 2007 when I added
> write support[1] and I suspect I copied it from cifs. cifs got it in 2005
> when Steve added writepages support[2]. I think he must've got it from
> fs/mpage.c and the comment there is prehistoric.
The ultimate source is Hugh Dickins, it would seem:
commit 820ef9df32856bb54fe5bc995153feb276420e15
Author: Andrew Morton <[email protected]>
Date: Fri Nov 15 18:52:38 2002 -0800
[PATCH] handle pages which alter their ->mapping
Patch from Hugh Dickins <[email protected]>
tmpfs failed fsx+swapout tests after many hours, a page found zeroed.
Not a truncate problem, but mirror image of earlier truncate problems:
swap goes through mpage_writepages, which must therefore allow for a
sudden swizzle back to file identity.
Second time this caught us, so I've audited the tree for other places
which might be surprised by such swizzling. The only others I found
were (perhaps) in the parisc and sparc64 flush_dcache_page called
from do_generic_mapping_read on a looped tmpfs file which is also
mmapped; but that's a very marginal case, I wanted to understand it
better before making any edit, and now realize that hch's sendfile
in loop eliminates it (now go through do_shmem_file_read instead:
similar but crucially this locks the page when raising its count,
which is enough to keep vmscan from interfering).
Maybe we should delete or amend the comment now?
David
On Tue, 21 Feb 2023, David Howells wrote:
> David Howells <[email protected]> wrote:
>
> > > > + /* At this point we hold neither the i_pages lock nor the
> > > > + * page lock: the page may be truncated or invalidated
> > > > + * (changing page->mapping to NULL), or even swizzled
> > > > + * back from swapper_space to tmpfs file mapping
> > >
> > > Where does this comment come from? This is cifs, not tmpfs. You'll
> > > never be asked to writeback a page from the swap cache. Dirty pages
> > > can be truncated, so the first half of the comment is still accurate.
> > > I'd rather it moved down to below the folio lock, and was rephrased
> > > so it described why we're checking everything again.
> >
> > Actually, it's in v6.2 cifs and I just move it in the patch where I copy the
> > afs writepages implementation into cifs. afs got it in 2007 when I added
> > write support[1] and I suspect I copied it from cifs. cifs got it in 2005
> > when Steve added writepages support[2]. I think he must've got it from
> > fs/mpage.c and the comment there is prehistoric.
>
> The ultimate source is Hugh Dickins, it would seem:
>
> commit 820ef9df32856bb54fe5bc995153feb276420e15
> Author: Andrew Morton <[email protected]>
> Date: Fri Nov 15 18:52:38 2002 -0800
>
> [PATCH] handle pages which alter their ->mapping
>
> Patch from Hugh Dickins <[email protected]>
>
> tmpfs failed fsx+swapout tests after many hours, a page found zeroed.
> Not a truncate problem, but mirror image of earlier truncate problems:
> swap goes through mpage_writepages, which must therefore allow for a
> sudden swizzle back to file identity.
>
> Second time this caught us, so I've audited the tree for other places
> which might be surprised by such swizzling. The only others I found
> were (perhaps) in the parisc and sparc64 flush_dcache_page called
> from do_generic_mapping_read on a looped tmpfs file which is also
> mmapped; but that's a very marginal case, I wanted to understand it
> better before making any edit, and now realize that hch's sendfile
> in loop eliminates it (now go through do_shmem_file_read instead:
> similar but crucially this locks the page when raising its count,
> which is enough to keep vmscan from interfering).
>
> Maybe we should delete or amend the comment now?
Yes, that comment does not belong in afs or btrfs or cifs - though it
does explain why we have sometimes chosen to compare folio_mapping(folio)
with expected mapping, rather than against NULL.
But "now" is not the moment to amend it: it looks like these sources
are in flux at present. And truncate_cleanup_folio() has a "swizzles"
comment without even a mapping to compare with.
Hugh
Hi David,
On Tue, 21 Feb 2023 14:39:24 +0000 David Howells <[email protected]> wrote:
>
> Stephen Rothwell <[email protected]> wrote:
>
> > Andrew has already asked for it to be merged, so its up to Linus.
> >
> > You could fetch it yourself and do a trial merge and send me your
> > resolution ..
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2023-02-20-13-37
>
> Okay, did that. See attached. Also here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=iov-cifs-mm
>
> David
> ---
> commit 71ad4f67439e60fe04bbf7aed8870e6f83a5d15e
> Author: David Howells <[email protected]>
> Date: Tue Feb 21 13:23:05 2023 +0000
>
> cifs: Handle transition to filemap_get_folios_tag()
OK, so in the merge of mm-stable, I used the cifs version of
fs/cifs/file.c with this patch applied. The merge resolution for that
file looks like this:
diff --cc fs/cifs/file.c
index 0e602173ac76,162fab5a4583..000000000000
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@@ -2850,154 -2853,29 +2850,161 @@@ err_xid
return rc;
}
-static int
-cifs_writepage_locked(struct page *page, struct writeback_control *wbc)
+/*
+ * write a region of pages back to the server
+ */
+static int cifs_writepages_region(struct address_space *mapping,
+ struct writeback_control *wbc,
+ loff_t start, loff_t end, loff_t *_next)
{
- int rc;
- unsigned int xid;
++ struct folio_batch fbatch;
+ struct folio *folio;
- struct page *head_page;
++ unsigned int i;
+ ssize_t ret;
+ int n, skips = 0;
- xid = get_xid();
-/* BB add check for wbc flags */
- get_page(page);
- if (!PageUptodate(page))
- cifs_dbg(FYI, "ppw - page not up to date\n");
++ folio_batch_init(&fbatch);
+
- /*
- * Set the "writeback" flag, and clear "dirty" in the radix tree.
- *
- * A writepage() implementation always needs to do either this,
- * or re-dirty the page with "redirty_page_for_writepage()" in
- * the case of a failure.
- *
- * Just unlocking the page will cause the radix tree tag-bits
- * to fail to update with the state of the page correctly.
- */
- set_page_writeback(page);
+ do {
+ pgoff_t index = start / PAGE_SIZE;
+
- n = find_get_pages_range_tag(mapping, &index, end / PAGE_SIZE,
- PAGECACHE_TAG_DIRTY, 1, &head_page);
++ n = filemap_get_folios_tag(mapping, &index, end / PAGE_SIZE,
++ PAGECACHE_TAG_DIRTY, &fbatch);
+ if (!n)
+ break;
+
- folio = page_folio(head_page);
- start = folio_pos(folio); /* May regress with THPs */
++ for (i = 0; i < n; i++) {
++ folio = fbatch.folios[i];
++ start = folio_pos(folio); /* May regress with THPs */
+
- /* At this point we hold neither the i_pages lock nor the
- * page lock: the page may be truncated or invalidated
- * (changing page->mapping to NULL), or even swizzled
- * back from swapper_space to tmpfs file mapping
- */
- if (wbc->sync_mode != WB_SYNC_NONE) {
- ret = folio_lock_killable(folio);
- if (ret < 0) {
- folio_put(folio);
- return ret;
- }
- } else {
- if (!folio_trylock(folio)) {
- folio_put(folio);
- return 0;
++ /* At this point we hold neither the i_pages lock nor the
++ * page lock: the page may be truncated or invalidated
++ * (changing page->mapping to NULL), or even swizzled
++ * back from swapper_space to tmpfs file mapping
++ */
++ if (wbc->sync_mode != WB_SYNC_NONE) {
++ ret = folio_lock_killable(folio);
++ if (ret < 0) {
++ folio_batch_release(&fbatch);
++ return ret;
++ }
++ } else {
++ if (!folio_trylock(folio))
++ continue;
+ }
- }
+
- if (folio_mapping(folio) != mapping ||
- !folio_test_dirty(folio)) {
- start += folio_size(folio);
- folio_unlock(folio);
- folio_put(folio);
- continue;
- }
++ if (folio->mapping != mapping ||
++ !folio_test_dirty(folio)) {
++ start += folio_size(folio);
++ folio_unlock(folio);
++ continue;
++ }
+
- if (folio_test_writeback(folio) ||
- folio_test_fscache(folio)) {
- folio_unlock(folio);
- if (wbc->sync_mode != WB_SYNC_NONE) {
- folio_wait_writeback(folio);
++ if (folio_test_writeback(folio) ||
++ folio_test_fscache(folio)) {
++ folio_unlock(folio);
++ if (wbc->sync_mode != WB_SYNC_NONE) {
++ folio_wait_writeback(folio);
+#ifdef CONFIG_CIFS_FSCACHE
- folio_wait_fscache(folio);
++ folio_wait_fscache(folio);
+#endif
- } else {
- start += folio_size(folio);
- }
- folio_put(folio);
- if (wbc->sync_mode == WB_SYNC_NONE) {
- if (skips >= 5 || need_resched())
- break;
- skips++;
++ } else {
++ start += folio_size(folio);
++ }
++ if (wbc->sync_mode == WB_SYNC_NONE) {
++ if (skips >= 5 || need_resched()) {
++ *_next = start;
++ return 0;
++ }
++ skips++;
++ }
++ continue;
+ }
- continue;
- }
+
- if (!folio_clear_dirty_for_io(folio))
- /* We hold the page lock - it should've been dirty. */
- WARN_ON(1);
++ if (!folio_clear_dirty_for_io(folio))
++ /* We hold the page lock - it should've been dirty. */
++ WARN_ON(1);
+
- ret = cifs_write_back_from_locked_folio(mapping, wbc, folio, start, end);
- folio_put(folio);
- if (ret < 0)
- return ret;
++ ret = cifs_write_back_from_locked_folio(mapping, wbc,
++ folio, start, end);
++ if (ret < 0) {
++ folio_batch_release(&fbatch);
++ return ret;
++ }
++
++ start += ret;
++ }
+
- start += ret;
++ folio_batch_release(&fbatch);
+ cond_resched();
+ } while (wbc->nr_to_write > 0);
+
+ *_next = start;
+ return 0;
+}
+
+/*
+ * Write some of the pending data back to the server
+ */
+static int cifs_writepages(struct address_space *mapping,
+ struct writeback_control *wbc)
+{
+ loff_t start, next;
+ int ret;
+
+ /* We have to be careful as we can end up racing with setattr()
+ * truncating the pagecache since the caller doesn't take a lock here
+ * to prevent it.
+ */
+
+ if (wbc->range_cyclic) {
+ start = mapping->writeback_index * PAGE_SIZE;
+ ret = cifs_writepages_region(mapping, wbc, start, LLONG_MAX, &next);
+ if (ret == 0) {
+ mapping->writeback_index = next / PAGE_SIZE;
+ if (start > 0 && wbc->nr_to_write > 0) {
+ ret = cifs_writepages_region(mapping, wbc, 0,
+ start, &next);
+ if (ret == 0)
+ mapping->writeback_index =
+ next / PAGE_SIZE;
+ }
+ }
+ } else if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX) {
+ ret = cifs_writepages_region(mapping, wbc, 0, LLONG_MAX, &next);
+ if (wbc->nr_to_write > 0 && ret == 0)
+ mapping->writeback_index = next / PAGE_SIZE;
+ } else {
+ ret = cifs_writepages_region(mapping, wbc,
+ wbc->range_start, wbc->range_end, &next);
+ }
+
+ return ret;
+}
+
+static int
+cifs_writepage_locked(struct page *page, struct writeback_control *wbc)
+{
+ int rc;
+ unsigned int xid;
+
+ xid = get_xid();
+/* BB add check for wbc flags */
+ get_page(page);
+ if (!PageUptodate(page))
+ cifs_dbg(FYI, "ppw - page not up to date\n");
+
+ /*
+ * Set the "writeback" flag, and clear "dirty" in the radix tree.
+ *
+ * A writepage() implementation always needs to do either this,
+ * or re-dirty the page with "redirty_page_for_writepage()" in
+ * the case of a failure.
+ *
+ * Just unlocking the page will cause the radix tree tag-bits
+ * to fail to update with the state of the page correctly.
+ */
+ set_page_writeback(page);
retry_write:
rc = cifs_partialpagewrite(page, 0, PAGE_SIZE);
if (is_retryable_error(rc)) {
Which is much less obvious :-)
--
Cheers,
Stephen Rothwell
Stephen Rothwell <[email protected]> wrote:
> OK, so in the merge of mm-stable, I used the cifs version of
> fs/cifs/file.c with this patch applied. The merge resolution for that
> file looks like this:
I checked it out and diffed it against mine. Looks right, thanks!
David
Hi David,
On Wed, 22 Feb 2023 08:27:50 +0000 David Howells <[email protected]> wrote:
>
> Stephen Rothwell <[email protected]> wrote:
>
> > OK, so in the merge of mm-stable, I used the cifs version of
> > fs/cifs/file.c with this patch applied. The merge resolution for that
> > file looks like this:
>
> I checked it out and diffed it against mine. Looks right, thanks!
Thanks for checking.
--
Cheers,
Stephen Rothwell