btrfs: fix broken drop_caches on extent buffer folios

The (correct) commit e41c81d0d30e ("mm/truncate: Replace page_mapped()
call in invalidate_inode_page()") replaced the page_mapped(page) check
with a refcount check. However, this refcount check does not work as
expected with drop_caches for btrfs's metadata pages.

Btrfs has a per-sb metadata inode with cached pages, and when not in
active use by btrfs, they have a refcount of 3. One from the initial
call to alloc_pages(), one (nr_pages == 1) from filemap_add_folio(), and
one from folio_attach_private(). We would expect such pages to get dropped
by drop_caches. However, drop_caches calls into mapping_evict_folio() via
mapping_try_invalidate() which gets a reference on the folio with
find_lock_entries(). As a result, these pages have a refcount of 4, and
fail this check.

For what it's worth, such pages do get reclaimed under memory pressure,
so I would say that while this behavior is surprising, it is not really
dangerously broken.

When I asked the mm folks about the expected refcount in this case, I
was told that the correct thing to do is to donate the refcount from the
original allocation to the page cache after inserting it.

Therefore, attempt to fix this by adding a put_folio() to the critical
spot in alloc_extent_buffer() where we are sure that we have really
allocated and attached new pages. We must also adjust
folio_detach_private() to properly handle being the last reference to the
folio and not do a use-after-free after folio_detach_private().

extent_buffers allocated by clone_extent_buffer() and
alloc_dummy_extent_buffer() are unmapped, so this transfer of ownership
from allocation to insertion in the mapping does not apply to them.
However, we can still folio_put() them safely once they are finished
being allocated and have called folio_attach_private().

Finally, removing the generic put_folio() for the allocation from
btrfs_detach_extent_buffer_folios() means we need to be careful to do
the appropriate put_folio() in allocation failure paths in
alloc_extent_buffer(), clone_extent_buffer() and
alloc_dummy_extent_buffer().

Link: https://lore.kernel.org/linux-mm/ZrwhTXKzgDnCK76Z@casper.infradead.org/
Tested-by: Klara Modin <klarasmodin@gmail.com>
Reviewed-by: Daniel Vacek <neelx@suse.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Boris Burkov <boris@bur.io>
Signed-off-by: David Sterba <dsterba@suse.com>

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8467b0128c9352bb38040d7497002e07a08b6eda..e43f6280f9543a83cf695507b8880799710e31ae 100644 (file)
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2747,6 +2747,7 @@ static bool folio_range_has_eb(struct folio *folio)
 static void detach_extent_buffer_folio(const struct extent_buffer *eb, struct folio *folio)
 {
        struct btrfs_fs_info *fs_info = eb->fs_info;
+       struct address_space *mapping = folio->mapping;
        const bool mapped = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
 
        /*
@@ -2754,11 +2755,11 @@ static void detach_extent_buffer_folio(const struct extent_buffer *eb, struct fo
         * be done under the i_private_lock.
         */
        if (mapped)
-               spin_lock(&folio->mapping->i_private_lock);
+               spin_lock(&mapping->i_private_lock);
 
        if (!folio_test_private(folio)) {
                if (mapped)
-                       spin_unlock(&folio->mapping->i_private_lock);
+                       spin_unlock(&mapping->i_private_lock);
                return;
        }
 
@@ -2777,7 +2778,7 @@ static void detach_extent_buffer_folio(const struct extent_buffer *eb, struct fo
                        folio_detach_private(folio);
                }
                if (mapped)
-                       spin_unlock(&folio->mapping->i_private_lock);
+                       spin_unlock(&mapping->i_private_lock);
                return;
        }
 
@@ -2800,7 +2801,7 @@ static void detach_extent_buffer_folio(const struct extent_buffer *eb, struct fo
        if (!folio_range_has_eb(folio))
                btrfs_detach_subpage(fs_info, folio, BTRFS_SUBPAGE_METADATA);
 
-       spin_unlock(&folio->mapping->i_private_lock);
+       spin_unlock(&mapping->i_private_lock);
 }
 
 /* Release all folios attached to the extent buffer */
@@ -2815,9 +2816,6 @@ static void btrfs_release_extent_buffer_folios(const struct extent_buffer *eb)
                        continue;
 
                detach_extent_buffer_folio(eb, folio);
-
-               /* One for when we allocated the folio. */
-               folio_put(folio);
        }
 }
 
@@ -2852,9 +2850,28 @@ static struct extent_buffer *__alloc_extent_buffer(struct btrfs_fs_info *fs_info
        return eb;
 }
 
+/*
+ * For use in eb allocation error cleanup paths, as btrfs_release_extent_buffer()
+ * does not call folio_put(), and we need to set the folios to NULL so that
+ * btrfs_release_extent_buffer() will not detach them a second time.
+ */
+static void cleanup_extent_buffer_folios(struct extent_buffer *eb)
+{
+       const int num_folios = num_extent_folios(eb);
+
+       /* We canont use num_extent_folios() as loop bound as eb->folios changes. */
+       for (int i = 0; i < num_folios; i++) {
+               ASSERT(eb->folios[i]);
+               detach_extent_buffer_folio(eb, eb->folios[i]);
+               folio_put(eb->folios[i]);
+               eb->folios[i] = NULL;
+       }
+}
+
 struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
 {
        struct extent_buffer *new;
+       int num_folios;
        int ret;
 
        new = __alloc_extent_buffer(src->fs_info, src->start);
@@ -2869,25 +2886,34 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
        set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags);
 
        ret = alloc_eb_folio_array(new, false);
