[PATCH RFC 0/4] dm thin: support blk-throttle on data and metadata device

[PATCH RFC 0/4] dm thin: support blk-throttle on data and metadata device

[Hou Tao]

Hi all,

We need to throttle the O_DIRECT IO on data and metadata device
of a dm-thin pool and encounter some problems. If we set the
limitation on the root blkcg, the throttle works. If we set the
limitation on a child blkcg, the throttle doesn't work well.

The reason why the throttle doesn't work is that dm-thin defers
the process of bio when the physical block of bio has not been
allocated. The bio will be submitted by the pool worker, and the
blkcg of the bio will be the blkcg of the pool worker, namely,
the root blkcg instead of the blkcg of the original IO thread.
We only set a limitation on the blkcg of the original IO thread,
so the blk-throttle doesn't work well.

In order to handle the situation, we add a "keep_bio_blkcg" feature
to dm-thin. If the feature is enabled, the original blkcg of bio
will be saved at thin_map() and will be used during blk-throttle.

Tao

Hou Tao (4):
  dm thin: add a pool feature "keep_bio_blkcg"
  dm thin: parse "keep_bio_blkcg" from userspace tools
  dm thin: show the enabled status of keep_bio_blkcg feature
  dm thin: associate bio with current task if keep_bio_blkcg is enabled

 drivers/md/dm-thin.c | 26 ++++++++++++++++++++++++--
 drivers/md/dm-thin.h | 17 +++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 drivers/md/dm-thin.h

-- 
2.5.0

[PATCH RFC 1/4] dm thin: add a pool feature "keep_bio_blkcg"

[Hou Tao]

"keep_bio_blkcg" is used to control whether or not
dm-thin needs to save the original blkcg of bio

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-thin.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index d1c05c1..8178ee8 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -211,6 +211,7 @@ struct pool_features {
 	bool discard_enabled:1;
 	bool discard_passdown:1;
 	bool error_if_no_space:1;
+	bool keep_bio_blkcg:1;
 };
 
 struct thin_c;
