misc bcache cleanups

misc bcache cleanups

[Christoph Hellwig]

Hi Coly,

this series has a bunch of misc cleanups for bcache by using better kernel
interfaces.

Diffstat:
 bcache.h  |    4 ----
 btree.c   |    2 +-
 debug.c   |   15 +++++++--------
 io.c      |   16 ++++++++--------
 request.c |    6 +++---
 super.c   |   55 +++++++++++++++++++++++--------------------------------
 sysfs.c   |    2 +-
 util.h    |    8 --------
 8 files changed, 43 insertions(+), 65 deletions(-)

[PATCH 1/4] bcache: remove the cache_dev_name field from struct cache

[Christoph Hellwig]

Just use the %pg format specifier to print the name directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/bcache/bcache.h | 2 --
 drivers/md/bcache/io.c     | 8 ++++----
 drivers/md/bcache/super.c  | 7 +++----
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 5fc989a6d4528..47ff9ecea2e29 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -470,8 +470,6 @@ struct cache {
 	atomic_long_t		meta_sectors_written;
 	atomic_long_t		btree_sectors_written;
 	atomic_long_t		sectors_written;
-
-	char			cache_dev_name[BDEVNAME_SIZE];
 };
 
 struct gc_stat {
diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
index e4388fe3ab7ef..564357de76404 100644
--- a/drivers/md/bcache/io.c
+++ b/drivers/md/bcache/io.c
@@ -123,13 +123,13 @@ void bch_count_io_errors(struct cache *ca,
 		errors >>= IO_ERROR_SHIFT;
 
 		if (errors < ca->set->error_limit)
-			pr_err("%s: IO error on %s%s\n",
-			       ca->cache_dev_name, m,
+			pr_err("%pg: IO error on %s%s\n",
+			       ca->bdev, m,
 			       is_read ? ", recovering." : ".");
 		else
 			bch_cache_set_error(ca->set,
-					    "%s: too many IO errors %s\n",
-					    ca->cache_dev_name, m);
+					    "%pg: too many IO errors %s\n",
+					    ca->bdev, m);
 	}
 }
 
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index f2874c77ff797..d0d0257252adc 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2338,7 +2338,7 @@ static int cache_alloc(struct cache *ca)
 err_free:
 	module_put(THIS_MODULE);
 	if (err)
-		pr_notice("error %s: %s\n", ca->cache_dev_name, err);
+		pr_notice("error %pg: %s\n", ca->bdev, err);
 	return ret;
 }
 
@@ -2348,7 +2348,6 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 	const char *err = NULL; /* must be set for any error case */
 	int ret = 0;
 
-	bdevname(bdev, ca->cache_dev_name);
 	memcpy(&ca->sb, sb, sizeof(struct cache_sb));
 	ca->bdev = bdev;
 	ca->bdev->bd_holder = ca;
@@ -2390,14 +2389,14 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 		goto out;
 	}
 
-	pr_info("registered cache device %s\n", ca->cache_dev_name);
+	pr_info("registered cache device %pg\n", ca->bdev);
 
 out:
 	kobject_put(&ca->kobj);
 
 err:
 	if (err)
-		pr_notice("error %s: %s\n", ca->cache_dev_name, err);
+		pr_notice("error %pg: %s\n", ca->bdev, err);
 
 	return ret;
 }
-- 
2.30.2

[PATCH 2/4] bcache: remove the backing_dev_name field from struct cached_dev

[Christoph Hellwig]

Just use the %pg format specifier to print the name directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/bcache/bcache.h  |  2 --
 drivers/md/bcache/debug.c   |  4 ++--
 drivers/md/bcache/io.c      |  8 +++----
 drivers/md/bcache/request.c |  4 ++--
 drivers/md/bcache/super.c   | 48 ++++++++++++++++---------------------
 drivers/md/bcache/sysfs.c   |  2 +-
 6 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 47ff9ecea2e29..941685409c68f 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -395,8 +395,6 @@ struct cached_dev {
 	atomic_t		io_errors;
 	unsigned int		error_limit;
 	unsigned int		offline_seconds;
-
-	char			backing_dev_name[BDEVNAME_SIZE];
 };
 
 enum alloc_reserve {
diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
index 116edda845c37..e803cad864be7 100644
--- a/drivers/md/bcache/debug.c
+++ b/drivers/md/bcache/debug.c
@@ -137,8 +137,8 @@ void bch_data_verify(struct cached_dev *dc, struct bio *bio)
 					p2 + bv.bv_offset,
 					bv.bv_len),
 				 dc->disk.c,
-				 "verify failed at dev %s sector %llu",
-				 dc->backing_dev_name,
+				 "verify failed at dev %pg sector %llu",
+				 dc->bdev,
 				 (uint64_t) bio->bi_iter.bi_sector);
 
 		kunmap_atomic(p1);
diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
index 564357de76404..9c6f9ec55b724 100644
--- a/drivers/md/bcache/io.c
+++ b/drivers/md/bcache/io.c
@@ -65,15 +65,15 @@ void bch_count_backing_io_errors(struct cached_dev *dc, struct bio *bio)
 	 * we shouldn't count failed REQ_RAHEAD bio to dc->io_errors.
 	 */
 	if (bio->bi_opf & REQ_RAHEAD) {
-		pr_warn_ratelimited("%s: Read-ahead I/O failed on backing device, ignore\n",
-				    dc->backing_dev_name);
+		pr_warn_ratelimited("%pg: Read-ahead I/O failed on backing device, ignore\n",
+				    dc->bdev);
 		return;
 	}
 
 	errors = atomic_add_return(1, &dc->io_errors);
 	if (errors < dc->error_limit)
-		pr_err("%s: IO error on backing device, unrecoverable\n",
-			dc->backing_dev_name);
+		pr_err("%pg: IO error on backing device, unrecoverable\n",
+			dc->bdev);
 	else
 		bch_cached_dev_error(dc);
 }
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 6d1de889baeb1..64ce5788f80cb 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -651,8 +651,8 @@ static void backing_request_endio(struct bio *bio)
 		 */
 		if (unlikely(s->iop.writeback &&
 			     bio->bi_opf & REQ_PREFLUSH)) {
-			pr_err("Can't flush %s: returned bi_status %i\n",
-				dc->backing_dev_name, bio->bi_status);
+			pr_err("Can't flush %pg: returned bi_status %i\n",
+				dc->bdev, bio->bi_status);
 		} else {
 			/* set to orig_bio->bi_status in bio_complete() */
 			s->iop.status = bio->bi_status;
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index d0d0257252adc..bf9dfdde1f033 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1026,8 +1026,8 @@ static int cached_dev_status_update(void *arg)
 			dc->offline_seconds = 0;
 
 		if (dc->offline_seconds >= BACKING_DEV_OFFLINE_TIMEOUT) {
-			pr_err("%s: device offline for %d seconds\n",
-			       dc->backing_dev_name,
+			pr_err("%pg: device offline for %d seconds\n",
+			       dc->bdev,
 			       BACKING_DEV_OFFLINE_TIMEOUT);
 			pr_err("%s: disable I/O request due to backing device offline\n",
 			       dc->disk.name);
@@ -1058,15 +1058,13 @@ int bch_cached_dev_run(struct cached_dev *dc)
 	};
 
 	if (dc->io_disable) {
-		pr_err("I/O disabled on cached dev %s\n",
-		       dc->backing_dev_name);
+		pr_err("I/O disabled on cached dev %pg\n", dc->bdev);
 		ret = -EIO;
 		goto out;
 	}
 
 	if (atomic_xchg(&dc->running, 1)) {
-		pr_info("cached dev %s is running already\n",
-		       dc->backing_dev_name);
+		pr_info("cached dev %pg is running already\n", dc->bdev);
 		ret = -EBUSY;
 		goto out;
 	}
@@ -1163,7 +1161,7 @@ static void cached_dev_detach_finish(struct work_struct *w)
 
 	mutex_unlock(&bch_register_lock);
 
-	pr_info("Caching disabled for %s\n", dc->backing_dev_name);
+	pr_info("Caching disabled for %pg\n", dc->bdev);
 
 	/* Drop ref we took in cached_dev_detach() */
 	closure_put(&dc->disk.cl);
@@ -1203,29 +1201,27 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 		return -ENOENT;
 
 	if (dc->disk.c) {
-		pr_err("Can't attach %s: already attached\n",
-		       dc->backing_dev_name);
+		pr_err("Can't attach %pg: already attached\n", dc->bdev);
 		return -EINVAL;
 	}
 
 	if (test_bit(CACHE_SET_STOPPING, &c->flags)) {
-		pr_err("Can't attach %s: shutting down\n",
-		       dc->backing_dev_name);
+		pr_err("Can't attach %pg: shutting down\n", dc->bdev);
 		return -EINVAL;
 	}
 
 	if (dc->sb.block_size < c->cache->sb.block_size) {
 		/* Will die */
-		pr_err("Couldn't attach %s: block size less than set's block size\n",
-		       dc->backing_dev_name);
+		pr_err("Couldn't attach %pg: block size less than set's block size\n",
+		       dc->bdev);
 		return -EINVAL;
 	}
 
 	/* Check whether already attached */
 	list_for_each_entry_safe(exist_dc, t, &c->cached_devs, list) {
 		if (!memcmp(dc->sb.uuid, exist_dc->sb.uuid, 16)) {
-			pr_err("Tried to attach %s but duplicate UUID already attached\n",
-				dc->backing_dev_name);
+			pr_err("Tried to attach %pg but duplicate UUID already attached\n",
+				dc->bdev);
 
 			return -EINVAL;
 		}
@@ -1243,15 +1239,13 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 
 	if (!u) {
 		if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
-			pr_err("Couldn't find uuid for %s in set\n",
-			       dc->backing_dev_name);
+			pr_err("Couldn't find uuid for %pg in set\n", dc->bdev);
 			return -ENOENT;
 		}
 
 		u = uuid_find_empty(c);
 		if (!u) {
-			pr_err("Not caching %s, no room for UUID\n",
-			       dc->backing_dev_name);
+			pr_err("Not caching %pg, no room for UUID\n", dc->bdev);
 			return -EINVAL;
 		}
 	}
@@ -1319,8 +1313,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 		 */
 		kthread_stop(dc->writeback_thread);
 		cancel_writeback_rate_update_dwork(dc);
-		pr_err("Couldn't run cached device %s\n",
-		       dc->backing_dev_name);
+		pr_err("Couldn't run cached device %pg\n", dc->bdev);
 		return ret;
 	}
 
@@ -1336,8 +1329,8 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 	/* Allow the writeback thread to proceed */
 	up_write(&dc->writeback_lock);
 
-	pr_info("Caching %s as %s on set %pU\n",
-		dc->backing_dev_name,
+	pr_info("Caching %pg as %s on set %pU\n",
+		dc->bdev,
 		dc->disk.disk->disk_name,
 		dc->disk.c->set_uuid);
 	return 0;
@@ -1461,7 +1454,6 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 	struct cache_set *c;
 	int ret = -ENOMEM;
 
-	bdevname(bdev, dc->backing_dev_name);
 	memcpy(&dc->sb, sb, sizeof(struct cache_sb));
 	dc->bdev = bdev;
 	dc->bdev->bd_holder = dc;
@@ -1476,7 +1468,7 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 	if (bch_cache_accounting_add_kobjs(&dc->accounting, &dc->disk.kobj))
 		goto err;
 
-	pr_info("registered backing device %s\n", dc->backing_dev_name);
+	pr_info("registered backing device %pg\n", dc->bdev);
 
 	list_add(&dc->list, &uncached_devices);
 	/* attach to a matched cache set if it exists */
@@ -1493,7 +1485,7 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
 
 	return 0;
 err:
-	pr_notice("error %s: %s\n", dc->backing_dev_name, err);
+	pr_notice("error %pg: %s\n", dc->bdev, err);
 	bcache_device_stop(&dc->disk);
 	return ret;
 }
@@ -1621,8 +1613,8 @@ bool bch_cached_dev_error(struct cached_dev *dc)
 	/* make others know io_disable is true earlier */
 	smp_mb();
 
-	pr_err("stop %s: too many IO errors on backing device %s\n",
-	       dc->disk.disk->disk_name, dc->backing_dev_name);
+	pr_err("stop %s: too many IO errors on backing device %pg\n",
+	       dc->disk.disk->disk_name, dc->bdev);
 
 	bcache_device_stop(&dc->disk);
 	return true;
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index 05ac1d6fbbf35..1f0dce30fa759 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -271,7 +271,7 @@ SHOW(__bch_cached_dev)
 	}
 
 	if (attr == &sysfs_backing_dev_name) {
-		snprintf(buf, BDEVNAME_SIZE + 1, "%s", dc->backing_dev_name);
+		snprintf(buf, BDEVNAME_SIZE + 1, "%pg", dc->bdev);
 		strcat(buf, "\n");
 		return strlen(buf);
 	}
