Re: [PATCH 0/5] Remove extent_map::bdev

[David Sterba <dsterba@suse.cz>]

On Mon, Nov 18, 2019 at 10:41:54AM -0500, Josef Bacik wrote:
> On Mon, Oct 07, 2019 at 09:37:40PM +0200, David Sterba wrote:
> > The extent_map::bdev is unused and and can be removed, but it's not
> > straightforward so there are several steps. The first patch decouples
> > bdev from map_lookup. The remaining patches clean up use of the bdev,
> > removing a few bio_set_dev on the way. In the end, submit_extent_page is
> > one parameter lighter.
> > 
> > This has survived several fstests runs
> > 
> > David Sterba (5):
> >   btrfs: assert extent_map bdevs and lookup_map and split
> >   btrfs: get bdev from latest_dev for dio bh_result
> >   btrfs: drop bio_set_dev where not needed
> >   btrfs: remove extent_map::bdev
> >   btrfs: drop bdev argument from submit_extent_page
> > 
> >  fs/btrfs/compression.c | 10 ----------
> >  fs/btrfs/disk-io.c     |  3 ---
> >  fs/btrfs/extent_io.c   | 15 +++------------
> >  fs/btrfs/extent_map.c  |  6 +++++-
> >  fs/btrfs/extent_map.h  | 11 ++---------
> >  fs/btrfs/file-item.c   |  1 -
> >  fs/btrfs/file.c        |  3 ---
> >  fs/btrfs/inode.c       | 14 ++++----------
> >  fs/btrfs/relocation.c  |  2 --
> >  9 files changed, 14 insertions(+), 51 deletions(-)
> > 
> 
> This series needs to be dropped from misc-next, it causes any box with cgroups
> enabled to crash on boot.  The bio requires having a bio->bi_disk set when we do
> wbc_init_bio, which we no longer have with this patch.
> 
> To fix this you would need to do something similar to what we do with
> compression, save the wbc css in the bbio and then call the associate_blkg at
> submit time.
> 
> However, this is problematic right now because we don't get notified when the
> bbio is free'd.  We have no way to free the ref on the css we save, which means
> infrastructure needs to be added to biosets so we can get called at free time
> for every bio in a bioset.  Or we can add back the bi_css to the bio, but that
> seems like a less good idea.
> 
> Either way this needs to be dropped until this is addressed.  Thanks,

I found a simple fix that does not need the reverts.

The bdev passed to submit_extent_bio is the latest_bdev, that's what the
cleanup series got rid of, pointlessly passing around a known pointer.

For equivalent change, the bdev can be pulled out of the page in a
series of 5 dereferences.

@@ -2987,13 +2987,16 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
        }
 
        bio = btrfs_bio_alloc(offset);
-       bio_set_dev(bio, bdev);
        bio_add_page(bio, page, page_size, pg_offset);
        bio->bi_end_io = end_io_func;
        bio->bi_private = tree;
        bio->bi_write_hint = page->mapping->host->i_write_hint;
        bio->bi_opf = opf;
        if (wbc) {
+               struct block_device *bdev;
+
+               bdev = BTRFS_I(page->mapping->host)->root->fs_info->fs_devices->latest_bdev;
+               bio_set_dev(bio, bdev);
                wbc_init_bio(wbc, bio);
                wbc_account_cgroup_owner(wbc, page, page_size);
        }
---

To avoid problems with bisection, I'll put the series to after this patch but
otherwise I don't see any reasons to revert.