From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4AAD2C3DA61 for ; Fri, 19 Jul 2024 00:25:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B3FA06B0082; Thu, 18 Jul 2024 20:25:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AEFD86B0083; Thu, 18 Jul 2024 20:25:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9901B6B0085; Thu, 18 Jul 2024 20:25:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 75EE16B0082 for ; Thu, 18 Jul 2024 20:25:28 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id E90FF402D1 for ; Fri, 19 Jul 2024 00:25:27 +0000 (UTC) X-FDA: 82354608294.17.9F788ED Received: from mail-pg1-f176.google.com (mail-pg1-f176.google.com [209.85.215.176]) by imf30.hostedemail.com (Postfix) with ESMTP id E5A5A80004 for ; Fri, 19 Jul 2024 00:25:25 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=WyMV06mW; spf=pass (imf30.hostedemail.com: domain of david@fromorbit.com designates 209.85.215.176 as permitted sender) smtp.mailfrom=david@fromorbit.com; dmarc=pass (policy=quarantine) header.from=fromorbit.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721348693; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=eOcvsNPNJsOW5of/uY6NJFdmB3X8eXRaur0mZXAcEBY=; b=ssKpTfDVGNIqBSqRsdZSYy5a+P/0/sbGBgiLir4DmeqskUJDZNsklsJFpyZm6V3vtewsA2 b31u7kD6iPmv+W+/NgkijS4iwixEsIvdrqTVWt9Hmd1pRfHq0Oey2DnNRAKjMG7AB2IRTY I2XiO30ZgvaydXi/MMvQmNYT3+gxC5k= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=WyMV06mW; spf=pass (imf30.hostedemail.com: domain of david@fromorbit.com designates 209.85.215.176 as permitted sender) smtp.mailfrom=david@fromorbit.com; dmarc=pass (policy=quarantine) header.from=fromorbit.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721348693; a=rsa-sha256; cv=none; b=QhCDnWuDfA6Y9qZDZXPYoSE4mtsuWHsxjhh5XV5treOb5jOIlvoJfGJ+uJSUb6acYVyJ3K Jz4ymfx3KRDZVEoTye3jpPveGFQm0LJ42HizMJIlBS45AaS7Brc+jPj+PkgGgn5TfVCDg9 KmSpZSUZqxqlZ88fqgtE2+GZKyy4MVI= Received: by mail-pg1-f176.google.com with SMTP id 41be03b00d2f7-75a6c290528so963644a12.1 for ; Thu, 18 Jul 2024 17:25:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1721348724; x=1721953524; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=eOcvsNPNJsOW5of/uY6NJFdmB3X8eXRaur0mZXAcEBY=; b=WyMV06mWCEIqvJKmY/+1edydPjd7jtglbKYdkBo/eA1liqEy31KEF1sid6XaZDBzKc Olaszj/LL0gK053pCPz5FXy/pLtOv49e82N2MvDSDEYqLYIrhZSBDLVEEozTewFB2jIN pAlGAC85EuV/7mkAhaIgCmY8VNXBCUR2rkLR9RSN8Khze4HXwn6iUCYRF+oFrkxdVgnW NL/fmLLmziHziwT4bd+Rjlp+sUkWJbWUbxmiNqwTSLkE4/Xrmey47mHqoN7mDtuC3z50 /z0+dRaCoRqYlJYjAsGdID9FaIhSwP50vyXkwT4rl5Kt0vnaFIyl4LeOBiB9hfEGVAwq k13w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721348724; x=1721953524; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=eOcvsNPNJsOW5of/uY6NJFdmB3X8eXRaur0mZXAcEBY=; b=tmmnBu5eYccsjIgxuzyxF6dldw5w0RJl+jTGsxNX7gUo/CG2VuNe5GEj9kMiUOYFTn G6MoUFEgorAoyuSgtIqeZr9NqUxXG5VSIsfulyBJu5Tk7VZp5g3WI+Yu4a7mOinQmk0G S5WUf4WZIRFnKge+1+S+rHXjKy23931aBo4F/uslqKG2yQQ/dRpgwpIrP/Fx43klv/QJ ktVzkSKcnUFyWAxesxy4Y+F3P3tP82sFihLmUB0Aw26r6EKf1fGm6n1OR9OsrRqUUzrs R5cJ4GYYPBfAvgAAMnQhiTG+HPOWWf29uYRYSuXvu1S97JejR6dUBAprt0HhCFcnGtsM sVDg== X-Forwarded-Encrypted: i=1; AJvYcCVxoUKzQFw4/vqbu6wpuWOrBfrFqDz8SLO/zU3GHmsWPAk+7KI9dBgOdTan10+gYlDBg+IybUPRhuneKfsW6gOhL9g= X-Gm-Message-State: AOJu0YwSdecgdcmLFPQyddPvARj6azn1+tbCk8yYWAiIE56++yLiulwz 5CB0dlKAgUzV0Otul6tIdXlNyBRivUA0lUgfv1Ids/hf0q6vW5dUqqc425OimkM= X-Google-Smtp-Source: AGHT+IHVRTt+YTASMX6gtjziVqWXoTgY5Jeq2DjrB0huGt95tcGCxDjV8hgTw93Gk6UrlB0ObIF8lw== X-Received: by 2002:a05:6a20:6a27:b0:1c2:9070:90ce with SMTP id adf61e73a8af0-1c3fdd30a79mr7668339637.43.1721348724405; Thu, 18 Jul 2024 17:25:24 -0700 (PDT) Received: from dread.disaster.area (pa49-179-32-121.pa.nsw.optusnet.com.au. [49.179.32.121]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2ccf7b5c342sm248371a91.19.2024.07.18.17.25.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Jul 2024 17:25:23 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1sUbR2-003Db1-2P; Fri, 19 Jul 2024 10:25:20 +1000 Date: Fri, 19 Jul 2024 10:25:20 +1000 From: Dave Chinner To: Brian Foster Cc: linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH 3/4] iomap: fix handling of dirty folios over unwritten extents Message-ID: References: <20240718130212.23905-1-bfoster@redhat.com> <20240718130212.23905-4-bfoster@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240718130212.23905-4-bfoster@redhat.com> X-Stat-Signature: 6a3fmxi5dm46wz6fxuq83eritwcus8ii X-Rspam-User: X-Rspamd-Queue-Id: E5A5A80004 X-Rspamd-Server: rspam02 X-HE-Tag: 1721348725-640908 X-HE-Meta: U2FsdGVkX192M4kFsDFhuNQQsks+zQH283uMdR/LKpPuIbmR1MR2ZmGHs2h1lTsEp+XrHaTod7U32/b2hnuij/+fkiAFZoz0KG6fER2ortWMdtazS71rbqnFMw5vdfMNun8MGvmWnqpYPo7aKskuxBGSFoU8LB82FNYmPLxGvSpAsDG35D31qUGQaD/ln/gWxD3mARErZo4F/RYDyLmQxTTqze7c2tBWTlZ0AN7k13Gmmw82NItWOpVAV54NFPzzBWXHoASL97fTLZa18Li6FyN0GGu4ZXsny11+CSv8XozdtiO2e6sHaXCgXu2J/bhrBJ+vZt3vQmISfQKwcQvxGAGZPpweRmwLVZAB5lwZaIfqencWFzxOjMH7iQLrFNky+bDk+/459tHTKYIC7JaQOGQ4n73zP7+oAVCCCtPkc8XSummwCO0HEtb36JLFqP0AyH/4NRGnNhVaoyNSBl1POaK0jS0m2X06e0U7NSSf8q+vwapn7wbn0OulZczx2hT2UvwNI1+nhjHI4yCh4wZUw1in1ZreVOAyLcRCxDQUG6ITJR14SBwpiqLmIA2cgvora5W4HPkBia69a4lbjJqQdIZFGBz0OIqqkLEQ4cVKdEC7jtkHwh7cGJcqbhkgqyHXMMm1KR0/MT5lTiyINfDk95NEp1WuwJWq8/GuApVZi+7uWK4B5yUGAsNdxbTxaZKtu+ms4VN7aLTh5DIOUZgPArduc4uVuYLiOHQw9Kjj8kpC99/WfIhix3Ph+ExkeKngqhHVvt7bQt/+rGfmD3cXg7rNfQD6fpH5MMg60FqS14Pmyhj6BvD+XbfMR22T1QBNyiea4jo/SWRzfBQdZ9TCaRK4BabuRFZWotUmJruOf6iXw36c3ghSsXbjwum3OvWT7/MimmAG3IDGl4ypcLZParXzi1EVoTuZmEYG4piMKMtQF0iHMeIwvQRdIVRJBLEr1nOvNaJZPn8KmFKM34x si6bovWm q6XJj2aN7kDHRH7PUe9pl1NLeYGlKikaj0DnFv3C2e1sv1C6WuCB+H+H4HuvzCKyole0oQrtkgP2zvdU9u/hfm6S57cFTl5ugAU2gRX1psLOrKII0ER57W3YOg8qnCbmPMyCiTsnvtWaiHBjsPv4lClX5fz/FlGojhduHLbdV/rLNTFsm6eN5vumdKIFHstAxibYEQIDDgCAzIBouWrMQmSc2IWStSoFmHp2SeHdiliGF6lLPl377uf9XcI4dJ01PV4YbOlGXk3LUDkVGbNv6MMEK8lL6BSDbAJ9nakptAk7NGl4dAb59LYQQmSZWeObk8ZuM0gb5gA/eamIg2Ccp7oCXldkTLWponDziH3/n67cjQdpyNBY8ekcprgL/JCo/rutXc8HdmFcXEfmlt7hnT0k6H7aHTtLbCeT9hY3ZfM3Us+ou5WjzAz7hmevzrAuyWe6FeB3Mj+y+iPratAvee3U8Rn9kR1/vc0QSmhm/iFuHsL8l2DL8uPg4Hqh0lPocEIovcl3R5Sr36rzahLBJzDdGnQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Thu, Jul 18, 2024 at 09:02:11AM -0400, Brian Foster wrote: > iomap_zero_range() does not correctly handle unwritten mappings with > dirty folios in pagecache. It skips unwritten mappings > unconditionally as if they were already zeroed, and thus potentially > exposes stale data from a previous write if affected folios are not > written back before the zero range. > > Most callers already flush the target range of the zero for > unrelated, context specific reasons, so this problem is not > necessarily prevalent. The known outliers (in XFS) are file > extension via buffered write and truncate. The truncate path issues > a flush to work around this iomap problem, but the file extension > path does not and thus can expose stale data if current EOF is > unaligned and has a dirty folio over an unwritten block. > > This patch implements a mechanism for making zero range pagecache > aware for filesystems that support mapping validation (i.e. > folio_ops->iomap_valid()). Instead of just skipping unwritten > mappings, scan the corresponding pagecache range for dirty or > writeback folios. If found, explicitly zero them via buffered write. > Clean or uncached subranges of unwritten mappings are skipped, as > before. > > The quirk with a post-iomap_begin() pagecache scan is that it is > racy with writeback and reclaim activity. Even if the higher level > code holds the invalidate lock, nothing prevents a dirty folio from > being written back, cleaned, and even reclaimed sometime after > iomap_begin() returns an unwritten map but before a pagecache scan > might find the dirty folio. To handle this situation, we can rely on > the fact that writeback completion converts unwritten extents in the > fs before writeback state is cleared on the folio. > > This means that a pagecache scan followed by a mapping revalidate of > an unwritten mapping should either find a dirty folio if it exists, > or detect a mapping change if a dirty folio did exist and had been > cleaned sometime before the scan but after the unwritten mapping was > found. If the revalidation succeeds then we can safely assume > nothing has been written back and skip the range. If the > revalidation fails then we must assume any offset in the range could > have been modified by writeback. In other words, we must be > particularly careful to make sure that any uncached range we intend > to skip does not make it into iter.processed until the mapping is > revalidated. > > Altogether, this allows zero range to handle dirty folios over > unwritten extents without needing to flush and wait for writeback > completion. > > Signed-off-by: Brian Foster > --- > fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 50 insertions(+), 3 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index a9425170df72..ea1d396ef445 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1385,6 +1385,23 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > } > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > +/* > + * Scan an unwritten mapping for dirty pagecache and return the length of the > + * clean or uncached range leading up to it. This is the range that zeroing may > + * skip once the mapping is validated. > + */ > +static inline loff_t > +iomap_zero_iter_unwritten(struct iomap_iter *iter, loff_t pos, loff_t length) > +{ > + struct address_space *mapping = iter->inode->i_mapping; > + loff_t fpos = pos; > + > + if (!filemap_range_has_writeback(mapping, &fpos, length)) > + return length; > + /* fpos can be smaller if the start folio is dirty */ > + return max(fpos, pos) - pos; I'm not sure this is safe. filemap_range_has_writeback() doesn't do checks for invalidation races or that the folio is actually valid. It also treats locked folios as dirty and a locked folio isn't necessarily dirty. IOWs, this check assumes that we'll do all these checks during the actual writeback operation that would follow this check and so skip anything that might have given a false positive here. iomap_write_begin() doesn't do those sorts of check. If there's no folio in the cache, it will simply instantiate a new one and dirty it. If there's an existing folio, it will simply dirty it. Hence I don't think a "is there a folio in this range that is a potential writeback candidate" check is correct here. I think we need to be more robust in determining if a cached folio in the range exists and needs zeroing. Ideas on that to follow... > static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > { > const struct iomap *srcmap = iomap_iter_srcmap(iter); > @@ -1393,16 +1410,46 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > loff_t written = 0; > > /* already zeroed? we're done. */ > - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > + if (srcmap->type == IOMAP_HOLE) > return length; > > do { > struct folio *folio; > int status; > size_t offset; > - size_t bytes = min_t(u64, SIZE_MAX, length); > + size_t bytes; > + loff_t pending = 0; > bool ret; > > + /* > + * Determine the range of the unwritten mapping that is clean in > + * pagecache. We can skip this range, but only if the mapping is > + * still valid after the pagecache scan. This is because > + * writeback may have cleaned folios after the mapping lookup > + * but before we were able to find them here. If that occurs, > + * then the mapping must now be stale and we must reprocess the > + * range. > + */ > + if (srcmap->type == IOMAP_UNWRITTEN) { > + pending = iomap_zero_iter_unwritten(iter, pos, length); > + if (pending == length) { > + /* no dirty cache, revalidate and bounce as we're > + * either done or the mapping is stale */ > + if (iomap_revalidate(iter)) > + written += pending; Ok, this isn't really how the revalidation was supposed to be used (i.e. it's not stabilising the data and page cache state first), but it looks like works for this situation. We can use this. :) > + break; > + } > + > + /* > + * Found a dirty folio. Update pos/length to point at > + * it. written is updated only after the mapping is > + * revalidated by iomap_write_begin(). > + */ > + pos += pending; > + length -= pending; > + } > + > + bytes = min_t(u64, SIZE_MAX, length); > status = iomap_write_begin(iter, pos, bytes, &folio); > if (status) > return status; We don't hold any references to the page cache between the iomap_zero_iter_unwritten() and then the actual folio lookup in iomap_write_begin() where we reference and lock the folio at the given offset. e.g. we get a "dirty" hit because of a locked folio, and that folio is clean and contains zeroes (e.g. mmap read, readahead, etc). We now write zeroes to that folio and dirty it when, we should actually be skipping it and leaving the range as unwritten. In previous patches that fixed this zeroing issue, this wasn't a problem because it used the existing iomap page cache lookup mechanisms from iomap_write_begin() to get referenced, locked folios over the zeroing range. Instead of skipping potential page cache holes, it prevented page cache instantiation over page cache holes from occurring when zeroing unwritten extents and that skipped page cache holes naturally. https://lore.kernel.org/linux-xfs/20240529095206.2568162-2-yi.zhang@huaweicloud.com/ This means the iteration would skip holes but still safely revalidate the iomap once it found and locked a folio in the given range. It could also then check the folio is dirty to determine if zeroing was necessary. Yes, this means it iterated holes in the range PAGE_SIZE by PAGE_SIZE to do lookups, so it was inefficient. However, we could still use this filemap_range_has_writeback() optimisation to skip unwritten extents that don't have any cached pages over them completely, but ti don't think it is really safe to use it during the iteration to discover regions that don't need zeroing. i.e. the fast path is this: if (srcmap->type == IOMAP_HOLE) return length; if (srcmap->type == IOMAP_UNWRITTEN && !filemap_range_has_writeback(mapping, pos, length)) { if (!iomap_revalidate(iter)) return 0; /* stale mapping */ return length; } And the slow path does something similar to the above patch. However, I'm still not sure that filemap_range_has_writeback() is the right set of checks to use here. I just thought of another option - after thinking about how you've modified filemap_range_has_writeback() to increment the iomap iterator position to skip empty ranges, I think we could actually drive that inwards to the page cache lookup. We could use use filemap_get_folios() instead of __filemap_get_folio() for unwritten extent zeroing iteration. This would allow the page cache lookup to return the first folio in the range as a referenced folio which we can then lock and validate similar to __filemap_get_folio(). We can then increment the iterator position based on the folio position, the iomap_write_begin() code will do the revalidation of the iomap, and the zeroing code can then determine if zeroing is needed based on whether the folio is dirty/writeback or not. As this all happens under the folio lock, it is largely safe from both page cache data and state and extent state changes racing with the zeroing operation. Thoughts? -Dave. -- Dave Chinner david@fromorbit.com