-- 
2.30.2

[PATCH 3/4] bcache: use bvec_kmap_local in bch_data_verify

[Christoph Hellwig]

Using local kmaps slightly reduces the chances to stray writes, and
the bvec interface cleans up the code a little bit.

Also switch from page_address to bvec_kmap_local for cbv to be on the
safe side and to avoid pointlessly poking into bvec internals.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/bcache/debug.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
index e803cad864be7..6230dfdd9286e 100644
--- a/drivers/md/bcache/debug.c
+++ b/drivers/md/bcache/debug.c
@@ -127,21 +127,20 @@ void bch_data_verify(struct cached_dev *dc, struct bio *bio)
 
 	citer.bi_size = UINT_MAX;
 	bio_for_each_segment(bv, bio, iter) {
-		void *p1 = kmap_atomic(bv.bv_page);
+		void *p1 = bvec_kmap_local(&bv);
 		void *p2;
 
 		cbv = bio_iter_iovec(check, citer);
-		p2 = page_address(cbv.bv_page);
+		p2 = bvec_kmap_local(&cbv);
 
-		cache_set_err_on(memcmp(p1 + bv.bv_offset,
-					p2 + bv.bv_offset,
-					bv.bv_len),
+		cache_set_err_on(memcmp(p1, p2, bv.bv_len),
 				 dc->disk.c,
 				 "verify failed at dev %pg sector %llu",
 				 dc->bdev,
 				 (uint64_t) bio->bi_iter.bi_sector);
 
-		kunmap_atomic(p1);
+		kunmap_local(p2);
+		kunmap_local(p1);
 		bio_advance_iter(check, &citer, bv.bv_len);
 	}
 
-- 
2.30.2

[PATCH 4/4] bcache: remove bch_crc64_update

[Christoph Hellwig]

bch_crc64_update is an entirely pointless wrapper around crc64_be.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/bcache/btree.c   | 2 +-
 drivers/md/bcache/request.c | 2 +-
 drivers/md/bcache/util.h    | 8 --------
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 0595559de174a..93b67b8d31c3d 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -141,7 +141,7 @@ static uint64_t btree_csum_set(struct btree *b, struct bset *i)
 	uint64_t crc = b->key.ptr[0];
 	void *data = (void *) i + 8, *end = bset_bkey_last(i);
 
-	crc = bch_crc64_update(crc, data, end - data);
+	crc = crc64_be(crc, data, end - data);
 	return crc ^ 0xffffffffffffffffULL;
 }
 
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 64ce5788f80cb..3f10f82483047 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -46,7 +46,7 @@ static void bio_csum(struct bio *bio, struct bkey *k)
 	bio_for_each_segment(bv, bio, iter) {
 		void *d = kmap(bv.bv_page) + bv.bv_offset;
 
-		csum = bch_crc64_update(csum, d, bv.bv_len);
+		csum = crc64_be(csum, d, bv.bv_len);
 		kunmap(bv.bv_page);
 	}
 
