cgroup, writeback, btrfs: make sure btrfs issues metadata IOs from the root cgroup

Export blkcg_root_css so that filesystem modules can use it.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-cgroup.c | 1 +
 1 file changed, 1 insertion(+)

block/blk-cgroup.c +1 -0

index d3f56ba..597a457 100644
@@ -45,6 +45,7 @@ struct blkcg blkcg_root;
 EXPORT_SYMBOL_GPL(blkcg_root);
 
 struct cgroup_subsys_state * const blkcg_root_css = &blkcg_root.css;
+EXPORT_SYMBOL_GPL(blkcg_root_css);
 
 static struct blkcg_policy *blkcg_policy[BLKCG_MAX_POLS];
 

Signature · 1 line

2.9.5

submit_bh_wbc() creates a bio matching the specific @bh and submits it at the end. This patch separates out the bio creation part to its own function, create_bh_bio(), and reimplement submit_bh[_wbc]() using the function.

As bio can now be manipulated before submitted, we can move out @wbc handling into submit_bh_wbc() and similarly this will make adding more submit_bh variants straight-forward.

This patch is pure refactoring and doesn't cause any functional changes.

Signed-off-by Tejun Heo <[email protected]>

Suggested-by Jan Kara <[email protected]>

---
 fs/buffer.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

fs/buffer.c +22 -8

index 170df85..b4b2169 100644
@@ -3086,8 +3086,8 @@ void guard_bio_eod(int op, struct bio *bio)
 	}
 }
 
