block: make loop block device cgroup aware
Subsystems (3)
cover block: make loop block device cgroup aware
From: Shaohua Li <[email protected]>
Hi,
The IO dispatched to under layer disk by loop block device isn't cloned from original bio, so the IO loses cgroup information of original bio. These IO escapes from cgroup control. The patches try to address this issue. The idea is quite generic, but we currently only make it work for blkcg.
Thanks,
Shaohua
Shaohua Li (3):
kthread: add a mechanism to store cgroup info
block: make blkcg aware of kthread stored original cgroup info block/loop: make loop cgroup aware
block/bio.c | 31 ------------------------- drivers/block/loop.c | 13 +++++++++++ drivers/block/loop.h | 1 + include/linux/bio.h | 2 -- include/linux/blk-cgroup.h | 25 ++++++-------------- include/linux/kthread.h | 13 +++++++++++ include/linux/sched.h | 1 + kernel/kthread.c | 57 +++++++++++++++++++++++++++++++++++++++++++++- 8 files changed, 91 insertions(+), 52 deletions(-)
Signature · 1 line
2.9.5
PATCH 1/3 kthread: add a mechanism to store cgroup info
From: Shaohua Li <[email protected]>
kthread usually runs jobs on behalf of other threads. The jobs should be charged to cgroup of original threads. But the jobs run in a kthread, where we lose the cgroup context of original threads. The patch adds a machanism to record cgroup info of original threads in kthread context. Later we can retrieve the cgroup info and attach the cgroup info to jobs.
Since this mechanism is only required by kthread, we store the cgroup info in kthread data instead of generic task_struct.
--- include/linux/kthread.h | 13 +++++++++++ include/linux/sched.h | 1 + kernel/kthread.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 70 insertions(+), 1 deletion(-)
include/linux/kthread.h +13 -0
@@ -3,6 +3,7 @@/* Simple interface for creating and stopping kernel threads without mess. */ #include <linux/err.h> #include <linux/sched.h> +#include <linux/cgroup.h> __printf(4, 5) struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
@@ -198,4 +199,16 @@ bool kthread_cancel_delayed_work_sync(struct kthread_delayed_work *work);void kthread_destroy_worker (struct kthread_worker *worker); +#ifdef CONFIG_CGROUPS +void kthread_set_orig_css (struct cgroup_subsys_state *css); +struct cgroup_subsys_state *kthread_get_orig_css (void ); +void kthread_reset_orig_css (void ); +#else +static inline void kthread_set_orig_css (struct cgroup_subsys_state *css) { } +static inline struct cgroup_subsys_state *kthread_get_orig_css (void ) +{ +return NULL ; +} +static inline void kthread_reset_orig_css (void ) { } +#endif #endif /* _LINUX_KTHREAD_H */
include/linux/sched.h +1 -0
@@ -1276,6 +1276,7 @@ extern struct pid *cad_pid;#define PF_SWAPWRITE 0x00800000/* Allowed to write to swap */ #define PF_NO_SETAFFINITY 0x04000000/* Userland is not allowed to meddle with cpus_allowed */ #define PF_MCE_EARLY 0x08000000/* Early kill for mce process policy */ +#define PF_KTHREAD_HAS_CSS 0x10000000/* kthread has css info attached */ #define PF_MUTEX_TESTER 0x20000000/* Thread belongs to the rt mutex tester */ #define PF_FREEZER_SKIP 0x40000000/* Freezer should not count it as freezable */ #define PF_SUSPEND_TASK 0x80000000/* This thread called freeze_processes() and should not be frozen */
kernel/kthread.c +56 -1
72 lines not on this page · raw message
Signature · 1 line
2.9.5
Tejun Heo
maintainerRe: [PATCH 1/3] kthread: add a mechanism to store cgroup info
Hello,
include/linux/kthread.h bool kthread_cancel_delayed_work_sync(struct kthread_delayed_work *work); · line 205
Quoted 4 lines · Shaohua Li
include/linux/kthread.h +4
-0+#ifdef CONFIG_CGROUPS +void kthread_set_orig_css (struct cgroup_subsys_state *css); +struct cgroup_subsys_state *kthread_get_orig_css (void ); +void kthread_reset_orig_css (void );
* It's a bit weird to associate a kthread with a css without being specific. If what's needed is a generic association (this kthread is temporarily servicing this cgroup), it should be associated with the cgroup. But, I think it'd be better to make it specific instead
- ie. kthread_set_io_css().
* Do we need the reset function to be separate? Can't we just clear it when the set function is called with NULL css?
* For the accessor, can we do sth like kthread_orig_css() (or kthread_io_css())? "get" is overloaded between set/get and get/put, so it can get confusing.
include/linux/sched.h extern struct pid *cad_pid; · line 1279
Quoted 9 lines
include/linux/sched.h +1
-0@@ -1276,6 +1276,7 @@ extern struct pid *cad_pid;#define PF_SWAPWRITE 0x00800000/* Allowed to write to swap */ #define PF_NO_SETAFFINITY 0x04000000/* Userland is not allowed to meddle with cpus_allowed */ #define PF_MCE_EARLY 0x08000000/* Early kill for mce process policy */ +#define PF_KTHREAD_HAS_CSS 0x10000000/* kthread has css info attached */
Do we need a separate flag for this? kthreads already have PF_KTHREAD set. I'm not sure why we'd need another flag. Once we know it's a kthread, we can just access its css pointer field, right?
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerRe: [PATCH 1/3] kthread: add a mechanism to store cgroup info
Quoted 5 lines · Tejun Heo
* It's a bit weird to associate a kthread with a css without being specific. If what's needed is a generic association (this kthread is temporarily servicing this cgroup), it should be associated with the cgroup. But, I think it'd be better to make it specific instead
- ie.kthread_set_io_css().
kthread[_set]_blkcg_css() probably is a more consistent name.
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerRe: [PATCH 1/3] kthread: add a mechanism to store cgroup info
Quoted 8 lines · Tejun Heo
Quoted 5 lines · Tejun Heo
* It's a bit weird to associate a kthread with a css without being specific. If what's needed is a generic association (this kthread is temporarily servicing this cgroup), it should be associated with the cgroup. But, I think it'd be better to make it specific instead
- ie.kthread_set_io_css().kthread[_set]_blkcg_css() probably is a more consistent name.
Sorry, I meant kthread[_associate]_blkcg() so that it's consistent with bio[_associate]_blkcg().
Thanks.
Signature · 1 line
tejun
Re: [PATCH 1/3] kthread: add a mechanism to store cgroup info
In reply to Tejun HeoQuoted 21 lines · Tejun Heo
Hello,
include/linux/kthread.h bool kthread_cancel_delayed_work_sync(struct kthread_delayed_work *work); · line 205
Quoted 4 lines · Shaohua Li
include/linux/kthread.h +4
-0+#ifdef CONFIG_CGROUPS +void kthread_set_orig_css (struct cgroup_subsys_state *css); +struct cgroup_subsys_state *kthread_get_orig_css (void ); +void kthread_reset_orig_css (void );* It's a bit weird to associate a kthread with a css without being specific. If what's needed is a generic association (this kthread is temporarily servicing this cgroup), it should be associated with the cgroup. But, I think it'd be better to make it specific instead
- ie.kthread_set_io_css().* Do we need the reset function to be separate? Can't we just clear it when the set function is called with NULL css?
* For the accessor, can we do sth like
kthread_orig_css()(orkthread_io_css())? "get" is overloaded between set/get and get/put, so it can get confusing.
Quoted 13 lines
include/linux/sched.h extern struct pid *cad_pid; · line 1279
Quoted 9 lines
include/linux/sched.h +1
-0@@ -1276,6 +1276,7 @@ extern struct pid *cad_pid;#define PF_SWAPWRITE 0x00800000/* Allowed to write to swap */ #define PF_NO_SETAFFINITY 0x04000000/* Userland is not allowed to meddle with cpus_allowed */ #define PF_MCE_EARLY 0x08000000/* Early kill for mce process policy */ +#define PF_KTHREAD_HAS_CSS 0x10000000/* kthread has css info attached */ Do we need a separate flag for this? kthreads already have
PF_KTHREADset. I'm not sure why we'd need another flag. Once we know it's a kthread, we can just access its css pointer field, right?
Ok, all suggestions are good, will update.
Thanks,
Shaohua
PATCH 2/3 block: make blkcg aware of kthread stored original cgroup info
In reply to Shaohua LiFrom: Shaohua Li <[email protected]>
Several blkcg APIs are deprecated. After removing them, bio_blkcg is the only API to get cgroup info for a bio. If bio_blkcg finds current task is a kthread and has original css recorded, it will use the css instead of associating the bio to current task.
--- block/bio.c | 31 ------------------------------- include/linux/bio.h | 2 -- include/linux/blk-cgroup.h | 25 +++++++------------------ 3 files changed, 7 insertions(+), 51 deletions(-)
block/bio.c +0 -31
@@ -2033,37 +2033,6 @@ int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css)EXPORT_SYMBOL_GPL (bio_associate_blkcg);/** -* bio_associate_current - associate a bio with %current -* @bio: target bio -* -* Associate @bio with %current if it hasn't been associated yet. Block -* layer will treat @bio as if it were issued by %current no matter which -* task actually issues it. -* -* This function takes an extra reference of @task's io_context and blkcg -* which will be put when @bio is released. The caller must own @bio, -* ensure %current->io_context exists, and is responsible for synchronizing -* calls to this function. -*/ -int bio_associate_current (struct bio *bio) -{ -struct io_context *ioc; - -if (bio->bi_css) -return -EBUSY ; - - ioc = current->io_context; -if (!ioc) -return -ENOENT ; - -get_io_context_active (ioc); - bio->bi_ioc = ioc; - bio->bi_css =task_get_css (current, io_cgrp_id); -return 0 ; -} -EXPORT_SYMBOL_GPL (bio_associate_current); - -/** * bio_disassociate_task - undo bio_associate_current() * @bio: target bio */
include/linux/bio.h +0 -2
@@ -514,13 +514,11 @@ do { \#ifdef CONFIG_BLK_CGROUP int bio_associate_blkcg (struct bio *bio,struct cgroup_subsys_state *blkcg_css); -int bio_associate_current (struct bio *bio);void bio_disassociate_task (struct bio *bio);void bio_clone_blkcg_association (struct bio *dst,struct bio *src);#else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg (struct bio *bio,struct cgroup_subsys_state *blkcg_css) {return 0 ; } -static inline int bio_associate_current (struct bio *bio) {return -ENOENT ; }static inline void bio_disassociate_task (struct bio *bio) { }static inline void bio_clone_blkcg_association (struct bio *dst,struct bio *src) { }
include/linux/blk-cgroup.h +7 -18
@@ -19,6 +19,7 @@#include <linux/radix-tree.h> #include <linux/blkdev.h> #include <linux/atomic.h> +#include <linux/kthread.h> /* percpu_counter batch for blkg_[rw]stats, per-cpu drift doesn't matter */ #define BLKG_STAT_CPU_BATCH (INT_MAX / 2)
@@ -223,22 +224,16 @@ static inline struct blkcg *css_to_blkcg(struct cgroup_subsys_state *css) return css ? container_of(css, struct blkcg, css) : NULL; } -static inline struct blkcg *task_blkcg (struct task_struct *tsk) -{ -return css_to_blkcg (task_css (tsk, io_cgrp_id)); -} -static inline struct blkcg *bio_blkcg (struct bio *bio) { +struct cgroup_subsys_state *css; +if (bio && bio->bi_css)return css_to_blkcg (bio->bi_css); -return task_blkcg (current); -} - -static inline struct cgroup_subsys_state * -task_get_blkcg_css (struct task_struct *task) -{ -return task_get_css (task, io_cgrp_id); + css =kthread_get_orig_css (); +if (css) +return css_to_blkcg (css); +return css_to_blkcg (task_css (current, io_cgrp_id)); }/**
@@ -735,12 +730,6 @@ struct blkcg_policy {#define blkcg_root_css ((struct cgroup_subsys_state *)ERR_PTR(-EINVAL)) -static inline struct cgroup_subsys_state * -task_get_blkcg_css (struct task_struct *task) -{ -return NULL ; -} -#ifdef CONFIG_BLOCK static inline struct blkcg_gq *blkg_lookup (struct blkcg *blkcg,void *key) {return NULL ; }
Signature · 1 line
2.9.5
Tejun Heo
maintainerRe: [PATCH 2/3] block: make blkcg aware of kthread stored original cgroup info
Hello,
Quoted 6 lines · Shaohua Li
From: Shaohua Li <[email protected]>
Several blkcg APIs are deprecated. After removing them,
bio_blkcgis the only API to get cgroup info for a bio. Ifbio_blkcgfinds current task is a kthread and has original css recorded, it will use the css instead of associating the bio to current task.
Can you please separate out removal of unused APIs from modifications of existing ones? The patch is a bit confusing to follow.
Thanks.
Signature · 1 line
tejun
PATCH 3/3 block/loop: make loop cgroup aware
In reply to Shaohua LiFrom: Shaohua Li <[email protected]>
loop block device handles IO in a separate thread. The actual IO dispatched isn't cloned from the IO loop device received, so the dispatched IO loses the cgroup context.
I'm ignoring buffer IO case now, which is quite complicated. Making the loop thread aware cgroup context doesn't really help. The loop device only writes to a single file. In current writeback cgroup implementation, the file can only belong to one cgroup.
For direct IO case, we could workaround the issue in theory. For example, say we assign cgroup1 5M/s BW for loop device and cgroup2 10M/s. We can create a special cgroup for loop thread and assign at least 15M/s for the underlayer disk. In this way, we correctly throttle the two cgroups. But this is tricky to setup.
This patch tries to address the issue. We record bio's css in loop command. When loop thread is handling the command, we then use the API provided in patch 1 to set the css for current task. The bio layer will use the css for new IO.
--- drivers/block/loop.c | 13 +++++++++++++ drivers/block/loop.h | 1 + 2 files changed, 14 insertions(+)
drivers/block/loop.c +13 -0
@@ -482,6 +482,8 @@ static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2) { struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb); +if (cmd->css) +css_put (cmd->css); cmd->ret = ret >0 ?0 : ret;lo_rw_aio_do_completion (cmd); }
@@ -541,6 +543,8 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, cmd->iocb.ki_filp = file; cmd->iocb.ki_complete = lo_rw_aio_complete; cmd->iocb.ki_flags =IOCB_DIRECT ; +if (cmd->css) +kthread_set_orig_css (cmd->css);if (rw ==WRITE ) ret =call_write_iter (file, &cmd->iocb, &iter);
@@ -548,6 +552,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, ret =call_read_iter (file, &cmd->iocb, &iter);lo_rw_aio_do_completion (cmd); +kthread_reset_orig_css ();if (ret != -EIOCBQUEUED ) cmd->iocb.ki_complete (&cmd->iocb, ret,0 );
@@ -1692,6 +1697,14 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,break ; } +/* always use the first bio's css */ +#ifdef CONFIG_CGROUPS +if (cmd->use_aio && cmd->rq->bio && cmd->rq->bio->bi_css) { + cmd->css = cmd->rq->bio->bi_css; +css_get (cmd->css); + } else +#endif + cmd->css =NULL ;kthread_queue_work (&lo->worker, &cmd->work);return BLK_STS_OK ;
drivers/block/loop.h +1 -0
@@ -74,6 +74,7 @@ struct loop_cmd {long ret;struct kiocb iocb;struct bio_vec *bvec; +struct cgroup_subsys_state *css; };/* Support for loadable transfer modules */
Signature · 1 line
2.9.5
Tejun Heo
maintainerRe: [PATCH 3/3] block/loop: make loop cgroup aware
Hello,
drivers/block/loop.c static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2) · line 488
Quoted 12 lines · Shaohua Li
drivers/block/loop.c +2
-0@@ -482,6 +482,8 @@ static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2) { struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb); +if (cmd->css) +css_put (cmd->css); cmd->ret = ret >0 ?0 : ret;lo_rw_aio_do_completion (cmd);
The fact that we're forwarding explicitly in loop still bothers me a bit. Can you please elaborate why we don't want to do this generically through aio?
Thanks.
Signature · 1 line
tejun
Re: [PATCH 3/3] block/loop: make loop cgroup aware
Quoted 11 lines · Tejun Heo
Hello,
Quoted 8 lines · Shaohua Li
drivers/block/loop.c +0
-0@@ -482,6 +482,8 @@ static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2) { struct loop_cmd *cmd = container_of(iocb, struct loop_cmd, iocb);
Quoted 8 lines
drivers/block/loop.c static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2) · line 488
Quoted 4 lines
drivers/block/loop.c +2
-0+if (cmd->css) +css_put (cmd->css); cmd->ret = ret >0 ?0 : ret;lo_rw_aio_do_completion (cmd);The fact that we're forwarding explicitly in loop still bothers me a bit. Can you please elaborate why we don't want to do this generically through aio?
I think we must forward in loop, because each cmd could come from different cgroup, so we must explicitly forward for each cmd.
The main reason not to do the forward in aio is complexity. We at least have 3 different implementations for dio:
- __blockdev_direct_IO for ext4 and btrfs
- iomap dio for xfs
- blockdev dio implementation
Forwarding in dio means hooking the cgroup association for each bio dispatched in the implementations, which is a little messy. I'd like to avoid this if there is no strong reason to do it.
Thanks,
Shaohua
Tejun Heo
maintainerRe: [PATCH 3/3] block/loop: make loop cgroup aware
Hello, Shaohua.
Quoted 13 lines · Shaohua Li
Quoted 3 lines
The fact that we're forwarding explicitly in loop still bothers me a bit. Can you please elaborate why we don't want to do this generically through aio?
I think we must forward in loop, because each cmd could come from different cgroup, so we must explicitly forward for each cmd.
The main reason not to do the forward in aio is complexity. We at least have 3 different implementations for dio:
-__blockdev_direct_IOfor ext4 and btrfs
- iomap dio for xfs
- blockdev dio implementation
Quoted 3 lines
Forwarding in dio means hooking the cgroup association for each bio dispatched in the implementations, which is a little messy. I'd like to avoid this if there is no strong reason to do it.
I see. I think the important questions is whether we're failing to forward io cgroup membership propagation on some aios? If we are, that is an obvious bug which should be addressed one way or the other, and there's a fair chance that we wouldn't need to do anything special for loop.
Given how simple the loop changes are, we sure can go with loop specific changes for now; however, I'm a bit unconvinced that aio changes would be that much more complex. Can you please look into it? If it is actually complex, we sure can do it later but I'd much prefer to plug the hole as soon as possible.
Thanks.
Signature · 1 line
tejun