@@ -277,6 +278,7 @@ struct pool {
 
 static enum pool_mode get_pool_mode(struct pool *pool);
 static void metadata_operation_failed(struct pool *pool, const char *op, int r);
+static bool keep_pool_bio_blkcg(struct pool *pool);
 
 /*
  * Target context for a pool.
@@ -2390,6 +2392,11 @@ static void noflush_work(struct thin_c *tc, void (*fn)(struct work_struct *))
 
 /*----------------------------------------------------------------*/
 
+static bool keep_pool_bio_blkcg(struct pool *pool)
+{
+	return pool->pf.keep_bio_blkcg;
+}
+
 static enum pool_mode get_pool_mode(struct pool *pool)
 {
 	return pool->pf.mode;
-- 
2.5.0

[PATCH RFC 2/4] dm thin: parse "keep_bio_blkcg" from userspace tools

[Hou Tao]

keep_bio_blkcg feature is off by default, and it can
be turned on by using "keep_bio_blkcg" argument.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-thin.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 8178ee8..57d6202 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -2824,6 +2824,7 @@ static void pool_features_init(struct pool_features *pf)
 	pf->discard_enabled = true;
 	pf->discard_passdown = true;
 	pf->error_if_no_space = false;
+	pf->keep_bio_blkcg = false;
 }
 
 static void __pool_destroy(struct pool *pool)
@@ -3053,7 +3054,7 @@ static int parse_pool_features(struct dm_arg_set *as, struct pool_features *pf,
 	const char *arg_name;
 
 	static struct dm_arg _args[] = {
-		{0, 4, "Invalid number of pool feature arguments"},
+		{0, 5, "Invalid number of pool feature arguments"},
 	};
 
 	/*
@@ -3085,6 +3086,9 @@ static int parse_pool_features(struct dm_arg_set *as, struct pool_features *pf,
 		else if (!strcasecmp(arg_name, "error_if_no_space"))
 			pf->error_if_no_space = true;
 
+		else if (!strcasecmp(arg_name, "keep_bio_blkcg"))
+			pf->keep_bio_blkcg = true;
+
 		else {
 			ti->error = "Unrecognised pool feature requested";
 			r = -EINVAL;
-- 
2.5.0

[PATCH RFC 3/4] dm thin: show the enabled status of keep_bio_blkcg feature

[Hou Tao]

If keep_bio_blkcg feature is enabled, we can ensure that
by STATUSTYPE_TABLE or STATUSTYPE_INFO command.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-thin.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 57d6202..140cdae 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -3760,7 +3760,7 @@ static void emit_flags(struct pool_features *pf, char *result,
 {
 	unsigned count = !pf->zero_new_blocks + !pf->discard_enabled +
 		!pf->discard_passdown + (pf->mode == PM_READ_ONLY) +
-		pf->error_if_no_space;
+		pf->error_if_no_space + pf->keep_bio_blkcg;
 	DMEMIT("%u ", count);
 
 	if (!pf->zero_new_blocks)
@@ -3777,6 +3777,9 @@ static void emit_flags(struct pool_features *pf, char *result,
 
 	if (pf->error_if_no_space)
 		DMEMIT("error_if_no_space ");
+
+	if (pf->keep_bio_blkcg)
+		DMEMIT("keep_bio_blkcg ");
 }
 
 /*
@@ -3885,6 +3888,9 @@ static void pool_status(struct dm_target *ti, status_type_t type,
 		else
 			DMEMIT("queue_if_no_space ");
 
+		if (pool->pf.keep_bio_blkcg)
+			DMEMIT("keep_bio_blkcg ");
+
 		if (dm_pool_metadata_needs_check(pool->pmd))
 			DMEMIT("needs_check ");
 		else
-- 
2.5.0

[PATCH RFC 4/4] dm thin: associate bio with current task if keep_bio_blkcg is enabled

[Hou Tao]

If keep_bio_blkcg is enabled, assign the io_context and the blkcg of
current task to bio before processing the bio.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-thin.c |  5 +++++
 drivers/md/dm-thin.h | 17 +++++++++++++++++
 2 files changed, 22 insertions(+)
 create mode 100644 drivers/md/dm-thin.h

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 140cdae..0efbdbe 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -22,6 +22,8 @@
 #include <linux/sort.h>
 #include <linux/rbtree.h>
 
+#include "dm-thin.h"
+
 #define	DM_MSG_PREFIX	"thin"
 
 /*
@@ -2629,6 +2631,9 @@ static int thin_bio_map(struct dm_target *ti, struct bio *bio)
 	struct dm_bio_prison_cell *virt_cell, *data_cell;
 	struct dm_cell_key key;
 
+	if (keep_pool_bio_blkcg(tc->pool))
+		thin_keep_bio_blkcg(bio);
+
 	thin_hook_bio(tc, bio);
 
 	if (tc->requeue_mode) {
diff --git a/drivers/md/dm-thin.h b/drivers/md/dm-thin.h
new file mode 100644
index 0000000..09e920a
--- /dev/null
+++ b/drivers/md/dm-thin.h
@@ -0,0 +1,17 @@
+#ifndef DM_THIN_H
+#define DM_THIN_H
+
+#include <linux/bio.h>
+
+#ifdef CONFIG_BLK_CGROUP
+static inline void thin_keep_bio_blkcg(struct bio *bio)
+{
+	if (!bio->bi_css)
+		bio_associate_current(bio);
+}
+#else
+static inline void thin_keep_bio_blkcg(struct bio *bio) {}
+#endif /* CONFIG_BLK_CGROUP */
+
+#endif
+
-- 
2.5.0

Re: [PATCH RFC 0/4] dm thin: support blk-throttle on data and metadata device

[Jeff Moyer]

Hou Tao <houtao1@huawei.com> writes:

> Hi all,
>
> We need to throttle the O_DIRECT IO on data and metadata device
> of a dm-thin pool and encounter some problems. If we set the
> limitation on the root blkcg, the throttle works. If we set the
> limitation on a child blkcg, the throttle doesn't work well.
>
> The reason why the throttle doesn't work is that dm-thin defers
> the process of bio when the physical block of bio has not been
> allocated. The bio will be submitted by the pool worker, and the
> blkcg of the bio will be the blkcg of the pool worker, namely,
> the root blkcg instead of the blkcg of the original IO thread.
> We only set a limitation on the blkcg of the original IO thread,
> so the blk-throttle doesn't work well.
>
> In order to handle the situation, we add a "keep_bio_blkcg" feature
> to dm-thin. If the feature is enabled, the original blkcg of bio
> will be saved at thin_map() and will be used during blk-throttle.

Why is this even an option?  I would think that you would always want
this behavior.

-Jeff

Re: [PATCH RFC 0/4] dm thin: support blk-throttle on data and metadata device

[Mike Snitzer]

On Fri, Jan 20 2017 at 10:19am -0500,
Jeff Moyer <jmoyer@redhat.com> wrote:

> Hou Tao <houtao1@huawei.com> writes:
> 
> > Hi all,
> >
> > We need to throttle the O_DIRECT IO on data and metadata device
> > of a dm-thin pool and encounter some problems. If we set the
> > limitation on the root blkcg, the throttle works. If we set the
> > limitation on a child blkcg, the throttle doesn't work well.
> >
> > The reason why the throttle doesn't work is that dm-thin defers
> > the process of bio when the physical block of bio has not been
> > allocated. The bio will be submitted by the pool worker, and the
> > blkcg of the bio will be the blkcg of the pool worker, namely,
> > the root blkcg instead of the blkcg of the original IO thread.
> > We only set a limitation on the blkcg of the original IO thread,
> > so the blk-throttle doesn't work well.
> >
> > In order to handle the situation, we add a "keep_bio_blkcg" feature
> > to dm-thin. If the feature is enabled, the original blkcg of bio
> > will be saved at thin_map() and will be used during blk-throttle.
> 
> Why is this even an option?  I would think that you would always want
> this behavior.

Right, shouldn't be an optional feature.

Also, this implementation is very dm-thin specific.  I still have this
line of work on my TODO because there should be a more generic way to
wire up these associations in either block core or DM core.

Now that there is both dm-crypt and dm-thin specific RFC patches to fix
this I'll see about finding a solution that works for both but that is
more generic.

Not sure how quickly I'll get to this but I'll do my best.

Re: [PATCH RFC 0/4] dm thin: support blk-throttle on data and metadata device

[Vivek Goyal]

On Fri, Jan 20, 2017 at 10:19:22AM -0500, Jeff Moyer wrote:
> Hou Tao <houtao1@huawei.com> writes:
> 
> > Hi all,
> >
> > We need to throttle the O_DIRECT IO on data and metadata device
> > of a dm-thin pool and encounter some problems. If we set the
> > limitation on the root blkcg, the throttle works. If we set the
> > limitation on a child blkcg, the throttle doesn't work well.
> >
> > The reason why the throttle doesn't work is that dm-thin defers
> > the process of bio when the physical block of bio has not been
> > allocated. The bio will be submitted by the pool worker, and the
> > blkcg of the bio will be the blkcg of the pool worker, namely,
> > the root blkcg instead of the blkcg of the original IO thread.
> > We only set a limitation on the blkcg of the original IO thread,
> > so the blk-throttle doesn't work well.
> >
> > In order to handle the situation, we add a "keep_bio_blkcg" feature
> > to dm-thin. If the feature is enabled, the original blkcg of bio
> > will be saved at thin_map() and will be used during blk-throttle.
> 
> Why is this even an option?  I would think that you would always want
> this behavior.

Agreed that this should not be an optional feature.

One thing which always bothers me is that often we have dependencies
on finishing of these bios. So if we assign bios to their original
cgroups and if we create a cgroup with very low limit, then its
possible that everything slows down.

I guess answer to that is its a configuration issue and don't
create groups with too small a limits. And don't allow unprivileged
users to specify group limits.

Vivek