[PULL] bcache fixes and updates for-4.13

[PULL] bcache fixes and updates for-4.13

[Eric Wheeler]

Hi Jens,

It looks like you're getting ready for 4.13, so here are the bcache 
updates rebased and ready from git.kernel.dk/linux-block for-4.13/block 
This tree builds, boots, and works as expected.  Please pull the new fixes 
and features below.

Thank you for your help!


The following changes since commit f1d4ef7d88832444e8dfeb0e85e19d3b6ecb5011:

  nvmet-rdma: register ib_client to not deadlock in device removal (2017-06-28 08:14:13 -0600)

are available in the git repository at:

  https://bitbucket.org/ewheelerinc/linux.git bcache-updates-linux-block-for-4.13

for you to fetch changes up to 960ce81bdbd46bc7faaeb2f9fcddfea1e48ee388:

  bcache: Update continue_at() documentation (2017-06-28 15:18:02 -0700)

----------------------------------------------------------------
Dan Carpenter (2):
      bcache: silence static checker warning
      bcache: Update continue_at() documentation

Eric Wheeler (3):
      bcache: introduce bcache sysfs entries for ioprio-based bypass/writeback hints
      bcache: documentation for sysfs entries describing bcache cache hinting
      bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

Jan Kara (1):
      bcache: Fix leak of bdev reference

Liang Chen (1):
      bcache: explicitly destory mutex while exiting

Tang Junhui (11):
      bcache: fix sequential large write IO bypass
      bcache: do not subtract sectors_to_gc for bypassed IO
      bcache: fix wrong cache_misses statistics
      bcache: fix calling ida_simple_remove() with incorrect minor
      bcache: initialize stripe_sectors_dirty correctly for thin flash device
      bcache: Subtract dirty sectors of thin flash from cache_sectors in calculating writeback rate
      bcache: update bucket_in_use periodically
      bcache: delete redundant calling set_gc_sectors()
      bcache: fix issue of writeback rate at minimum 1 key per second
      bcache: increase the number of open buckets
      bcache: fix for gc and write-back race

Tony Asleson (1):
      bcache: Correct return value for sysfs attach errors

 Documentation/bcache.txt      | 80 ++++++++++++++++++++++++++++++++++++++++++
 drivers/md/bcache/alloc.c     |  4 ++-
 drivers/md/bcache/bcache.h    |  4 +++
 drivers/md/bcache/btree.c     | 30 ++++++++++++++--
 drivers/md/bcache/closure.h   |  4 ---
 drivers/md/bcache/request.c   | 45 ++++++++++++++++++------
 drivers/md/bcache/super.c     | 26 +++++++++-----
 drivers/md/bcache/sysfs.c     | 75 +++++++++++++++++++++++++++++++++++++--
 drivers/md/bcache/util.c      |  9 ++++-
 drivers/md/bcache/writeback.c | 37 +++++++++++++------
 drivers/md/bcache/writeback.h | 48 +++++++++++++++++++++++--
 11 files changed, 319 insertions(+), 43 deletions(-)



--
Eric Wheeler

Re: [PULL] bcache fixes and updates for-4.13

[Christoph Hellwig]

On Wed, Jun 28, 2017 at 11:06:46PM +0000, Eric Wheeler wrote:
> Hi Jens,
> 
> It looks like you're getting ready for 4.13, so here are the bcache 
> updates rebased and ready from git.kernel.dk/linux-block for-4.13/block 
> This tree builds, boots, and works as expected.  Please pull the new fixes 
> and features below.
> 
> Thank you for your help!

Can you please send all the patches to linux-block for review as
a series?  Some of these subjects sound a little suspect.

[PATCH 07/19] bcache: introduce bcache sysfs entries for ioprio-based bypass/writeback hints

[Eric Wheeler]

Add sysfs entries to support to hint for bypass/writeback by the ioprio
assigned to the bio.  If the bio is unassigned, use current's io-context
ioprio for cache writeback or bypass (configured per-process with
`ionice`).

Having idle IOs bypass the cache can increase performance elsewhere
since you probably don't care about their performance.  In addition,
this prevents idle IOs from promoting into (polluting) your cache and
evicting blocks that are more important elsewhere.

If you really nead the performance at the expense of SSD wearout,
then configure ioprio_writeback and set your `ionice` appropriately.

For example:
	echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
	echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback

See the documentation commit for details.

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
Acked-by: Kent Overstreet <kent.overstreet@gmail.com>
Tested-by: Kai Krakow <kai@kaishome.de>
Cc: nix@esperi.org.uk
---
 drivers/md/bcache/bcache.h    |  3 ++
 drivers/md/bcache/request.c   | 24 +++++++++++++++
 drivers/md/bcache/sysfs.c     | 71 +++++++++++++++++++++++++++++++++++++++++++
 drivers/md/bcache/writeback.c |  8 +++++
 drivers/md/bcache/writeback.h | 24 +++++++++++++++
 5 files changed, 130 insertions(+)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index dee542f..44123e4 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -367,6 +367,9 @@ struct cached_dev {
 	unsigned		writeback_rate_update_seconds;
 	unsigned		writeback_rate_d_term;
 	unsigned		writeback_rate_p_term_inverse;
+
+	unsigned short		ioprio_writeback;
+	unsigned short		ioprio_bypass;
 };
 
 enum alloc_reserve {
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index d27707d..a95609f 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -373,6 +373,8 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	unsigned sectors, congested = bch_get_congested(c);
 	struct task_struct *task = current;
 	struct io *i;
+	struct io_context *ioc;
+	unsigned short ioprio;
 
 	if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
 	    c->gc_stats.in_use > CUTOFF_CACHE_ADD ||
@@ -384,6 +386,28 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	     op_is_write(bio_op(bio))))
 		goto skip;
 
+	/* If the ioprio already exists on the bio, use that.  We assume that
+	 * the upper layer properly assigned the calling process's ioprio to
+	 * the bio being passed to bcache. Otherwise, use current's ioc. */
+	ioprio = bio_prio(bio);
+	if (!ioprio_valid(ioprio)) {
+		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
+		if (ioc) {
+			if (ioprio_valid(ioc->ioprio))
+				ioprio = ioc->ioprio;
+			put_io_context(ioc);
+			ioc = NULL;
+		}
+	}
+
+	/* If process ioprio is lower-or-equal to dc->ioprio_bypass, then
+	 * hint for bypass. Note that a lower-priority IO class+value
+	 * has a greater numeric value. */
+	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
+		&& ioprio >= dc->ioprio_bypass) {
+		return true;
+	}
+
 	if (bio->bi_iter.bi_sector & (c->sb.block_size - 1) ||
 	    bio_sectors(bio) & (c->sb.block_size - 1)) {
 		pr_debug("skipping unaligned io");
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index f90f136..cc0076d 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -107,6 +107,9 @@ rw_attribute(btree_shrinker_disabled);
 rw_attribute(copy_gc_enabled);
 rw_attribute(size);
 
+rw_attribute(ioprio_writeback);
+rw_attribute(ioprio_bypass);
+
 SHOW(__bch_cached_dev)
 {
 	struct cached_dev *dc = container_of(kobj, struct cached_dev,
@@ -183,6 +186,17 @@ SHOW(__bch_cached_dev)
 		return strlen(buf);
 	}
 
+	if (attr == &sysfs_ioprio_bypass)
+		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
+			IOPRIO_PRIO_CLASS(dc->ioprio_bypass),
+			IOPRIO_PRIO_DATA(dc->ioprio_bypass));
+
+	if (attr == &sysfs_ioprio_writeback)
+		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
+			IOPRIO_PRIO_CLASS(dc->ioprio_writeback),
+			IOPRIO_PRIO_DATA(dc->ioprio_writeback));
+
+
 #undef var
 	return 0;
 }
@@ -195,6 +209,10 @@ STORE(__cached_dev)
 	unsigned v = size;
 	struct cache_set *c;
 	struct kobj_uevent_env *env;
+	unsigned ioprio_class = 0; /* invalid initial ioprio values */
+	unsigned ioprio_level = IOPRIO_BE_NR;
+	unsigned short *ioprio_hint = NULL;
+	char *ioprio_type = NULL;
 
 #define d_strtoul(var)		sysfs_strtoul(var, dc->var)
 #define d_strtoul_nonzero(var)	sysfs_strtoul_clamp(var, dc->var, 1, INT_MAX)
@@ -283,6 +301,57 @@ STORE(__cached_dev)
 	if (attr == &sysfs_stop)
 		bcache_device_stop(&dc->disk);
 
+	/* ioprio hinting: we use ioprio_hint to reduce duplicate printk verbiage */
+	if (attr == &sysfs_ioprio_writeback) {
+		ioprio_hint = &dc->ioprio_writeback;
+		ioprio_type = "writeback";
+	}
+
+	if (attr == &sysfs_ioprio_bypass) {
+		ioprio_hint = &dc->ioprio_bypass;
+		ioprio_type = "bypass";
+	}
+
+	if (ioprio_hint != NULL)
+	{
+		if (sscanf(buf, "%u,%u", &ioprio_class, &ioprio_level) != 2
+			|| ioprio_class > IOPRIO_CLASS_IDLE
+			|| ioprio_level >= IOPRIO_BE_NR) {
+			pr_err("ioprio_%s invalid, expecting: (class,level) but parsed (%u,%u); ignored.",
+				ioprio_type,
+				ioprio_class, ioprio_level);
+			return size;
+		}
+
+		/* Use the maximum(/minimum) value in the class shift space to make integer
+		  comparison correct for ioprio_writeback(/ioprio_bypass) for IOPRIO_CLASS_IDLE.
+		  This is necessary because there are no ioprio levels for the idle class. */
+		if (ioprio_class == IOPRIO_CLASS_IDLE) {
+			if (ioprio_hint == &dc->ioprio_writeback)
+				ioprio_level = IOPRIO_PRIO_MASK;
+			else
+				/* Same, but 0 for bypass (inverted vs. writeback) */
+				ioprio_level = 0;
+		}
+
+		*ioprio_hint = IOPRIO_PRIO_VALUE(ioprio_class, ioprio_level);
+
+		if (!ioprio_valid(*ioprio_hint))
+			pr_info("disabled ioprio_%s hints.", ioprio_type);
+		else
+			pr_info("set hint for cache %s with priority %s: (class,level) = (%u,%u)",
+				ioprio_type,
+				( ioprio_hint == &dc->ioprio_writeback ? "at-or-above" : "at-or-below" ),
+				ioprio_class, ioprio_level);
+
+		if (ioprio_valid(dc->ioprio_writeback)
+			&& ioprio_valid(dc->ioprio_bypass)
+			&& dc->ioprio_writeback >= dc->ioprio_bypass)
+			pr_warning(
+				"warning: ioprio_writeback hint is neither disabled nor higher priority than the bypass hint; "
+				"will always writeback!");
+	}
+
 	return size;
 }
 
@@ -335,6 +404,8 @@ static struct attribute *bch_cached_dev_files[] = {
 	&sysfs_verify,
 	&sysfs_bypass_torture_test,
 #endif
+	&sysfs_ioprio_bypass,
+	&sysfs_ioprio_writeback,
 	NULL
 };
 KTYPE(bch_cached_dev);
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 42c66e7..3d463f0 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -511,6 +511,14 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
 	dc->writeback_rate_d_term	= 30;
 	dc->writeback_rate_p_term_inverse = 6000;
 
+	/* These defaults provide the best SSD life by enabling bypass
+	 for priorities at-or-below BE-7. This also provides better
+	 performance (cache hits) by preventing (near-)idle processes from
+	 polluting the cache working set.  Only set ioprio_writeback if
+	 you really need it: it will wear out your SSD sooner. */
+	dc->ioprio_writeback = IOPRIO_PRIO_VALUE(0, 0);
+	dc->ioprio_bypass    = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, (IOPRIO_BE_NR-1));
+
 	INIT_DELAYED_WORK(&dc->writeback_rate_update, update_writeback_rate);
 }
 
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index 629bd1a..cd82fe8 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -43,6 +43,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 				    unsigned cache_mode, bool would_skip)
 {
 	unsigned in_use = dc->disk.c->gc_stats.in_use;
+	struct io_context *ioc;
+	unsigned short ioprio;
 
 	if (cache_mode != CACHE_MODE_WRITEBACK ||
 	    test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
@@ -57,6 +59,28 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 	if (would_skip)
 		return false;
 
+	/* If the ioprio already exists on the bio, use that.  We assume that
+	 * the upper layer properly assigned the calling process's ioprio to
+	 * the bio being passed to bcache. Otherwise, use current's ioc. */
+	ioprio = bio_prio(bio);
+	if (!ioprio_valid(ioprio)) {
+		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
+		if (ioc) {
+			if (ioprio_valid(ioc->ioprio))
+				ioprio = ioc->ioprio;
+			put_io_context(ioc);
+			ioc = NULL;
+		}
+	}
+
+	/* If process ioprio is higher-or-equal to dc->ioprio_writeback, then
+	 * hint for writeback. Note that a higher-priority IO class+value
+	 * has a lesser numeric value. */
+	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
+		&& ioprio <= dc->ioprio_writeback) {
+		return true;
+	}
+
 	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
 }
 
-- 
1.8.3.1

[PATCH 07/19] bcache: introduce bcache sysfs entries for ioprio-based bypass/writeback hints

[Eric Wheeler]

Add sysfs entries to support to hint for bypass/writeback by the ioprio
assigned to the bio.  If the bio is unassigned, use current's io-context
ioprio for cache writeback or bypass (configured per-process with
`ionice`).

Having idle IOs bypass the cache can increase performance elsewhere
since you probably don't care about their performance.  In addition,
this prevents idle IOs from promoting into (polluting) your cache and
evicting blocks that are more important elsewhere.

If you really nead the performance at the expense of SSD wearout,
then configure ioprio_writeback and set your `ionice` appropriately.

For example:
	echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
	echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback

See the documentation commit for details.

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
Acked-by: Kent Overstreet <kent.overstreet@gmail.com>
Tested-by: Kai Krakow <kai@kaishome.de>
Cc: nix@esperi.org.uk
---
 drivers/md/bcache/bcache.h    |  3 ++
 drivers/md/bcache/request.c   | 24 +++++++++++++++
 drivers/md/bcache/sysfs.c     | 71 +++++++++++++++++++++++++++++++++++++++++++
 drivers/md/bcache/writeback.c |  8 +++++
 drivers/md/bcache/writeback.h | 24 +++++++++++++++
 5 files changed, 130 insertions(+)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index dee542f..44123e4 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -367,6 +367,9 @@ struct cached_dev {
 	unsigned		writeback_rate_update_seconds;
 	unsigned		writeback_rate_d_term;
 	unsigned		writeback_rate_p_term_inverse;
+
+	unsigned short		ioprio_writeback;
+	unsigned short		ioprio_bypass;
 };
 
 enum alloc_reserve {
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index d27707d..a95609f 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -373,6 +373,8 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	unsigned sectors, congested = bch_get_congested(c);
 	struct task_struct *task = current;
 	struct io *i;
+	struct io_context *ioc;
+	unsigned short ioprio;
 
 	if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
 	    c->gc_stats.in_use > CUTOFF_CACHE_ADD ||
@@ -384,6 +386,28 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	     op_is_write(bio_op(bio))))
 		goto skip;
 
+	/* If the ioprio already exists on the bio, use that.  We assume that
+	 * the upper layer properly assigned the calling process's ioprio to
+	 * the bio being passed to bcache. Otherwise, use current's ioc. */
+	ioprio = bio_prio(bio);
+	if (!ioprio_valid(ioprio)) {
+		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
+		if (ioc) {
+			if (ioprio_valid(ioc->ioprio))
+				ioprio = ioc->ioprio;
+			put_io_context(ioc);
+			ioc = NULL;
+		}
+	}
+
+	/* If process ioprio is lower-or-equal to dc->ioprio_bypass, then
+	 * hint for bypass. Note that a lower-priority IO class+value
+	 * has a greater numeric value. */
+	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
+		&& ioprio >= dc->ioprio_bypass) {
+		return true;
+	}
+
 	if (bio->bi_iter.bi_sector & (c->sb.block_size - 1) ||
 	    bio_sectors(bio) & (c->sb.block_size - 1)) {
 		pr_debug("skipping unaligned io");
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index f90f136..cc0076d 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -107,6 +107,9 @@ rw_attribute(btree_shrinker_disabled);
 rw_attribute(copy_gc_enabled);
 rw_attribute(size);
 
+rw_attribute(ioprio_writeback);
+rw_attribute(ioprio_bypass);
+
 SHOW(__bch_cached_dev)
 {
 	struct cached_dev *dc = container_of(kobj, struct cached_dev,
@@ -183,6 +186,17 @@ SHOW(__bch_cached_dev)
 		return strlen(buf);
 	}
 
+	if (attr == &sysfs_ioprio_bypass)
+		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
+			IOPRIO_PRIO_CLASS(dc->ioprio_bypass),
+			IOPRIO_PRIO_DATA(dc->ioprio_bypass));
+
+	if (attr == &sysfs_ioprio_writeback)
+		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
+			IOPRIO_PRIO_CLASS(dc->ioprio_writeback),
+			IOPRIO_PRIO_DATA(dc->ioprio_writeback));
+
+
 #undef var
 	return 0;
 }
@@ -195,6 +209,10 @@ STORE(__cached_dev)
 	unsigned v = size;
 	struct cache_set *c;
 	struct kobj_uevent_env *env;
+	unsigned ioprio_class = 0; /* invalid initial ioprio values */
+	unsigned ioprio_level = IOPRIO_BE_NR;
+	unsigned short *ioprio_hint = NULL;
+	char *ioprio_type = NULL;
 
 #define d_strtoul(var)		sysfs_strtoul(var, dc->var)
 #define d_strtoul_nonzero(var)	sysfs_strtoul_clamp(var, dc->var, 1, INT_MAX)
@@ -283,6 +301,57 @@ STORE(__cached_dev)
 	if (attr == &sysfs_stop)
 		bcache_device_stop(&dc->disk);
 
+	/* ioprio hinting: we use ioprio_hint to reduce duplicate printk verbiage */
+	if (attr == &sysfs_ioprio_writeback) {
+		ioprio_hint = &dc->ioprio_writeback;
+		ioprio_type = "writeback";
+	}
+
+	if (attr == &sysfs_ioprio_bypass) {
+		ioprio_hint = &dc->ioprio_bypass;
+		ioprio_type = "bypass";
+	}
+
+	if (ioprio_hint != NULL)
+	{
+		if (sscanf(buf, "%u,%u", &ioprio_class, &ioprio_level) != 2
+			|| ioprio_class > IOPRIO_CLASS_IDLE
+			|| ioprio_level >= IOPRIO_BE_NR) {
+			pr_err("ioprio_%s invalid, expecting: (class,level) but parsed (%u,%u); ignored.",
+				ioprio_type,
+				ioprio_class, ioprio_level);
+			return size;
+		}
+
+		/* Use the maximum(/minimum) value in the class shift space to make integer
+		  comparison correct for ioprio_writeback(/ioprio_bypass) for IOPRIO_CLASS_IDLE.
+		  This is necessary because there are no ioprio levels for the idle class. */
+		if (ioprio_class == IOPRIO_CLASS_IDLE) {
+			if (ioprio_hint == &dc->ioprio_writeback)
+				ioprio_level = IOPRIO_PRIO_MASK;
+			else
+				/* Same, but 0 for bypass (inverted vs. writeback) */
+				ioprio_level = 0;
+		}
+
+		*ioprio_hint = IOPRIO_PRIO_VALUE(ioprio_class, ioprio_level);
+
+		if (!ioprio_valid(*ioprio_hint))
+			pr_info("disabled ioprio_%s hints.", ioprio_type);
+		else
+			pr_info("set hint for cache %s with priority %s: (class,level) = (%u,%u)",
+				ioprio_type,
+				( ioprio_hint == &dc->ioprio_writeback ? "at-or-above" : "at-or-below" ),
+				ioprio_class, ioprio_level);
+
+		if (ioprio_valid(dc->ioprio_writeback)
+			&& ioprio_valid(dc->ioprio_bypass)
+			&& dc->ioprio_writeback >= dc->ioprio_bypass)
+			pr_warning(
+				"warning: ioprio_writeback hint is neither disabled nor higher priority than the bypass hint; "
+				"will always writeback!");
+	}
+
 	return size;
 }
 
@@ -335,6 +404,8 @@ static struct attribute *bch_cached_dev_files[] = {
 	&sysfs_verify,
 	&sysfs_bypass_torture_test,
 #endif
+	&sysfs_ioprio_bypass,
+	&sysfs_ioprio_writeback,
 	NULL
 };
 KTYPE(bch_cached_dev);
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 42c66e7..3d463f0 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -511,6 +511,14 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
 	dc->writeback_rate_d_term	= 30;
 	dc->writeback_rate_p_term_inverse = 6000;
 
+	/* These defaults provide the best SSD life by enabling bypass
+	 for priorities at-or-below BE-7. This also provides better
+	 performance (cache hits) by preventing (near-)idle processes from
+	 polluting the cache working set.  Only set ioprio_writeback if
+	 you really need it: it will wear out your SSD sooner. */
+	dc->ioprio_writeback = IOPRIO_PRIO_VALUE(0, 0);
+	dc->ioprio_bypass    = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, (IOPRIO_BE_NR-1));
+
 	INIT_DELAYED_WORK(&dc->writeback_rate_update, update_writeback_rate);
 }
 
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index 629bd1a..cd82fe8 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -43,6 +43,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 				    unsigned cache_mode, bool would_skip)
 {
 	unsigned in_use = dc->disk.c->gc_stats.in_use;
+	struct io_context *ioc;
+	unsigned short ioprio;
 
 	if (cache_mode != CACHE_MODE_WRITEBACK ||
 	    test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
@@ -57,6 +59,28 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 	if (would_skip)
 		return false;
 
+	/* If the ioprio already exists on the bio, use that.  We assume that
+	 * the upper layer properly assigned the calling process's ioprio to
+	 * the bio being passed to bcache. Otherwise, use current's ioc. */
+	ioprio = bio_prio(bio);
+	if (!ioprio_valid(ioprio)) {
+		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
+		if (ioc) {
+			if (ioprio_valid(ioc->ioprio))
+				ioprio = ioc->ioprio;
+			put_io_context(ioc);
+			ioc = NULL;
+		}
+	}
+
+	/* If process ioprio is higher-or-equal to dc->ioprio_writeback, then
+	 * hint for writeback. Note that a higher-priority IO class+value
+	 * has a lesser numeric value. */
+	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
+		&& ioprio <= dc->ioprio_writeback) {
+		return true;
+	}
+
 	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
 }
 