diff --git a/drivers/md/bcache/util.h b/drivers/md/bcache/util.h
index b64460a762677..6274d6a17e5e7 100644
--- a/drivers/md/bcache/util.h
+++ b/drivers/md/bcache/util.h
@@ -548,14 +548,6 @@ static inline uint64_t bch_crc64(const void *p, size_t len)
 	return crc ^ 0xffffffffffffffffULL;
 }
 
-static inline uint64_t bch_crc64_update(uint64_t crc,
-					const void *p,
-					size_t len)
-{
-	crc = crc64_be(crc, p, len);
-	return crc;
-}
-
 /*
  * A stepwise-linear pseudo-exponential.  This returns 1 << (x >>
  * frac_bits), with the less-significant bits filled in by linear
-- 
2.30.2

Re: [PATCH 1/4] bcache: remove the cache_dev_name field from struct cache

[Coly Li]

On 10/18/21 2:09 PM, Christoph Hellwig wrote:
> Just use the %pg format specifier to print the name directly.

Hi  Christoph,

NACK for this patch.  The buffer cache_dev_name is added on purpose, in 
case ca->bdev cannot be referenced correctly for some special condition 
when underlying device fails.

Thanks.

Coly Li
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/md/bcache/bcache.h | 2 --
>   drivers/md/bcache/io.c     | 8 ++++----
>   drivers/md/bcache/super.c  | 7 +++----
>   3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 5fc989a6d4528..47ff9ecea2e29 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -470,8 +470,6 @@ struct cache {
>   	atomic_long_t		meta_sectors_written;
>   	atomic_long_t		btree_sectors_written;
>   	atomic_long_t		sectors_written;
> -
> -	char			cache_dev_name[BDEVNAME_SIZE];
>   };
>   
>   struct gc_stat {
> diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
> index e4388fe3ab7ef..564357de76404 100644
> --- a/drivers/md/bcache/io.c
> +++ b/drivers/md/bcache/io.c
> @@ -123,13 +123,13 @@ void bch_count_io_errors(struct cache *ca,
>   		errors >>= IO_ERROR_SHIFT;
>   
>   		if (errors < ca->set->error_limit)
> -			pr_err("%s: IO error on %s%s\n",
> -			       ca->cache_dev_name, m,
> +			pr_err("%pg: IO error on %s%s\n",
> +			       ca->bdev, m,
>   			       is_read ? ", recovering." : ".");
>   		else
>   			bch_cache_set_error(ca->set,
> -					    "%s: too many IO errors %s\n",
> -					    ca->cache_dev_name, m);
> +					    "%pg: too many IO errors %s\n",
> +					    ca->bdev, m);
>   	}
>   }
>   
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index f2874c77ff797..d0d0257252adc 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2338,7 +2338,7 @@ static int cache_alloc(struct cache *ca)
>   err_free:
>   	module_put(THIS_MODULE);
>   	if (err)
> -		pr_notice("error %s: %s\n", ca->cache_dev_name, err);
> +		pr_notice("error %pg: %s\n", ca->bdev, err);
>   	return ret;
>   }
>   
> @@ -2348,7 +2348,6 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
>   	const char *err = NULL; /* must be set for any error case */
>   	int ret = 0;
>   
> -	bdevname(bdev, ca->cache_dev_name);
>   	memcpy(&ca->sb, sb, sizeof(struct cache_sb));
>   	ca->bdev = bdev;
>   	ca->bdev->bd_holder = ca;
> @@ -2390,14 +2389,14 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
>   		goto out;
>   	}
>   
> -	pr_info("registered cache device %s\n", ca->cache_dev_name);
> +	pr_info("registered cache device %pg\n", ca->bdev);
>   
>   out:
>   	kobject_put(&ca->kobj);
>   
>   err:
>   	if (err)
> -		pr_notice("error %s: %s\n", ca->cache_dev_name, err);
> +		pr_notice("error %pg: %s\n", ca->bdev, err);
>   
>   	return ret;
>   }