-static int submit_bh_wbc(int op, int op_flags, struct buffer_head *bh,
-			 enum rw_hint write_hint, struct writeback_control *wbc)
+struct bio *create_bh_bio(int op, int op_flags, struct buffer_head *bh,
+                          enum rw_hint write_hint)
 {
 	struct bio *bio;
 

@@ -3109,11 +3109,6 @@ static int submit_bh_wbc(int op, int op_flags, struct buffer_head *bh,
 	 */
 	bio = bio_alloc(GFP_NOIO, 1);
 
-	if (wbc) {
-		wbc_init_bio(wbc, bio);
-		wbc_account_io(wbc, bh->b_page, bh->b_size);
-	}
-
 	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
 	bio_set_dev(bio, bh->b_bdev);
 	bio->bi_write_hint = write_hint;

@@ -3133,13 +3128,32 @@ static int submit_bh_wbc(int op, int op_flags, struct buffer_head *bh,
 		op_flags |= REQ_PRIO;
 	bio_set_op_attrs(bio, op, op_flags);
 
+	return bio;
+}
+
+static int submit_bh_wbc(int op, int op_flags, struct buffer_head *bh,
+			 enum rw_hint write_hint, struct writeback_control *wbc)
+{
+	struct bio *bio;
+
+	bio = create_bh_bio(op, op_flags, bh, write_hint);
+
+	if (wbc) {
+		wbc_init_bio(wbc, bio);
+		wbc_account_io(wbc, bh->b_page, bh->b_size);
+	}
+
 	submit_bio(bio);
 	return 0;
 }
 
 int submit_bh(int op, int op_flags, struct buffer_head *bh)
 {
-	return submit_bh_wbc(op, op_flags, bh, 0, NULL);
+	struct bio *bio;
+
+	bio = create_bh_bio(op, op_flags, bh, 0);
+	submit_bio(bio);
+	return 0;
 }
 EXPORT_SYMBOL(submit_bh);
 

Signature · 1 line

2.9.5

Implement submit_bh_blkcg_css() which will be used to override cgroup membership on specific buffer_heads.

v2: Reimplemented using create_bh_bio() as suggested by Jan.

Signed-off-by Tejun Heo <[email protected]>

Cc Jan Kara <[email protected]>

Cc Jens Axboe <[email protected]>

---
 fs/buffer.c                 | 12 ++++++++++++
 include/linux/buffer_head.h |  3 +++
 2 files changed, 15 insertions(+)

fs/buffer.c +12 -0

index b4b2169..ed0e473 100644
@@ -3147,6 +3147,18 @@ static int submit_bh_wbc(int op, int op_flags, struct buffer_head *bh,
 	return 0;
 }
 
+int submit_bh_blkcg_css(int op, int op_flags, struct buffer_head *bh,
+			struct cgroup_subsys_state *blkcg_css)
+{
+	struct bio *bio;
+
+	bio = create_bh_bio(op, op_flags, bh, 0);
+	bio_associate_blkcg(bio, blkcg_css);
+	submit_bio(bio);
+	return 0;
+}
+EXPORT_SYMBOL(submit_bh_blkcg_css);
+
 int submit_bh(int op, int op_flags, struct buffer_head *bh)
 {
 	struct bio *bio;

include/linux/buffer_head.h +3 -0

index c8dae55..dca5b3b 100644
@@ -13,6 +13,7 @@
 #include <linux/pagemap.h>
 #include <linux/wait.h>
 #include <linux/atomic.h>
+#include <linux/cgroup.h>
 
 #ifdef CONFIG_BLOCK
 

@@ -197,6 +198,8 @@ void ll_rw_block(int, int, int, struct buffer_head * bh[]);
 int sync_dirty_buffer(struct buffer_head *bh);
 int __sync_dirty_buffer(struct buffer_head *bh, int op_flags);
 void write_dirty_buffer(struct buffer_head *bh, int op_flags);
+int submit_bh_blkcg_css(int op, int op_flags, struct buffer_head *bh,
+			struct cgroup_subsys_state *blkcg_css);
 int submit_bh(int, int, struct buffer_head *);
 void write_boundary_block(struct block_device *bdev,
 			sector_t bblock, unsigned blocksize);

Signature · 1 line

2.9.5

From 3bbed8c7747739cda48f592f165e8839da076a3a Mon Sep 17 00:00:00 2001

From 3bbed8c7747739cda48f592f165e8839da076a3a Mon Sep 17 00:00:00 2001

Issuing metdata or otherwise shared IOs from !root cgroup can lead to priority inversion. This patch ensures that those IOs are always issued from the root cgroup.

This patch updates btrfs_update_iflags() to not set S_CGROUPWB on btree_inodes. This isn't strictly necessary as those inodes don't call the function during init; however, this serves as documentation and prevents possible future mistakes. If this isn't desirable, please feel free to drop the section.

v2: Fixed missing @bh in submit_bh_blkcg_css() call.

Signed-off-by Tejun Heo <[email protected]>

Cc Chris Mason <[email protected]>

Cc Josef Bacik <[email protected]>

---
 fs/btrfs/check-integrity.c | 2 +-
 fs/btrfs/disk-io.c         | 4 ++++
 fs/btrfs/ioctl.c           | 4 +++-
 3 files changed, 8 insertions(+), 2 deletions(-)

fs/btrfs/check-integrity.c +1 -1

index 7d5a9b5..d66774e 100644
@@ -2741,7 +2741,7 @@ int btrfsic_submit_bh(int op, int op_flags, struct buffer_head *bh)
 	struct btrfsic_dev_state *dev_state;
 
 	if (!btrfsic_is_initialized)
-		return submit_bh(op, op_flags, bh);
+		return submit_bh_blkcg_css(op, op_flags, bh, blkcg_root_css);
 
 	mutex_lock(&btrfsic_mutex);
 	/* since btrfsic_submit_bh() might also be called before

fs/btrfs/disk-io.c +4 -0

index dfdab84..fe8bbe1 100644
@@ -1025,6 +1025,8 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
 	int async = check_async_write(bio_flags);
 	blk_status_t ret;
 
+	bio_associate_blkcg(bio, blkcg_root_css);
+
 	if (bio_op(bio) != REQ_OP_WRITE) {
 		/*
 		 * called for a read, do the setup so that checksum validation

@@ -3512,6 +3514,8 @@ static void write_dev_flush(struct btrfs_device *device)
 		return;
 
 	bio_reset(bio);
+	bio_associate_blkcg(bio, blkcg_root_css);
+
 	bio->bi_end_io = btrfs_end_empty_barrier;
 	bio_set_dev(bio, device->bdev);
 	bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH;

fs/btrfs/ioctl.c +3 -1

index 117cc63..8a7db6c 100644
@@ -150,7 +150,9 @@ void btrfs_update_iflags(struct inode *inode)
 		new_fl |= S_NOATIME;
 	if (ip->flags & BTRFS_INODE_DIRSYNC)
 		new_fl |= S_DIRSYNC;
-	new_fl |= S_CGROUPWB;
+	/* btree_inodes are always in the root cgroup */
+	if (btrfs_ino(ip) != BTRFS_BTREE_INODE_OBJECTID)
+		new_fl |= S_CGROUPWB;
 
 	set_mask_bits(&inode->i_flags,
 		      S_SYNC | S_APPEND | S_IMMUTABLE | S_NOATIME | S_DIRSYNC |

Signature · 6 lines

2.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 1 line

From 3bbed8c7747739cda48f592f165e8839da076a3a Mon Sep 17 00:00:00 2001

Issuing metdata or otherwise shared IOs from !root cgroup can lead to priority inversion. This patch ensures that those IOs are always issued from the root cgroup.

This patch updates btrfs_update_iflags() to not set S_CGROUPWB on btree_inodes.

This isn't strictly necessary as those inodes don't
call the function during init; however, this serves as documentation and prevents possible future mistakes. If this isn't desirable, please feel free to drop the section.

v2: Fixed missing @bh in submit_bh_blkcg_css() call.

Signed-off-by Tejun Heo <[email protected]>

Cc Chris Mason <[email protected]>

Cc Josef Bacik <[email protected]>

---
 fs/btrfs/check-integrity.c | 2 +-
 fs/btrfs/disk-io.c         | 4 ++++
 fs/btrfs/ioctl.c           | 4 +++-
 3 files changed, 8 insertions(+), 2 deletions(-)

fs/btrfs/check-integrity.c +1 -1

index 7d5a9b5..d66774e 100644
@@ -2741,7 +2741,7 @@ int btrfsic_submit_bh(int op, int op_flags, struct buffer_head *bh)
 	struct btrfsic_dev_state *dev_state;
 
 	if (!btrfsic_is_initialized)
-		return submit_bh(op, op_flags, bh);
+		return submit_bh_blkcg_css(op, op_flags, bh, blkcg_root_css);
 
 	mutex_lock(&btrfsic_mutex);
 	/* since btrfsic_submit_bh() might also be called before

fs/btrfs/disk-io.c +4 -0

index dfdab84..fe8bbe1 100644
@@ -1025,6 +1025,8 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
 	int async = check_async_write(bio_flags);
 	blk_status_t ret;
 
+	bio_associate_blkcg(bio, blkcg_root_css);
+
 	if (bio_op(bio) != REQ_OP_WRITE) {
 		/*
 		 * called for a read, do the setup so that checksum validation

@@ -3512,6 +3514,8 @@ static void write_dev_flush(struct btrfs_device *device)
 		return;
 
 	bio_reset(bio);
+	bio_associate_blkcg(bio, blkcg_root_css);
+
 	bio->bi_end_io = btrfs_end_empty_barrier;
 	bio_set_dev(bio, device->bdev);
 	bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH;

fs/btrfs/ioctl.c +0 -0

index 117cc63..8a7db6c 100644

fs/btrfs/ioctl.c +0 -0

 	set_mask_bits(&inode->i_flags,
 		      S_SYNC | S_APPEND | S_IMMUTABLE | S_NOATIME | S_DIRSYNC |

The comment is useful, but the condition will be always true, so I don't see the point.

	/*
	 * The btree_inode will be always in the root cgroup. The cgroup
	 * writeback can be enabled on regular inodes selectively.
	 */
	new_fl |= S_CGROUPWB;

is IMHO enough, based on my reading of patch 2/5 changelog.

Issuing metdata or otherwise shared IOs from !root cgroup can lead to priority inversion. This patch ensures that those IOs are always issued from the root cgroup.

v3: Dropped unnecessary btree_inode handling as suggested by David
Sterba.

v2: Fixed missing @bh in submit_bh_blkcg_css() call.

Signed-off-by Tejun Heo <[email protected]>

Reviewed-by Liu Bo <[email protected]>

Cc David Sterba <[email protected]>

Changes from the last version are

* blkcg_root_css exported to fix build breakage on modular btrfs.

* Use ext4_should_journal_data() test instead of
EXT4_MOUNT_JOURNAL_DATA.

* Separated out create_bh_bio() and used it to implement submit_bh_blkcg_css() as suggested by Jan.

btrfs has different ways to issue metadata IOs and may end up issuing metadata or otherwise shared IOs from a non-root cgroup, which can lead to priority inversion and ineffective IO isolation.

This patchset makes sure that btrfs issues all metadata and shared IOs from the root cgroup by exempting btree_inodes from cgroup writeback and explicitly associating shared IOs with the root cgroup.

This patchset containst he following three patches

[PATCH 1/5] blkcg: export blkcg_root_css
[PATCH 2/5] cgroup, writeback: replace SB_I_CGROUPWB with per-inode [PATCH 3/5] buffer_head: separate out create_bh_bio() from [PATCH 4/5] cgroup, buffer_head: implement submit_bh_blkcg_css() [PATCH 5/5] btrfs: ensure that metadata and flush are issued from the

and is also available in the following git branch

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup-btrfs-metadata-v2

diffstat follows. Thanks.

 block/blk-cgroup.c          |    1 +
 fs/block_dev.c              |    3 +--
 fs/btrfs/check-integrity.c  |    2 +-
 fs/btrfs/disk-io.c          |    4 ++++
 fs/btrfs/ioctl.c            |    6 +++++-
 fs/btrfs/super.c            |    1 -
 fs/buffer.c                 |   42 ++++++++++++++++++++++++++++++++++--------
 fs/ext2/inode.c             |    3 ++-
 fs/ext2/super.c             |    1 -
 fs/ext4/inode.c             |    5 ++++-
 fs/ext4/super.c             |    2 --
 include/linux/backing-dev.h |    2 +-
 include/linux/buffer_head.h |    3 +++
 include/linux/fs.h          |    3 ++-
 14 files changed, 58 insertions(+), 20 deletions(-)

--
tejun

What has happened with this patch set?

Hello,

Quoted 1 line · Jan Kara

What has happened with this patch set?

No idea. cc'ing Chris directly. Chris, if the patchset looks good, can you please route them through the btrfs tree?

Quoted 3 lines · Tejun Heo

Hello,

On Wed, Nov 29, 2017 at 05:56:08PM +0100, Jan Kara wrote:

Quoted 4 lines

Quoted 1 line

What has happened with this patch set?

No idea. cc'ing Chris directly. Chris, if the patchset looks good, can you please route them through the btrfs tree?

lol looking at the patchset again, I'm not sure that's obviously the right tree. It can either be cgroup, block or btrfs. If no one objects, I'll just route them through cgroup.

Quoted 9 lines · Tejun Heo

Quoted 7 lines · Tejun Heo

Hello,

Quoted 1 line · Jan Kara

What has happened with this patch set?

No idea. cc'ing Chris directly. Chris, if the patchset looks good, can you please route them through the btrfs tree?

Quoted 3 lines

lol looking at the patchset again, I'm not sure that's obviously the right tree. It can either be cgroup, block or btrfs. If no one objects, I'll just route them through cgroup.

We'll have to coordinate a bit during the next merge window but I don't have a problem with these going in through cgroup. Dave does this sound good to you?

I'd like to include my patch to do all crcs inline (instead of handing off to helper threads) when io controls are in place. By the merge window we should have some good data on how much it's all helping.

Quoted 10 lines · Chris Mason

Quoted 9 lines · Tejun Heo

Quoted 7 lines · Tejun Heo

Hello,

Quoted 1 line · Jan Kara

What has happened with this patch set?

No idea. cc'ing Chris directly. Chris, if the patchset looks good, can you please route them through the btrfs tree?

Quoted 7 lines

Quoted 3 lines

lol looking at the patchset again, I'm not sure that's obviously the right tree. It can either be cgroup, block or btrfs. If no one objects, I'll just route them through cgroup.

We'll have to coordinate a bit during the next merge window but I don't have a problem with these going in through cgroup. Dave does this sound good to you?

There are only minor changes to btrfs code so cgroup tree would be better.

Quoted 3 lines

I'd like to include my patch to do all crcs inline (instead of handing off to helper threads) when io controls are in place. By the merge window we should have some good data on how much it's all helping.

Are there any problems in sight if the inline crc and cgroup chnanges go separately? I assume there's a runtime dependency, not a code dependency, so it could be sorted by the right merge order.

Quoted 9 lines · Tejun Heo

Quoted 7 lines · Tejun Heo

Hello,

Quoted 1 line · Jan Kara

What has happened with this patch set?

No idea. cc'ing Chris directly. Chris, if the patchset looks good, can you please route them through the btrfs tree?

Quoted 3 lines

lol looking at the patchset again, I'm not sure that's obviously the right tree. It can either be cgroup, block or btrfs. If no one objects, I'll just route them through cgroup.

We'll have to coordinate a bit during the next merge window but I don't have a problem with these going in through cgroup. Dave does this sound good to you?