-- 
1.8.3.1

[PATCH 08/19] bcache: documentation for sysfs entries describing bcache cache hinting

[Eric Wheeler]

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
---
 Documentation/bcache.txt | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/Documentation/bcache.txt b/Documentation/bcache.txt
index a9259b5..c78c012 100644
--- a/Documentation/bcache.txt
+++ b/Documentation/bcache.txt
@@ -133,6 +133,86 @@ the backing devices to passthrough mode.
    writeback mode). It currently doesn't do anything intelligent if it fails to
    read some of the dirty data, though.
 
+SSD LONGEVITY: PER-PROCESS CACHE HINTING WITH IO PRIORITY
+---------------------------------------------------------
+
+Processes can be assigned an IO priority using `ionice` and bcache will
+either try to writeback or bypass the cache based on the IO priority
+level assigned to the process and the configuration of the syfs ioprio
+hints.  If configured properly for your workload, this can both increase
+performance and reduce SSD wear (erase/write cycles).
+
+Having idle IOs bypass the cache can increase performance elsewhere
+since you probably don't care about their performance.  In addition,
+this prevents idle IOs from promoting into (polluting) your cache and
+evicting blocks that are more important elsewhere.
+
+Default sysfs values:
+	2,7: ioprio_bypass is hinted for process IOs at-or-below best-effort-7.
+	0,0: ioprio_writeback hinting is disabled by default.
+
+Cache hinting is configured by writing 'class,level' pairs to sysfs.
+In this example, we write the following:
+
+    echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
+    echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback
+
+Thus, processes with the following IO class (ionice -c) and level (-n)
+will the behave as shown in this table:
+
+	(-c) IO Class    (-n) Class level       Action
+	-----------------------------------------------------
+	(1) Realtime      0-7                   Writeback
+	(2) Best-effort     0                   Writeback
+	(2) Best-effort   1-6                   Normal, as if hinting were disabled
+	(2) Best-effort     7                   Bypass cache
+	(3) Idle          n/a                   Bypass cache
+
+For processes at-or-below best-effort-7 (ionice -c2 -n7), the
+ioprio_bypass behavior is as follows:
+
+* Reads will come from the backing device and will not promote into
+  (pollute) your cache.  If the block being read was already in the cache,
+  then it will be read from the cache (and remain cached).
+
+* If you are using writeback mode, then low-priority bypass-hinted writes
+  will go directly to the backing device.  If the write was dirty in
+  cache, it will cache-invalidate and write directly to the backing
+  device.  If a high-priority task later writes the same block then it
+  will writeback so no performance is lost for write-after-write.
+
+  For read-after-bypassed-write, the block will be read from the backing
+  device (not cached) so there may be a miss penalty when a low-priority
+  process write bypasses the cache followed by a high-priority read that
+  would otherwise have hit.  In practice, this is not an issue; to date,
+  none have wanted low-priority writes and high-priority reads of the
+  same block.
+
+For processes in our example at-or-above best-effort-0 (ionice -c2 -n0),
+the ioprio_writeback behavior is as follows:
+
+* The writeback hint has no effect unless your 'cache_mode' is writeback.
+  Assuming writeback mode, all writes at this priority will writeback.
+  Of course this will increase SSD wear, so only use writeback hinting
+  if you need it.
+
+* Reads are unaffected by ioprio_writeback, except that read-after-write
+  will of course read from the cache.
+
+Linux assigns processes the best-effort class with a level of 4 if
+no process is assigned  Thus, without `ionice` your processes will
+follow normal bcache should_writeback/should_bypass symantecs as if the
+ioprio_writeback/ioprio_bypass sysfs flags were disabled.
+
+Also note that in order to be hinted by ioprio_writeback/ioprio_bypass,
+the process must have a valid ioprio setting as returned by
+get_task_io_context()->ioprio. Thus, a process without an IO context
+will be ignored by the ioprio_writeback/ioprio_bypass hints even if your
+sysfs hints specify that best-effort-4 should be flagged for bypass
+or writeback.  If in doubt, explicitly set the process IO priority with
+`ionice`.
+
+See `man ionice` for more detail about per-process IO priority in Linux.
 
 HOWTO/COOKBOOK
 --------------
-- 
1.8.3.1

[PATCH 08/19] bcache: documentation for sysfs entries describing bcache cache hinting

[Eric Wheeler]

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
---
 Documentation/bcache.txt | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/Documentation/bcache.txt b/Documentation/bcache.txt
index a9259b5..c78c012 100644
--- a/Documentation/bcache.txt
+++ b/Documentation/bcache.txt
@@ -133,6 +133,86 @@ the backing devices to passthrough mode.
    writeback mode). It currently doesn't do anything intelligent if it fails to
    read some of the dirty data, though.
 
+SSD LONGEVITY: PER-PROCESS CACHE HINTING WITH IO PRIORITY
+---------------------------------------------------------
+
+Processes can be assigned an IO priority using `ionice` and bcache will
+either try to writeback or bypass the cache based on the IO priority
+level assigned to the process and the configuration of the syfs ioprio
+hints.  If configured properly for your workload, this can both increase
+performance and reduce SSD wear (erase/write cycles).
+
+Having idle IOs bypass the cache can increase performance elsewhere
+since you probably don't care about their performance.  In addition,
+this prevents idle IOs from promoting into (polluting) your cache and
+evicting blocks that are more important elsewhere.
+
+Default sysfs values:
+	2,7: ioprio_bypass is hinted for process IOs at-or-below best-effort-7.
+	0,0: ioprio_writeback hinting is disabled by default.
+
+Cache hinting is configured by writing 'class,level' pairs to sysfs.
+In this example, we write the following:
+
+    echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
+    echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback
+
+Thus, processes with the following IO class (ionice -c) and level (-n)
+will the behave as shown in this table:
+
+	(-c) IO Class    (-n) Class level       Action
+	-----------------------------------------------------
+	(1) Realtime      0-7                   Writeback
+	(2) Best-effort     0                   Writeback
+	(2) Best-effort   1-6                   Normal, as if hinting were disabled
+	(2) Best-effort     7                   Bypass cache
+	(3) Idle          n/a                   Bypass cache
+
+For processes at-or-below best-effort-7 (ionice -c2 -n7), the
+ioprio_bypass behavior is as follows:
+
+* Reads will come from the backing device and will not promote into
+  (pollute) your cache.  If the block being read was already in the cache,
+  then it will be read from the cache (and remain cached).
+
+* If you are using writeback mode, then low-priority bypass-hinted writes
+  will go directly to the backing device.  If the write was dirty in
+  cache, it will cache-invalidate and write directly to the backing
+  device.  If a high-priority task later writes the same block then it
+  will writeback so no performance is lost for write-after-write.
+
+  For read-after-bypassed-write, the block will be read from the backing
+  device (not cached) so there may be a miss penalty when a low-priority
+  process write bypasses the cache followed by a high-priority read that
+  would otherwise have hit.  In practice, this is not an issue; to date,
+  none have wanted low-priority writes and high-priority reads of the
+  same block.
+
+For processes in our example at-or-above best-effort-0 (ionice -c2 -n0),
+the ioprio_writeback behavior is as follows:
+
+* The writeback hint has no effect unless your 'cache_mode' is writeback.
+  Assuming writeback mode, all writes at this priority will writeback.
+  Of course this will increase SSD wear, so only use writeback hinting
+  if you need it.
+
+* Reads are unaffected by ioprio_writeback, except that read-after-write
+  will of course read from the cache.
+
+Linux assigns processes the best-effort class with a level of 4 if
+no process is assigned  Thus, without `ionice` your processes will
+follow normal bcache should_writeback/should_bypass symantecs as if the
+ioprio_writeback/ioprio_bypass sysfs flags were disabled.
+
+Also note that in order to be hinted by ioprio_writeback/ioprio_bypass,
+the process must have a valid ioprio setting as returned by
+get_task_io_context()->ioprio. Thus, a process without an IO context
+will be ignored by the ioprio_writeback/ioprio_bypass hints even if your
+sysfs hints specify that best-effort-4 should be flagged for bypass
+or writeback.  If in doubt, explicitly set the process IO priority with
+`ionice`.
+
+See `man ionice` for more detail about per-process IO priority in Linux.
 
 HOWTO/COOKBOOK
 --------------
-- 
1.8.3.1

[PATCH 09/19] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Eric Wheeler]

Bypass if:     bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND)

Writeback if:  op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
---
 drivers/md/bcache/request.c   | 3 +++
 drivers/md/bcache/writeback.h | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index a95609f..4629b0c 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -386,6 +386,9 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	     op_is_write(bio_op(bio))))
 		goto skip;
 
+	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))
+		goto skip;
+
 	/* If the ioprio already exists on the bio, use that.  We assume that
 	 * the upper layer properly assigned the calling process's ioprio to
 	 * the bio being passed to bcache. Otherwise, use current's ioc. */
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index cd82fe8..ea2f92e 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 		return true;
 	}
 
-	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
+	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
+		|| in_use <= CUTOFF_WRITEBACK;
 }
 
 static inline void bch_writeback_queue(struct cached_dev *dc)
-- 
1.8.3.1

[PATCH 09/19] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Eric Wheeler]

Bypass if:     bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND)

Writeback if:  op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
---
 drivers/md/bcache/request.c   | 3 +++
 drivers/md/bcache/writeback.h | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index a95609f..4629b0c 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -386,6 +386,9 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	     op_is_write(bio_op(bio))))
 		goto skip;
 
+	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))
+		goto skip;
+
 	/* If the ioprio already exists on the bio, use that.  We assume that
 	 * the upper layer properly assigned the calling process's ioprio to
 	 * the bio being passed to bcache. Otherwise, use current's ioc. */
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index cd82fe8..ea2f92e 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 		return true;
 	}
 
-	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
+	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
+		|| in_use <= CUTOFF_WRITEBACK;
 }
 
 static inline void bch_writeback_queue(struct cached_dev *dc)
-- 
1.8.3.1

[PATCH 01/19] bcache: Fix leak of bdev reference

[Jan Kara]

If blkdev_get_by_path() in register_bcache() fails, we try to lookup the
block device using lookup_bdev() to detect which situation we are in to
properly report error. However we never drop the reference returned to
us from lookup_bdev(). Fix that.

Signed-off-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 8352fad..9a2c190 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1964,6 +1964,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
 			else
 				err = "device busy";
 			mutex_unlock(&bch_register_lock);
+			if (!IS_ERR(bdev))
+				bdput(bdev);
 			if (attr == &ksysfs_register_quiet)
 				goto out;
 		}
-- 
1.8.3.1

[PATCH 01/19] bcache: Fix leak of bdev reference

[Jan Kara]

If blkdev_get_by_path() in register_bcache() fails, we try to lookup the
block device using lookup_bdev() to detect which situation we are in to
properly report error. However we never drop the reference returned to
us from lookup_bdev(). Fix that.

Signed-off-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 8352fad..9a2c190 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1964,6 +1964,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
 			else
 				err = "device busy";
 			mutex_unlock(&bch_register_lock);
+			if (!IS_ERR(bdev))
+				bdput(bdev);
 			if (attr == &ksysfs_register_quiet)
 				goto out;
 		}
-- 
1.8.3.1

[PATCH 02/19] bcache: fix sequential large write IO bypass

[Tang Junhui]

Sequential write IOs were tested with bs=1M by FIO in writeback cache
mode, these IOs were expected to be bypassed, but actually they did not.
We debug the code, and find in check_should_bypass():
    if (!congested &&
        mode == CACHE_MODE_WRITEBACK &&
        op_is_write(bio_op(bio)) &&
        (bio-＞bi_opf & REQ_SYNC))
        goto rescale
that means, If in writeback mode, a write IO with REQ_SYNC flag will not
be bypassed though it is a sequential large IO, It's not a correct thing
to do actually, so this patch remove these codes.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/request.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 019b3df..958072a 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -400,12 +400,6 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	if (!congested && !dc->sequential_cutoff)
 		goto rescale;
 
-	if (!congested &&
-	    mode == CACHE_MODE_WRITEBACK &&
-	    op_is_write(bio->bi_opf) &&
-	    op_is_sync(bio->bi_opf))
-		goto rescale;
-
 	spin_lock(&dc->io_lock);
 
 	hlist_for_each_entry(i, iohash(dc, bio->bi_iter.bi_sector), hash)
-- 
1.8.3.1

[PATCH 02/19] bcache: fix sequential large write IO bypass

[Tang Junhui]

Sequential write IOs were tested with bs=1M by FIO in writeback cache
mode, these IOs were expected to be bypassed, but actually they did not.
We debug the code, and find in check_should_bypass():
    if (!congested &&
        mode == CACHE_MODE_WRITEBACK &&
        op_is_write(bio_op(bio)) &&
        (bio-＞bi_opf & REQ_SYNC))
        goto rescale
that means, If in writeback mode, a write IO with REQ_SYNC flag will not
be bypassed though it is a sequential large IO, It's not a correct thing
to do actually, so this patch remove these codes.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/request.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 019b3df..958072a 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -400,12 +400,6 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	if (!congested && !dc->sequential_cutoff)
 		goto rescale;
 
-	if (!congested &&
-	    mode == CACHE_MODE_WRITEBACK &&
-	    op_is_write(bio->bi_opf) &&
-	    op_is_sync(bio->bi_opf))
-		goto rescale;
-
 	spin_lock(&dc->io_lock);
 
 	hlist_for_each_entry(i, iohash(dc, bio->bi_iter.bi_sector), hash)
-- 
1.8.3.1

[PATCH 03/19] bcache: do not subtract sectors_to_gc for bypassed IO

[Tang Junhui]

Since bypassed IOs use no bucket, so do not subtract sectors_to_gc to
trigger gc thread.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/request.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 958072a..4b413db 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -196,12 +196,12 @@ static void bch_data_insert_start(struct closure *cl)
 	struct data_insert_op *op = container_of(cl, struct data_insert_op, cl);
 	struct bio *bio = op->bio, *n;
 
-	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
-		wake_up_gc(op->c);
-
 	if (op->bypass)
 		return bch_data_invalidate(cl);
 
+	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
+		wake_up_gc(op->c);
+
 	/*
 	 * Journal writes are marked REQ_PREFLUSH; if the original write was a
 	 * flush, it'll wait on the journal write.
-- 
1.8.3.1

[PATCH 03/19] bcache: do not subtract sectors_to_gc for bypassed IO

[Tang Junhui]

Since bypassed IOs use no bucket, so do not subtract sectors_to_gc to
trigger gc thread.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/request.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 958072a..4b413db 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -196,12 +196,12 @@ static void bch_data_insert_start(struct closure *cl)
 	struct data_insert_op *op = container_of(cl, struct data_insert_op, cl);
 	struct bio *bio = op->bio, *n;
 
-	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
-		wake_up_gc(op->c);
-
 	if (op->bypass)
 		return bch_data_invalidate(cl);
 
+	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
+		wake_up_gc(op->c);
+
 	/*
 	 * Journal writes are marked REQ_PREFLUSH; if the original write was a
 	 * flush, it'll wait on the journal write.
-- 
1.8.3.1

[PATCH 05/19] bcache: fix calling ida_simple_remove() with incorrect minor

[Tang Junhui]

bcache called ida_simple_remove() with minor which have multiplied by
BCACHE_MINORS, it would cause minor wrong release and leakage.

In addition, when adding partition support to bcache, the name assignment
was not updated, resulting in numbers jumping (bcache0, bcache16,
bcache32...). This has been fixed implicitly by the rework.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Coly Li <colyli@suse.de>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org # 4.10
Cc: Stefan Bader <stefan.bader@canonical.com>
Fixes: b8c0d91 (bcache: partition support: add 16 minors per bcacheN device)
BugLink: https://bugs.launchpad.net/bugs/1667078
---
 drivers/md/bcache/super.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 9a2c190..48b8c20 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -58,7 +58,10 @@ static wait_queue_head_t unregister_wait;
 struct workqueue_struct *bcache_wq;
 
 #define BTREE_MAX_PAGES		(256 * 1024 / PAGE_SIZE)
-#define BCACHE_MINORS		16 /* partition support */
+#define BCACHE_MINORS_BITS                4 /* bcache partition support */
+#define BCACHE_MINORS                     (1 << BCACHE_MINORS_BITS)
+#define BCACHE_TO_IDA_MINORS(first_minor) ((first_minor) >> BCACHE_MINORS_BITS)
+#define IDA_TO_BCACHE_MINORS(minor)       ((minor) << BCACHE_MINORS_BITS)
 
 /* Superblock */
 
@@ -734,7 +737,8 @@ static void bcache_device_free(struct bcache_device *d)
 	if (d->disk && d->disk->queue)
 		blk_cleanup_queue(d->disk->queue);
 	if (d->disk) {
-		ida_simple_remove(&bcache_minor, d->disk->first_minor);
+		ida_simple_remove(&bcache_minor,
+			BCACHE_TO_IDA_MINORS(d->disk->first_minor));
 		put_disk(d->disk);
 	}
 
@@ -776,11 +780,11 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	if (!d->full_dirty_stripes)
 		return -ENOMEM;
 
-	minor = ida_simple_get(&bcache_minor, 0, MINORMASK + 1, GFP_KERNEL);
+	minor = ida_simple_get(&bcache_minor, 0,
+		BCACHE_TO_IDA_MINORS(MINORMASK) + 1, GFP_KERNEL);
 	if (minor < 0)
 		return minor;
 