-       if (ret) {
-               btrfs_release_extent_buffer(new);
-               return NULL;
-       }
+       if (ret)
+               goto release_eb;
 
-       for (int i = 0; i < num_extent_folios(src); i++) {
+       ASSERT(num_extent_folios(src) == num_extent_folios(new),
+              "%d != %d", num_extent_folios(src), num_extent_folios(new));
+       /* Explicitly use the cached num_extent value from now on. */
+       num_folios = num_extent_folios(src);
+       for (int i = 0; i < num_folios; i++) {
                struct folio *folio = new->folios[i];
 
                ret = attach_extent_buffer_folio(new, folio, NULL);
-               if (ret < 0) {
-                       btrfs_release_extent_buffer(new);
-                       return NULL;
-               }
+               if (ret < 0)
+                       goto cleanup_folios;
                WARN_ON(folio_test_dirty(folio));
        }
+       for (int i = 0; i < num_folios; i++)
+               folio_put(new->folios[i]);
+
        copy_extent_buffer_full(new, src);
        set_extent_buffer_uptodate(new);
 
        return new;
+
+cleanup_folios:
+       cleanup_extent_buffer_folios(new);
+release_eb:
+       btrfs_release_extent_buffer(new);
+       return NULL;
 }
 
 struct extent_buffer *alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
@@ -2902,13 +2928,15 @@ struct extent_buffer *alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 
        ret = alloc_eb_folio_array(eb, false);
        if (ret)
-               goto out;
+               goto release_eb;
 
        for (int i = 0; i < num_extent_folios(eb); i++) {
                ret = attach_extent_buffer_folio(eb, eb->folios[i], NULL);
                if (ret < 0)
-                       goto out_detach;
+                       goto cleanup_folios;
        }
+       for (int i = 0; i < num_extent_folios(eb); i++)
+               folio_put(eb->folios[i]);
 
        set_extent_buffer_uptodate(eb);
        btrfs_set_header_nritems(eb, 0);
@@ -2916,15 +2944,10 @@ struct extent_buffer *alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 
        return eb;
 
-out_detach:
-       for (int i = 0; i < num_extent_folios(eb); i++) {
-               if (eb->folios[i]) {
-                       detach_extent_buffer_folio(eb, eb->folios[i]);
-                       folio_put(eb->folios[i]);
-               }
-       }
-out:
-       kmem_cache_free(extent_buffer_cache, eb);
+cleanup_folios:
+       cleanup_extent_buffer_folios(eb);
+release_eb:
+       btrfs_release_extent_buffer(eb);
        return NULL;
 }
 
@@ -3357,8 +3380,15 @@ again:
         * btree_release_folio will correctly detect that a page belongs to a
         * live buffer and won't free them prematurely.
         */
-       for (int i = 0; i < num_extent_folios(eb); i++)
+       for (int i = 0; i < num_extent_folios(eb); i++) {
                folio_unlock(eb->folios[i]);
+               /*
+                * A folio that has been added to an address_space mapping
+                * should not continue holding the refcount from its original
+                * allocation indefinitely.
+                */
+               folio_put(eb->folios[i]);
+       }
        return eb;
 
 out:
@@ -3372,26 +3402,22 @@ out:
         * want that to grab this eb, as we're getting ready to free it.  So we
         * have to detach it first and then unlock it.
         *
-        * We have to drop our reference and NULL it out here because in the
-        * subpage case detaching does a btrfs_folio_dec_eb_refs() for our eb.
-        * Below when we call btrfs_release_extent_buffer() we will call
-        * detach_extent_buffer_folio() on our remaining pages in the !subpage
-        * case.  If we left eb->folios[i] populated in the subpage case we'd
-        * double put our reference and be super sad.
+        * Note: the bounds is num_extent_pages() as we need to go through all slots.
         */
-       for (int i = 0; i < attached; i++) {
-               ASSERT(eb->folios[i]);
-               detach_extent_buffer_folio(eb, eb->folios[i]);
-               folio_unlock(eb->folios[i]);
-               folio_put(eb->folios[i]);
+       for (int i = 0; i < num_extent_pages(eb); i++) {
+               struct folio *folio = eb->folios[i];
+
+               if (i < attached) {
+                       ASSERT(folio);
+                       detach_extent_buffer_folio(eb, folio);
+                       folio_unlock(folio);
+               } else if (!folio) {
+                       continue;
+               }
+
+               folio_put(folio);
                eb->folios[i] = NULL;
        }
-       /*
-        * Now all pages of that extent buffer is unmapped, set UNMAPPED flag,
-        * so it can be cleaned up without utilizing folio->mapping.
-        */
-       set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
-
        btrfs_release_extent_buffer(eb);
        if (ret < 0)
                return ERR_PTR(ret);