Re: [PATCH 2/4] bcache: remove the backing_dev_name field from struct cached_dev

[Coly Li]

On 10/18/21 2:09 PM, Christoph Hellwig wrote:
> Just use the %pg format specifier to print the name directly.

Hi Christoph,

NACK for same reason.  We do this on purpose, when the real block device 
fails, the bcache code may continue to print the device name without 
worry about whether the dc->bdev is valid or not.

Thanks.

Coly Li

>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/md/bcache/bcache.h  |  2 --
>   drivers/md/bcache/debug.c   |  4 ++--
>   drivers/md/bcache/io.c      |  8 +++----
>   drivers/md/bcache/request.c |  4 ++--
>   drivers/md/bcache/super.c   | 48 ++++++++++++++++---------------------
>   drivers/md/bcache/sysfs.c   |  2 +-
>   6 files changed, 29 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 47ff9ecea2e29..941685409c68f 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -395,8 +395,6 @@ struct cached_dev {
>   	atomic_t		io_errors;
>   	unsigned int		error_limit;
>   	unsigned int		offline_seconds;
> -
> -	char			backing_dev_name[BDEVNAME_SIZE];
>   };
>   
>   enum alloc_reserve {
> diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
> index 116edda845c37..e803cad864be7 100644
> --- a/drivers/md/bcache/debug.c
> +++ b/drivers/md/bcache/debug.c
> @@ -137,8 +137,8 @@ void bch_data_verify(struct cached_dev *dc, struct bio *bio)
>   					p2 + bv.bv_offset,
>   					bv.bv_len),
>   				 dc->disk.c,
> -				 "verify failed at dev %s sector %llu",
> -				 dc->backing_dev_name,
> +				 "verify failed at dev %pg sector %llu",
> +				 dc->bdev,
>   				 (uint64_t) bio->bi_iter.bi_sector);
>   
>   		kunmap_atomic(p1);
> diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
> index 564357de76404..9c6f9ec55b724 100644
> --- a/drivers/md/bcache/io.c
> +++ b/drivers/md/bcache/io.c
> @@ -65,15 +65,15 @@ void bch_count_backing_io_errors(struct cached_dev *dc, struct bio *bio)
>   	 * we shouldn't count failed REQ_RAHEAD bio to dc->io_errors.
>   	 */
>   	if (bio->bi_opf & REQ_RAHEAD) {
> -		pr_warn_ratelimited("%s: Read-ahead I/O failed on backing device, ignore\n",
> -				    dc->backing_dev_name);
> +		pr_warn_ratelimited("%pg: Read-ahead I/O failed on backing device, ignore\n",
> +				    dc->bdev);
>   		return;
>   	}
>   
>   	errors = atomic_add_return(1, &dc->io_errors);
>   	if (errors < dc->error_limit)
> -		pr_err("%s: IO error on backing device, unrecoverable\n",
> -			dc->backing_dev_name);
> +		pr_err("%pg: IO error on backing device, unrecoverable\n",
> +			dc->bdev);
>   	else
>   		bch_cached_dev_error(dc);
>   }
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 6d1de889baeb1..64ce5788f80cb 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -651,8 +651,8 @@ static void backing_request_endio(struct bio *bio)
>   		 */
>   		if (unlikely(s->iop.writeback &&
>   			     bio->bi_opf & REQ_PREFLUSH)) {
> -			pr_err("Can't flush %s: returned bi_status %i\n",
> -				dc->backing_dev_name, bio->bi_status);
> +			pr_err("Can't flush %pg: returned bi_status %i\n",
> +				dc->bdev, bio->bi_status);
>   		} else {
>   			/* set to orig_bio->bi_status in bio_complete() */
>   			s->iop.status = bio->bi_status;
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index d0d0257252adc..bf9dfdde1f033 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1026,8 +1026,8 @@ static int cached_dev_status_update(void *arg)
>   			dc->offline_seconds = 0;
>   
>   		if (dc->offline_seconds >= BACKING_DEV_OFFLINE_TIMEOUT) {
> -			pr_err("%s: device offline for %d seconds\n",
> -			       dc->backing_dev_name,
> +			pr_err("%pg: device offline for %d seconds\n",
> +			       dc->bdev,
>   			       BACKING_DEV_OFFLINE_TIMEOUT);
>   			pr_err("%s: disable I/O request due to backing device offline\n",
>   			       dc->disk.name);
> @@ -1058,15 +1058,13 @@ int bch_cached_dev_run(struct cached_dev *dc)
>   	};
>   
>   	if (dc->io_disable) {
> -		pr_err("I/O disabled on cached dev %s\n",
> -		       dc->backing_dev_name);
> +		pr_err("I/O disabled on cached dev %pg\n", dc->bdev);
>   		ret = -EIO;
>   		goto out;
>   	}
>   
>   	if (atomic_xchg(&dc->running, 1)) {
> -		pr_info("cached dev %s is running already\n",
> -		       dc->backing_dev_name);
> +		pr_info("cached dev %pg is running already\n", dc->bdev);
>   		ret = -EBUSY;
>   		goto out;
>   	}
> @@ -1163,7 +1161,7 @@ static void cached_dev_detach_finish(struct work_struct *w)
>   
>   	mutex_unlock(&bch_register_lock);
>   
> -	pr_info("Caching disabled for %s\n", dc->backing_dev_name);
> +	pr_info("Caching disabled for %pg\n", dc->bdev);
>   
>   	/* Drop ref we took in cached_dev_detach() */
>   	closure_put(&dc->disk.cl);
> @@ -1203,29 +1201,27 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
>   		return -ENOENT;
>   
>   	if (dc->disk.c) {
> -		pr_err("Can't attach %s: already attached\n",
> -		       dc->backing_dev_name);
> +		pr_err("Can't attach %pg: already attached\n", dc->bdev);
>   		return -EINVAL;
>   	}
>   
>   	if (test_bit(CACHE_SET_STOPPING, &c->flags)) {
> -		pr_err("Can't attach %s: shutting down\n",
> -		       dc->backing_dev_name);
> +		pr_err("Can't attach %pg: shutting down\n", dc->bdev);
>   		return -EINVAL;
>   	}
>   
>   	if (dc->sb.block_size < c->cache->sb.block_size) {
>   		/* Will die */
> -		pr_err("Couldn't attach %s: block size less than set's block size\n",
> -		       dc->backing_dev_name);
> +		pr_err("Couldn't attach %pg: block size less than set's block size\n",
> +		       dc->bdev);
>   		return -EINVAL;
>   	}
>   
>   	/* Check whether already attached */
>   	list_for_each_entry_safe(exist_dc, t, &c->cached_devs, list) {
>   		if (!memcmp(dc->sb.uuid, exist_dc->sb.uuid, 16)) {
> -			pr_err("Tried to attach %s but duplicate UUID already attached\n",
> -				dc->backing_dev_name);
> +			pr_err("Tried to attach %pg but duplicate UUID already attached\n",
> +				dc->bdev);
>   
>   			return -EINVAL;
>   		}
> @@ -1243,15 +1239,13 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
>   
>   	if (!u) {
>   		if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) {
> -			pr_err("Couldn't find uuid for %s in set\n",
> -			       dc->backing_dev_name);
> +			pr_err("Couldn't find uuid for %pg in set\n", dc->bdev);
>   			return -ENOENT;
>   		}
>   
>   		u = uuid_find_empty(c);
>   		if (!u) {
> -			pr_err("Not caching %s, no room for UUID\n",
> -			       dc->backing_dev_name);
> +			pr_err("Not caching %pg, no room for UUID\n", dc->bdev);
>   			return -EINVAL;
>   		}
>   	}
> @@ -1319,8 +1313,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
>   		 */
>   		kthread_stop(dc->writeback_thread);
>   		cancel_writeback_rate_update_dwork(dc);
> -		pr_err("Couldn't run cached device %s\n",
> -		       dc->backing_dev_name);
> +		pr_err("Couldn't run cached device %pg\n", dc->bdev);
>   		return ret;
>   	}
>   
> @@ -1336,8 +1329,8 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
>   	/* Allow the writeback thread to proceed */
>   	up_write(&dc->writeback_lock);
>   
> -	pr_info("Caching %s as %s on set %pU\n",
> -		dc->backing_dev_name,
> +	pr_info("Caching %pg as %s on set %pU\n",
> +		dc->bdev,
>   		dc->disk.disk->disk_name,
>   		dc->disk.c->set_uuid);
>   	return 0;
> @@ -1461,7 +1454,6 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
>   	struct cache_set *c;
>   	int ret = -ENOMEM;
>   
> -	bdevname(bdev, dc->backing_dev_name);
>   	memcpy(&dc->sb, sb, sizeof(struct cache_sb));
>   	dc->bdev = bdev;
>   	dc->bdev->bd_holder = dc;
> @@ -1476,7 +1468,7 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
>   	if (bch_cache_accounting_add_kobjs(&dc->accounting, &dc->disk.kobj))
>   		goto err;
>   
> -	pr_info("registered backing device %s\n", dc->backing_dev_name);
> +	pr_info("registered backing device %pg\n", dc->bdev);
>   
>   	list_add(&dc->list, &uncached_devices);
>   	/* attach to a matched cache set if it exists */
> @@ -1493,7 +1485,7 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
>   
>   	return 0;
>   err:
> -	pr_notice("error %s: %s\n", dc->backing_dev_name, err);
> +	pr_notice("error %pg: %s\n", dc->bdev, err);
>   	bcache_device_stop(&dc->disk);
>   	return ret;
>   }
> @@ -1621,8 +1613,8 @@ bool bch_cached_dev_error(struct cached_dev *dc)
>   	/* make others know io_disable is true earlier */
>   	smp_mb();
>   
> -	pr_err("stop %s: too many IO errors on backing device %s\n",
> -	       dc->disk.disk->disk_name, dc->backing_dev_name);
> +	pr_err("stop %s: too many IO errors on backing device %pg\n",
> +	       dc->disk.disk->disk_name, dc->bdev);
>   
>   	bcache_device_stop(&dc->disk);
>   	return true;
> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> index 05ac1d6fbbf35..1f0dce30fa759 100644
> --- a/drivers/md/bcache/sysfs.c
> +++ b/drivers/md/bcache/sysfs.c
> @@ -271,7 +271,7 @@ SHOW(__bch_cached_dev)
>   	}
>   
>   	if (attr == &sysfs_backing_dev_name) {
> -		snprintf(buf, BDEVNAME_SIZE + 1, "%s", dc->backing_dev_name);
> +		snprintf(buf, BDEVNAME_SIZE + 1, "%pg", dc->bdev);
>   		strcat(buf, "\n");
>   		return strlen(buf);
>   	}