-	minor *= BCACHE_MINORS;
 
 	if (!(d->bio_split = bioset_create(4, offsetof(struct bbio, bio),
 					   BIOSET_NEED_BVECS |
@@ -794,7 +798,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	snprintf(d->disk->disk_name, DISK_NAME_LEN, "bcache%i", minor);
 
 	d->disk->major		= bcache_major;
-	d->disk->first_minor	= minor;
+	d->disk->first_minor	= IDA_TO_BCACHE_MINORS(minor);
 	d->disk->fops		= &bcache_ops;
 	d->disk->private_data	= d;
 
-- 
1.8.3.1

[PATCH 05/19] bcache: fix calling ida_simple_remove() with incorrect minor

[Tang Junhui]

bcache called ida_simple_remove() with minor which have multiplied by
BCACHE_MINORS, it would cause minor wrong release and leakage.

In addition, when adding partition support to bcache, the name assignment
was not updated, resulting in numbers jumping (bcache0, bcache16,
bcache32...). This has been fixed implicitly by the rework.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Coly Li <colyli@suse.de>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org # 4.10
Cc: Stefan Bader <stefan.bader@canonical.com>
Fixes: b8c0d91 (bcache: partition support: add 16 minors per bcacheN device)
BugLink: https://bugs.launchpad.net/bugs/1667078
---
 drivers/md/bcache/super.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 9a2c190..48b8c20 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -58,7 +58,10 @@ static wait_queue_head_t unregister_wait;
 struct workqueue_struct *bcache_wq;
 
 #define BTREE_MAX_PAGES		(256 * 1024 / PAGE_SIZE)
-#define BCACHE_MINORS		16 /* partition support */
+#define BCACHE_MINORS_BITS                4 /* bcache partition support */
+#define BCACHE_MINORS                     (1 << BCACHE_MINORS_BITS)
+#define BCACHE_TO_IDA_MINORS(first_minor) ((first_minor) >> BCACHE_MINORS_BITS)
+#define IDA_TO_BCACHE_MINORS(minor)       ((minor) << BCACHE_MINORS_BITS)
 
 /* Superblock */
 
@@ -734,7 +737,8 @@ static void bcache_device_free(struct bcache_device *d)
 	if (d->disk && d->disk->queue)
 		blk_cleanup_queue(d->disk->queue);
 	if (d->disk) {
-		ida_simple_remove(&bcache_minor, d->disk->first_minor);
+		ida_simple_remove(&bcache_minor,
+			BCACHE_TO_IDA_MINORS(d->disk->first_minor));
 		put_disk(d->disk);
 	}
 
@@ -776,11 +780,11 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	if (!d->full_dirty_stripes)
 		return -ENOMEM;
 
-	minor = ida_simple_get(&bcache_minor, 0, MINORMASK + 1, GFP_KERNEL);
+	minor = ida_simple_get(&bcache_minor, 0,
+		BCACHE_TO_IDA_MINORS(MINORMASK) + 1, GFP_KERNEL);
 	if (minor < 0)
 		return minor;
 
-	minor *= BCACHE_MINORS;
 
 	if (!(d->bio_split = bioset_create(4, offsetof(struct bbio, bio),
 					   BIOSET_NEED_BVECS |
@@ -794,7 +798,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	snprintf(d->disk->disk_name, DISK_NAME_LEN, "bcache%i", minor);
 
 	d->disk->major		= bcache_major;
-	d->disk->first_minor	= minor;
+	d->disk->first_minor	= IDA_TO_BCACHE_MINORS(minor);
 	d->disk->fops		= &bcache_ops;
 	d->disk->private_data	= d;
 
-- 
1.8.3.1

[PATCH 10/19] bcache: initialize stripe_sectors_dirty correctly for thin flash device

[Tang Junhui]

Thin flash device does not initialize stripe_sectors_dirty correctly, this
patch fixes this issue.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/super.c     | 3 ++-
 drivers/md/bcache/writeback.c | 8 ++++----
 drivers/md/bcache/writeback.h | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 1f84791..e06641e 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1030,7 +1030,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c)
 	}
 
 	if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
-		bch_sectors_dirty_init(dc);
+		bch_sectors_dirty_init(&dc->disk);
 		atomic_set(&dc->has_dirty, 1);
 		atomic_inc(&dc->count);
 		bch_writeback_queue(dc);
@@ -1232,6 +1232,7 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 		goto err;
 
 	bcache_device_attach(d, c, u - c->uuids);
+	bch_sectors_dirty_init(d);
 	bch_flash_dev_request_init(d);
 	add_disk(d->disk);
 
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 3d463f0..4ac8b13 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -482,17 +482,17 @@ static int sectors_dirty_init_fn(struct btree_op *_op, struct btree *b,
 	return MAP_CONTINUE;
 }
 
-void bch_sectors_dirty_init(struct cached_dev *dc)
+void bch_sectors_dirty_init(struct bcache_device *d)
 {
 	struct sectors_dirty_init op;
 
 	bch_btree_op_init(&op.op, -1);
-	op.inode = dc->disk.id;
+	op.inode = d->id;
 
-	bch_btree_map_keys(&op.op, dc->disk.c, &KEY(op.inode, 0, 0),
+	bch_btree_map_keys(&op.op, d->c, &KEY(op.inode, 0, 0),
 			   sectors_dirty_init_fn, 0);
 
-	dc->disk.sectors_dirty_last = bcache_dev_sectors_dirty(&dc->disk);
+	d->sectors_dirty_last = bcache_dev_sectors_dirty(d);
 }
 
 void bch_cached_dev_writeback_init(struct cached_dev *dc)
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index ea2f92e..c2ab4b4 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -109,7 +109,7 @@ static inline void bch_writeback_add(struct cached_dev *dc)
 
 void bcache_dev_sectors_dirty_add(struct cache_set *, unsigned, uint64_t, int);
 
-void bch_sectors_dirty_init(struct cached_dev *dc);
+void bch_sectors_dirty_init(struct bcache_device *);
 void bch_cached_dev_writeback_init(struct cached_dev *);
 int bch_cached_dev_writeback_start(struct cached_dev *);
 
-- 
1.8.3.1

[PATCH 10/19] bcache: initialize stripe_sectors_dirty correctly for thin flash device

[Tang Junhui]

Thin flash device does not initialize stripe_sectors_dirty correctly, this
patch fixes this issue.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/super.c     | 3 ++-
 drivers/md/bcache/writeback.c | 8 ++++----
 drivers/md/bcache/writeback.h | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 1f84791..e06641e 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1030,7 +1030,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c)
 	}
 
 	if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
-		bch_sectors_dirty_init(dc);
+		bch_sectors_dirty_init(&dc->disk);
 		atomic_set(&dc->has_dirty, 1);
 		atomic_inc(&dc->count);
 		bch_writeback_queue(dc);
@@ -1232,6 +1232,7 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 		goto err;
 
 	bcache_device_attach(d, c, u - c->uuids);
+	bch_sectors_dirty_init(d);
 	bch_flash_dev_request_init(d);
 	add_disk(d->disk);
 
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 3d463f0..4ac8b13 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -482,17 +482,17 @@ static int sectors_dirty_init_fn(struct btree_op *_op, struct btree *b,
 	return MAP_CONTINUE;
 }
 
-void bch_sectors_dirty_init(struct cached_dev *dc)
+void bch_sectors_dirty_init(struct bcache_device *d)
 {
 	struct sectors_dirty_init op;
 
 	bch_btree_op_init(&op.op, -1);
-	op.inode = dc->disk.id;
+	op.inode = d->id;
 
-	bch_btree_map_keys(&op.op, dc->disk.c, &KEY(op.inode, 0, 0),
+	bch_btree_map_keys(&op.op, d->c, &KEY(op.inode, 0, 0),
 			   sectors_dirty_init_fn, 0);
 
-	dc->disk.sectors_dirty_last = bcache_dev_sectors_dirty(&dc->disk);
+	d->sectors_dirty_last = bcache_dev_sectors_dirty(d);
 }
 
 void bch_cached_dev_writeback_init(struct cached_dev *dc)
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index ea2f92e..c2ab4b4 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -109,7 +109,7 @@ static inline void bch_writeback_add(struct cached_dev *dc)
 
 void bcache_dev_sectors_dirty_add(struct cache_set *, unsigned, uint64_t, int);
 
-void bch_sectors_dirty_init(struct cached_dev *dc);
+void bch_sectors_dirty_init(struct bcache_device *);
 void bch_cached_dev_writeback_init(struct cached_dev *);
 int bch_cached_dev_writeback_start(struct cached_dev *);
 
-- 
1.8.3.1

[PATCH 14/19] bcache: Correct return value for sysfs attach errors

[Tony Asleson]

If you encounter any errors in bch_cached_dev_attach it will return a negative
error code.  The variable 'v' which stores the result is unsigned, thus user
space sees a very large value returned for bytes written which can cause
incorrect user space behavior.  Utilize 1 signed variable to use throughout
the function to preserve error return capability.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
Acked-by: Coly Li <colyli@suse.de>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index cc0076d..7579ca6 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -206,7 +206,7 @@ STORE(__cached_dev)
 {
 	struct cached_dev *dc = container_of(kobj, struct cached_dev,
 					     disk.kobj);
-	unsigned v = size;
+	ssize_t v = size;
 	struct cache_set *c;
 	struct kobj_uevent_env *env;
 	unsigned ioprio_class = 0; /* invalid initial ioprio values */
@@ -245,7 +245,7 @@ STORE(__cached_dev)
 		bch_cached_dev_run(dc);
 
 	if (attr == &sysfs_cache_mode) {
-		ssize_t v = bch_read_string_list(buf, bch_cache_modes + 1);
+		v = bch_read_string_list(buf, bch_cache_modes + 1);
 
 		if (v < 0)
 			return v;
-- 
1.8.3.1

[PATCH 14/19] bcache: Correct return value for sysfs attach errors

[Tony Asleson]

If you encounter any errors in bch_cached_dev_attach it will return a negative
error code.  The variable 'v' which stores the result is unsigned, thus user
space sees a very large value returned for bytes written which can cause
incorrect user space behavior.  Utilize 1 signed variable to use throughout
the function to preserve error return capability.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
Acked-by: Coly Li <colyli@suse.de>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index cc0076d..7579ca6 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -206,7 +206,7 @@ STORE(__cached_dev)
 {
 	struct cached_dev *dc = container_of(kobj, struct cached_dev,
 					     disk.kobj);
-	unsigned v = size;
+	ssize_t v = size;
 	struct cache_set *c;
 	struct kobj_uevent_env *env;
 	unsigned ioprio_class = 0; /* invalid initial ioprio values */
@@ -245,7 +245,7 @@ STORE(__cached_dev)
 		bch_cached_dev_run(dc);
 
 	if (attr == &sysfs_cache_mode) {
-		ssize_t v = bch_read_string_list(buf, bch_cache_modes + 1);
+		v = bch_read_string_list(buf, bch_cache_modes + 1);
 
 		if (v < 0)
 			return v;
-- 
1.8.3.1

[PATCH 13/19] bcache: delete redundant calling set_gc_sectors()

[Tang Junhui]

set_gc_sectors() has been called in bch_gc_thread(), and it was called
again in bch_btree_gc_finish() . The following call is unnecessary, so
delete it.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/btree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 77aa20b..66d8036 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -1662,7 +1662,6 @@ static size_t bch_btree_gc_finish(struct cache_set *c)
 
 	mutex_lock(&c->bucket_lock);
 
-	set_gc_sectors(c);
 	c->gc_mark_valid = 1;
 	c->need_gc	= 0;
 
-- 
1.8.3.1

[PATCH 13/19] bcache: delete redundant calling set_gc_sectors()

[Tang Junhui]

set_gc_sectors() has been called in bch_gc_thread(), and it was called
again in bch_btree_gc_finish() . The following call is unnecessary, so
delete it.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/btree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 77aa20b..66d8036 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -1662,7 +1662,6 @@ static size_t bch_btree_gc_finish(struct cache_set *c)
 
 	mutex_lock(&c->bucket_lock);
 
-	set_gc_sectors(c);
 	c->gc_mark_valid = 1;
 	c->need_gc	= 0;
 
-- 
1.8.3.1

[PATCH 16/19] bcache: increase the number of open buckets

[Tang Junhui]

In currently, we only alloc 6 open buckets for each cache set,
but in usually, we always attach about 10 or so backend devices for
each cache set, and the each bcache device are always accessed by
about 10 or so threads in top application layer. So 6 open buckets
are too few, It has led to that each of the same thread write data
to different buckets, which would cause low efficiency write-back,
and also cause buckets inefficient, and would be Very easy to run
out of.

I add debug message in bch_open_buckets_alloc() to print alloc bucket
info, and test with ten bcache devices with a cache set, and each
bcache device is accessed by ten threads.

>From the debug message, we can see that, after the modification, One
bucket is more likely to assign to the same thread, and the data from
the same thread are more likely to write the same bucket. Usually the
same thread always write/read the same backend device, so it is good
for write-back and also promote the usage efficiency of buckets.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/alloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index ca4abe1..cacbe2d 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -68,6 +68,8 @@
 #include <linux/random.h>
 #include <trace/events/bcache.h>
 
+#define MAX_OPEN_BUCKETS 128
+
 /* Bucket heap / gen */
 
 uint8_t bch_inc_gen(struct cache *ca, struct bucket *b)
@@ -671,7 +673,7 @@ int bch_open_buckets_alloc(struct cache_set *c)
 
 	spin_lock_init(&c->data_bucket_lock);
 
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < MAX_OPEN_BUCKETS; i++) {
 		struct open_bucket *b = kzalloc(sizeof(*b), GFP_KERNEL);
 		if (!b)
 			return -ENOMEM;
-- 
1.8.3.1

[PATCH 16/19] bcache: increase the number of open buckets

[Tang Junhui]

In currently, we only alloc 6 open buckets for each cache set,
but in usually, we always attach about 10 or so backend devices for
each cache set, and the each bcache device are always accessed by
about 10 or so threads in top application layer. So 6 open buckets
are too few, It has led to that each of the same thread write data
to different buckets, which would cause low efficiency write-back,
and also cause buckets inefficient, and would be Very easy to run
out of.

I add debug message in bch_open_buckets_alloc() to print alloc bucket
info, and test with ten bcache devices with a cache set, and each
bcache device is accessed by ten threads.

>From the debug message, we can see that, after the modification, One
bucket is more likely to assign to the same thread, and the data from
the same thread are more likely to write the same bucket. Usually the
same thread always write/read the same backend device, so it is good
for write-back and also promote the usage efficiency of buckets.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/alloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index ca4abe1..cacbe2d 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -68,6 +68,8 @@
 #include <linux/random.h>
 #include <trace/events/bcache.h>
 
+#define MAX_OPEN_BUCKETS 128
+
 /* Bucket heap / gen */
 
 uint8_t bch_inc_gen(struct cache *ca, struct bucket *b)
@@ -671,7 +673,7 @@ int bch_open_buckets_alloc(struct cache_set *c)
 
 	spin_lock_init(&c->data_bucket_lock);
 
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < MAX_OPEN_BUCKETS; i++) {
 		struct open_bucket *b = kzalloc(sizeof(*b), GFP_KERNEL);
 		if (!b)
 			return -ENOMEM;
-- 
1.8.3.1

[PATCH 17/19] bcache: fix for gc and write-back race

[Tang Junhui]

gc and write-back get raced (see the email "bcache get stucked" I sended
before):
gc thread						write-back thread
|							|bch_writeback_thread()
|bch_gc_thread()					|
|							|==>read_dirty()
|==>bch_btree_gc()					|
|==>btree_root() //get btree root			|
|			node write locker		|
|==>bch_btree_gc_root()					|
|							|==>read_dirty_submit()
|							|==>write_dirty()
|							|==>continue_at(cl, write_dirty_finish, system_wq);
|							|==>write_dirty_finish()//excute in system_wq
|							|==>bch_btree_insert()
|							|==>bch_btree_map_leaf_nodes()
|							|==>__bch_btree_map_nodes()
|							|==>btree_root //try to get btree root node read lock
|							|-----stuck here
|==>bch_btree_set_root()				|
|==>bch_journal_meta()					|
|==>bch_journal()					|
|==>journal_try_write()					|
|==>journal_write_unlocked() //journal_full(&c->journal) condition satisfied
|==>continue_at(cl, journal_write, system_wq); //try to excute journal_write in system_wq
|					//but work queue is excuting write_dirty_finish()
|==>closure_sync(); //wait journal_write execute over and wake up gc,
|			--stuck here
|==>release root node write locker

This patch alloc a separate work-queue for write-back thread to avoid such
race.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/bcache.h    | 1 +
 drivers/md/bcache/super.c     | 2 ++
 drivers/md/bcache/writeback.c | 8 ++++++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 44123e4..deb0a6c 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -333,6 +333,7 @@ struct cached_dev {
 	/* Limit number of writeback bios in flight */
 	struct semaphore	in_flight;
 	struct task_struct	*writeback_thread;
+	struct workqueue_struct	*writeback_write_wq;
 
 	struct keybuf		writeback_keys;
 
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e06641e..24cb9b7 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1063,6 +1063,8 @@ static void cached_dev_free(struct closure *cl)
 	cancel_delayed_work_sync(&dc->writeback_rate_update);
 	if (!IS_ERR_OR_NULL(dc->writeback_thread))
 		kthread_stop(dc->writeback_thread);
+	if (dc->writeback_write_wq)
+		destroy_workqueue(dc->writeback_write_wq);
 
 	mutex_lock(&bch_register_lock);
 
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 4104eaa..4bc5daa 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -189,7 +189,7 @@ static void write_dirty(struct closure *cl)
 
 	closure_bio_submit(&io->bio, cl);
 
-	continue_at(cl, write_dirty_finish, system_wq);
+	continue_at(cl, write_dirty_finish, io->dc->writeback_write_wq);
 }
 
 static void read_dirty_endio(struct bio *bio)
@@ -209,7 +209,7 @@ static void read_dirty_submit(struct closure *cl)
 
 	closure_bio_submit(&io->bio, cl);
 
-	continue_at(cl, write_dirty, system_wq);
+	continue_at(cl, write_dirty, io->dc->writeback_write_wq);
 }
 
 static void read_dirty(struct cached_dev *dc)
@@ -527,6 +527,10 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
 
 int bch_cached_dev_writeback_start(struct cached_dev *dc)
 {
+	dc->writeback_write_wq = alloc_workqueue("bcache_writeback_wq", WQ_MEM_RECLAIM, 0);
+	if (!dc->writeback_write_wq)
+		return -ENOMEM;
+
 	dc->writeback_thread = kthread_create(bch_writeback_thread, dc,
 					      "bcache_writeback");
 	if (IS_ERR(dc->writeback_thread))
-- 
1.8.3.1

[PATCH 17/19] bcache: fix for gc and write-back race

[Tang Junhui]

gc and write-back get raced (see the email "bcache get stucked" I sended
before):
gc thread						write-back thread
|							|bch_writeback_thread()
|bch_gc_thread()					|
|							|==>read_dirty()
|==>bch_btree_gc()					|
|==>btree_root() //get btree root			|
|			node write locker		|
|==>bch_btree_gc_root()					|
|							|==>read_dirty_submit()
|							|==>write_dirty()
|							|==>continue_at(cl, write_dirty_finish, system_wq);
|							|==>write_dirty_finish()//excute in system_wq
|							|==>bch_btree_insert()
|							|==>bch_btree_map_leaf_nodes()
|							|==>__bch_btree_map_nodes()
|							|==>btree_root //try to get btree root node read lock
|							|-----stuck here
|==>bch_btree_set_root()				|
|==>bch_journal_meta()					|
|==>bch_journal()					|
|==>journal_try_write()					|
|==>journal_write_unlocked() //journal_full(&c->journal) condition satisfied
|==>continue_at(cl, journal_write, system_wq); //try to excute journal_write in system_wq
|					//but work queue is excuting write_dirty_finish()
|==>closure_sync(); //wait journal_write execute over and wake up gc,
|			--stuck here
|==>release root node write locker

This patch alloc a separate work-queue for write-back thread to avoid such
race.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/bcache.h    | 1 +
 drivers/md/bcache/super.c     | 2 ++
 drivers/md/bcache/writeback.c | 8 ++++++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 44123e4..deb0a6c 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -333,6 +333,7 @@ struct cached_dev {
 	/* Limit number of writeback bios in flight */
 	struct semaphore	in_flight;
 	struct task_struct	*writeback_thread;
+	struct workqueue_struct	*writeback_write_wq;
 
 	struct keybuf		writeback_keys;
 
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e06641e..24cb9b7 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1063,6 +1063,8 @@ static void cached_dev_free(struct closure *cl)
 	cancel_delayed_work_sync(&dc->writeback_rate_update);
 	if (!IS_ERR_OR_NULL(dc->writeback_thread))
 		kthread_stop(dc->writeback_thread);
+	if (dc->writeback_write_wq)
+		destroy_workqueue(dc->writeback_write_wq);
 
 	mutex_lock(&bch_register_lock);
 
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 4104eaa..4bc5daa 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -189,7 +189,7 @@ static void write_dirty(struct closure *cl)
 
 	closure_bio_submit(&io->bio, cl);
 
-	continue_at(cl, write_dirty_finish, system_wq);
+	continue_at(cl, write_dirty_finish, io->dc->writeback_write_wq);
 }
 
 static void read_dirty_endio(struct bio *bio)
@@ -209,7 +209,7 @@ static void read_dirty_submit(struct closure *cl)
 
 	closure_bio_submit(&io->bio, cl);
 
-	continue_at(cl, write_dirty, system_wq);
+	continue_at(cl, write_dirty, io->dc->writeback_write_wq);
 }
 
 static void read_dirty(struct cached_dev *dc)
@@ -527,6 +527,10 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
 
 int bch_cached_dev_writeback_start(struct cached_dev *dc)
 {
+	dc->writeback_write_wq = alloc_workqueue("bcache_writeback_wq", WQ_MEM_RECLAIM, 0);
+	if (!dc->writeback_write_wq)
+		return -ENOMEM;
+
 	dc->writeback_thread = kthread_create(bch_writeback_thread, dc,
 					      "bcache_writeback");
 	if (IS_ERR(dc->writeback_thread))
-- 
1.8.3.1

[PATCH 18/19] bcache: silence static checker warning

[Dan Carpenter]

In olden times, closure_return() used to have a hidden return built in.
We removed the hidden return but forgot to add a new return here.  If
"c" were NULL we would oops on the next line, but fortunately "c" is
never NULL.  Let's just remove the if statement.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/md/bcache/super.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 24cb9b7..243391d 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1381,9 +1381,6 @@ static void cache_set_flush(struct closure *cl)
 	struct btree *b;
 	unsigned i;
 
-	if (!c)
-		closure_return(cl);
-
 	bch_cache_accounting_destroy(&c->accounting);
 
 	kobject_put(&c->internal);
-- 
1.8.3.1

[PATCH 18/19] bcache: silence static checker warning

[Dan Carpenter]

In olden times, closure_return() used to have a hidden return built in.
We removed the hidden return but forgot to add a new return here.  If
"c" were NULL we would oops on the next line, but fortunately "c" is
never NULL.  Let's just remove the if statement.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/md/bcache/super.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 24cb9b7..243391d 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1381,9 +1381,6 @@ static void cache_set_flush(struct closure *cl)
 	struct btree *b;
 	unsigned i;
 
-	if (!c)
-		closure_return(cl);
-
 	bch_cache_accounting_destroy(&c->accounting);
 
 	kobject_put(&c->internal);
-- 
1.8.3.1

[PATCH 19/19] bcache: Update continue_at() documentation

[Dan Carpenter]

continue_at() doesn't have a return statement anymore.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/md/bcache/closure.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
index 1ec84ca..295b7e4 100644
--- a/drivers/md/bcache/closure.h
+++ b/drivers/md/bcache/closure.h
@@ -312,8 +312,6 @@ static inline void closure_wake_up(struct closure_waitlist *list)
  * been dropped with closure_put()), it will resume execution at @fn running out
  * of @wq (or, if @wq is NULL, @fn will be called by closure_put() directly).
  *
- * NOTE: This macro expands to a return in the calling function!
- *
  * This is because after calling continue_at() you no longer have a ref on @cl,
  * and whatever @cl owns may be freed out from under you - a running closure fn
  * has a ref on its own closure which continue_at() drops.
@@ -340,8 +338,6 @@ do {									\
  * Causes @fn to be executed out of @cl, in @wq context (or called directly if
  * @wq is NULL).
  *
- * NOTE: like continue_at(), this macro expands to a return in the caller!
- *
  * The ref the caller of continue_at_nobarrier() had on @cl is now owned by @fn,
  * thus it's not safe to touch anything protected by @cl after a
  * continue_at_nobarrier().
-- 
1.8.3.1

[PATCH 19/19] bcache: Update continue_at() documentation

[Dan Carpenter]

continue_at() doesn't have a return statement anymore.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/md/bcache/closure.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
index 1ec84ca..295b7e4 100644
--- a/drivers/md/bcache/closure.h
+++ b/drivers/md/bcache/closure.h
@@ -312,8 +312,6 @@ static inline void closure_wake_up(struct closure_waitlist *list)
  * been dropped with closure_put()), it will resume execution at @fn running out
  * of @wq (or, if @wq is NULL, @fn will be called by closure_put() directly).
  *
- * NOTE: This macro expands to a return in the calling function!
- *
  * This is because after calling continue_at() you no longer have a ref on @cl,
  * and whatever @cl owns may be freed out from under you - a running closure fn
  * has a ref on its own closure which continue_at() drops.
@@ -340,8 +338,6 @@ do {									\
  * Causes @fn to be executed out of @cl, in @wq context (or called directly if
  * @wq is NULL).
  *
- * NOTE: like continue_at(), this macro expands to a return in the caller!
- *
  * The ref the caller of continue_at_nobarrier() had on @cl is now owned by @fn,
  * thus it's not safe to touch anything protected by @cl after a
  * continue_at_nobarrier().
-- 
1.8.3.1

Re: [PULL] bcache fixes and updates for-4.13

[Coly Li]

On 2017/6/29 下午9:45, Christoph Hellwig wrote:
> On Wed, Jun 28, 2017 at 11:06:46PM +0000, Eric Wheeler wrote:
>> Hi Jens,
>>
>> It looks like you're getting ready for 4.13, so here are the bcache 
>> updates rebased and ready from git.kernel.dk/linux-block for-4.13/block 
>> This tree builds, boots, and works as expected.  Please pull the new fixes 
>> and features below.
>>
>> Thank you for your help!
> 
> Can you please send all the patches to linux-block for review as
> a series?  Some of these subjects sound a little suspect.
> 

We need more eyes to look at bcache code, maybe we can always CC bcache
patches to linux-block for more review.

-- 
Coly Li

Re: [PULL] bcache fixes and updates for-4.13

[Eric Wheeler]

On Thu, 29 Jun 2017, Christoph Hellwig wrote:

> On Wed, Jun 28, 2017 at 11:06:46PM +0000, Eric Wheeler wrote:
> > Hi Jens,
> > 
> > It looks like you're getting ready for 4.13, so here are the bcache 
> > updates rebased and ready from git.kernel.dk/linux-block for-4.13/block 
> > This tree builds, boots, and works as expected.  Please pull the new fixes 
> > and features below.
> > 
> > Thank you for your help!
> 
> Can you please send all the patches to linux-block for review as
> a series?  Some of these subjects sound a little suspect.


On their way, IRT this thread.

--
Eric Wheeler



> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PULL] bcache fixes and updates for-4.13

[Eric Wheeler]

On Thu, 29 Jun 2017, Eric Wheeler wrote:

> 
> On Thu, 29 Jun 2017, Christoph Hellwig wrote:
> 
> > On Wed, Jun 28, 2017 at 11:06:46PM +0000, Eric Wheeler wrote:
> > > Hi Jens,
> > > 
> > > It looks like you're getting ready for 4.13, so here are the bcache 
> > > updates rebased and ready from git.kernel.dk/linux-block for-4.13/block 
> > > This tree builds, boots, and works as expected.  Please pull the new fixes 
> > > and features below.
> > > 
> > > Thank you for your help!
> > 
> > Can you please send all the patches to linux-block for review as
> > a series?  Some of these subjects sound a little suspect.
> 
> 
> On their way, IRT this thread.

Hmm, I think vger might not be letting them in because the From: header 
differs from the original sender (ie, I am not @oracle.com) so 
DNS/SPF/DKIM and such are wrong.

What do you do in these cases?  Should I rewrite the From: header?  
Somehow the original author should remain.

--
Eric Wheeler



> 
> --
> Eric Wheeler
> 
> 
> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PULL] bcache fixes and updates for-4.13

[Nick Alcock]

On 29 Jun 2017, Eric Wheeler verbalised:
> Hmm, I think vger might not be letting them in because the From: header 
> differs from the original sender (ie, I am not @oracle.com) so 
> DNS/SPF/DKIM and such are wrong.
>
> What do you do in these cases?  Should I rewrite the From: header?  
> Somehow the original author should remain.

If these are patches from 'git format-patch', you do it by putting your
name in the email From: and the author you want in the commit as a From
header in the very first line of the email. 'git am' will then ignore
the header line in favour of the first-line override. I believe 'git
send-email' should be doing this automatically if your email address is
not the same as the patch author, but I could be wrong...

See the DISCUSSION section in 'man git-am'.

[PATCH 01/19] bcache: Fix leak of bdev reference

[bcache]

From: Jan Kara <jack@suse.cz>

If blkdev_get_by_path() in register_bcache() fails, we try to lookup the
block device using lookup_bdev() to detect which situation we are in to
properly report error. However we never drop the reference returned to
us from lookup_bdev(). Fix that.

Signed-off-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 8352fad..9a2c190 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1964,6 +1964,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
 			else
 				err = "device busy";
 			mutex_unlock(&bch_register_lock);
+			if (!IS_ERR(bdev))
+				bdput(bdev);
 			if (attr == &ksysfs_register_quiet)
 				goto out;
 		}
-- 
1.8.3.1

[PATCH 02/19] bcache: fix sequential large write IO bypass

[bcache]

From: Tang Junhui <tang.junhui@zte.com.cn>

Sequential write IOs were tested with bs=1M by FIO in writeback cache
mode, these IOs were expected to be bypassed, but actually they did not.
We debug the code, and find in check_should_bypass():
    if (!congested &&
        mode == CACHE_MODE_WRITEBACK &&
        op_is_write(bio_op(bio)) &&
        (bio-＞bi_opf & REQ_SYNC))
        goto rescale
that means, If in writeback mode, a write IO with REQ_SYNC flag will not
be bypassed though it is a sequential large IO, It's not a correct thing
to do actually, so this patch remove these codes.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/request.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 019b3df..958072a 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -400,12 +400,6 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	if (!congested && !dc->sequential_cutoff)
 		goto rescale;
 
-	if (!congested &&
-	    mode == CACHE_MODE_WRITEBACK &&
-	    op_is_write(bio->bi_opf) &&
-	    op_is_sync(bio->bi_opf))
-		goto rescale;
-
 	spin_lock(&dc->io_lock);
 
 	hlist_for_each_entry(i, iohash(dc, bio->bi_iter.bi_sector), hash)
-- 
1.8.3.1

Re: [PATCH 02/19] bcache: fix sequential large write IO bypass

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 03/19] bcache: do not subtract sectors_to_gc for bypassed IO

[bcache]

From: Tang Junhui <tang.junhui@zte.com.cn>

Since bypassed IOs use no bucket, so do not subtract sectors_to_gc to
trigger gc thread.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/request.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 958072a..4b413db 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -196,12 +196,12 @@ static void bch_data_insert_start(struct closure *cl)
 	struct data_insert_op *op = container_of(cl, struct data_insert_op, cl);
 	struct bio *bio = op->bio, *n;
 
-	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
-		wake_up_gc(op->c);
-
 	if (op->bypass)
 		return bch_data_invalidate(cl);
 
+	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
+		wake_up_gc(op->c);
+
 	/*
 	 * Journal writes are marked REQ_PREFLUSH; if the original write was a
 	 * flush, it'll wait on the journal write.
-- 
1.8.3.1

Re: [PATCH 03/19] bcache: do not subtract sectors_to_gc for bypassed IO

[Coly Li]

On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
> Since bypassed IOs use no bucket, so do not subtract sectors_to_gc to
> trigger gc thread.
> 
> Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
> Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
> Cc: stable@vger.kernel.org

Acked-by: Coly Li <colyli@suse.de>

Thanks.

> ---
>  drivers/md/bcache/request.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 958072a..4b413db 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -196,12 +196,12 @@ static void bch_data_insert_start(struct closure *cl)
>  	struct data_insert_op *op = container_of(cl, struct data_insert_op, cl);
>  	struct bio *bio = op->bio, *n;
>  
> -	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
> -		wake_up_gc(op->c);
> -
>  	if (op->bypass)
>  		return bch_data_invalidate(cl);
>  
> +	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
> +		wake_up_gc(op->c);
> +
>  	/*
>  	 * Journal writes are marked REQ_PREFLUSH; if the original write was a
>  	 * flush, it'll wait on the journal write.
> 


-- 
Coly Li

Re: [PATCH 03/19] bcache: do not subtract sectors_to_gc for bypassed IO

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 05/19] bcache: fix calling ida_simple_remove() with incorrect minor

[bcache]

From: Tang Junhui <tang.junhui@zte.com.cn>

bcache called ida_simple_remove() with minor which have multiplied by
BCACHE_MINORS, it would cause minor wrong release and leakage.

In addition, when adding partition support to bcache, the name assignment
was not updated, resulting in numbers jumping (bcache0, bcache16,
bcache32...). This has been fixed implicitly by the rework.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Coly Li <colyli@suse.de>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org # 4.10
Cc: Stefan Bader <stefan.bader@canonical.com>
Fixes: b8c0d91 (bcache: partition support: add 16 minors per bcacheN device)
BugLink: https://bugs.launchpad.net/bugs/1667078
---
 drivers/md/bcache/super.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 9a2c190..48b8c20 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -58,7 +58,10 @@ static wait_queue_head_t unregister_wait;
 struct workqueue_struct *bcache_wq;
 
 #define BTREE_MAX_PAGES		(256 * 1024 / PAGE_SIZE)
-#define BCACHE_MINORS		16 /* partition support */
+#define BCACHE_MINORS_BITS                4 /* bcache partition support */
+#define BCACHE_MINORS                     (1 << BCACHE_MINORS_BITS)
+#define BCACHE_TO_IDA_MINORS(first_minor) ((first_minor) >> BCACHE_MINORS_BITS)
+#define IDA_TO_BCACHE_MINORS(minor)       ((minor) << BCACHE_MINORS_BITS)
 
 /* Superblock */
 
@@ -734,7 +737,8 @@ static void bcache_device_free(struct bcache_device *d)
 	if (d->disk && d->disk->queue)
 		blk_cleanup_queue(d->disk->queue);
 	if (d->disk) {
-		ida_simple_remove(&bcache_minor, d->disk->first_minor);
+		ida_simple_remove(&bcache_minor,
+			BCACHE_TO_IDA_MINORS(d->disk->first_minor));
 		put_disk(d->disk);
 	}
 
@@ -776,11 +780,11 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	if (!d->full_dirty_stripes)
 		return -ENOMEM;
 
-	minor = ida_simple_get(&bcache_minor, 0, MINORMASK + 1, GFP_KERNEL);
+	minor = ida_simple_get(&bcache_minor, 0,
+		BCACHE_TO_IDA_MINORS(MINORMASK) + 1, GFP_KERNEL);
 	if (minor < 0)
 		return minor;
 
-	minor *= BCACHE_MINORS;
 
 	if (!(d->bio_split = bioset_create(4, offsetof(struct bbio, bio),
 					   BIOSET_NEED_BVECS |
@@ -794,7 +798,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	snprintf(d->disk->disk_name, DISK_NAME_LEN, "bcache%i", minor);
 
 	d->disk->major		= bcache_major;
-	d->disk->first_minor	= minor;
+	d->disk->first_minor	= IDA_TO_BCACHE_MINORS(minor);
 	d->disk->fops		= &bcache_ops;
 	d->disk->private_data	= d;
 
-- 
1.8.3.1

Re: [PATCH 05/19] bcache: fix calling ida_simple_remove() with incorrect minor

[Christoph Hellwig]

> +#define BCACHE_TO_IDA_MINORS(first_minor) ((first_minor) >> BCACHE_MINORS_BITS)
> +#define IDA_TO_BCACHE_MINORS(minor)       ((minor) << BCACHE_MINORS_BITS)

Please use inline functions and lower case for these.

Re: Re: [PATCH 05/19] bcache: fix calling ida_simple_remove() with incorrect minor

[tang.junhui]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

Hello Christoph

> > +#define BCACHE_TO_IDA_MINORS(first_minor) ((first_minor) >> 
BCACHE_MINORS_BITS)
> > +#define IDA_TO_BCACHE_MINORS(minor)       ((minor) << 
BCACHE_MINORS_BITS)
> 
> Please use inline functions and lower case for these.

I have send you a v2 patch, please have a review again.

Thanks
Tang Junhui

[-- Attachment #2: Type: text/html, Size: 582 bytes --]

[PATCH 06/19] bcache: explicitly destory mutex while exiting

[bcache]

From: Liang Chen <liangchen.linux@gmail.com>

mutex_destroy does nothing most of time, but it's better to call
it to make the code future proof and it also has some meaning
for like mutex debug.

Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 48b8c20..1f84791 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2089,6 +2089,7 @@ static void bcache_exit(void)
 	if (bcache_major)
 		unregister_blkdev(bcache_major, "bcache");
 	unregister_reboot_notifier(&reboot);
+	mutex_destroy(&bch_register_lock);
 }
 
 static int __init bcache_init(void)
@@ -2106,6 +2107,7 @@ static int __init bcache_init(void)
 
 	bcache_major = register_blkdev(0, "bcache");
 	if (bcache_major < 0) {
+		mutex_destroy(&bch_register_lock);
 		unregister_reboot_notifier(&reboot);
 		return bcache_major;
 	}
-- 
1.8.3.1

Re: [PATCH 06/19] bcache: explicitly destory mutex while exiting

[Coly Li]

On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
> From: Liang Chen <liangchen.linux@gmail.com>
> 
> mutex_destroy does nothing most of time, but it's better to call
> it to make the code future proof and it also has some meaning
> for like mutex debug.
> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/bcache/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 48b8c20..1f84791 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2089,6 +2089,7 @@ static void bcache_exit(void)
>  	if (bcache_major)
>  		unregister_blkdev(bcache_major, "bcache");
>  	unregister_reboot_notifier(&reboot);
> +	mutex_destroy(&bch_register_lock>  }
>  
>  static int __init bcache_init(void)
> @@ -2106,6 +2107,7 @@ static int __init bcache_init(void)
>  
>  	bcache_major = register_blkdev(0, "bcache");
>  	if (bcache_major < 0) {
> +		mutex_destroy(&bch_register_lock);
>  		unregister_reboot_notifier(&reboot);
>  		return bcache_major;
>  	}
> 

Hi Liang,

Current code might have a potential race in a very corner case, see,
2084 static int __init bcache_init(void)
2085 {
2086         static const struct attribute *files[] = {
2087                 &ksysfs_register.attr,
2088                 &ksysfs_register_quiet.attr,
2089                 NULL
2090         };
2091
2092         mutex_init(&bch_register_lock);
2093         init_waitqueue_head(&unregister_wait);
2094         register_reboot_notifier(&reboot);
2095         closure_debug_init();
2096
2097         bcache_major = register_blkdev(0, "bcache");
2098         if (bcache_major < 0) {
2099                 unregister_reboot_notifier(&reboot);
2100                 return bcache_major;
2101         }
2102
2103         if (!(bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM,
0)) ||
2104             !(bcache_kobj = kobject_create_and_add("bcache",
fs_kobj)) ||
2105             sysfs_create_files(bcache_kobj, files) ||
2106             bch_request_init() ||
2107             bch_debug_init(bcache_kobj))
2108                 goto err;
2109
2110         return 0;
2111 err:
2112         bcache_exit();
2113         return -ENOMEM;
2114 }

At line 2107, most of bache stuffs are ready to work, only a debugfs
entry not created yet. If in the time gap between line 2106 and line
2017, another user space tool just registers cache and backing devices.
Then bch_debug_init() failed, and bcache_exit() gets called. In this
case, I doubt bcache_exit() can handle all the references correctly.

The race is very rare, and almost won't happen in real life. So, if you
don't care about it, the patch can be simpler like this,
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e57353e39168..fb5453a46a03 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2070,6 +2070,7 @@ static struct notifier_block reboot = {

 static void bcache_exit(void)
 {
+       mutex_destroy(&bch_register_lock);
        bch_debug_exit();
        bch_request_exit();
        if (bcache_kobj)
@@ -2089,7 +2090,6 @@ static int __init bcache_init(void)
                NULL
        };

-       mutex_init(&bch_register_lock);
        init_waitqueue_head(&unregister_wait);
        register_reboot_notifier(&reboot);
        closure_debug_init();
@@ -2107,6 +2107,7 @@ static int __init bcache_init(void)
            bch_debug_init(bcache_kobj))
                goto err;

+       mutex_init(&bch_register_lock);
        return 0;
 err:
        bcache_exit();
---
And if you do care about the race, maybe you should do something like this,
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e57353e39168..ca1d8b7a7815 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2079,6 +2079,7 @@ static void bcache_exit(void)
        if (bcache_major)
                unregister_blkdev(bcache_major, "bcache");
        unregister_reboot_notifier(&reboot);
+       mutex_unlock(&bch_register_lock);
 }

 static int __init bcache_init(void)
@@ -2090,6 +2091,7 @@ static int __init bcache_init(void)
        };

        mutex_init(&bch_register_lock);
+       mutex_lock(&bch_register_lock);
        init_waitqueue_head(&unregister_wait);
        register_reboot_notifier(&reboot);
        closure_debug_init();
@@ -2097,6 +2099,8 @@ static int __init bcache_init(void)
        bcache_major = register_blkdev(0, "bcache");
        if (bcache_major < 0) {
                unregister_reboot_notifier(&reboot);
+               mutex_unlock(&bch_register_lock);
+               mutex_destroy(&bch_register_lock);
                return bcache_major;
        }

@@ -2104,9 +2108,12 @@ static int __init bcache_init(void)
            !(bcache_kobj = kobject_create_and_add("bcache", fs_kobj)) ||
            sysfs_create_files(bcache_kobj, files) ||
            bch_request_init() ||
-           bch_debug_init(bcache_kobj))
+           bch_debug_init(bcache_kobj)) {
+               mutex_unlock(&bch_register_lock);
                goto err;
+       }

