public inbox for linux-cifs@vger.kernel.org
 help / color / mirror / Atom feed
From: David Howells <dhowells@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Steve French <smfrench@gmail.com>
Cc: dhowells@redhat.com, Vishal Moola <vishal.moola@gmail.com>,
	Shyam Prasad N <nspmangalore@gmail.com>,
	Rohith Surabattula <rohiths.msft@gmail.com>,
	Tom Talpey <tom@talpey.com>, Stefan Metzmacher <metze@samba.org>,
	Paulo Alcantara <pc@cjr.nz>, Jeff Layton <jlayton@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Marc Dionne <marc.dionne@auristor.com>,
	linux-afs@lists.infradead.org, linux-cifs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: cifs test patch to make cifs use its own version of write_cache_pages()
Date: Thu, 02 Mar 2023 23:41:39 +0000	[thread overview]
Message-ID: <522532.1677800499@warthog.procyon.org.uk> (raw)
In-Reply-To: <522331.1677800209@warthog.procyon.org.uk>

David Howells <dhowells@redhat.com> wrote:

> And then CIFS. ...
> 
>   Base + Own write_cache_pages():
> 	WRITE: bw=451MiB/s (473MB/s), 113MiB/s-113MiB/s (118MB/s-118MB/s)
> 	WRITE: bw=455MiB/s (478MB/s), 114MiB/s-114MiB/s (119MB/s-120MB/s)
> 	WRITE: bw=453MiB/s (475MB/s), 113MiB/s-113MiB/s (119MB/s-119MB/s)
> 	WRITE: bw=459MiB/s (481MB/s), 115MiB/s-115MiB/s (120MB/s-120MB/s)

Here's my patch to give cifs its own copy of write_cache_pages() so that the
function pointer can be eliminated in case some sort of spectre thing is
causing a slowdown.

This goes on top of "cifs test patch to convert to using write_cache_pages()".

David
---
 fs/cifs/file.c |  137 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 136 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 04e2466609d9..c33c7db729c7 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2802,6 +2802,141 @@ static int cifs_writepages_add_folio(struct folio *folio,
 	return 0;
 }
 
+static int cifs_write_cache_pages(struct address_space *mapping,
+				  struct writeback_control *wbc,
+				  struct cifs_writepages_context *ctx)
+{
+	int ret = 0;
+	int done = 0;
+	int error;
+	struct folio_batch fbatch;
+	int nr_folios;
+	pgoff_t index;
+	pgoff_t end;		/* Inclusive */
+	pgoff_t done_index;
+	int range_whole = 0;
+	xa_mark_t tag;
+
+	folio_batch_init(&fbatch);
+	if (wbc->range_cyclic) {
+		index = mapping->writeback_index; /* prev offset */
+		end = -1;
+	} else {
+		index = wbc->range_start >> PAGE_SHIFT;
+		end = wbc->range_end >> PAGE_SHIFT;
+		if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX)
+			range_whole = 1;
+	}
+	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages) {
+		tag_pages_for_writeback(mapping, index, end);
+		tag = PAGECACHE_TAG_TOWRITE;
+	} else {
+		tag = PAGECACHE_TAG_DIRTY;
+	}
+	done_index = index;
+	while (!done && (index <= end)) {
+		int i;
+
+		nr_folios = filemap_get_folios_tag(mapping, &index, end,
+				tag, &fbatch);
+
+		if (nr_folios == 0)
+			break;
+
+		for (i = 0; i < nr_folios; i++) {
+			struct folio *folio = fbatch.folios[i];
+
+			done_index = folio->index;
+
+			folio_lock(folio);
+
+			/*
+			 * Page truncated or invalidated. We can freely skip it
+			 * then, even for data integrity operations: the page
+			 * has disappeared concurrently, so there could be no
+			 * real expectation of this data integrity operation
+			 * even if there is now a new, dirty page at the same
+			 * pagecache address.
+			 */
+			if (unlikely(folio->mapping != mapping)) {
+continue_unlock:
+				folio_unlock(folio);
+				continue;
+			}
+
+			if (!folio_test_dirty(folio)) {
+				/* someone wrote it for us */
+				goto continue_unlock;
+			}
+
+			if (folio_test_writeback(folio)) {
+				if (wbc->sync_mode != WB_SYNC_NONE)
+					folio_wait_writeback(folio);
+				else
+					goto continue_unlock;
+			}
+
+			BUG_ON(folio_test_writeback(folio));
+			if (!folio_clear_dirty_for_io(folio))
+				goto continue_unlock;
+
+			error = cifs_writepages_add_folio(folio, wbc, ctx);
+			if (unlikely(error)) {
+				/*
+				 * Handle errors according to the type of
+				 * writeback. There's no need to continue for
+				 * background writeback. Just push done_index
+				 * past this page so media errors won't choke
+				 * writeout for the entire file. For integrity
+				 * writeback, we must process the entire dirty
+				 * set regardless of errors because the fs may
+				 * still have state to clear for each page. In
+				 * that case we continue processing and return
+				 * the first error.
+				 */
+				if (error == AOP_WRITEPAGE_ACTIVATE) {
+					folio_unlock(folio);
+					error = 0;
+				} else if (wbc->sync_mode != WB_SYNC_ALL) {
+					ret = error;
+					done_index = folio->index +
+						folio_nr_pages(folio);
+					done = 1;
+					break;
+				}
+				if (!ret)
+					ret = error;
+			}
+
+			/*
+			 * We stop writing back only if we are not doing
+			 * integrity sync. In case of integrity sync we have to
+			 * keep going until we have written all the pages
+			 * we tagged for writeback prior to entering this loop.
+			 */
+			if (--wbc->nr_to_write <= 0 &&
+			    wbc->sync_mode == WB_SYNC_NONE) {
+				done = 1;
+				break;
+			}
+		}
+		folio_batch_release(&fbatch);
+		cond_resched();
+	}
+
+	/*
+	 * If we hit the last page and there is more work to be done: wrap
+	 * back the index back to the start of the file for the next
+	 * time we are called.
+	 */
+	if (wbc->range_cyclic && !done)
+		done_index = 0;
+	if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0))
+		mapping->writeback_index = done_index;
+
+	return ret;
+}
+
 /*
  * Write some of the pending data back to the server
  */
