linux-kernel - [PATCH 5/6] iomap: drop unnecessary state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240731091305.2896873-6-yi.zhang@huaweicloud.com>
Date: Wed, 31 Jul 2024 17:13:04 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: linux-xfs@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Cc: linux-kernel@...r.kernel.org,
	djwong@...nel.org,
	hch@...radead.org,
	brauner@...nel.org,
	david@...morbit.com,
	jack@...e.cz,
	yi.zhang@...wei.com,
	yi.zhang@...weicloud.com,
	chengzhihao1@...wei.com,
	yukuai3@...wei.com
Subject: [PATCH 5/6] iomap: drop unnecessary state_lock when setting ifs uptodate bits

From: Zhang Yi <yi.zhang@...wei.com>

Commit '1cea335d1db1 ("iomap: fix sub-page uptodate handling")' fix a
race issue when submitting multiple read bios for a page spans more than
one file system block by adding a spinlock(which names state_lock now)
to make the page uptodate synchronous. However, the race condition only
happened between the read I/O submitting and completeing threads, it's
sufficient to use page lock to protect other paths, e.g. buffered write
path. After large folio is supported, the spinlock could affect more
about the buffered write performance, so drop it could reduce some
unnecessary locking overhead.

Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
---
 fs/iomap/buffered-io.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index f5668895df66..248f4a586f8f 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -73,14 +73,10 @@ static void iomap_set_range_uptodate(struct folio *folio, size_t off,
 		size_t len)
 {
 	struct iomap_folio_state *ifs = folio->private;
-	unsigned long flags;
 	bool uptodate = true;
 
-	if (ifs) {
-		spin_lock_irqsave(&ifs->state_lock, flags);
+	if (ifs)
 		uptodate = ifs_set_range_uptodate(folio, ifs, off, len);
-		spin_unlock_irqrestore(&ifs->state_lock, flags);
-	}
 
 	if (uptodate)
 		folio_mark_uptodate(folio);
@@ -395,7 +391,18 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
 
 	if (iomap_block_needs_zeroing(iter, pos)) {
 		folio_zero_range(folio, poff, plen);
-		iomap_set_range_uptodate(folio, poff, plen);
+		if (ifs) {
+			/*
+			 * Hold state_lock to protect from submitting multiple
+			 * bios for the same locked folio and then get multiple
+			 * callbacks in parallel.
+			 */
+			spin_lock_irq(&ifs->state_lock);
+			iomap_set_range_uptodate(folio, poff, plen);
+			spin_unlock_irq(&ifs->state_lock);
+		} else {
+			folio_mark_uptodate(folio);
+		}
 		goto done;
 	}
 
-- 
2.39.2