+       mutex_unlock(&bch_register_lock);
        return 0;
 err:
        bcache_exit();
---

Personally I think the first approach with only one new line code added,
your original version will add two new lines of code.

Just FYI. Thanks.

-- 
Coly Li

Re: [PATCH 06/19] bcache: explicitly destory mutex while exiting

[Liang Chen]

Hi Coly,
Thanks for reviewing the patch! You raised a good point about the race. I a=
lso
think it should be addressed. Even though the time window is small, it will
still happen sooner or later.

I would like to keep this "destory mutex" patch unchanged, and send another
patch to fix the issue based on your approach. Please take a look. Thanks!

Thanks,
Liang

On Sun, Jul 2, 2017 at 2:43 AM, Coly Li <i@coly.li> wrote:
> On 2017/7/1 =E4=B8=8A=E5=8D=884:42, bcache@lists.ewheeler.net wrote:
>> From: Liang Chen <liangchen.linux@gmail.com>
>>
>> mutex_destroy does nothing most of time, but it's better to call
>> it to make the code future proof and it also has some meaning
>> for like mutex debug.
>>
>> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
>> Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/md/bcache/super.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>> index 48b8c20..1f84791 100644
>> --- a/drivers/md/bcache/super.c
>> +++ b/drivers/md/bcache/super.c
>> @@ -2089,6 +2089,7 @@ static void bcache_exit(void)
>>       if (bcache_major)
>>               unregister_blkdev(bcache_major, "bcache");
>>       unregister_reboot_notifier(&reboot);
>> +     mutex_destroy(&bch_register_lock>  }
>>
>>  static int __init bcache_init(void)
>> @@ -2106,6 +2107,7 @@ static int __init bcache_init(void)
>>
>>       bcache_major =3D register_blkdev(0, "bcache");
>>       if (bcache_major < 0) {
>> +             mutex_destroy(&bch_register_lock);
>>               unregister_reboot_notifier(&reboot);
>>               return bcache_major;
>>       }
>>
>
> Hi Liang,
>
> Current code might have a potential race in a very corner case, see,
> 2084 static int __init bcache_init(void)
> 2085 {
> 2086         static const struct attribute *files[] =3D {
> 2087                 &ksysfs_register.attr,
> 2088                 &ksysfs_register_quiet.attr,
> 2089                 NULL
> 2090         };
> 2091
> 2092         mutex_init(&bch_register_lock);
> 2093         init_waitqueue_head(&unregister_wait);
> 2094         register_reboot_notifier(&reboot);
> 2095         closure_debug_init();
> 2096
> 2097         bcache_major =3D register_blkdev(0, "bcache");
> 2098         if (bcache_major < 0) {
> 2099                 unregister_reboot_notifier(&reboot);
> 2100                 return bcache_major;
> 2101         }
> 2102
> 2103         if (!(bcache_wq =3D alloc_workqueue("bcache", WQ_MEM_RECLAIM=
,
> 0)) ||
> 2104             !(bcache_kobj =3D kobject_create_and_add("bcache",
> fs_kobj)) ||
> 2105             sysfs_create_files(bcache_kobj, files) ||
> 2106             bch_request_init() ||
> 2107             bch_debug_init(bcache_kobj))
> 2108                 goto err;
> 2109
> 2110         return 0;
> 2111 err:
> 2112         bcache_exit();
> 2113         return -ENOMEM;
> 2114 }
>
> At line 2107, most of bache stuffs are ready to work, only a debugfs
> entry not created yet. If in the time gap between line 2106 and line
> 2017, another user space tool just registers cache and backing devices.
> Then bch_debug_init() failed, and bcache_exit() gets called. In this
> case, I doubt bcache_exit() can handle all the references correctly.
>
> The race is very rare, and almost won't happen in real life. So, if you
> don't care about it, the patch can be simpler like this,
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index e57353e39168..fb5453a46a03 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2070,6 +2070,7 @@ static struct notifier_block reboot =3D {
>
>  static void bcache_exit(void)
>  {
> +       mutex_destroy(&bch_register_lock);
>         bch_debug_exit();
>         bch_request_exit();
>         if (bcache_kobj)
> @@ -2089,7 +2090,6 @@ static int __init bcache_init(void)
>                 NULL
>         };
>
> -       mutex_init(&bch_register_lock);
>         init_waitqueue_head(&unregister_wait);
>         register_reboot_notifier(&reboot);
>         closure_debug_init();
> @@ -2107,6 +2107,7 @@ static int __init bcache_init(void)
>             bch_debug_init(bcache_kobj))
>                 goto err;
>
> +       mutex_init(&bch_register_lock);
>         return 0;
>  err:
>         bcache_exit();
> ---
> And if you do care about the race, maybe you should do something like thi=
s,
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index e57353e39168..ca1d8b7a7815 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2079,6 +2079,7 @@ static void bcache_exit(void)
>         if (bcache_major)
>                 unregister_blkdev(bcache_major, "bcache");
>         unregister_reboot_notifier(&reboot);
> +       mutex_unlock(&bch_register_lock);
>  }
>
>  static int __init bcache_init(void)
> @@ -2090,6 +2091,7 @@ static int __init bcache_init(void)
>         };
>
>         mutex_init(&bch_register_lock);
> +       mutex_lock(&bch_register_lock);
>         init_waitqueue_head(&unregister_wait);
>         register_reboot_notifier(&reboot);
>         closure_debug_init();
> @@ -2097,6 +2099,8 @@ static int __init bcache_init(void)
>         bcache_major =3D register_blkdev(0, "bcache");
>         if (bcache_major < 0) {
>                 unregister_reboot_notifier(&reboot);
> +               mutex_unlock(&bch_register_lock);
> +               mutex_destroy(&bch_register_lock);
>                 return bcache_major;
>         }
>
> @@ -2104,9 +2108,12 @@ static int __init bcache_init(void)
>             !(bcache_kobj =3D kobject_create_and_add("bcache", fs_kobj)) =
||
>             sysfs_create_files(bcache_kobj, files) ||
>             bch_request_init() ||
> -           bch_debug_init(bcache_kobj))
> +           bch_debug_init(bcache_kobj)) {
> +               mutex_unlock(&bch_register_lock);
>                 goto err;
> +       }
>
> +       mutex_unlock(&bch_register_lock);
>         return 0;
>  err:
>         bcache_exit();
> ---
>
> Personally I think the first approach with only one new line code added,
> your original version will add two new lines of code.
>
> Just FYI. Thanks.
>
> --
> Coly Li

