[0/5] Remove extent_map::bdev
mbox series

Message ID cover.1570474492.git.dsterba@suse.com
Headers show
Series
  • Remove extent_map::bdev
Related show

Message

David Sterba Oct. 7, 2019, 7:37 p.m. UTC
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(-)

Comments

David Sterba Oct. 22, 2019, 2 p.m. UTC | #1
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

Moved from topic branch to misc-next.
Josef Bacik Nov. 18, 2019, 3:41 p.m. UTC | #2
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,

Josef
David Sterba Nov. 18, 2019, 3:49 p.m. UTC | #3
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.

Do you have the stack trace of the crash?
David Sterba Nov. 18, 2019, 9:57 p.m. UTC | #4
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.