| Message ID | cover.1570474492.git.dsterba@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Remove extent_map::bdev | expand |
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.
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
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?
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.