Re: [PATCH 06/19] bcache: explicitly destory mutex while exiting

[Coly Li]

On 2017/7/5 下午7:58, Liang Chen wrote:
> Hi Coly,
> Thanks for reviewing the patch! You raised a good point about the race. I also
> think it should be addressed. Even though the time window is small, it will
> still happen sooner or later.
> 
> I would like to keep this "destory mutex" patch unchanged, and send another
> patch to fix the issue based on your approach. Please take a look. Thanks!
> 

Sure, good idea. I'd like to review the next fix, and provide my feed
back together. Thanks.

Coly



> Thanks,
> Liang
> 
> On Sun, Jul 2, 2017 at 2:43 AM, Coly Li <i@coly.li> wrote:
>> On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
>>> From: Liang Chen <liangchen.linux@gmail.com>
>>>
>>> mutex_destroy does nothing most of time, but it's better to call
>>> it to make the code future proof and it also has some meaning
>>> for like mutex debug.
>>>
>>> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
>>> Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/md/bcache/super.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>>> index 48b8c20..1f84791 100644
>>> --- a/drivers/md/bcache/super.c
>>> +++ b/drivers/md/bcache/super.c
>>> @@ -2089,6 +2089,7 @@ static void bcache_exit(void)
>>>       if (bcache_major)
>>>               unregister_blkdev(bcache_major, "bcache");
>>>       unregister_reboot_notifier(&reboot);
>>> +     mutex_destroy(&bch_register_lock>  }
>>>
>>>  static int __init bcache_init(void)
>>> @@ -2106,6 +2107,7 @@ static int __init bcache_init(void)
>>>
>>>       bcache_major = register_blkdev(0, "bcache");
>>>       if (bcache_major < 0) {
>>> +             mutex_destroy(&bch_register_lock);
>>>               unregister_reboot_notifier(&reboot);
>>>               return bcache_major;
>>>       }
>>>
>>
>> Hi Liang,
>>
>> Current code might have a potential race in a very corner case, see,
>> 2084 static int __init bcache_init(void)
>> 2085 {
>> 2086         static const struct attribute *files[] = {
>> 2087                 &ksysfs_register.attr,
>> 2088                 &ksysfs_register_quiet.attr,
>> 2089                 NULL
>> 2090         };
>> 2091
>> 2092         mutex_init(&bch_register_lock);
>> 2093         init_waitqueue_head(&unregister_wait);
>> 2094         register_reboot_notifier(&reboot);
>> 2095         closure_debug_init();
>> 2096
>> 2097         bcache_major = register_blkdev(0, "bcache");
>> 2098         if (bcache_major < 0) {
>> 2099                 unregister_reboot_notifier(&reboot);
>> 2100                 return bcache_major;
>> 2101         }
>> 2102
>> 2103         if (!(bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM,
>> 0)) ||
>> 2104             !(bcache_kobj = kobject_create_and_add("bcache",
>> fs_kobj)) ||
>> 2105             sysfs_create_files(bcache_kobj, files) ||
>> 2106             bch_request_init() ||
>> 2107             bch_debug_init(bcache_kobj))
>> 2108                 goto err;
>> 2109
>> 2110         return 0;
>> 2111 err:
>> 2112         bcache_exit();
>> 2113         return -ENOMEM;
>> 2114 }
>>
>> At line 2107, most of bache stuffs are ready to work, only a debugfs
>> entry not created yet. If in the time gap between line 2106 and line
>> 2017, another user space tool just registers cache and backing devices.
>> Then bch_debug_init() failed, and bcache_exit() gets called. In this
>> case, I doubt bcache_exit() can handle all the references correctly.
>>
>> The race is very rare, and almost won't happen in real life. So, if you
>> don't care about it, the patch can be simpler like this,
>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>> index e57353e39168..fb5453a46a03 100644
>> --- a/drivers/md/bcache/super.c
>> +++ b/drivers/md/bcache/super.c
>> @@ -2070,6 +2070,7 @@ static struct notifier_block reboot = {
>>
>>  static void bcache_exit(void)
>>  {
>> +       mutex_destroy(&bch_register_lock);
>>         bch_debug_exit();
>>         bch_request_exit();
>>         if (bcache_kobj)
>> @@ -2089,7 +2090,6 @@ static int __init bcache_init(void)
>>                 NULL
>>         };
>>
>> -       mutex_init(&bch_register_lock);
>>         init_waitqueue_head(&unregister_wait);
>>         register_reboot_notifier(&reboot);
>>         closure_debug_init();
>> @@ -2107,6 +2107,7 @@ static int __init bcache_init(void)
>>             bch_debug_init(bcache_kobj))
>>                 goto err;
>>
>> +       mutex_init(&bch_register_lock);
>>         return 0;
>>  err:
>>         bcache_exit();
>> ---
>> And if you do care about the race, maybe you should do something like this,
>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>> index e57353e39168..ca1d8b7a7815 100644
>> --- a/drivers/md/bcache/super.c
>> +++ b/drivers/md/bcache/super.c
>> @@ -2079,6 +2079,7 @@ static void bcache_exit(void)
>>         if (bcache_major)
>>                 unregister_blkdev(bcache_major, "bcache");
>>         unregister_reboot_notifier(&reboot);
>> +       mutex_unlock(&bch_register_lock);
>>  }
>>
>>  static int __init bcache_init(void)
>> @@ -2090,6 +2091,7 @@ static int __init bcache_init(void)
>>         };
>>
>>         mutex_init(&bch_register_lock);
>> +       mutex_lock(&bch_register_lock);
>>         init_waitqueue_head(&unregister_wait);
>>         register_reboot_notifier(&reboot);
>>         closure_debug_init();
>> @@ -2097,6 +2099,8 @@ static int __init bcache_init(void)
>>         bcache_major = register_blkdev(0, "bcache");
>>         if (bcache_major < 0) {
>>                 unregister_reboot_notifier(&reboot);
>> +               mutex_unlock(&bch_register_lock);
>> +               mutex_destroy(&bch_register_lock);
>>                 return bcache_major;
>>         }
>>
>> @@ -2104,9 +2108,12 @@ static int __init bcache_init(void)
>>             !(bcache_kobj = kobject_create_and_add("bcache", fs_kobj)) ||
>>             sysfs_create_files(bcache_kobj, files) ||
>>             bch_request_init() ||
>> -           bch_debug_init(bcache_kobj))
>> +           bch_debug_init(bcache_kobj)) {
>> +               mutex_unlock(&bch_register_lock);
>>                 goto err;
>> +       }
>>
>> +       mutex_unlock(&bch_register_lock);
>>         return 0;
>>  err:
>>         bcache_exit();
>> ---
>>
>> Personally I think the first approach with only one new line code added,
>> your original version will add two new lines of code.
>>
>> Just FYI. Thanks.
>>
>> --
>> Coly Li


-- 
Coly Li

Re: [PATCH 06/19] bcache: explicitly destory mutex while exiting

[Christoph Hellwig]

On Fri, Jun 30, 2017 at 01:42:55PM -0700, bcache@lists.ewheeler.net wrote:
> From: Liang Chen <liangchen.linux@gmail.com>
> 
> mutex_destroy does nothing most of time, but it's better to call
> it to make the code future proof and it also has some meaning
> for like mutex debug.

It shouldn't really - we should get the destroy behavior for free
when doing a slab free of the area.

What issue are you trying to solve?

Re: [PATCH 06/19] bcache: explicitly destory mutex while exiting

[Liang Chen]

mutex_destroy does nothing normally (may not be true in the future), but
when debug mutex is turned on it helps with debugging - mutex_destroy
in mutex-debug.c.
It's not about freeing of the memory. It's more about consistency of the
use of mutex and making the code future proof.

Thanks,
Linag

On Thu, Jul 6, 2017 at 2:27 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Jun 30, 2017 at 01:42:55PM -0700, bcache@lists.ewheeler.net wrote:
>> From: Liang Chen <liangchen.linux@gmail.com>
>>
>> mutex_destroy does nothing most of time, but it's better to call
>> it to make the code future proof and it also has some meaning
>> for like mutex debug.
>
> It shouldn't really - we should get the destroy behavior for free
> when doing a slab free of the area.
>
> What issue are you trying to solve?

[PATCH 07/19] bcache: introduce bcache sysfs entries for ioprio-based bypass/writeback hints

[bcache]

From: Eric Wheeler <git@linux.ewheeler.net>

Add sysfs entries to support to hint for bypass/writeback by the ioprio
assigned to the bio.  If the bio is unassigned, use current's io-context
ioprio for cache writeback or bypass (configured per-process with
`ionice`).

Having idle IOs bypass the cache can increase performance elsewhere
since you probably don't care about their performance.  In addition,
this prevents idle IOs from promoting into (polluting) your cache and
evicting blocks that are more important elsewhere.

If you really nead the performance at the expense of SSD wearout,
then configure ioprio_writeback and set your `ionice` appropriately.

For example:
	echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
	echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback

See the documentation commit for details.

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
Acked-by: Kent Overstreet <kent.overstreet@gmail.com>
Tested-by: Kai Krakow <kai@kaishome.de>
Cc: nix@esperi.org.uk
---
 drivers/md/bcache/bcache.h    |  3 ++
 drivers/md/bcache/request.c   | 24 +++++++++++++++
 drivers/md/bcache/sysfs.c     | 71 +++++++++++++++++++++++++++++++++++++++++++
 drivers/md/bcache/writeback.c |  8 +++++
 drivers/md/bcache/writeback.h | 24 +++++++++++++++
 5 files changed, 130 insertions(+)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index dee542f..44123e4 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -367,6 +367,9 @@ struct cached_dev {
 	unsigned		writeback_rate_update_seconds;
 	unsigned		writeback_rate_d_term;
 	unsigned		writeback_rate_p_term_inverse;
+
+	unsigned short		ioprio_writeback;
+	unsigned short		ioprio_bypass;
 };
 
 enum alloc_reserve {
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index d27707d..a95609f 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -373,6 +373,8 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	unsigned sectors, congested = bch_get_congested(c);
 	struct task_struct *task = current;
 	struct io *i;
+	struct io_context *ioc;
+	unsigned short ioprio;
 
 	if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
 	    c->gc_stats.in_use > CUTOFF_CACHE_ADD ||
@@ -384,6 +386,28 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	     op_is_write(bio_op(bio))))
 		goto skip;
 
+	/* If the ioprio already exists on the bio, use that.  We assume that
+	 * the upper layer properly assigned the calling process's ioprio to
+	 * the bio being passed to bcache. Otherwise, use current's ioc. */
+	ioprio = bio_prio(bio);
+	if (!ioprio_valid(ioprio)) {
+		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
+		if (ioc) {
+			if (ioprio_valid(ioc->ioprio))
+				ioprio = ioc->ioprio;
+			put_io_context(ioc);
+			ioc = NULL;
+		}
+	}
+
+	/* If process ioprio is lower-or-equal to dc->ioprio_bypass, then
+	 * hint for bypass. Note that a lower-priority IO class+value
+	 * has a greater numeric value. */
+	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
+		&& ioprio >= dc->ioprio_bypass) {
+		return true;
+	}
+
 	if (bio->bi_iter.bi_sector & (c->sb.block_size - 1) ||
 	    bio_sectors(bio) & (c->sb.block_size - 1)) {
 		pr_debug("skipping unaligned io");
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index f90f136..cc0076d 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -107,6 +107,9 @@ rw_attribute(btree_shrinker_disabled);
 rw_attribute(copy_gc_enabled);
 rw_attribute(size);
 
+rw_attribute(ioprio_writeback);
+rw_attribute(ioprio_bypass);
+
 SHOW(__bch_cached_dev)
 {
 	struct cached_dev *dc = container_of(kobj, struct cached_dev,
@@ -183,6 +186,17 @@ SHOW(__bch_cached_dev)
 		return strlen(buf);
 	}
 
+	if (attr == &sysfs_ioprio_bypass)
+		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
+			IOPRIO_PRIO_CLASS(dc->ioprio_bypass),
+			IOPRIO_PRIO_DATA(dc->ioprio_bypass));
+
+	if (attr == &sysfs_ioprio_writeback)
+		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
+			IOPRIO_PRIO_CLASS(dc->ioprio_writeback),
+			IOPRIO_PRIO_DATA(dc->ioprio_writeback));
+
+
 #undef var
 	return 0;
 }
@@ -195,6 +209,10 @@ STORE(__cached_dev)
 	unsigned v = size;
 	struct cache_set *c;
 	struct kobj_uevent_env *env;
+	unsigned ioprio_class = 0; /* invalid initial ioprio values */
+	unsigned ioprio_level = IOPRIO_BE_NR;
+	unsigned short *ioprio_hint = NULL;
+	char *ioprio_type = NULL;
 
 #define d_strtoul(var)		sysfs_strtoul(var, dc->var)
 #define d_strtoul_nonzero(var)	sysfs_strtoul_clamp(var, dc->var, 1, INT_MAX)
@@ -283,6 +301,57 @@ STORE(__cached_dev)
 	if (attr == &sysfs_stop)
 		bcache_device_stop(&dc->disk);
 
+	/* ioprio hinting: we use ioprio_hint to reduce duplicate printk verbiage */
+	if (attr == &sysfs_ioprio_writeback) {
+		ioprio_hint = &dc->ioprio_writeback;
+		ioprio_type = "writeback";
+	}
+
+	if (attr == &sysfs_ioprio_bypass) {
+		ioprio_hint = &dc->ioprio_bypass;
+		ioprio_type = "bypass";
+	}
+
+	if (ioprio_hint != NULL)
+	{
+		if (sscanf(buf, "%u,%u", &ioprio_class, &ioprio_level) != 2
+			|| ioprio_class > IOPRIO_CLASS_IDLE
+			|| ioprio_level >= IOPRIO_BE_NR) {
+			pr_err("ioprio_%s invalid, expecting: (class,level) but parsed (%u,%u); ignored.",
+				ioprio_type,
+				ioprio_class, ioprio_level);
+			return size;
+		}
+
+		/* Use the maximum(/minimum) value in the class shift space to make integer
+		  comparison correct for ioprio_writeback(/ioprio_bypass) for IOPRIO_CLASS_IDLE.
+		  This is necessary because there are no ioprio levels for the idle class. */
+		if (ioprio_class == IOPRIO_CLASS_IDLE) {
+			if (ioprio_hint == &dc->ioprio_writeback)
+				ioprio_level = IOPRIO_PRIO_MASK;
+			else
+				/* Same, but 0 for bypass (inverted vs. writeback) */
+				ioprio_level = 0;
+		}
+
+		*ioprio_hint = IOPRIO_PRIO_VALUE(ioprio_class, ioprio_level);
+
+		if (!ioprio_valid(*ioprio_hint))
+			pr_info("disabled ioprio_%s hints.", ioprio_type);
+		else
+			pr_info("set hint for cache %s with priority %s: (class,level) = (%u,%u)",
+				ioprio_type,
+				( ioprio_hint == &dc->ioprio_writeback ? "at-or-above" : "at-or-below" ),
+				ioprio_class, ioprio_level);
+
+		if (ioprio_valid(dc->ioprio_writeback)
+			&& ioprio_valid(dc->ioprio_bypass)
+			&& dc->ioprio_writeback >= dc->ioprio_bypass)
+			pr_warning(
+				"warning: ioprio_writeback hint is neither disabled nor higher priority than the bypass hint; "
+				"will always writeback!");
+	}
+
 	return size;
 }
 
@@ -335,6 +404,8 @@ static struct attribute *bch_cached_dev_files[] = {
 	&sysfs_verify,
 	&sysfs_bypass_torture_test,
 #endif
+	&sysfs_ioprio_bypass,
+	&sysfs_ioprio_writeback,
 	NULL
 };
 KTYPE(bch_cached_dev);
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 42c66e7..3d463f0 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -511,6 +511,14 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
 	dc->writeback_rate_d_term	= 30;
 	dc->writeback_rate_p_term_inverse = 6000;
 
+	/* These defaults provide the best SSD life by enabling bypass
+	 for priorities at-or-below BE-7. This also provides better
+	 performance (cache hits) by preventing (near-)idle processes from
+	 polluting the cache working set.  Only set ioprio_writeback if
+	 you really need it: it will wear out your SSD sooner. */
+	dc->ioprio_writeback = IOPRIO_PRIO_VALUE(0, 0);
+	dc->ioprio_bypass    = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, (IOPRIO_BE_NR-1));
+
 	INIT_DELAYED_WORK(&dc->writeback_rate_update, update_writeback_rate);
 }
 
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index 629bd1a..cd82fe8 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -43,6 +43,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 				    unsigned cache_mode, bool would_skip)
 {
 	unsigned in_use = dc->disk.c->gc_stats.in_use;
+	struct io_context *ioc;
+	unsigned short ioprio;
 
 	if (cache_mode != CACHE_MODE_WRITEBACK ||
 	    test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
@@ -57,6 +59,28 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 	if (would_skip)
 		return false;
 
+	/* If the ioprio already exists on the bio, use that.  We assume that
+	 * the upper layer properly assigned the calling process's ioprio to
+	 * the bio being passed to bcache. Otherwise, use current's ioc. */
+	ioprio = bio_prio(bio);
+	if (!ioprio_valid(ioprio)) {
+		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
+		if (ioc) {
+			if (ioprio_valid(ioc->ioprio))
+				ioprio = ioc->ioprio;
+			put_io_context(ioc);
+			ioc = NULL;
+		}
+	}
+
+	/* If process ioprio is higher-or-equal to dc->ioprio_writeback, then
+	 * hint for writeback. Note that a higher-priority IO class+value
+	 * has a lesser numeric value. */
+	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
+		&& ioprio <= dc->ioprio_writeback) {
+		return true;
+	}
+
 	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
 }
 
-- 
1.8.3.1

Re: [PATCH 07/19] bcache: introduce bcache sysfs entries for ioprio-based bypass/writeback hints

[Christoph Hellwig]

On Fri, Jun 30, 2017 at 01:42:56PM -0700, bcache@lists.ewheeler.net wrote:
> From: Eric Wheeler <git@linux.ewheeler.net>
> 
> Add sysfs entries to support to hint for bypass/writeback by the ioprio
> assigned to the bio.  If the bio is unassigned, use current's io-context
> ioprio for cache writeback or bypass (configured per-process with
> `ionice`).
> 
> Having idle IOs bypass the cache can increase performance elsewhere
> since you probably don't care about their performance.  In addition,
> this prevents idle IOs from promoting into (polluting) your cache and
> evicting blocks that are more important elsewhere.
> 
> If you really nead the performance at the expense of SSD wearout,
> then configure ioprio_writeback and set your `ionice` appropriately.
> 
> For example:
> 	echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
> 	echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback
> 
> See the documentation commit for details.

I'm really worried about this interface, as it basically uses the
ioprio field for side channel communication - your app must know
which value it wants, and you need to configure bcache to fit
exacltly that scheme.

> +	/* If the ioprio already exists on the bio, use that.  We assume that
> +	 * the upper layer properly assigned the calling process's ioprio to
> +	 * the bio being passed to bcache. Otherwise, use current's ioc. */

Please make this fit the normal kernel comment style.

> +	ioprio = bio_prio(bio);
> +	if (!ioprio_valid(ioprio)) {
> +		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
> +		if (ioc) {
> +			if (ioprio_valid(ioc->ioprio))
> +				ioprio = ioc->ioprio;
> +			put_io_context(ioc);
> +			ioc = NULL;
> +		}
> +	}

While get_task_io_context currently is exported it really should not
be - we should only allocate on when setting the io priority or when
forking.

What this code really wants is the ioprio related lines of code from
blk_init_request_from_bio, which should be factored into a new helper.

> +	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
> +		&& ioprio >= dc->ioprio_bypass) {
> +		return true;
> +	}

Incorrect indentation, this shold be:

	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback) &&
	    ioprio >= dc->ioprio_bypass)
		return true;

And there is some more of this in this and the following patches.
Please run them through something like checkpatch.pl

> +
>  SHOW(__bch_cached_dev)
>  {
>  	struct cached_dev *dc = container_of(kobj, struct cached_dev,
> @@ -183,6 +186,17 @@ SHOW(__bch_cached_dev)
>  		return strlen(buf);
>  	}
>  
> +	if (attr == &sysfs_ioprio_bypass)
> +		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
> +			IOPRIO_PRIO_CLASS(dc->ioprio_bypass),
> +			IOPRIO_PRIO_DATA(dc->ioprio_bypass));
> +
> +	if (attr == &sysfs_ioprio_writeback)
> +		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
> +			IOPRIO_PRIO_CLASS(dc->ioprio_writeback),
> +			IOPRIO_PRIO_DATA(dc->ioprio_writeback));
> +
> +

Please implement separate sysfs show and store function for your new
attributes instead of overloading all of them into a giant mess.

Re: [PATCH 07/19] bcache: introduce bcache sysfs entries for ioprio-based bypass/writeback hints

[Eric Wheeler]