Re: [PATCH 3/4] bcache: use bvec_kmap_local in bch_data_verify

[Coly Li]

On 10/18/21 2:09 PM, Christoph Hellwig wrote:
> Using local kmaps slightly reduces the chances to stray writes, and
> the bvec interface cleans up the code a little bit.
>
> Also switch from page_address to bvec_kmap_local for cbv to be on the
> safe side and to avoid pointlessly poking into bvec internals.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks.

Coly Li

> ---
>   drivers/md/bcache/debug.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
> index e803cad864be7..6230dfdd9286e 100644
> --- a/drivers/md/bcache/debug.c
> +++ b/drivers/md/bcache/debug.c
> @@ -127,21 +127,20 @@ void bch_data_verify(struct cached_dev *dc, struct bio *bio)
>   
>   	citer.bi_size = UINT_MAX;
>   	bio_for_each_segment(bv, bio, iter) {
> -		void *p1 = kmap_atomic(bv.bv_page);
> +		void *p1 = bvec_kmap_local(&bv);
>   		void *p2;
>   
>   		cbv = bio_iter_iovec(check, citer);
> -		p2 = page_address(cbv.bv_page);
> +		p2 = bvec_kmap_local(&cbv);
>   
> -		cache_set_err_on(memcmp(p1 + bv.bv_offset,
> -					p2 + bv.bv_offset,
> -					bv.bv_len),
> +		cache_set_err_on(memcmp(p1, p2, bv.bv_len),
>   				 dc->disk.c,
>   				 "verify failed at dev %pg sector %llu",
>   				 dc->bdev,
>   				 (uint64_t) bio->bi_iter.bi_sector);
>   
> -		kunmap_atomic(p1);
> +		kunmap_local(p2);
> +		kunmap_local(p1);
>   		bio_advance_iter(check, &citer, bv.bv_len);
>   	}
>   

Re: [PATCH 4/4] bcache: remove bch_crc64_update

[Coly Li]

On 10/18/21 2:09 PM, Christoph Hellwig wrote:
> bch_crc64_update is an entirely pointless wrapper around crc64_be.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks.

Coly Li

> ---
>   drivers/md/bcache/btree.c   | 2 +-
>   drivers/md/bcache/request.c | 2 +-
>   drivers/md/bcache/util.h    | 8 --------
>   3 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 0595559de174a..93b67b8d31c3d 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -141,7 +141,7 @@ static uint64_t btree_csum_set(struct btree *b, struct bset *i)
>   	uint64_t crc = b->key.ptr[0];
>   	void *data = (void *) i + 8, *end = bset_bkey_last(i);
>   
> -	crc = bch_crc64_update(crc, data, end - data);
> +	crc = crc64_be(crc, data, end - data);
>   	return crc ^ 0xffffffffffffffffULL;
>   }
>   
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 64ce5788f80cb..3f10f82483047 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -46,7 +46,7 @@ static void bio_csum(struct bio *bio, struct bkey *k)
>   	bio_for_each_segment(bv, bio, iter) {
>   		void *d = kmap(bv.bv_page) + bv.bv_offset;
>   
> -		csum = bch_crc64_update(csum, d, bv.bv_len);
> +		csum = crc64_be(csum, d, bv.bv_len);
>   		kunmap(bv.bv_page);
>   	}
>   
> diff --git a/drivers/md/bcache/util.h b/drivers/md/bcache/util.h
> index b64460a762677..6274d6a17e5e7 100644
> --- a/drivers/md/bcache/util.h
> +++ b/drivers/md/bcache/util.h
> @@ -548,14 +548,6 @@ static inline uint64_t bch_crc64(const void *p, size_t len)
>   	return crc ^ 0xffffffffffffffffULL;
>   }
>   
> -static inline uint64_t bch_crc64_update(uint64_t crc,
> -					const void *p,
> -					size_t len)
> -{
> -	crc = crc64_be(crc, p, len);
> -	return crc;
> -}
> -
>   /*
>    * A stepwise-linear pseudo-exponential.  This returns 1 << (x >>
>    * frac_bits), with the less-significant bits filled in by linear

Re: misc bcache cleanups

[Coly Li]

On 10/18/21 2:09 PM, Christoph Hellwig wrote:
> Hi Coly,
>
> this series has a bunch of misc cleanups for bcache by using better kernel
> interfaces.

Hi Christoph,

The last 2 patches are good to me. Do you want me to pick these two 
patches, or you handle them directly?

Thanks.

Coly Li

>
> Diffstat:
>   bcache.h  |    4 ----
>   btree.c   |    2 +-
>   debug.c   |   15 +++++++--------
>   io.c      |   16 ++++++++--------
>   request.c |    6 +++---
>   super.c   |   55 +++++++++++++++++++++++--------------------------------
>   sysfs.c   |    2 +-
>   util.h    |    8 --------
>   8 files changed, 43 insertions(+), 65 deletions(-)

Re: [PATCH 1/4] bcache: remove the cache_dev_name field from struct cache

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 10:26:18PM +0800, Coly Li wrote:
> On 10/18/21 2:09 PM, Christoph Hellwig wrote:
>> Just use the %pg format specifier to print the name directly.
>
> Hi  Christoph,
>
> NACK for this patch.  The buffer cache_dev_name is added on purpose, in 
> case ca->bdev cannot be referenced correctly for some special condition 
> when underlying device fails.

Where exactly?  ->bdev is never cleared and only dropped after we
waited for the I/O to complete.

Re: [PATCH 1/4] bcache: remove the cache_dev_name field from struct cache

[Coly Li]

On 10/18/21 11:21 PM, Christoph Hellwig wrote:
> On Mon, Oct 18, 2021 at 10:26:18PM +0800, Coly Li wrote:
>> On 10/18/21 2:09 PM, Christoph Hellwig wrote:
>>> Just use the %pg format specifier to print the name directly.
>> Hi  Christoph,
>>
>> NACK for this patch.  The buffer cache_dev_name is added on purpose, in
>> case ca->bdev cannot be referenced correctly for some special condition
>> when underlying device fails.
> Where exactly?  ->bdev is never cleared and only dropped after we
> waited for the I/O to complete.

It was in Linux v4.17 time, when I did the device failure handling. If 
the underlying device broken and gone, printing the device name string 
will be a "null" string in kmesg. But the device name was necessary for 
proper device failure information, we stored the device name string when 
it was initialized.

Coly Li

Re: [PATCH 1/4] bcache: remove the cache_dev_name field from struct cache

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 11:32:26PM +0800, Coly Li wrote:
> It was in Linux v4.17 time, when I did the device failure handling. If the 
> underlying device broken and gone, printing the device name string will be 
> a "null" string in kmesg. But the device name was necessary for proper 
> device failure information, we stored the device name string when it was 
> initialized.

I'm pretty sure that was before the %pg specifier and the
hd_struct/block_device unification.  With the current code there is no
way it could print null.

Re: [PATCH 1/4] bcache: remove the cache_dev_name field from struct cache

[Coly Li]

On 10/18/21 11:35 PM, Christoph Hellwig wrote:
> On Mon, Oct 18, 2021 at 11:32:26PM +0800, Coly Li wrote:
>> It was in Linux v4.17 time, when I did the device failure handling. If the
>> underlying device broken and gone, printing the device name string will be
>> a "null" string in kmesg. But the device name was necessary for proper
>> device failure information, we stored the device name string when it was
>> initialized.
> I'm pretty sure that was before the %pg specifier and the
> hd_struct/block_device unification.  With the current code there is no
> way it could print null.

Cool. Please add my acked-by to the first 2 patches,

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

Thanks for the hint.

Coly Li

Re: misc bcache cleanups

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 10:34:18PM +0800, Coly Li wrote:
> On 10/18/21 2:09 PM, Christoph Hellwig wrote:
>> Hi Coly,
>>
>> this series has a bunch of misc cleanups for bcache by using better kernel
>> interfaces.
>
> Hi Christoph,
>
> The last 2 patches are good to me. Do you want me to pick these two 
> patches, or you handle them directly?

Can you send the series to Jens as a normal bcache pull request?

Re: misc bcache cleanups

[Coly Li]

On 10/19/21 1:31 PM, Christoph Hellwig wrote:
> On Mon, Oct 18, 2021 at 10:34:18PM +0800, Coly Li wrote:
>> On 10/18/21 2:09 PM, Christoph Hellwig wrote:
>>> Hi Coly,
>>>
>>> this series has a bunch of misc cleanups for bcache by using better kernel
>>> interfaces.
>> Hi Christoph,
>>
>> The last 2 patches are good to me. Do you want me to pick these two
>> patches, or you handle them directly?
> Can you send the series to Jens as a normal bcache pull request?

Sure, I pick them. Thanks.

Coly Li