[PATCH 1/2] btrfs: Simplify page unlocking in alloc_extent_buffer

[PATCH 1/2] btrfs: Simplify page unlocking in alloc_extent_buffer

[Nikolay Borisov]

Current version of the page unlocking code was added in
727011e07cbd ("Btrfs: allow metadata blocks larger than the page size")
but even in this commit that particular flag was never used per-se. In
fact, btrfs only uses PageChecked for data pages to identify pages
which have been dirtied but don't have ORDERED bit set. For more
information see 247e743cbe6e ("Btrfs: Use async helpers to deal with
pages that have been improperly dirtied"). However, this doesn't apply
to extent buffer pages. The important bit here is that the pages are
unlocked AFTER the extent buffer has been properly recorded in the
radix tree to avoid races with btree_releasepage. Let's exploit this
fact and simplify the page unlocking sequence by unlocking the pages
in-order and removing the redundant PageChecked flag setting/clearing.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d0482832bd6b..2f5c6721d3bc 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5039,13 +5039,8 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	 * after the extent buffer is in the radix tree so
 	 * it doesn't get lost
 	 */
-	SetPageChecked(eb->pages[0]);
-	for (i = 1; i < num_pages; i++) {
-		p = eb->pages[i];
-		ClearPageChecked(p);
-		unlock_page(p);
-	}
-	unlock_page(eb->pages[0]);
+	for (i = 0; i < num_pages; i++)
+		unlock_page(eb->pages[i]);
 	return eb;
 
 free_eb:
-- 
2.7.4

[PATCH 2/2] btrfs: Reword dodgy comments

[Nikolay Borisov]

Commit eb14ab8ed24a ("Btrfs: fix page->private races") fixed a genuine
race between extent buffer initialisation and btree_releaseage.
Unfortunately as the code has evolved the comments weren't changed
which made them slightly wrong and they weren't very clear in the
fist place. Fix this by (hopefully) rewording them in a more
approachable manner.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2f5c6721d3bc..07b143577c28 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5001,8 +5001,11 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 			uptodate = 0;
 
 		/*
-		 * see below about how we avoid a nasty race with release page
-		 * and why we unlock later
+		 * We can't unlock the pages just yet since the extent buffer
+		 * hasn't been properly inserted in the radix tree, this
+		 * opens a race with btree_releasepage which can free a page
+		 * while we are still filling in all pages for the buffer and
+		 * we crash.
 		 */
 	}
 	if (uptodate)
@@ -5031,13 +5034,9 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	set_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags);
 
 	/*
-	 * there is a race where release page may have
-	 * tried to find this extent buffer in the radix
-	 * but failed.  It will tell the VM it is safe to
-	 * reclaim the, and it will clear the page private bit.
-	 * We must make sure to set the page private bit properly
-	 * after the extent buffer is in the radix tree so
-	 * it doesn't get lost
+	 * Now it's safe to unlock the pages because any calls to
+	 * btree_release page will correctly detect that a page belongs to a
+	 * live buffer and won't free them pre-maturely.
 	 */
 	for (i = 0; i < num_pages; i++)
 		unlock_page(eb->pages[i]);
-- 
2.7.4

Re: [PATCH 1/2] btrfs: Simplify page unlocking in alloc_extent_buffer

[David Sterba]

On Wed, Jul 04, 2018 at 10:24:51AM +0300, Nikolay Borisov wrote:
> Current version of the page unlocking code was added in
> 727011e07cbd ("Btrfs: allow metadata blocks larger than the page size")
> but even in this commit that particular flag was never used per-se. In
> fact, btrfs only uses PageChecked for data pages to identify pages
> which have been dirtied but don't have ORDERED bit set. For more
> information see 247e743cbe6e ("Btrfs: Use async helpers to deal with
> pages that have been improperly dirtied"). However, this doesn't apply
> to extent buffer pages. The important bit here is that the pages are
> unlocked AFTER the extent buffer has been properly recorded in the
> radix tree to avoid races with btree_releasepage. Let's exploit this
> fact and simplify the page unlocking sequence by unlocking the pages
> in-order and removing the redundant PageChecked flag setting/clearing.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>