On Wed, 5 Jul 2017, Christoph Hellwig wrote:
> On Fri, Jun 30, 2017 at 01:42:56PM -0700, bcache@lists.ewheeler.net wrote:
> > From: Eric Wheeler <git@linux.ewheeler.net>
> > 
> > Add sysfs entries to support to hint for bypass/writeback by the ioprio
> > assigned to the bio.  If the bio is unassigned, use current's io-context
> > ioprio for cache writeback or bypass (configured per-process with
> > `ionice`).
> > 
> > Having idle IOs bypass the cache can increase performance elsewhere
> > since you probably don't care about their performance.  In addition,
> > this prevents idle IOs from promoting into (polluting) your cache and
> > evicting blocks that are more important elsewhere.
> > 
> > If you really nead the performance at the expense of SSD wearout,
> > then configure ioprio_writeback and set your `ionice` appropriately.
> > 
> > For example:
> > 	echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
> > 	echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback
> > 
> > See the documentation commit for details.
> 
> I'm really worried about this interface, as it basically uses the
> ioprio field for side channel communication - your app must know
> which value it wants, and you need to configure bcache to fit
> exacltly that scheme.
>
> 
> > +	/* If the ioprio already exists on the bio, use that.  We assume that
> > +	 * the upper layer properly assigned the calling process's ioprio to
> > +	 * the bio being passed to bcache. Otherwise, use current's ioc. */
> 
> Please make this fit the normal kernel comment style.

ok
 
> > +	ioprio = bio_prio(bio);
> > +	if (!ioprio_valid(ioprio)) {
> > +		ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
> > +		if (ioc) {
> > +			if (ioprio_valid(ioc->ioprio))
> > +				ioprio = ioc->ioprio;
> > +			put_io_context(ioc);
> > +			ioc = NULL;
> > +		}
> > +	}
> 
> While get_task_io_context currently is exported it really should not
> be - we should only allocate on when setting the io priority or when
> forking.
> 
> What this code really wants is the ioprio related lines of code from
> blk_init_request_from_bio, which should be factored into a new helper.
> 
> > +	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
> > +		&& ioprio >= dc->ioprio_bypass) {
> > +		return true;
> > +	}
> 
> Incorrect indentation, this shold be:
> 
> 	if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback) &&
> 	    ioprio >= dc->ioprio_bypass)
> 		return true;
> 
> And there is some more of this in this and the following patches.
> Please run them through something like checkpatch.pl

Good idea, will do.

> 
> > +
> >  SHOW(__bch_cached_dev)
> >  {
> >  	struct cached_dev *dc = container_of(kobj, struct cached_dev,
> > @@ -183,6 +186,17 @@ SHOW(__bch_cached_dev)
> >  		return strlen(buf);
> >  	}
> >  
> > +	if (attr == &sysfs_ioprio_bypass)
> > +		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
> > +			IOPRIO_PRIO_CLASS(dc->ioprio_bypass),
> > +			IOPRIO_PRIO_DATA(dc->ioprio_bypass));
> > +
> > +	if (attr == &sysfs_ioprio_writeback)
> > +		return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
> > +			IOPRIO_PRIO_CLASS(dc->ioprio_writeback),
> > +			IOPRIO_PRIO_DATA(dc->ioprio_writeback));
> > +
> > +
> 
> Please implement separate sysfs show and store function for your new
> attributes instead of overloading all of them into a giant mess.

ok.

>>

Christoph, thank you for your commentary and quick turn around on all of 
these patches!

--
Eric Wheeler




> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 08/19] bcache: documentation for sysfs entries describing bcache cache hinting

[bcache]

From: Eric Wheeler <git@linux.ewheeler.net>

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
---
 Documentation/bcache.txt | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/Documentation/bcache.txt b/Documentation/bcache.txt
index a9259b5..c78c012 100644
--- a/Documentation/bcache.txt
+++ b/Documentation/bcache.txt
@@ -133,6 +133,86 @@ the backing devices to passthrough mode.
    writeback mode). It currently doesn't do anything intelligent if it fails to
    read some of the dirty data, though.
 
+SSD LONGEVITY: PER-PROCESS CACHE HINTING WITH IO PRIORITY
+---------------------------------------------------------
+
+Processes can be assigned an IO priority using `ionice` and bcache will
+either try to writeback or bypass the cache based on the IO priority
+level assigned to the process and the configuration of the syfs ioprio
+hints.  If configured properly for your workload, this can both increase
+performance and reduce SSD wear (erase/write cycles).
+
+Having idle IOs bypass the cache can increase performance elsewhere
+since you probably don't care about their performance.  In addition,
+this prevents idle IOs from promoting into (polluting) your cache and
+evicting blocks that are more important elsewhere.
+
+Default sysfs values:
+	2,7: ioprio_bypass is hinted for process IOs at-or-below best-effort-7.
+	0,0: ioprio_writeback hinting is disabled by default.
+
+Cache hinting is configured by writing 'class,level' pairs to sysfs.
+In this example, we write the following:
+
+    echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
+    echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback
+
+Thus, processes with the following IO class (ionice -c) and level (-n)
+will the behave as shown in this table:
+
+	(-c) IO Class    (-n) Class level       Action
+	-----------------------------------------------------
+	(1) Realtime      0-7                   Writeback
+	(2) Best-effort     0                   Writeback
+	(2) Best-effort   1-6                   Normal, as if hinting were disabled
+	(2) Best-effort     7                   Bypass cache
+	(3) Idle          n/a                   Bypass cache
+
+For processes at-or-below best-effort-7 (ionice -c2 -n7), the
+ioprio_bypass behavior is as follows:
+
+* Reads will come from the backing device and will not promote into
+  (pollute) your cache.  If the block being read was already in the cache,
+  then it will be read from the cache (and remain cached).
+
+* If you are using writeback mode, then low-priority bypass-hinted writes
+  will go directly to the backing device.  If the write was dirty in
+  cache, it will cache-invalidate and write directly to the backing
+  device.  If a high-priority task later writes the same block then it
+  will writeback so no performance is lost for write-after-write.
+
+  For read-after-bypassed-write, the block will be read from the backing
+  device (not cached) so there may be a miss penalty when a low-priority
+  process write bypasses the cache followed by a high-priority read that
+  would otherwise have hit.  In practice, this is not an issue; to date,
+  none have wanted low-priority writes and high-priority reads of the
+  same block.
+
+For processes in our example at-or-above best-effort-0 (ionice -c2 -n0),
+the ioprio_writeback behavior is as follows:
+
+* The writeback hint has no effect unless your 'cache_mode' is writeback.
+  Assuming writeback mode, all writes at this priority will writeback.
+  Of course this will increase SSD wear, so only use writeback hinting
+  if you need it.
+
+* Reads are unaffected by ioprio_writeback, except that read-after-write
+  will of course read from the cache.
+
+Linux assigns processes the best-effort class with a level of 4 if
+no process is assigned  Thus, without `ionice` your processes will
+follow normal bcache should_writeback/should_bypass symantecs as if the
+ioprio_writeback/ioprio_bypass sysfs flags were disabled.
+
+Also note that in order to be hinted by ioprio_writeback/ioprio_bypass,
+the process must have a valid ioprio setting as returned by
+get_task_io_context()->ioprio. Thus, a process without an IO context
+will be ignored by the ioprio_writeback/ioprio_bypass hints even if your
+sysfs hints specify that best-effort-4 should be flagged for bypass
+or writeback.  If in doubt, explicitly set the process IO priority with
+`ionice`.
+
+See `man ionice` for more detail about per-process IO priority in Linux.
 
 HOWTO/COOKBOOK
 --------------
-- 
1.8.3.1

Re: [PATCH 08/19] bcache: documentation for sysfs entries describing bcache cache hinting

[Christoph Hellwig]

should go into the previous patch.

[PATCH 09/19] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[bcache]

From: Eric Wheeler <git@linux.ewheeler.net>

Bypass if:     bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND)

Writeback if:  op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
---
 drivers/md/bcache/request.c   | 3 +++
 drivers/md/bcache/writeback.h | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index a95609f..4629b0c 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -386,6 +386,9 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	     op_is_write(bio_op(bio))))
 		goto skip;
 
+	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))
+		goto skip;
+
 	/* If the ioprio already exists on the bio, use that.  We assume that
 	 * the upper layer properly assigned the calling process's ioprio to
 	 * the bio being passed to bcache. Otherwise, use current's ioc. */
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index cd82fe8..ea2f92e 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 		return true;
 	}
 
-	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
+	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
+		|| in_use <= CUTOFF_WRITEBACK;
 }
 
 static inline void bch_writeback_queue(struct cached_dev *dc)
-- 
1.8.3.1

Re: [PATCH 09/19] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Coly Li]

On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
> From: Eric Wheeler <git@linux.ewheeler.net>
> 
> Bypass if:     bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND)
> 
> Writeback if:  op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
> 

Hi Eric,

Could you please explain a bit how the above policy is designed ? I'd
like to understand it more.

Thanks.

Coly


> Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
> ---
>  drivers/md/bcache/request.c   | 3 +++
>  drivers/md/bcache/writeback.h | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index a95609f..4629b0c 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -386,6 +386,9 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
>  	     op_is_write(bio_op(bio))))
>  		goto skip;
>  
> +	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))
> +		goto skip;
> +
>  	/* If the ioprio already exists on the bio, use that.  We assume that
>  	 * the upper layer properly assigned the calling process's ioprio to
>  	 * the bio being passed to bcache. Otherwise, use current's ioc. */
> diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
> index cd82fe8..ea2f92e 100644
> --- a/drivers/md/bcache/writeback.h
> +++ b/drivers/md/bcache/writeback.h
> @@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
>  		return true;
>  	}
>  
> -	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
> +	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
> +		|| in_use <= CUTOFF_WRITEBACK;
>  }
>  
>  static inline void bch_writeback_queue(struct cached_dev *dc)
> 

Re: [PATCH 09/19] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Eric Wheeler]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2909 bytes --]

On Sun, 2 Jul 2017, Coly Li wrote:

> On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
> > From: Eric Wheeler <git@linux.ewheeler.net>
> > 
> > Bypass if:     bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND)
> > 
> > Writeback if:  op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
> > 
> 
> Hi Eric,
> 
> Could you please explain a bit how the above policy is designed ? I'd
> like to understand it more.

Hi Coly,

It is pretty trivial, all of the processing code exists already.  This 
patch only adds to the existing functions for the constraints noted in the 
patch.

What happens is this: cached_dev_make_request() in request.c takes the IO 
decides where the IO should go by using these two functions to decide 
whether a bio should writeback or bypass:
	check_should_bypass()
and
	should_writeback()

In check_should_bypass(), `if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))`
means bypass the cache because the bio has been flagged as a readahead or 
background IO (no sense in polluting cache in either case).

Later, if the bio is a write, then cached_dev_write() checks 
should_writeback()

In bio->bi_opf & (REQ_META|REQ_PRIO), it means writeback to cache.  Some 
filesystems use REQ_META for metadata operations which are best to keep 
low-latency for high transactional performance.  REQ_PRIO is a CFQ hint 
that the priority of the IO has been raised, so writeback is faster here.  


--
Eric Wheeler



> 
> Thanks.
> 
> Coly
> 
> 
> > Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
> > ---
> >  drivers/md/bcache/request.c   | 3 +++
> >  drivers/md/bcache/writeback.h | 3 ++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> > index a95609f..4629b0c 100644
> > --- a/drivers/md/bcache/request.c
> > +++ b/drivers/md/bcache/request.c
> > @@ -386,6 +386,9 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
> >  	     op_is_write(bio_op(bio))))
> >  		goto skip;
> >  
> > +	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))
> > +		goto skip;
> > +
> >  	/* If the ioprio already exists on the bio, use that.  We assume that
> >  	 * the upper layer properly assigned the calling process's ioprio to
> >  	 * the bio being passed to bcache. Otherwise, use current's ioc. */
> > diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
> > index cd82fe8..ea2f92e 100644
> > --- a/drivers/md/bcache/writeback.h
> > +++ b/drivers/md/bcache/writeback.h
> > @@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
> >  		return true;
> >  	}
> >  
> > -	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
> > +	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
> > +		|| in_use <= CUTOFF_WRITEBACK;
> >  }
> >  
> >  static inline void bch_writeback_queue(struct cached_dev *dc)
> > 
> 

Re: [PATCH 09/19] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Coly Li]

On 2017/7/2 上午3:39, Eric Wheeler wrote:
> On Sun, 2 Jul 2017, Coly Li wrote:
> 
>> On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
>>> From: Eric Wheeler <git@linux.ewheeler.net>
>>>
>>> Bypass if:     bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND)
>>>
>>> Writeback if:  op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
>>>
>>
>> Hi Eric,
>>
>> Could you please explain a bit how the above policy is designed ? I'd
>> like to understand it more.
> 
> Hi Coly,
> 
> It is pretty trivial, all of the processing code exists already.  This 
> patch only adds to the existing functions for the constraints noted in the 
> patch.
> 
> What happens is this: cached_dev_make_request() in request.c takes the IO 
> decides where the IO should go by using these two functions to decide 
> whether a bio should writeback or bypass:
> 	check_should_bypass()
> and
> 	should_writeback()
> 
> In check_should_bypass(), `if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))`
> means bypass the cache because the bio has been flagged as a readahead or 
> background IO (no sense in polluting cache in either case).
> 
> Later, if the bio is a write, then cached_dev_write() checks 
> should_writeback()
> 
> In bio->bi_opf & (REQ_META|REQ_PRIO), it means writeback to cache.  Some 
> filesystems use REQ_META for metadata operations which are best to keep 
> low-latency for high transactional performance.  REQ_PRIO is a CFQ hint 
> that the priority of the IO has been raised, so writeback is faster here.  
> 
> 

Thanks for the detailed information. I come to understand :-)

For writing case, I agree that metadata should be kept in cache device.
For reading case, there is a special case for gfs2. gfs2 sets
(REQ_RAHEAD | REQ_META) both for meta data read ahead code path, all
other file systems use (REQ_META | REQ_PRIO) when doing metadata read
ahead. I don't know whether this is something should be fixed in gfs2,
but currently maybe we should also check REQ_META,

+ /* if the read ahead request is for metadata, don't skip it */
+ if ((bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND)) &&
+     !(bio->bi_opf & REQ_META))
+ 	goto skip;

At least we can avoid a potential performance regression here.

And could you please add the above information in patch comments.

Thank you in advance.

Coly

>>
>>> Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
>>> ---
>>>  drivers/md/bcache/request.c   | 3 +++
>>>  drivers/md/bcache/writeback.h | 3 ++-
>>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
>>> index a95609f..4629b0c 100644
>>> --- a/drivers/md/bcache/request.c
>>> +++ b/drivers/md/bcache/request.c
>>> @@ -386,6 +386,9 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
>>>  	     op_is_write(bio_op(bio))))
>>>  		goto skip;
>>>  
>>> +	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND))
>>> +		goto skip;
>>> +
>>>  	/* If the ioprio already exists on the bio, use that.  We assume that
>>>  	 * the upper layer properly assigned the calling process's ioprio to
>>>  	 * the bio being passed to bcache. Otherwise, use current's ioc. */
>>> diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
>>> index cd82fe8..ea2f92e 100644
>>> --- a/drivers/md/bcache/writeback.h
>>> +++ b/drivers/md/bcache/writeback.h
>>> @@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
>>>  		return true;
>>>  	}
>>>  
>>> -	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
>>> +	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
>>> +		|| in_use <= CUTOFF_WRITEBACK;
>>>  }
>>>  
>>>  static inline void bch_writeback_queue(struct cached_dev *dc)
>>>

[PATCH 09/19 v2] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[bcache]

From: Eric Wheeler <git@linux.ewheeler.net>

Flag for bypass if the IO is for read-ahead or background, unless the
read-ahead request is for metadata (eg, from gfs2).
	Bypass if:
		bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) && !(bio->bi_opf & REQ_META))

	Writeback if:
		op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)

Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>
---
 drivers/md/bcache/request.c   | 7 +++++++
 drivers/md/bcache/writeback.h | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index a95609f..859d08d 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -386,6 +386,13 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	     op_is_write(bio_op(bio))))
 		goto skip;
 
+	/* Flag for bypass if the IO is for read-ahead or background,
+	 * unless the read-ahead request is for metadata (eg, for gfs2).
+	 */
+	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
+		!(bio->bi_opf & REQ_META))
+		goto skip;
+
 	/* If the ioprio already exists on the bio, use that.  We assume that
 	 * the upper layer properly assigned the calling process's ioprio to
 	 * the bio being passed to bcache. Otherwise, use current's ioc. */
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index cd82fe8..ea2f92e 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
 		return true;
 	}
 
-	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
+	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
+		|| in_use <= CUTOFF_WRITEBACK;
 }
 
 static inline void bch_writeback_queue(struct cached_dev *dc)
-- 
1.8.3.1

Re: [PATCH 09/19 v2] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Coly Li]

On 2017/7/4 上午6:51, bcache@lists.ewheeler.net wrote:
> From: Eric Wheeler <git@linux.ewheeler.net>
> 
> Flag for bypass if the IO is for read-ahead or background, unless the
> read-ahead request is for metadata (eg, from gfs2).
> 	Bypass if:
> 		bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) && !(bio->bi_opf & REQ_META))
> 
> 	Writeback if:
> 		op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
> 
> Signed-off-by: Eric Wheeler <bcache@linux.ewheeler.net>

Reviewed-by: Coly Li <colyli@suse.de>

I don't have any more suggestion, thanks.

Coly

> ---
>  drivers/md/bcache/request.c   | 7 +++++++
>  drivers/md/bcache/writeback.h | 3 ++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index a95609f..859d08d 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -386,6 +386,13 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
>  	     op_is_write(bio_op(bio))))
>  		goto skip;
>  
> +	/* Flag for bypass if the IO is for read-ahead or background,
> +	 * unless the read-ahead request is for metadata (eg, for gfs2).
> +	 */
> +	if (bio->bi_opf & (REQ_RAHEAD|REQ_BACKGROUND) &&
> +		!(bio->bi_opf & REQ_META))
> +		goto skip;
> +
>  	/* If the ioprio already exists on the bio, use that.  We assume that
>  	 * the upper layer properly assigned the calling process's ioprio to
>  	 * the bio being passed to bcache. Otherwise, use current's ioc. */
> diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
> index cd82fe8..ea2f92e 100644
> --- a/drivers/md/bcache/writeback.h
> +++ b/drivers/md/bcache/writeback.h
> @@ -81,7 +81,8 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
>  		return true;
>  	}
>  
> -	return op_is_sync(bio->bi_opf) || in_use <= CUTOFF_WRITEBACK;
> +	return op_is_sync(bio->bi_opf) || bio->bi_opf & (REQ_META|REQ_PRIO)
> +		|| in_use <= CUTOFF_WRITEBACK;
>  }
>  
>  static inline void bch_writeback_queue(struct cached_dev *dc)

Re: [PATCH 09/19 v2] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Christoph Hellwig]

REQ_META should be purely a hint for blktrace, please don't use it
in the I/O path - that's what REQ_PRIO is for.

Re: [PATCH 09/19 v2] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Coly Li]

On 2017/7/6 上午2:48, Christoph Hellwig wrote:
> REQ_META should be purely a hint for blktrace, please don't use it
> in the I/O path - that's what REQ_PRIO is for.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Hi Christoph,

Then gfs2 breaks the above rule ?  in gfs2_metapath_ra() and
gfs2_dir_readahead(), only REQ_META is used in submit_bh(). It seems an
extra REQ_PRIO should be there.

I see 'commit 70246286e94c3 ("block: get rid of bio_rw and READA")' is
from you, could you please provide your point on adding REQ_PRIO on the
above 2 locations ?

IMHO another patch to add REQ_PRIO to gfs2 meta data read ahead code
path should be necessary.

Thanks.


-- 
Coly Li

Re: [PATCH 09/19 v2] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Christoph Hellwig]

On Thu, Jul 06, 2017 at 03:35:48PM +0800, Coly Li wrote:
> Then gfs2 breaks the above rule ?  in gfs2_metapath_ra() and
> gfs2_dir_readahead(), only REQ_META is used in submit_bh(). It seems an
> extra REQ_PRIO should be there.

Or maybe not.  E.g. XFS absolutely avoids using REQ_PRIO for any of
the buffer writeback as it does not have priority.  You'll need to
ask gfs2 folks if they want this I/O to have priority or not.

Re: [PATCH 09/19 v2] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Coly Li]

On 2017/7/6 下午11:24, Christoph Hellwig wrote:
> On Thu, Jul 06, 2017 at 03:35:48PM +0800, Coly Li wrote:
>> Then gfs2 breaks the above rule ?  in gfs2_metapath_ra() and
>> gfs2_dir_readahead(), only REQ_META is used in submit_bh(). It seems an
>> extra REQ_PRIO should be there.
> 
> Or maybe not.  E.g. XFS absolutely avoids using REQ_PRIO for any of
> the buffer writeback as it does not have priority.  You'll need to
> ask gfs2 folks if they want this I/O to have priority or not.
> 

I see. Just sent a RFC patch to gfs2 development list, and wait for
response.

Thanks for the hint.

Re: [PATCH 09/19 v2] bcache: update bio->bi_opf bypass/writeback REQ_ flag hints

[Coly Li]

On 2017/7/11 上午11:48, Coly Li wrote:
> On 2017/7/6 下午11:24, Christoph Hellwig wrote:
>> On Thu, Jul 06, 2017 at 03:35:48PM +0800, Coly Li wrote:
>>> Then gfs2 breaks the above rule ?  in gfs2_metapath_ra() and
>>> gfs2_dir_readahead(), only REQ_META is used in submit_bh(). It seems an
>>> extra REQ_PRIO should be there.
>>
>> Or maybe not.  E.g. XFS absolutely avoids using REQ_PRIO for any of
>> the buffer writeback as it does not have priority.  You'll need to
>> ask gfs2 folks if they want this I/O to have priority or not.
>>
> 
> I see. Just sent a RFC patch to gfs2 development list, and wait for
> response.
> 
> Thanks for the hint.

Hi Eric,

By comments from Christoph, I think the original version is correct
enough. gfs2 is the only file system uses REQ_READAHEAD for metadata
readahead, if gfs2 developers agree to add REQ_PRIO when issue metadata
bio, we can use another patch to cache its metadata in bcache. Currently
we can just let this patch go ahead.