@@ -2816,7 +2951,7 @@ static int cifs_writepages(struct address_space *mapping,
 	 * to prevent it.
 	 */
 
-	ret = write_cache_pages(mapping, wbc, cifs_writepages_add_folio, &ctx);
+	ret = cifs_write_cache_pages(mapping, wbc, &ctx);
 	if (ret >= 0 && ctx.begun) {
 		ret = cifs_writepages_submit(mapping, wbc, &ctx);
 		if (ret < 0)


      reply	other threads:[~2023-03-02 23:42 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-02 23:16 [PATCH 0/3] smb3, afs: Revert changes to {cifs,afs}_writepages_region() David Howells
2023-03-02 23:16 ` [PATCH 1/3] mm: Add a function to get a single tagged folio from a file David Howells
2023-03-02 23:21   ` Matthew Wilcox
2023-03-02 23:16 ` [PATCH 2/3] afs: Partially revert and use filemap_get_folio_tag() David Howells
2023-03-02 23:16 ` [PATCH 3/3] cifs: " David Howells
2023-03-02 23:20 ` [PATCH 0/3] smb3, afs: Revert changes to {cifs,afs}_writepages_region() David Howells
2023-03-02 23:23 ` Test patch to remove per-page dirty region tracking from afs David Howells
2023-03-02 23:29   ` Test patch to make afs use write_cache_pages() David Howells
2023-03-02 23:32     ` Test patch to make afs use its own version of write_cache_pages() David Howells
2023-03-02 23:36 ` cifs test patch to convert to using write_cache_pages() David Howells
2023-03-02 23:41   ` David Howells [this message]

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=522532.1677800499@warthog.procyon.org.uk \
    --to=dhowells@redhat.com \
    --cc=jlayton@kernel.org \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.dionne@auristor.com \
    --cc=metze@samba.org \
    --cc=nspmangalore@gmail.com \
    --cc=pc@cjr.nz \
    --cc=rohiths.msft@gmail.com \
    --cc=smfrench@gmail.com \
    --cc=tom@talpey.com \
    --cc=torvalds@linux-foundation.org \
    --cc=vishal.moola@gmail.com \
    --cc=willy@infradead.org \
    /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