Thanks for your patient explaining to my question, and thanks to
Christoph to correct the concept of REQ_META and REQ_PRIO.

Please add Reviewed-by: Coly Li <colyli@suse.de> to your original
version patch.


Coly Li

[PATCH 10/19] bcache: initialize stripe_sectors_dirty correctly for thin flash device

[bcache]

From: Tang Junhui <tang.junhui@zte.com.cn>

Thin flash device does not initialize stripe_sectors_dirty correctly, this
patch fixes this issue.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/super.c     | 3 ++-
 drivers/md/bcache/writeback.c | 8 ++++----
 drivers/md/bcache/writeback.h | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 1f84791..e06641e 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1030,7 +1030,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c)
 	}
 
 	if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
-		bch_sectors_dirty_init(dc);
+		bch_sectors_dirty_init(&dc->disk);
 		atomic_set(&dc->has_dirty, 1);
 		atomic_inc(&dc->count);
 		bch_writeback_queue(dc);
@@ -1232,6 +1232,7 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 		goto err;
 
 	bcache_device_attach(d, c, u - c->uuids);
+	bch_sectors_dirty_init(d);
 	bch_flash_dev_request_init(d);
 	add_disk(d->disk);
 
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 3d463f0..4ac8b13 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -482,17 +482,17 @@ static int sectors_dirty_init_fn(struct btree_op *_op, struct btree *b,
 	return MAP_CONTINUE;
 }
 
-void bch_sectors_dirty_init(struct cached_dev *dc)
+void bch_sectors_dirty_init(struct bcache_device *d)
 {
 	struct sectors_dirty_init op;
 
 	bch_btree_op_init(&op.op, -1);
-	op.inode = dc->disk.id;
+	op.inode = d->id;
 
-	bch_btree_map_keys(&op.op, dc->disk.c, &KEY(op.inode, 0, 0),
+	bch_btree_map_keys(&op.op, d->c, &KEY(op.inode, 0, 0),
 			   sectors_dirty_init_fn, 0);
 
-	dc->disk.sectors_dirty_last = bcache_dev_sectors_dirty(&dc->disk);
+	d->sectors_dirty_last = bcache_dev_sectors_dirty(d);
 }
 
 void bch_cached_dev_writeback_init(struct cached_dev *dc)
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index ea2f92e..c2ab4b4 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -109,7 +109,7 @@ static inline void bch_writeback_add(struct cached_dev *dc)
 
 void bcache_dev_sectors_dirty_add(struct cache_set *, unsigned, uint64_t, int);
 
-void bch_sectors_dirty_init(struct cached_dev *dc);
+void bch_sectors_dirty_init(struct bcache_device *);
 void bch_cached_dev_writeback_init(struct cached_dev *);
 int bch_cached_dev_writeback_start(struct cached_dev *);
 
-- 
1.8.3.1

Re: [PATCH 10/19] bcache: initialize stripe_sectors_dirty correctly for thin flash device

[Coly Li]

On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
> Thin flash device does not initialize stripe_sectors_dirty correctly, this
> patch fixes this issue.

Hi Junhui,

Could you please explain why stripe_sectors_ditry is not correctly
initialized and how about its negative result ?

Thanks.

Coly

> 
> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
> Cc: stable@vger.kernel.org
> ---
>  drivers/md/bcache/super.c     | 3 ++-
>  drivers/md/bcache/writeback.c | 8 ++++----
>  drivers/md/bcache/writeback.h | 2 +-
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 1f84791..e06641e 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1030,7 +1030,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c)
>  	}
>  
>  	if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
> -		bch_sectors_dirty_init(dc);
> +		bch_sectors_dirty_init(&dc->disk);
>  		atomic_set(&dc->has_dirty, 1);
>  		atomic_inc(&dc->count);
>  		bch_writeback_queue(dc);
> @@ -1232,6 +1232,7 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
>  		goto err;
>  
>  	bcache_device_attach(d, c, u - c->uuids);
> +	bch_sectors_dirty_init(d);
>  	bch_flash_dev_request_init(d);
>  	add_disk(d->disk);
>  
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 3d463f0..4ac8b13 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -482,17 +482,17 @@ static int sectors_dirty_init_fn(struct btree_op *_op, struct btree *b,
>  	return MAP_CONTINUE;
>  }
>  
> -void bch_sectors_dirty_init(struct cached_dev *dc)
> +void bch_sectors_dirty_init(struct bcache_device *d)
>  {
>  	struct sectors_dirty_init op;
>  
>  	bch_btree_op_init(&op.op, -1);
> -	op.inode = dc->disk.id;
> +	op.inode = d->id;
>  
> -	bch_btree_map_keys(&op.op, dc->disk.c, &KEY(op.inode, 0, 0),
> +	bch_btree_map_keys(&op.op, d->c, &KEY(op.inode, 0, 0),
>  			   sectors_dirty_init_fn, 0);
>  
> -	dc->disk.sectors_dirty_last = bcache_dev_sectors_dirty(&dc->disk);
> +	d->sectors_dirty_last = bcache_dev_sectors_dirty(d);
>  }
>  
>  void bch_cached_dev_writeback_init(struct cached_dev *dc)
> diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
> index ea2f92e..c2ab4b4 100644
> --- a/drivers/md/bcache/writeback.h
> +++ b/drivers/md/bcache/writeback.h
> @@ -109,7 +109,7 @@ static inline void bch_writeback_add(struct cached_dev *dc)
>  
>  void bcache_dev_sectors_dirty_add(struct cache_set *, unsigned, uint64_t, int);
>  
> -void bch_sectors_dirty_init(struct cached_dev *dc);
> +void bch_sectors_dirty_init(struct bcache_device *);
>  void bch_cached_dev_writeback_init(struct cached_dev *);
>  int bch_cached_dev_writeback_start(struct cached_dev *);

Re: [PATCH 10/19] bcache: initialize stripe_sectors_dirty correctly for thin flash device

[Eric Wheeler]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3222 bytes --]

On Sun, 2 Jul 2017, Coly Li wrote:

> On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
> > From: Tang Junhui <tang.junhui@zte.com.cn>
> > 
> > Thin flash device does not initialize stripe_sectors_dirty correctly, this
> > patch fixes this issue.
> 
> Hi Junhui,
> 
> Could you please explain why stripe_sectors_ditry is not correctly
> initialized and how about its negative result ?

Hi Tang,
   
I'm waiting to queue this patch pending your response to Coly.  Can you   
update the message send a v2?

--
Eric Wheeler



> 
> Thanks.
> 
> Coly
> 
> > 
> > Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/md/bcache/super.c     | 3 ++-
> >  drivers/md/bcache/writeback.c | 8 ++++----
> >  drivers/md/bcache/writeback.h | 2 +-
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index 1f84791..e06641e 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -1030,7 +1030,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c)
> >  	}
> >  
> >  	if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
> > -		bch_sectors_dirty_init(dc);
> > +		bch_sectors_dirty_init(&dc->disk);
> >  		atomic_set(&dc->has_dirty, 1);
> >  		atomic_inc(&dc->count);
> >  		bch_writeback_queue(dc);
> > @@ -1232,6 +1232,7 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
> >  		goto err;
> >  
> >  	bcache_device_attach(d, c, u - c->uuids);
> > +	bch_sectors_dirty_init(d);
> >  	bch_flash_dev_request_init(d);
> >  	add_disk(d->disk);
> >  
> > diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> > index 3d463f0..4ac8b13 100644
> > --- a/drivers/md/bcache/writeback.c
> > +++ b/drivers/md/bcache/writeback.c
> > @@ -482,17 +482,17 @@ static int sectors_dirty_init_fn(struct btree_op *_op, struct btree *b,
> >  	return MAP_CONTINUE;
> >  }
> >  
> > -void bch_sectors_dirty_init(struct cached_dev *dc)
> > +void bch_sectors_dirty_init(struct bcache_device *d)
> >  {
> >  	struct sectors_dirty_init op;
> >  
> >  	bch_btree_op_init(&op.op, -1);
> > -	op.inode = dc->disk.id;
> > +	op.inode = d->id;
> >  
> > -	bch_btree_map_keys(&op.op, dc->disk.c, &KEY(op.inode, 0, 0),
> > +	bch_btree_map_keys(&op.op, d->c, &KEY(op.inode, 0, 0),
> >  			   sectors_dirty_init_fn, 0);
> >  
> > -	dc->disk.sectors_dirty_last = bcache_dev_sectors_dirty(&dc->disk);
> > +	d->sectors_dirty_last = bcache_dev_sectors_dirty(d);
> >  }
> >  
> >  void bch_cached_dev_writeback_init(struct cached_dev *dc)
> > diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
> > index ea2f92e..c2ab4b4 100644
> > --- a/drivers/md/bcache/writeback.h
> > +++ b/drivers/md/bcache/writeback.h
> > @@ -109,7 +109,7 @@ static inline void bch_writeback_add(struct cached_dev *dc)
> >  
> >  void bcache_dev_sectors_dirty_add(struct cache_set *, unsigned, uint64_t, int);
> >  
> > -void bch_sectors_dirty_init(struct cached_dev *dc);
> > +void bch_sectors_dirty_init(struct bcache_device *);
> >  void bch_cached_dev_writeback_init(struct cached_dev *);
> >  int bch_cached_dev_writeback_start(struct cached_dev *);
> 
> 

[PATCH 13/19] bcache: delete redundant calling set_gc_sectors()

[bcache]

From: Tang Junhui <tang.junhui@zte.com.cn>

set_gc_sectors() has been called in bch_gc_thread(), and it was called
again in bch_btree_gc_finish() . The following call is unnecessary, so
delete it.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/btree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 77aa20b..66d8036 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -1662,7 +1662,6 @@ static size_t bch_btree_gc_finish(struct cache_set *c)
 
 	mutex_lock(&c->bucket_lock);
 
-	set_gc_sectors(c);
 	c->gc_mark_valid = 1;
 	c->need_gc	= 0;
 
-- 
1.8.3.1

Re: [PATCH 13/19] bcache: delete redundant calling set_gc_sectors()

[Eric Wheeler]

On Fri, 30 Jun 2017, bcache@lists.ewheeler.net wrote:

> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
> set_gc_sectors() has been called in bch_gc_thread(), and it was called
> again in bch_btree_gc_finish() . The following call is unnecessary, so
> delete it.


I'm trying to follow the call path that proves that this is safe, but I'm 
not seeing it.  Can you explain this in more detail?

--
Eric Wheeler



> 
> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
> ---
>  drivers/md/bcache/btree.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 77aa20b..66d8036 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -1662,7 +1662,6 @@ static size_t bch_btree_gc_finish(struct cache_set *c)
>  
>  	mutex_lock(&c->bucket_lock);
>  
> -	set_gc_sectors(c);
>  	c->gc_mark_valid = 1;
>  	c->need_gc	= 0;
>  
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 14/19] bcache: Correct return value for sysfs attach errors

[bcache]

From: Tony Asleson <tasleson@redhat.com>

If you encounter any errors in bch_cached_dev_attach it will return a negative
error code.  The variable 'v' which stores the result is unsigned, thus user
space sees a very large value returned for bytes written which can cause
incorrect user space behavior.  Utilize 1 signed variable to use throughout
the function to preserve error return capability.

Signed-off-by: Tony Asleson <tasleson@redhat.com>
Acked-by: Coly Li <colyli@suse.de>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index cc0076d..7579ca6 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -206,7 +206,7 @@ STORE(__cached_dev)
 {
 	struct cached_dev *dc = container_of(kobj, struct cached_dev,
 					     disk.kobj);
-	unsigned v = size;
+	ssize_t v = size;
 	struct cache_set *c;
 	struct kobj_uevent_env *env;
 	unsigned ioprio_class = 0; /* invalid initial ioprio values */
@@ -245,7 +245,7 @@ STORE(__cached_dev)
 		bch_cached_dev_run(dc);
 
 	if (attr == &sysfs_cache_mode) {
-		ssize_t v = bch_read_string_list(buf, bch_cache_modes + 1);
+		v = bch_read_string_list(buf, bch_cache_modes + 1);
 
 		if (v < 0)
 			return v;
-- 
1.8.3.1

[PATCH 16/19] bcache: increase the number of open buckets

[bcache]

From: Tang Junhui <tang.junhui@zte.com.cn>

In currently, we only alloc 6 open buckets for each cache set,
but in usually, we always attach about 10 or so backend devices for
each cache set, and the each bcache device are always accessed by
about 10 or so threads in top application layer. So 6 open buckets
are too few, It has led to that each of the same thread write data
to different buckets, which would cause low efficiency write-back,
and also cause buckets inefficient, and would be Very easy to run
out of.

I add debug message in bch_open_buckets_alloc() to print alloc bucket
info, and test with ten bcache devices with a cache set, and each
bcache device is accessed by ten threads.

>From the debug message, we can see that, after the modification, One
bucket is more likely to assign to the same thread, and the data from
the same thread are more likely to write the same bucket. Usually the
same thread always write/read the same backend device, so it is good
for write-back and also promote the usage efficiency of buckets.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/alloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index ca4abe1..cacbe2d 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -68,6 +68,8 @@
 #include <linux/random.h>
 #include <trace/events/bcache.h>
 
+#define MAX_OPEN_BUCKETS 128
+
 /* Bucket heap / gen */
 
 uint8_t bch_inc_gen(struct cache *ca, struct bucket *b)
@@ -671,7 +673,7 @@ int bch_open_buckets_alloc(struct cache_set *c)
 
 	spin_lock_init(&c->data_bucket_lock);
 
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < MAX_OPEN_BUCKETS; i++) {
 		struct open_bucket *b = kzalloc(sizeof(*b), GFP_KERNEL);
 		if (!b)
 			return -ENOMEM;
-- 
1.8.3.1

Re: [PATCH 16/19] bcache: increase the number of open buckets

[Coly Li]

On 2017/7/1 上午4:43, bcache@lists.ewheeler.net wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
> In currently, we only alloc 6 open buckets for each cache set,
> but in usually, we always attach about 10 or so backend devices for
> each cache set, and the each bcache device are always accessed by
> about 10 or so threads in top application layer. So 6 open buckets
> are too few, It has led to that each of the same thread write data
> to different buckets, which would cause low efficiency write-back,
> and also cause buckets inefficient, and would be Very easy to run
> out of.
> 
> I add debug message in bch_open_buckets_alloc() to print alloc bucket
> info, and test with ten bcache devices with a cache set, and each
> bcache device is accessed by ten threads.
> 
> From the debug message, we can see that, after the modification, One
> bucket is more likely to assign to the same thread, and the data from
> the same thread are more likely to write the same bucket. Usually the
> same thread always write/read the same backend device, so it is good
> for write-back and also promote the usage efficiency of buckets.
> 
> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>

Nice catch for performance !

Reviewed-by: Coly Li <colyli@suse.de>

Thanks.


> ---
>  drivers/md/bcache/alloc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> index ca4abe1..cacbe2d 100644
> --- a/drivers/md/bcache/alloc.c
> +++ b/drivers/md/bcache/alloc.c
> @@ -68,6 +68,8 @@
>  #include <linux/random.h>
>  #include <trace/events/bcache.h>
>  
> +#define MAX_OPEN_BUCKETS 128
> +
>  /* Bucket heap / gen */
>  
>  uint8_t bch_inc_gen(struct cache *ca, struct bucket *b)
> @@ -671,7 +673,7 @@ int bch_open_buckets_alloc(struct cache_set *c)
>  
>  	spin_lock_init(&c->data_bucket_lock);
>  
> -	for (i = 0; i < 6; i++) {
> +	for (i = 0; i < MAX_OPEN_BUCKETS; i++) {
>  		struct open_bucket *b = kzalloc(sizeof(*b), GFP_KERNEL);
>  		if (!b)
>  			return -ENOMEM;
> 


-- 
Coly Li

[PATCH 17/19] bcache: fix for gc and write-back race

[bcache]

From: Tang Junhui <tang.junhui@zte.com.cn>

gc and write-back get raced (see the email "bcache get stucked" I sended
before):
gc thread						write-back thread
|							|bch_writeback_thread()
|bch_gc_thread()					|
|							|==>read_dirty()
|==>bch_btree_gc()					|
|==>btree_root() //get btree root			|
|			node write locker		|
|==>bch_btree_gc_root()					|
|							|==>read_dirty_submit()
|							|==>write_dirty()
|							|==>continue_at(cl, write_dirty_finish, system_wq);
|							|==>write_dirty_finish()//excute in system_wq
|							|==>bch_btree_insert()
|							|==>bch_btree_map_leaf_nodes()
|							|==>__bch_btree_map_nodes()
|							|==>btree_root //try to get btree root node read lock
|							|-----stuck here
|==>bch_btree_set_root()				|
|==>bch_journal_meta()					|
|==>bch_journal()					|
|==>journal_try_write()					|
|==>journal_write_unlocked() //journal_full(&c->journal) condition satisfied
|==>continue_at(cl, journal_write, system_wq); //try to excute journal_write in system_wq
|					//but work queue is excuting write_dirty_finish()
|==>closure_sync(); //wait journal_write execute over and wake up gc,
|			--stuck here
|==>release root node write locker

This patch alloc a separate work-queue for write-back thread to avoid such
race.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
Cc: stable@vger.kernel.org
---
 drivers/md/bcache/bcache.h    | 1 +
 drivers/md/bcache/super.c     | 2 ++
 drivers/md/bcache/writeback.c | 8 ++++++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 44123e4..deb0a6c 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -333,6 +333,7 @@ struct cached_dev {
 	/* Limit number of writeback bios in flight */
 	struct semaphore	in_flight;
 	struct task_struct	*writeback_thread;
+	struct workqueue_struct	*writeback_write_wq;
 
 	struct keybuf		writeback_keys;
 
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e06641e..24cb9b7 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1063,6 +1063,8 @@ static void cached_dev_free(struct closure *cl)
 	cancel_delayed_work_sync(&dc->writeback_rate_update);
 	if (!IS_ERR_OR_NULL(dc->writeback_thread))
 		kthread_stop(dc->writeback_thread);
+	if (dc->writeback_write_wq)
+		destroy_workqueue(dc->writeback_write_wq);
 
 	mutex_lock(&bch_register_lock);
 
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 4104eaa..4bc5daa 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -189,7 +189,7 @@ static void write_dirty(struct closure *cl)
 
 	closure_bio_submit(&io->bio, cl);
 
-	continue_at(cl, write_dirty_finish, system_wq);
+	continue_at(cl, write_dirty_finish, io->dc->writeback_write_wq);
 }
 
 static void read_dirty_endio(struct bio *bio)
@@ -209,7 +209,7 @@ static void read_dirty_submit(struct closure *cl)
 
 	closure_bio_submit(&io->bio, cl);
 
-	continue_at(cl, write_dirty, system_wq);
+	continue_at(cl, write_dirty, io->dc->writeback_write_wq);
 }
 
 static void read_dirty(struct cached_dev *dc)
@@ -527,6 +527,10 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
 
 int bch_cached_dev_writeback_start(struct cached_dev *dc)
 {
+	dc->writeback_write_wq = alloc_workqueue("bcache_writeback_wq", WQ_MEM_RECLAIM, 0);
+	if (!dc->writeback_write_wq)
+		return -ENOMEM;
+
 	dc->writeback_thread = kthread_create(bch_writeback_thread, dc,
 					      "bcache_writeback");
 	if (IS_ERR(dc->writeback_thread))
-- 
1.8.3.1

Re: [PATCH 17/19] bcache: fix for gc and write-back race

[Coly Li]

On 2017/7/1 上午4:43, bcache@lists.ewheeler.net wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
> gc and write-back get raced (see the email "bcache get stucked" I sended
> before):
> gc thread						write-back thread
> |							|bch_writeback_thread()
> |bch_gc_thread()					|
> |							|==>read_dirty()
> |==>bch_btree_gc()					|
> |==>btree_root() //get btree root			|
> |			node write locker		|
> |==>bch_btree_gc_root()					|
> |							|==>read_dirty_submit()
> |							|==>write_dirty()
> |							|==>continue_at(cl, write_dirty_finish, system_wq);
> |							|==>write_dirty_finish()//excute in system_wq
> |							|==>bch_btree_insert()
> |							|==>bch_btree_map_leaf_nodes()
> |							|==>__bch_btree_map_nodes()
> |							|==>btree_root //try to get btree root node read lock
> |							|-----stuck here
> |==>bch_btree_set_root()				|
> |==>bch_journal_meta()					|
> |==>bch_journal()					|
> |==>journal_try_write()					|
> |==>journal_write_unlocked() //journal_full(&c->journal) condition satisfied
> |==>continue_at(cl, journal_write, system_wq); //try to excute journal_write in system_wq
> |					//but work queue is excuting write_dirty_finish()
> |==>closure_sync(); //wait journal_write execute over and wake up gc,
> |			--stuck here
> |==>release root node write locker
> 
> This patch alloc a separate work-queue for write-back thread to avoid such
> race.
> 
> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
> Cc: stable@vger.kernel.org

Add a per-cached device work queue is a good idea, it's OK to me.

Acked-by: Coly Li <colyli@suse.de>

Thansk.

Coly

> ---
>  drivers/md/bcache/bcache.h    | 1 +
>  drivers/md/bcache/super.c     | 2 ++
>  drivers/md/bcache/writeback.c | 8 ++++++--
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 44123e4..deb0a6c 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -333,6 +333,7 @@ struct cached_dev {
>  	/* Limit number of writeback bios in flight */
>  	struct semaphore	in_flight;
>  	struct task_struct	*writeback_thread;
> +	struct workqueue_struct	*writeback_write_wq;
>  
>  	struct keybuf		writeback_keys;
>  
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index e06641e..24cb9b7 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1063,6 +1063,8 @@ static void cached_dev_free(struct closure *cl)
>  	cancel_delayed_work_sync(&dc->writeback_rate_update);
>  	if (!IS_ERR_OR_NULL(dc->writeback_thread))
>  		kthread_stop(dc->writeback_thread);
> +	if (dc->writeback_write_wq)
> +		destroy_workqueue(dc->writeback_write_wq);
>  
>  	mutex_lock(&bch_register_lock);
>  
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 4104eaa..4bc5daa 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -189,7 +189,7 @@ static void write_dirty(struct closure *cl)
>  
>  	closure_bio_submit(&io->bio, cl);
>  
> -	continue_at(cl, write_dirty_finish, system_wq);
> +	continue_at(cl, write_dirty_finish, io->dc->writeback_write_wq);
>  }
>  
>  static void read_dirty_endio(struct bio *bio)
> @@ -209,7 +209,7 @@ static void read_dirty_submit(struct closure *cl)
>  
>  	closure_bio_submit(&io->bio, cl);
>  
> -	continue_at(cl, write_dirty, system_wq);
> +	continue_at(cl, write_dirty, io->dc->writeback_write_wq);
>  }
>  
>  static void read_dirty(struct cached_dev *dc)
> @@ -527,6 +527,10 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
>  
>  int bch_cached_dev_writeback_start(struct cached_dev *dc)
>  {
> +	dc->writeback_write_wq = alloc_workqueue("bcache_writeback_wq", WQ_MEM_RECLAIM, 0);
> +	if (!dc->writeback_write_wq)
> +		return -ENOMEM;
> +
>  	dc->writeback_thread = kthread_create(bch_writeback_thread, dc,
>  					      "bcache_writeback");
>  	if (IS_ERR(dc->writeback_thread))
> 

[PATCH 18/19] bcache: silence static checker warning

[bcache]

From: Dan Carpenter <dan.carpenter@oracle.com>

In olden times, closure_return() used to have a hidden return built in.
We removed the hidden return but forgot to add a new return here.  If
"c" were NULL we would oops on the next line, but fortunately "c" is
never NULL.  Let's just remove the if statement.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/md/bcache/super.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 24cb9b7..243391d 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1381,9 +1381,6 @@ static void cache_set_flush(struct closure *cl)
 	struct btree *b;
 	unsigned i;
 
-	if (!c)
-		closure_return(cl);
-
 	bch_cache_accounting_destroy(&c->accounting);
 
 	kobject_put(&c->internal);
-- 
1.8.3.1

Re: [PATCH 18/19] bcache: silence static checker warning

[Coly Li]

On 2017/7/1 上午4:43, bcache@lists.ewheeler.net wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> 
> In olden times, closure_return() used to have a hidden return built in.
> We removed the hidden return but forgot to add a new return here.  If
> "c" were NULL we would oops on the next line, but fortunately "c" is
> never NULL.  Let's just remove the if statement.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/md/bcache/super.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 24cb9b7..243391d 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1381,9 +1381,6 @@ static void cache_set_flush(struct closure *cl)
>  	struct btree *b;
>  	unsigned i;
>  
> -	if (!c)
> -		closure_return(cl);
> -
>  	bch_cache_accounting_destroy(&c->accounting);
>  
>  	kobject_put(&c->internal);
> 
Agree, cache_set_flush() is only called from a continue_at() in
__cache_set_unregister(). In this case, cl is always not NULL.

Reviewed-by: Coly Li <colyli@suse.de>

Thanks.

-- 
Coly Li

[PATCH 19/19] bcache: Update continue_at() documentation

[bcache]

From: Dan Carpenter <dan.carpenter@oracle.com>

continue_at() doesn't have a return statement anymore.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/md/bcache/closure.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
index 1ec84ca..295b7e4 100644
--- a/drivers/md/bcache/closure.h
+++ b/drivers/md/bcache/closure.h
@@ -312,8 +312,6 @@ static inline void closure_wake_up(struct closure_waitlist *list)
  * been dropped with closure_put()), it will resume execution at @fn running out
  * of @wq (or, if @wq is NULL, @fn will be called by closure_put() directly).
  *
- * NOTE: This macro expands to a return in the calling function!
- *
  * This is because after calling continue_at() you no longer have a ref on @cl,
  * and whatever @cl owns may be freed out from under you - a running closure fn
  * has a ref on its own closure which continue_at() drops.
@@ -340,8 +338,6 @@ do {									\
  * Causes @fn to be executed out of @cl, in @wq context (or called directly if
  * @wq is NULL).
  *
- * NOTE: like continue_at(), this macro expands to a return in the caller!
- *
  * The ref the caller of continue_at_nobarrier() had on @cl is now owned by @fn,
  * thus it's not safe to touch anything protected by @cl after a
  * continue_at_nobarrier().
-- 
1.8.3.1

Re: [PATCH 19/19] bcache: Update continue_at() documentation

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 19/19] bcache: Update continue_at() documentation

[Coly Li]

On 2017/7/1 上午4:43, bcache@lists.ewheeler.net wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> 
> continue_at() doesn't have a return statement anymore.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly

> ---
>  drivers/md/bcache/closure.h | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
> index 1ec84ca..295b7e4 100644
> --- a/drivers/md/bcache/closure.h
> +++ b/drivers/md/bcache/closure.h
> @@ -312,8 +312,6 @@ static inline void closure_wake_up(struct closure_waitlist *list)
>   * been dropped with closure_put()), it will resume execution at @fn running out
>   * of @wq (or, if @wq is NULL, @fn will be called by closure_put() directly).
>   *
> - * NOTE: This macro expands to a return in the calling function!
> - *
>   * This is because after calling continue_at() you no longer have a ref on @cl,
>   * and whatever @cl owns may be freed out from under you - a running closure fn
>   * has a ref on its own closure which continue_at() drops.
> @@ -340,8 +338,6 @@ do {									\
>   * Causes @fn to be executed out of @cl, in @wq context (or called directly if
>   * @wq is NULL).
>   *
> - * NOTE: like continue_at(), this macro expands to a return in the caller!
> - *
>   * The ref the caller of continue_at_nobarrier() had on @cl is now owned by @fn,
>   * thus it's not safe to touch anything protected by @cl after a
>   * continue_at_nobarrier().
> 

Re: [PATCH 01/19] bcache: Fix leak of bdev reference

[Coly Li]

On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote:
> From: Jan Kara <jack@suse.cz>
> 
> If blkdev_get_by_path() in register_bcache() fails, we try to lookup the
> block device using lookup_bdev() to detect which situation we are in to
> properly report error. However we never drop the reference returned to
> us from lookup_bdev(). Fix that.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org

Acked-by: Coly Li <colyli@suse.de>

Thanks.

> ---
>  drivers/md/bcache/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 8352fad..9a2c190 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1964,6 +1964,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
>  			else
>  				err = "device busy";
>  			mutex_unlock(&bch_register_lock);
> +			if (!IS_ERR(bdev))
> +				bdput(bdev);
>  			if (attr == &ksysfs_register_quiet)
>  				goto out;
>  		}
> 


-- 
Coly Li

Re: [PATCH 01/19] bcache: Fix leak of bdev reference

[Christoph Hellwig]

On Fri, Jun 30, 2017 at 01:42:50PM -0700, bcache@lists.ewheeler.net wrote:
> From: Jan Kara <jack@suse.cz>
> 
> If blkdev_get_by_path() in register_bcache() fails, we try to lookup the
> block device using lookup_bdev() to detect which situation we are in to
> properly report error. However we never drop the reference returned to
> us from lookup_bdev(). Fix that.

This look ok, but I think that whole chunk of code should just go
away - adding a lookup_bdev and resulting mess just for a slightly
different error message is just insane.

Re: [PATCH 01/19] bcache: Fix leak of bdev reference

[Coly Li]

On 2017/7/6 上午2:24, Christoph Hellwig wrote:
> On Fri, Jun 30, 2017 at 01:42:50PM -0700, bcache@lists.ewheeler.net wrote:
>> From: Jan Kara <jack@suse.cz>
>>
>> If blkdev_get_by_path() in register_bcache() fails, we try to lookup the
>> block device using lookup_bdev() to detect which situation we are in to
>> properly report error. However we never drop the reference returned to
>> us from lookup_bdev(). Fix that.
> 
> This look ok, but I think that whole chunk of code should just go
> away - adding a lookup_bdev and resulting mess just for a slightly
> different error message is just insane.

Hi Christoph,

When you mentioned "whole chunk of code", do you mean the following
block of code ?


1960         if (IS_ERR(bdev)) {
========= start of whole chunk of code ============
1961                 if (bdev == ERR_PTR(-EBUSY)) {
1962                         bdev = lookup_bdev(strim(path));
1963                         mutex_lock(&bch_register_lock);
1964                         if (!IS_ERR(bdev) && bch_is_open(bdev))
1965                                 err = "device already registered";
1966                         else
1967                                 err = "device busy";
1968                         mutex_unlock(&bch_register_lock);
1969                         if (!IS_ERR(bdev))
1970                                 bdput(bdev);
1971                         if (attr == &ksysfs_register_quiet)
1972                                 goto out;
1973                 }
========= end of whole chunk of code ============
1974                 goto err;
1975         }

I don't mind to remove it, just double check I don't misunderstand what
you meant.

Thanks.


-- 
Coly Li

Re: [PATCH 01/19] bcache: Fix leak of bdev reference

[Christoph Hellwig]

On Tue, Sep 05, 2017 at 01:30:04AM +0800, Coly Li wrote:
> 
> When you mentioned "whole chunk of code", do you mean the following
> block of code ?
> 
> 
> 1960         if (IS_ERR(bdev)) {
> ========= start of whole chunk of code ============
> 1961                 if (bdev == ERR_PTR(-EBUSY)) {
> 1962                         bdev = lookup_bdev(strim(path));
> 1963                         mutex_lock(&bch_register_lock);
> 1964                         if (!IS_ERR(bdev) && bch_is_open(bdev))
> 1965                                 err = "device already registered";
> 1966                         else
> 1967                                 err = "device busy";
> 1968                         mutex_unlock(&bch_register_lock);
> 1969                         if (!IS_ERR(bdev))
> 1970                                 bdput(bdev);
> 1971                         if (attr == &ksysfs_register_quiet)
> 1972                                 goto out;
> 1973                 }
> ========= end of whole chunk of code ============
> 1974                 goto err;
> 1975         }
> 
> I don't mind to remove it, just double check I don't misunderstand what
> you meant.

Yes, that's the problematic block.

Re: [PATCH 01/19] bcache: Fix leak of bdev reference

[Coly Li]

On 2017/9/5 下午2:43, Christoph Hellwig wrote:
> On Tue, Sep 05, 2017 at 01:30:04AM +0800, Coly Li wrote:
>>
>> When you mentioned "whole chunk of code", do you mean the following
>> block of code ?
>>
>>
>> 1960         if (IS_ERR(bdev)) {
>> ========= start of whole chunk of code ============
>> 1961                 if (bdev == ERR_PTR(-EBUSY)) {
>> 1962                         bdev = lookup_bdev(strim(path));
>> 1963                         mutex_lock(&bch_register_lock);
>> 1964                         if (!IS_ERR(bdev) && bch_is_open(bdev))
>> 1965                                 err = "device already registered";
>> 1966                         else
>> 1967                                 err = "device busy";
>> 1968                         mutex_unlock(&bch_register_lock);
>> 1969                         if (!IS_ERR(bdev))
>> 1970                                 bdput(bdev);
>> 1971                         if (attr == &ksysfs_register_quiet)
>> 1972                                 goto out;
>> 1973                 }
>> ========= end of whole chunk of code ============
>> 1974                 goto err;
>> 1975         }
>>
>> I don't mind to remove it, just double check I don't misunderstand what
>> you meant.
> 
> Yes, that's the problematic block.
> 
Understand, I will send out a patch, hopefully it can catch up 4.14
merge window.

Thanks for the hint.

-- 
Coly Li

Re: [PATCH 01/19] bcache: Fix leak of bdev reference

[Coly Li]

On 2017/9/5 下午2:43, Christoph Hellwig wrote:
> On Tue, Sep 05, 2017 at 01:30:04AM +0800, Coly Li wrote:
>>
>> When you mentioned "whole chunk of code", do you mean the following
>> block of code ?
>>
>>
>> 1960         if (IS_ERR(bdev)) {
>> ========= start of whole chunk of code ============
>> 1961                 if (bdev == ERR_PTR(-EBUSY)) {
>> 1962                         bdev = lookup_bdev(strim(path));
>> 1963                         mutex_lock(&bch_register_lock);
>> 1964                         if (!IS_ERR(bdev) && bch_is_open(bdev))
>> 1965                                 err = "device already registered";
>> 1966                         else
>> 1967                                 err = "device busy";
>> 1968                         mutex_unlock(&bch_register_lock);
>> 1969                         if (!IS_ERR(bdev))
>> 1970                                 bdput(bdev);
>> 1971                         if (attr == &ksysfs_register_quiet)
>> 1972                                 goto out;
>> 1973                 }
>> ========= end of whole chunk of code ============
>> 1974                 goto err;
>> 1975         }
>>
>> I don't mind to remove it, just double check I don't misunderstand what
>> you meant.
> 
> Yes, that's the problematic block.
> 
Hi Christoph,

I tested the code, and my patch which removes the above code. The result
is, I feel the above chunk of code is useful. When I tried to register a
device and it was already registered. Without the above code, I only saw
"failed to open device", didn't realize it was because this device is
registered already. After adding the above code back, I knew where the
problem was.

The above chunk of code improves user experience and provides more
detailed diagnose information, it is useful. Then I suggest to keep the
code here and pick up Jan's patch.

Thanks.

-- 
Coly Li

Re: [PATCH 15/19] bcache: fix issue of writeback rate at minimum 1 key per second

[Coly Li]

On 2017/10/28 上午3:09, Eric Wheeler wrote:
> 
> [+cc Michael Lyle]
> 
> On Fri, 27 Oct 2017, Eric Wheeler wrote:
> 
>> On Sun, 16 Jul 2017, Coly Li wrote:
>>
>>> On 2017/7/1 上午4:43, bcache@lists.ewheeler.net wrote:
>>>> From: Tang Junhui <tang.junhui@zte.com.cn>
>>>>
>>>> When there is not enough dirty data in writeback cache,
>>>> writeback rate is at minimum 1 key per second
>>>> util all dirty data to be cleaned, it is inefficiency,
>>>> and also causes waste of energy;
>>>
>>> Hi Junhui and Eric,
>>>
>>> What: /sys/block/<disk>/bcache/writeback_percent
>>> Description:
>>>       For backing devices: If nonzero, writeback from cache to
>>>       backing device only takes place when more than this percentage
>>>       of the cache is used, allowing more write coalescing to take
>>>       place and reducing total number of writes sent to the backing
>>>       device. Integer between 0 and 40.
>>>
>>> I see above text from Documentation/ABI/testing/sysfs-block-bcache (I
>>> know this document is quite old), it seems if "not enough" means dirty
>>> data percentage is less then writback_percent, bcache should not
>>> performance writeback I/O. But in __update_writeback_rate(),
>>> writeback_rate.rate is clamped in [1, NSEC_PER_MSEC]. It seems in PD
>>> controller code of __update_writeback_rate(), writeback_percent is only
>>> used to calculate dirty target number, its another functionality as
>>> writeback threshold is not handled here.
>>>
>>>>
>>>> in this patch, When there is not enough dirty data,
>>>> let the writeback rate to be 0, and writeback re-schedule
>>>> in bch_writeback_thread() periodically with schedule_timeout(),
>>>> the behaviors are as follows :
>>>>
>>>> 1) If no dirty data have been read into dc->writeback_keys,
>>>> goto step 2), otherwise keep writing these dirty data to
>>>> back-end device at 1 key per second, until all these dirty data
>>>> write over, then goto step 2).
>>>>
>>>> 2) Loop in bch_writeback_thread() to check if there is enough
>>>> dirty data for writeback. if there is not enough diry data for
>>>> writing, then sleep 10 seconds, otherwise, write dirty data to
>>>> back-end device.
>>>
>>> Bcache uses a Proportion-Differentiation Controller to control writeback
>>> rate. When dirty data is far from target, writeback rate is higher; when
>>> dirty data is close to target, writeback rate is slower. The advantage
>>> of PD controller here is, when regular I/O and writeback I/O happens in
>>> same time,
>>> - When there are a lot of dirty data, writeback I/O can have more chance
>>> to write them back to cached device, which in turns has positive impact
>>> to regular I/O.
>>> - When dirty data is decreased and close to target dirty number, less
>>> writeback I/O can help regular I/O has better throughput and latency.
>>>
>>> The root cause of 1 key per second is, the PD controller is designed for
>>> better I/O performance, not less energy consumption. When the existing
>>> dirty data gets closed to target dirty number, the PD controller chooses
>>> to use longer writeback time to make a better regular I/O performance.
>>> If it is designed for less energy consumption, it should keep the
>>> writeback rate in a high level and finish writing back all dirty data as
>>> soon as possible.
>>>
>>> This patch may introduce an unexpected behavior of dirty data writeback
>>> throughput, when regular write I/O and writeback I/O happen in same
>>> time. In this case, dirty data number may shake up and down around
>>> target dirty number, it is possible that change (the variable in
>>> __update_writeback_rate()) is a minus value, and the result of
>>> dc->writeback_rate.rate + change happens to be 0. This patch changes the
>>> clamp range of writeback_rate.rate to [0, NSEC_PER_MSEC], so
>>> writeback_rate.rate can be possible to be 0. And in bch_next_delay() if
>>> d->rate is zero, the write back I/O will be delayed to now +
>>> NSEC_PER_SEC. When there is no regular I/O it works well, but when there
>>> is regular I/O, this longer delay may cause more dirty data piled in
>>> cache device, and PD controller cannot generage a stable writeback rate.
>>> This is not an expected behavior for the writeback rate PD controller.
>>>
>>> Another method to fix might be,
>>> 1) define a sysfs to define writeback_rate with max/dynamic option.
>>> 2) dynamic writeback_rate as default
>>> 3) when max is set, in __update_writeback_rate() assign NSEC_PER_MSEC to
>>> writeback_rate.rate
>>> 4) in bch_writeback_thread(), if no writeback I/O on fly, and dirty data
>>> does not reach dc->writeback_percent, schedule out.
>>> 5) if writeback is necessary then do it in max rate and finish it as
>>> soon as possible, to save laptop energy.
>>>
>>> The above method might be helpful to energy save as well (perform dirty
>>> dat write back in batch), and does not change default PD controller
>>> behavior.
>>>
>>> Just for your reference. Or if you are too busy to look at it, I can try
>>> to compose a patch for review.
>>
>> Hi Coli,
>>
>> Did this go anywere?  I think the 1-key/sec fix is a good idea and your 
>> suggestion will help out mobile users.
>>

Hi Eric,

Michael is working on writeback improvement currently. He proposes some
patches to improve writeback efficiency from a little bit different
view, and after some quite deep discussion I feel some of his ideas are
promising. e.g. writeback more keys if backing device is idle.

Currently it seems a better writeback performance results more lock
contention in between with front end I/O. This is why Junhui posts a
realy time buckets in use counting patch. This is a start to reduce lock
contention in bcache tree writebac/gc/key insert.

I just feel this is a serieal continuous effort to improve writeback
efficiency. the 1-key/sec fix might be one of them, let's
improve-and-test :-)

Thanks.

Coly Li


-- 
Coly Li

[PATCH 05/19] bcache: fix calling ida_simple_remove() with incorrect minor

[Eric Wheeler]

From: Tang Junhui <tang.junhui@zte.com.cn>

bcache called ida_simple_remove() with minor which have multiplied by
BCACHE_MINORS, it would cause minor wrong release and leakage.

In addition, when adding partition support to bcache, the name assignment
was not updated, resulting in numbers jumping (bcache0, bcache16,
bcache32...). This has been fixed implicitly by the rework.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
Reviewed-by: Coly Li <colyli@suse.de>
Reviewed-by: Eric Wheeler <bcache@linux.ewheeler.net>
Cc: stable@vger.kernel.org # 4.10
Cc: Stefan Bader <stefan.bader@canonical.com>
Fixes: b8c0d91 (bcache: partition support: add 16 minors per bcacheN device)
BugLink: https://bugs.launchpad.net/bugs/1667078
---
 drivers/md/bcache/super.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 9a2c190..48b8c20 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -58,7 +58,10 @@ static wait_queue_head_t unregister_wait;
 struct workqueue_struct *bcache_wq;
 
 #define BTREE_MAX_PAGES		(256 * 1024 / PAGE_SIZE)
-#define BCACHE_MINORS		16 /* partition support */
+#define BCACHE_MINORS_BITS                4 /* bcache partition support */
+#define BCACHE_MINORS                     (1 << BCACHE_MINORS_BITS)
+#define BCACHE_TO_IDA_MINORS(first_minor) ((first_minor) >> BCACHE_MINORS_BITS)
+#define IDA_TO_BCACHE_MINORS(minor)       ((minor) << BCACHE_MINORS_BITS)
 
 /* Superblock */
 
@@ -734,7 +737,8 @@ static void bcache_device_free(struct bcache_device *d)
 	if (d->disk && d->disk->queue)
 		blk_cleanup_queue(d->disk->queue);
 	if (d->disk) {
-		ida_simple_remove(&bcache_minor, d->disk->first_minor);
+		ida_simple_remove(&bcache_minor,
+			BCACHE_TO_IDA_MINORS(d->disk->first_minor));
 		put_disk(d->disk);
 	}
 
@@ -776,11 +780,11 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	if (!d->full_dirty_stripes)
 		return -ENOMEM;
 
-	minor = ida_simple_get(&bcache_minor, 0, MINORMASK + 1, GFP_KERNEL);
+	minor = ida_simple_get(&bcache_minor, 0,
+		BCACHE_TO_IDA_MINORS(MINORMASK) + 1, GFP_KERNEL);
 	if (minor < 0)
 		return minor;
 
-	minor *= BCACHE_MINORS;
 
 	if (!(d->bio_split = bioset_create(4, offsetof(struct bbio, bio),
 					   BIOSET_NEED_BVECS |
@@ -794,7 +798,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 	snprintf(d->disk->disk_name, DISK_NAME_LEN, "bcache%i", minor);
 
 	d->disk->major		= bcache_major;
-	d->disk->first_minor	= minor;
+	d->disk->first_minor	= IDA_TO_BCACHE_MINORS(minor);
 	d->disk->fops		= &bcache_ops;
 	d->disk->private_data	= d;
 
-- 
1.8.3.1