[PATCH 00/16] bcache: Compiler, sparse and smatch fixes

[PATCH 00/16] bcache: Compiler, sparse and smatch fixes

[Bart Van Assche]

Hello Michael, Coly and Christoph,

A few weeks ago I analyzed the compiler (W=1), sparse and smatch warnings
triggered by the bcache driver. This patch series is what I came up with.
Although none of these patches have been tested I think these patches are
worth a closer look. Feedback is welcome.

Thanks,

Bart.

Bart Van Assche (16):
  bcache: Fix indentation
  bcache: Add __printf annotation to __bch_check_keys()
  bcache: Annotate switch fall-through
  bcache: Fix kernel-doc warnings
  bcache: Remove an unused variable
  bcache: Suppress more warnings about set-but-not-used variables
  bcache: Reduce the number of sparse complaints about lock imbalances
  bcache: Fix a compiler warning in bcache_device_init()
  bcache: Remove a redundant assignment
  bcache: Suppress a compiler warning in bch_##name##_h()
  bcache: Check the d->disk pointer before using it
  bcache: Make it easier for static analyzers to analyze
    bch_allocator_thread()
  bcache: Make bch_dump_read() fail if copying to user space fails
  bcache: Make csum_set() implementation easier to read
  bcache: Fix an endianness bug
  bcache: Fix endianness annotations

 drivers/md/bcache/alloc.c     |   7 +--
 drivers/md/bcache/bcache.h    |  22 ++++++--
 drivers/md/bcache/bset.c      |   4 +-
 drivers/md/bcache/bset.h      |   5 +-
 drivers/md/bcache/btree.c     |   4 +-
 drivers/md/bcache/closure.c   |   8 +--
 drivers/md/bcache/debug.c     |   5 +-
 drivers/md/bcache/extents.c   |   2 -
 drivers/md/bcache/journal.c   |   4 +-
 drivers/md/bcache/request.c   |   1 +
 drivers/md/bcache/super.c     |  23 ++++----
 drivers/md/bcache/util.c      |  33 +++++++-----
 drivers/md/bcache/writeback.h |   2 +-
 include/uapi/linux/bcache.h   | 118 ++++++++++++++++++++++--------------------
 14 files changed, 131 insertions(+), 107 deletions(-)

-- 
2.16.2

[PATCH 01/16] bcache: Fix indentation

[Bart Van Assche]

This patch avoids that smatch complains about inconsistent indentation.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/btree.c     | 2 +-
 drivers/md/bcache/writeback.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index fad9fe8817eb..c30609c54c03 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -2170,7 +2170,7 @@ int bch_btree_insert_check_key(struct btree *b, struct btree_op *op,
 
 		if (b->key.ptr[0] != btree_ptr ||
                    b->seq != seq + 1) {
-                       op->lock = b->level;
+			op->lock = b->level;
 			goto out;
                }
 	}
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index 587b25599856..a102d40a58bc 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -39,7 +39,7 @@ static inline uint64_t  bcache_flash_devs_sectors_dirty(struct cache_set *c)
 
 		if (!d || !UUID_FLASH_ONLY(&c->uuids[i]))
 			continue;
-	   ret += bcache_dev_sectors_dirty(d);
+		ret += bcache_dev_sectors_dirty(d);
 	}
 
 	mutex_unlock(&bch_register_lock);
-- 
2.16.2

Re: [PATCH 01/16] bcache: Fix indentation

[Coly Li]

On 15/03/2018 11:07 PM, Bart Van Assche wrote:
> This patch avoids that smatch complains about inconsistent indentation.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

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

Thanks.

Coly Li

> ---
>  drivers/md/bcache/btree.c     | 2 +-
>  drivers/md/bcache/writeback.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index fad9fe8817eb..c30609c54c03 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -2170,7 +2170,7 @@ int bch_btree_insert_check_key(struct btree *b, struct btree_op *op,
>  
>  		if (b->key.ptr[0] != btree_ptr ||
>                     b->seq != seq + 1) {
> -                       op->lock = b->level;
> +			op->lock = b->level;
>  			goto out;
>                 }
>  	}
> diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
> index 587b25599856..a102d40a58bc 100644
> --- a/drivers/md/bcache/writeback.h
> +++ b/drivers/md/bcache/writeback.h
> @@ -39,7 +39,7 @@ static inline uint64_t  bcache_flash_devs_sectors_dirty(struct cache_set *c)
>  
>  		if (!d || !UUID_FLASH_ONLY(&c->uuids[i]))
>  			continue;
> -	   ret += bcache_dev_sectors_dirty(d);
> +		ret += bcache_dev_sectors_dirty(d);
>  	}
>  
>  	mutex_unlock(&bch_register_lock);
> 

Re: [PATCH 01/16] bcache: Fix indentation

[Coly Li]

On 15/03/2018 11:07 PM, Bart Van Assche wrote:
> This patch avoids that smatch complains about inconsistent indentation.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

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

Thanks.

Coly Li

> ---
>  drivers/md/bcache/btree.c     | 2 +-
>  drivers/md/bcache/writeback.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index fad9fe8817eb..c30609c54c03 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -2170,7 +2170,7 @@ int bch_btree_insert_check_key(struct btree *b, struct btree_op *op,
>  
>  		if (b->key.ptr[0] != btree_ptr ||
>                     b->seq != seq + 1) {
> -                       op->lock = b->level;
> +			op->lock = b->level;
>  			goto out;
>                 }
>  	}
> diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
> index 587b25599856..a102d40a58bc 100644
> --- a/drivers/md/bcache/writeback.h
> +++ b/drivers/md/bcache/writeback.h
> @@ -39,7 +39,7 @@ static inline uint64_t  bcache_flash_devs_sectors_dirty(struct cache_set *c)
>  
>  		if (!d || !UUID_FLASH_ONLY(&c->uuids[i]))
>  			continue;
> -	   ret += bcache_dev_sectors_dirty(d);
> +		ret += bcache_dev_sectors_dirty(d);
>  	}
>  
>  	mutex_unlock(&bch_register_lock);
> 

Re: [PATCH 01/16] bcache: Fix indentation

[Michael Lyle]

On Thu, Mar 15, 2018 at 8:07 AM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> This patch avoids that smatch complains about inconsistent indentation.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
LGTM, applying

[PATCH 02/16] bcache: Add __printf annotation to __bch_check_keys()

[Bart Van Assche]

Make it possible for the compiler to verify the consistency of the
format string passed to __bch_check_keys() and the arguments that
should be formatted according to that format string.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/bset.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/bset.h b/drivers/md/bcache/bset.h
index fa506c1aa524..0c24280f3b98 100644
--- a/drivers/md/bcache/bset.h
+++ b/drivers/md/bcache/bset.h
@@ -531,14 +531,15 @@ int __bch_keylist_realloc(struct keylist *, unsigned);
 #ifdef CONFIG_BCACHE_DEBUG
 
 int __bch_count_data(struct btree_keys *);
-void __bch_check_keys(struct btree_keys *, const char *, ...);
+void __printf(2, 3) __bch_check_keys(struct btree_keys *, const char *, ...);
 void bch_dump_bset(struct btree_keys *, struct bset *, unsigned);
 void bch_dump_bucket(struct btree_keys *);
 
 #else
 
 static inline int __bch_count_data(struct btree_keys *b) { return -1; }
-static inline void __bch_check_keys(struct btree_keys *b, const char *fmt, ...) {}
+static inline void __printf(2, 3)
+	__bch_check_keys(struct btree_keys *b, const char *fmt, ...) {}
 static inline void bch_dump_bucket(struct btree_keys *b) {}
 void bch_dump_bset(struct btree_keys *, struct bset *, unsigned);
 
-- 
2.16.2

Re: [PATCH 02/16] bcache: Add __printf annotation to __bch_check_keys()

[Michael Lyle]

On Thu, Mar 15, 2018 at 8:08 AM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> Make it possible for the compiler to verify the consistency of the
> format string passed to __bch_check_keys() and the arguments that
> should be formatted according to that format string.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

LGTM, applying.

[PATCH 03/16] bcache: Annotate switch fall-through

[Bart Van Assche]

This patch avoids that building with W=1 triggers complaints about
switch fall-throughs.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/util.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index a23cd6a14b74..6198041f0ee2 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -32,20 +32,27 @@ int bch_ ## name ## _h(const char *cp, type *res)		\
 	case 'y':						\
 	case 'z':						\
 		u++;						\
+		/* fall through */				\
 	case 'e':						\
 		u++;						\
+		/* fall through */				\
 	case 'p':						\
 		u++;						\
+		/* fall through */				\
 	case 't':						\
 		u++;						\
+		/* fall through */				\
 	case 'g':						\
 		u++;						\
+		/* fall through */				\
 	case 'm':						\
 		u++;						\
+		/* fall through */				\
 	case 'k':						\
 		u++;						\
 		if (e++ == cp)					\
 			return -EINVAL;				\
+		/* fall through */				\
 	case '\n':						\
 	case '\0':						\
 		if (*e == '\n')					\
-- 
2.16.2

Re: [PATCH 03/16] bcache: Annotate switch fall-through

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> This patch avoids that building with W=1 triggers complaints about
> switch fall-throughs.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

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

Thanks.

Coly Li

> ---
>  drivers/md/bcache/util.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
> index a23cd6a14b74..6198041f0ee2 100644
> --- a/drivers/md/bcache/util.c
> +++ b/drivers/md/bcache/util.c
> @@ -32,20 +32,27 @@ int bch_ ## name ## _h(const char *cp, type *res)		\
>  	case 'y':						\
>  	case 'z':						\
>  		u++;						\
> +		/* fall through */				\
>  	case 'e':						\
>  		u++;						\
> +		/* fall through */				\
>  	case 'p':						\
>  		u++;						\
> +		/* fall through */				\
>  	case 't':						\
>  		u++;						\
> +		/* fall through */				\
>  	case 'g':						\
>  		u++;						\
> +		/* fall through */				\
>  	case 'm':						\
>  		u++;						\
> +		/* fall through */				\
>  	case 'k':						\
>  		u++;						\
>  		if (e++ == cp)					\
>  			return -EINVAL;				\
> +		/* fall through */				\
>  	case '\n':						\
>  	case '\0':						\
>  		if (*e == '\n')					\
> 

Re: [PATCH 03/16] bcache: Annotate switch fall-through

[Michael Lyle]

On 03/15/2018 08:08 AM, Bart Van Assche wrote:
> This patch avoids that building with W=1 triggers complaints about
> switch fall-throughs.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

LGTM, applying.

[PATCH 04/16] bcache: Fix kernel-doc warnings

[Bart Van Assche]

Avoid that building with W=1 triggers warnings about the kernel-doc
headers.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/btree.c   |  2 +-
 drivers/md/bcache/closure.c |  8 ++++----
 drivers/md/bcache/request.c |  1 +
 drivers/md/bcache/util.c    | 18 ++++++++----------
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index c30609c54c03..d20f963799b6 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -959,7 +959,7 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
 	return b;
 }
 
-/**
+/*
  * bch_btree_node_get - find a btree node in the cache and lock it, reading it
  * in from disk if necessary.
  *
diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 7f12920c14f7..87c45dfa7c38 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c
@@ -46,7 +46,7 @@ void closure_sub(struct closure *cl, int v)
 }
 EXPORT_SYMBOL(closure_sub);
 
-/**
+/*
  * closure_put - decrement a closure's refcount
  */
 void closure_put(struct closure *cl)
@@ -55,7 +55,7 @@ void closure_put(struct closure *cl)
 }
 EXPORT_SYMBOL(closure_put);
 
-/**
+/*
  * closure_wake_up - wake up all closures on a wait list, without memory barrier
  */
 void __closure_wake_up(struct closure_waitlist *wait_list)
@@ -79,9 +79,9 @@ EXPORT_SYMBOL(__closure_wake_up);
 
 /**
  * closure_wait - add a closure to a waitlist
- *
- * @waitlist will own a ref on @cl, which will be released when
+ * @waitlist: will own a ref on @cl, which will be released when
  * closure_wake_up() is called on @waitlist.
+ * @cl: closure pointer.
  *
  */
 bool closure_wait(struct closure_waitlist *waitlist, struct closure *cl)
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 6422846b546e..ed129ba557a8 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -295,6 +295,7 @@ static void bch_data_insert_start(struct closure *cl)
 
 /**
  * bch_data_insert - stick some data in the cache
+ * @cl: closure pointer.
  *
  * This is the starting point for any data to end up in a cache device; it could
  * be from a normal write, or a writeback write, or a write to a flash only
diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index 6198041f0ee2..74febd5230df 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -82,10 +82,9 @@ STRTO_H(strtoll, long long)
 STRTO_H(strtoull, unsigned long long)
 
 /**
- * bch_hprint() - formats @v to human readable string for sysfs.
- *
- * @v - signed 64 bit integer
- * @buf - the (at least 8 byte) buffer to format the result into.
+ * bch_hprint - formats @v to human readable string for sysfs.
+ * @buf: the (at least 8 byte) buffer to format the result into.
+ * @v: signed 64 bit integer
  *
  * Returns the number of bytes used by format.
  */
@@ -225,13 +224,12 @@ void bch_time_stats_update(struct time_stats *stats, uint64_t start_time)
 }
 
 /**
- * bch_next_delay() - increment @d by the amount of work done, and return how
- * long to delay until the next time to do some work.
- *
- * @d - the struct bch_ratelimit to update
- * @done - the amount of work done, in arbitrary units
+ * bch_next_delay() - update ratelimiting statistics and calculate next delay
+ * @d: the struct bch_ratelimit to update
+ * @done: the amount of work done, in arbitrary units
  *
- * Returns the amount of time to delay by, in jiffies
+ * Increment @d by the amount of work done, and return how long to delay in
+ * jiffies until the next time to do some work.
  */
 uint64_t bch_next_delay(struct bch_ratelimit *d, uint64_t done)
 {
-- 
2.16.2

Re: [PATCH 04/16] bcache: Fix kernel-doc warnings

[Michael Lyle]

On 03/15/2018 08:08 AM, Bart Van Assche wrote:
> Avoid that building with W=1 triggers warnings about the kernel-doc
> headers.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

LGTM, applying.

[PATCH 05/16] bcache: Remove an unused variable

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/extents.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/bcache/extents.c b/drivers/md/bcache/extents.c
index f9d391711595..c334e6666461 100644
--- a/drivers/md/bcache/extents.c
+++ b/drivers/md/bcache/extents.c
@@ -534,7 +534,6 @@ static bool bch_extent_bad_expensive(struct btree *b, const struct bkey *k,
 static bool bch_extent_bad(struct btree_keys *bk, const struct bkey *k)
 {
 	struct btree *b = container_of(bk, struct btree, keys);
-	struct bucket *g;
 	unsigned i, stale;
 
 	if (!KEY_PTRS(k) ||
@@ -549,7 +548,6 @@ static bool bch_extent_bad(struct btree_keys *bk, const struct bkey *k)
 		return false;
 
 	for (i = 0; i < KEY_PTRS(k); i++) {
-		g = PTR_BUCKET(b->c, k, i);
 		stale = ptr_stale(b->c, k, i);
 
 		btree_bug_on(stale > 96, b,
-- 
2.16.2

Re: [PATCH 05/16] bcache: Remove an unused variable

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

Hi Bart,

I suggest to add at least one line commit log, thanks in advance for it :-)

Coly Li

> ---
>  drivers/md/bcache/extents.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/extents.c b/drivers/md/bcache/extents.c
> index f9d391711595..c334e6666461 100644
> --- a/drivers/md/bcache/extents.c
> +++ b/drivers/md/bcache/extents.c
> @@ -534,7 +534,6 @@ static bool bch_extent_bad_expensive(struct btree *b, const struct bkey *k,
>  static bool bch_extent_bad(struct btree_keys *bk, const struct bkey *k)
>  {
>  	struct btree *b = container_of(bk, struct btree, keys);
> -	struct bucket *g;
>  	unsigned i, stale;
>  
>  	if (!KEY_PTRS(k) ||
> @@ -549,7 +548,6 @@ static bool bch_extent_bad(struct btree_keys *bk, const struct bkey *k)
>  		return false;
>  
>  	for (i = 0; i < KEY_PTRS(k); i++) {
> -		g = PTR_BUCKET(b->c, k, i);
>  		stale = ptr_stale(b->c, k, i);
>  
>  		btree_bug_on(stale > 96, b,
> 

Re: [PATCH 05/16] bcache: Remove an unused variable

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

Hi Bart,

Could you please to add at least one line commit log ? Thanks in advance
for this :-)

Coly Li

> ---
>  drivers/md/bcache/extents.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/extents.c b/drivers/md/bcache/extents.c
> index f9d391711595..c334e6666461 100644
> --- a/drivers/md/bcache/extents.c
> +++ b/drivers/md/bcache/extents.c
> @@ -534,7 +534,6 @@ static bool bch_extent_bad_expensive(struct btree *b, const struct bkey *k,
>  static bool bch_extent_bad(struct btree_keys *bk, const struct bkey *k)
>  {
>  	struct btree *b = container_of(bk, struct btree, keys);
> -	struct bucket *g;
>  	unsigned i, stale;
>  
>  	if (!KEY_PTRS(k) ||
> @@ -549,7 +548,6 @@ static bool bch_extent_bad(struct btree_keys *bk, const struct bkey *k)
>  		return false;
>  
>  	for (i = 0; i < KEY_PTRS(k); i++) {
> -		g = PTR_BUCKET(b->c, k, i);
>  		stale = ptr_stale(b->c, k, i);
>  
>  		btree_bug_on(stale > 96, b,
> 

Re: [PATCH 05/16] bcache: Remove an unused variable

[Christoph Hellwig]

On Fri, Mar 16, 2018 at 12:11:16AM +0800, Coly Li wrote:
> On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> 
> Hi Bart,
> 
> Could you please to add at least one line commit log ? Thanks in advance
> for this :-)

Hi subject line perfectly describes the patch, so why?

Re: [PATCH 05/16] bcache: Remove an unused variable

[Coly Li]

On 16/03/2018 12:13 AM, Christoph Hellwig wrote:
> On Fri, Mar 16, 2018 at 12:11:16AM +0800, Coly Li wrote:
>> On 15/03/2018 11:08 PM, Bart Van Assche wrote:
>>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>>
>> Hi Bart,
>>
>> Could you please to add at least one line commit log ? Thanks in advance
>> for this :-)
> 
> Hi subject line perfectly describes the patch, so why?
> 
Hi Christoph,

We are trying to avoid patches with empty commit log for bcache patches.
A patch without commit log works, but just like the fixes to remove
smatch/sparse warning, adding commit log is preferred.

Thanks.

Coly Li

Re: [PATCH 05/16] bcache: Remove an unused variable

[Michael Lyle]

On 03/15/2018 08:08 AM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

LGTM, applying.  (I don't see a problem with empty body).

[PATCH 06/16] bcache: Suppress more warnings about set-but-not-used variables

[Bart Van Assche]

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/bset.c    | 4 ++--
 drivers/md/bcache/journal.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/bset.c b/drivers/md/bcache/bset.c
index e56d3ecdbfcb..579c696a5fe0 100644
--- a/drivers/md/bcache/bset.c
+++ b/drivers/md/bcache/bset.c
@@ -1072,7 +1072,7 @@ EXPORT_SYMBOL(bch_btree_iter_init);
 static inline struct bkey *__bch_btree_iter_next(struct btree_iter *iter,
 						 btree_iter_cmp_fn *cmp)
 {
-	struct btree_iter_set unused;
+	struct btree_iter_set b __maybe_unused;
 	struct bkey *ret = NULL;
 
 	if (!btree_iter_end(iter)) {
@@ -1087,7 +1087,7 @@ static inline struct bkey *__bch_btree_iter_next(struct btree_iter *iter,
 		}
 
 		if (iter->data->k == iter->data->end)
-			heap_pop(iter, unused, cmp);
+			heap_pop(iter, b, cmp);
 		else
 			heap_sift(iter, 0, cmp);
 	}
diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
index 1b736b860739..605368ff13c9 100644
--- a/drivers/md/bcache/journal.c
+++ b/drivers/md/bcache/journal.c
@@ -493,7 +493,7 @@ static void journal_reclaim(struct cache_set *c)
 	struct cache *ca;
 	uint64_t last_seq;
 	unsigned iter, n = 0;
-	atomic_t p;
+	atomic_t p __maybe_unused;
 
 	atomic_long_inc(&c->reclaim);
 
-- 
2.16.2

Re: [PATCH 06/16] bcache: Suppress more warnings about set-but-not-used variables

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> This patch does not change any functionality.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

Hi Bart,

This patch looks good to me. A question is, does GCC later than 4.3
supports such annotation like '__maybe_unused' ?

Thanks.

Coly Li

> ---
>  drivers/md/bcache/bset.c    | 4 ++--
>  drivers/md/bcache/journal.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/bset.c b/drivers/md/bcache/bset.c
> index e56d3ecdbfcb..579c696a5fe0 100644
> --- a/drivers/md/bcache/bset.c
> +++ b/drivers/md/bcache/bset.c
> @@ -1072,7 +1072,7 @@ EXPORT_SYMBOL(bch_btree_iter_init);
>  static inline struct bkey *__bch_btree_iter_next(struct btree_iter *iter,
>  						 btree_iter_cmp_fn *cmp)
>  {
> -	struct btree_iter_set unused;
> +	struct btree_iter_set b __maybe_unused;
>  	struct bkey *ret = NULL;
>  
>  	if (!btree_iter_end(iter)) {
> @@ -1087,7 +1087,7 @@ static inline struct bkey *__bch_btree_iter_next(struct btree_iter *iter,
>  		}
>  
>  		if (iter->data->k == iter->data->end)
> -			heap_pop(iter, unused, cmp);
> +			heap_pop(iter, b, cmp);
>  		else
>  			heap_sift(iter, 0, cmp);
>  	}
> diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
> index 1b736b860739..605368ff13c9 100644
> --- a/drivers/md/bcache/journal.c
> +++ b/drivers/md/bcache/journal.c
> @@ -493,7 +493,7 @@ static void journal_reclaim(struct cache_set *c)
>  	struct cache *ca;
>  	uint64_t last_seq;
>  	unsigned iter, n = 0;
> -	atomic_t p;
> +	atomic_t p __maybe_unused;
>  
>  	atomic_long_inc(&c->reclaim);
>  
> 

Re: [PATCH 06/16] bcache: Suppress more warnings about set-but-not-used variables

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTE2IGF0IDAwOjIwICswODAwLCBDb2x5IExpIHdyb3RlOg0KPiBPbiAx
NS8wMy8yMDE4IDExOjA4IFBNLCBCYXJ0IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gVGhpcyBwYXRj
aCBkb2VzIG5vdCBjaGFuZ2UgYW55IGZ1bmN0aW9uYWxpdHkuDQo+ID4gDQo+ID4gU2lnbmVkLW9m
Zi1ieTogQmFydCBWYW4gQXNzY2hlIDxiYXJ0LnZhbmFzc2NoZUB3ZGMuY29tPg0KPiANCj4gSGkg
QmFydCwNCj4gDQo+IFRoaXMgcGF0Y2ggbG9va3MgZ29vZCB0byBtZS4gQSBxdWVzdGlvbiBpcywg
ZG9lcyBHQ0MgbGF0ZXIgdGhhbiA0LjMNCj4gc3VwcG9ydHMgc3VjaCBhbm5vdGF0aW9uIGxpa2Ug
J19fbWF5YmVfdW51c2VkJyA/DQoNCkhlbGxvIENvbHksDQoNCllvdSBtYXkgd2FudCB0byBoYXZl
IGEgbG9vayBhdCB0aGUgZm9sbG93aW5nIGNvbW1pdDoNCg0KY29tbWl0IDBkN2ViYmJjNmVhYTU1
MzlmNzhhYjIwZWQ2ZmYxNzI1YTRlMzMyZWYNCkF1dGhvcjogRGF2aWQgUmllbnRqZXMgPHJpZW50
amVzQGdvb2dsZS5jb20+DQpEYXRlOiAgIFdlZCBNYXkgOSAwMjozNToyNyAyMDA3IC0wNzAwDQoN
CiAgICBjb21waWxlcjogaW50cm9kdWNlIF9fdXNlZCBhbmQgX19tYXliZV91bnVzZWQNCg0KICAg
IF9fdXNlZCBpcyBkZWZpbmVkIHRvIGJlIF9fYXR0cmlidXRlX18oKHVudXNlZCkpIGZvciBhbGwg
cHJlLTMuMyBnY2MNCiAgICBjb21waWxlcnMgdG8gc3VwcHJlc3Mgd2FybmluZ3MgZm9yIHVudXNl
ZCBmdW5jdGlvbnMgYmVjYXVzZSBwZXJoYXBzIHRoZXkNCiAgICBhcmUgcmVmZXJlbmNlZCBvbmx5
IGluIGlubGluZSBhc3NlbWJseS4gIEl0IGlzIGRlZmluZWQgdG8gYmUNCiAgICBfX2F0dHJpYnV0
ZV9fKCh1c2VkKSkgZm9yIGdjYyAzLjMgYW5kIGxhdGVyIHNvIHRoYXQgdGhlIGNvZGUgaXMgc3Rp
bGwNCiAgICBlbWl0dGVkIGZvciBzdWNoIGZ1bmN0aW9ucy4NCg0KDQoNClRoYW5rcywNCg0KQmFy
dC4NCg0KDQo=

Re: [PATCH 06/16] bcache: Suppress more warnings about set-but-not-used variables

[Coly Li]

On 16/03/2018 12:50 AM, Bart Van Assche wrote:
> On Fri, 2018-03-16 at 00:20 +0800, Coly Li wrote:
>> On 15/03/2018 11:08 PM, Bart Van Assche wrote:
>>> This patch does not change any functionality.
>>>
>>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>>
>> Hi Bart,
>>
>> This patch looks good to me. A question is, does GCC later than 4.3
>> supports such annotation like '__maybe_unused' ?
> 
> Hello Coly,
> 
> You may want to have a look at the following commit:
> 
> commit 0d7ebbbc6eaa5539f78ab20ed6ff1725a4e332ef
> Author: David Rientjes <rientjes@google.com>
> Date:   Wed May 9 02:35:27 2007 -0700
> 
>     compiler: introduce __used and __maybe_unused
> 
>     __used is defined to be __attribute__((unused)) for all pre-3.3 gcc
>     compilers to suppress warnings for unused functions because perhaps they
>     are referenced only in inline assembly.  It is defined to be
>     __attribute__((used)) for gcc 3.3 and later so that the code is still
>     emitted for such functions.
> 
Hi Bart,

Thanks for the hint, I don't need to worry about it.

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

Coly Li

[PATCH 07/16] bcache: Reduce the number of sparse complaints about lock imbalances

[Bart Van Assche]

Add more annotations for sparse to inform it about which functions do
not have the same number of spin_lock() and spin_unlock() calls.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/journal.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
index 605368ff13c9..a191fd04212f 100644
--- a/drivers/md/bcache/journal.c
+++ b/drivers/md/bcache/journal.c
@@ -594,6 +594,7 @@ static void journal_write_done(struct closure *cl)
 }
 
 static void journal_write_unlock(struct closure *cl)
+	__releases(&c->journal.lock)
 {
 	struct cache_set *c = container_of(cl, struct cache_set, journal.io);
 
@@ -705,6 +706,7 @@ static void journal_try_write(struct cache_set *c)
 
 static struct journal_write *journal_wait_for_write(struct cache_set *c,
 						    unsigned nkeys)
+	__acquires(&c->journal.lock)
 {
 	size_t sectors;
 	struct closure cl;
-- 
2.16.2

Re: [PATCH 07/16] bcache: Reduce the number of sparse complaints about lock imbalances

[Michael Lyle]

On 03/15/2018 08:08 AM, Bart Van Assche wrote:
> Add more annotations for sparse to inform it about which functions do
> not have the same number of spin_lock() and spin_unlock() calls.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

LGTM, applying.  -mpl

[PATCH 08/16] bcache: Fix a compiler warning in bcache_device_init()

[Bart Van Assche]

Avoid that building with W=1 triggers the following compiler warning:

drivers/md/bcache/super.c:776:20: warning: comparison is always false due to limited range of data type [-Wtype-limits]
      d->nr_stripes > SIZE_MAX / sizeof(atomic_t)) {
                    ^

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e8dfa804bd98..87c1f853bbb3 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -769,6 +769,8 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 			      sector_t sectors)
 {
 	struct request_queue *q;
+	const size_t max_stripes = min_t(size_t, INT_MAX,
+					 SIZE_MAX / sizeof(atomic_t));
 	size_t n;
 	int idx;
 
@@ -777,9 +779,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
 
 	d->nr_stripes = DIV_ROUND_UP_ULL(sectors, d->stripe_size);
 
-	if (!d->nr_stripes ||
-	    d->nr_stripes > INT_MAX ||
-	    d->nr_stripes > SIZE_MAX / sizeof(atomic_t)) {
+	if (!d->nr_stripes || d->nr_stripes > max_stripes) {
 		pr_err("nr_stripes too large or invalid: %u (start sector beyond end of disk?)",
 			(unsigned)d->nr_stripes);
 		return -ENOMEM;
-- 
2.16.2

Re: [PATCH 08/16] bcache: Fix a compiler warning in bcache_device_init()

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> Avoid that building with W=1 triggers the following compiler warning:
> 
> drivers/md/bcache/super.c:776:20: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>       d->nr_stripes > SIZE_MAX / sizeof(atomic_t)) {
>                     ^
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

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

Thanks.

Coly Li

> ---
>  drivers/md/bcache/super.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index e8dfa804bd98..87c1f853bbb3 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -769,6 +769,8 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
>  			      sector_t sectors)
>  {
>  	struct request_queue *q;
> +	const size_t max_stripes = min_t(size_t, INT_MAX,
> +					 SIZE_MAX / sizeof(atomic_t));
>  	size_t n;
>  	int idx;
>  
> @@ -777,9 +779,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,
>  
>  	d->nr_stripes = DIV_ROUND_UP_ULL(sectors, d->stripe_size);
>  
> -	if (!d->nr_stripes ||
> -	    d->nr_stripes > INT_MAX ||
> -	    d->nr_stripes > SIZE_MAX / sizeof(atomic_t)) {
> +	if (!d->nr_stripes || d->nr_stripes > max_stripes) {
>  		pr_err("nr_stripes too large or invalid: %u (start sector beyond end of disk?)",
>  			(unsigned)d->nr_stripes);
>  		return -ENOMEM;
> 

Re: [PATCH 08/16] bcache: Fix a compiler warning in bcache_device_init()

[Michael Lyle]

On Thu, Mar 15, 2018 at 8:08 AM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> Avoid that building with W=1 triggers the following compiler warning:
>
> drivers/md/bcache/super.c:776:20: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>       d->nr_stripes > SIZE_MAX / sizeof(atomic_t)) {
>                     ^
>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

LGTM, applying.

[PATCH 09/16] bcache: Remove a redundant assignment

[Bart Van Assche]

A bio_set_op_attrs() call a little further down overwrites bio->bi_opf.
That means that the bio->bi_opf assignment is redundant. Hence remove it.
See also commit ad0d9e76a412 ("bcache: use bio op accessors").

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/md/bcache/super.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 87c1f853bbb3..d19a44cd1fd7 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -353,7 +353,6 @@ static void uuid_io(struct cache_set *c, int op, unsigned long op_flags,
 	for (i = 0; i < KEY_PTRS(k); i++) {
 		struct bio *bio = bch_bbio_alloc(c);
 
-		bio->bi_opf = REQ_SYNC | REQ_META | op_flags;
 		bio->bi_iter.bi_size = KEY_SIZE(k) << 9;
 
 		bio->bi_end_io	= uuid_endio;
-- 
2.16.2

Re: [PATCH 09/16] bcache: Remove a redundant assignment

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> A bio_set_op_attrs() call a little further down overwrites bio->bi_opf.
> That means that the bio->bi_opf assignment is redundant. Hence remove it.
> See also commit ad0d9e76a412 ("bcache: use bio op accessors").
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>

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

Thanks.

Coly Li

> ---
>  drivers/md/bcache/super.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 87c1f853bbb3..d19a44cd1fd7 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -353,7 +353,6 @@ static void uuid_io(struct cache_set *c, int op, unsigned long op_flags,
>  	for (i = 0; i < KEY_PTRS(k); i++) {
>  		struct bio *bio = bch_bbio_alloc(c);
>  
> -		bio->bi_opf = REQ_SYNC | REQ_META | op_flags;
>  		bio->bi_iter.bi_size = KEY_SIZE(k) << 9;
>  
>  		bio->bi_end_io	= uuid_endio;
> 

[PATCH 10/16] bcache: Suppress a compiler warning in bch_##name##_h()

[Bart Van Assche]

Avoid that building with W=1 triggers the following compiler warning:

drivers/md/bcache/util.c: In function 'bch_strtoull_h':
drivers/md/bcache/util.c:70:10: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
       (i < 0 && -ANYSINT_MAX(type) / 1024 > i)) \
          ^

The only functional change in this patch is that (type) ~0 is changed
into ~(type)0. That makes a difference for unsigned types for which
sizeof(type) > sizeof(int). I think this change is a bug fix.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/util.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index 74febd5230df..755523474649 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -25,6 +25,7 @@ int bch_ ## name ## _h(const char *cp, type *res)		\
 	int u = 0;						\
 	char *e;						\
 	type i = simple_ ## name(cp, &e, 10);			\
+	const type zero = 0;					\
 								\
 	switch (tolower(*e)) {					\
 	default:						\
@@ -63,11 +64,10 @@ int bch_ ## name ## _h(const char *cp, type *res)		\
 		return -EINVAL;					\
 								\
 	while (u--) {						\
-		if ((type) ~0 > 0 &&				\
-		    (type) ~0 / 1024 <= i)			\
+		if (~zero > 0 && ~zero / 1024 <= i)		\
 			return -EINVAL;				\
-		if ((i > 0 && ANYSINT_MAX(type) / 1024 < i) ||	\
-		    (i < 0 && -ANYSINT_MAX(type) / 1024 > i))	\
+		if ((i > zero && ANYSINT_MAX(type) / 1024 < i) ||\
+		    (i < zero && -ANYSINT_MAX(type) / 1024 > i))\
 			return -EINVAL;				\
 		i *= 1024;					\
 	}							\
-- 
2.16.2

[PATCH 11/16] bcache: Check the d->disk pointer before using it

[Bart Van Assche]

Since bcache_device_free() checks the d->disk pointer I think that
means that that pointer can be NULL. Hence test that pointer before
using it. This was detected by smatch.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index d19a44cd1fd7..39bec137f636 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -742,7 +742,7 @@ static void bcache_device_free(struct bcache_device *d)
 {
 	lockdep_assert_held(&bch_register_lock);
 
-	pr_info("%s stopped", d->disk->disk_name);
+	pr_info("%s stopped", d->disk ? d->disk->disk_name : "(?)");
 
 	if (d->c)
 		bcache_device_detach(d);
-- 
2.16.2

Re: [PATCH 11/16] bcache: Check the d->disk pointer before using it

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> Since bcache_device_free() checks the d->disk pointer I think that
> means that that pointer can be NULL. Hence test that pointer before
> using it. This was detected by smatch.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

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

Thanks.

Coly Li

> ---
>  drivers/md/bcache/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index d19a44cd1fd7..39bec137f636 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -742,7 +742,7 @@ static void bcache_device_free(struct bcache_device *d)
>  {
>  	lockdep_assert_held(&bch_register_lock);
>  
> -	pr_info("%s stopped", d->disk->disk_name);
> +	pr_info("%s stopped", d->disk ? d->disk->disk_name : "(?)");
>  
>  	if (d->c)
>  		bcache_device_detach(d);
> 

[PATCH 12/16] bcache: Make it easier for static analyzers to analyze bch_allocator_thread()

[Bart Van Assche]

This patch does not change any functionality but avoids that smatch
reports the following:

drivers/md/bcache/alloc.c:334: bch_allocator_thread() error: uninitialized symbol 'bucket'.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/alloc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index 458e1d38577d..6a98d48c5f5f 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -316,6 +316,7 @@ static int bch_allocator_push(struct cache *ca, long bucket)
 static int bch_allocator_thread(void *arg)
 {
 	struct cache *ca = arg;
+	long bucket;
 
 	mutex_lock(&ca->set->bucket_lock);
 
@@ -325,11 +326,7 @@ static int bch_allocator_thread(void *arg)
 		 * possibly issue discards to them, then we add the bucket to
 		 * the free list:
 		 */
-		while (!fifo_empty(&ca->free_inc)) {
-			long bucket;
-
-			fifo_pop(&ca->free_inc, bucket);
-
+		while (fifo_pop(&ca->free_inc, bucket)) {
 			if (ca->discard) {
 				mutex_unlock(&ca->set->bucket_lock);
 				blkdev_issue_discard(ca->bdev,
-- 
2.16.2

Re: [PATCH 12/16] bcache: Make it easier for static analyzers to analyze bch_allocator_thread()

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> This patch does not change any functionality but avoids that smatch
> reports the following:
> 
> drivers/md/bcache/alloc.c:334: bch_allocator_thread() error: uninitialized symbol 'bucket'.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

Hi Bart,

Your change is OK to me, but the original code seems OK too. Can I say
this is a bug should be fixed from smatch ?

Thanks.

Coly Li

> ---
>  drivers/md/bcache/alloc.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> index 458e1d38577d..6a98d48c5f5f 100644
> --- a/drivers/md/bcache/alloc.c
> +++ b/drivers/md/bcache/alloc.c
> @@ -316,6 +316,7 @@ static int bch_allocator_push(struct cache *ca, long bucket)
>  static int bch_allocator_thread(void *arg)
>  {
>  	struct cache *ca = arg;
> +	long bucket;
>  
>  	mutex_lock(&ca->set->bucket_lock);
>  
> @@ -325,11 +326,7 @@ static int bch_allocator_thread(void *arg)
>  		 * possibly issue discards to them, then we add the bucket to
>  		 * the free list:
>  		 */
> -		while (!fifo_empty(&ca->free_inc)) {
> -			long bucket;
> -
> -			fifo_pop(&ca->free_inc, bucket);
> -
> +		while (fifo_pop(&ca->free_inc, bucket)) {
>  			if (ca->discard) {
>  				mutex_unlock(&ca->set->bucket_lock);
>  				blkdev_issue_discard(ca->bdev,
> 

Re: [PATCH 12/16] bcache: Make it easier for static analyzers to analyze bch_allocator_thread()

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTE2IGF0IDAwOjI5ICswODAwLCBDb2x5IExpIHdyb3RlOg0KPiBPbiAx
NS8wMy8yMDE4IDExOjA4IFBNLCBCYXJ0IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gVGhpcyBwYXRj
aCBkb2VzIG5vdCBjaGFuZ2UgYW55IGZ1bmN0aW9uYWxpdHkgYnV0IGF2b2lkcyB0aGF0IHNtYXRj
aA0KPiA+IHJlcG9ydHMgdGhlIGZvbGxvd2luZzoNCj4gPiANCj4gPiBkcml2ZXJzL21kL2JjYWNo
ZS9hbGxvYy5jOjMzNDogYmNoX2FsbG9jYXRvcl90aHJlYWQoKSBlcnJvcjogdW5pbml0aWFsaXpl
ZCBzeW1ib2wgJ2J1Y2tldCcuDQo+ID4gDQo+ID4gU2lnbmVkLW9mZi1ieTogQmFydCBWYW4gQXNz
Y2hlIDxiYXJ0LnZhbmFzc2NoZUB3ZGMuY29tPg0KPiANCj4gSGkgQmFydCwNCj4gDQo+IFlvdXIg
Y2hhbmdlIGlzIE9LIHRvIG1lLCBidXQgdGhlIG9yaWdpbmFsIGNvZGUgc2VlbXMgT0sgdG9vLiBD
YW4gSSBzYXkNCj4gdGhpcyBpcyBhIGJ1ZyBzaG91bGQgYmUgZml4ZWQgZnJvbSBzbWF0Y2ggPw0K
DQpIZWxsbyBDb2x5LA0KDQpJdCB3b3VsZCBiZSBncmVhdCBpZiBzbWF0Y2ggd291bGQgYmUgbW9k
aWZpZWQgc3VjaCB0aGF0IGl0IGRvZXMgbm90IGNvbXBsYWluDQphYm91dCB0aGlzIGNvZGUuIEJ1
dCBJIHRoaW5rIHRoYXQgdGhpcyBwYXRjaCBkb2VzIG5vdCBvbmx5IG1ha2UgdGhlIGNvZGUNCmVh
c2llciB0byBhbmFseXplIGZvciBzdGF0aWMgYW5hbHl6ZXJzIGJ1dCBhbHNvIGZvciBodW1hbnMu
IFNvIHBsZWFzZSBldmFsdWF0ZQ0KdGhpcyBwYXRjaCBmcm9tIHRoYXQgcGVyc3BlY3RpdmUuDQoN
ClRoYW5rcywNCg0KQmFydC4NCg0KDQo=

Re: [PATCH 12/16] bcache: Make it easier for static analyzers to analyze bch_allocator_thread()

[Coly Li]

On 16/03/2018 12:52 AM, Bart Van Assche wrote:
> On Fri, 2018-03-16 at 00:29 +0800, Coly Li wrote:
>> On 15/03/2018 11:08 PM, Bart Van Assche wrote:
>>> This patch does not change any functionality but avoids that smatch
>>> reports the following:
>>>
>>> drivers/md/bcache/alloc.c:334: bch_allocator_thread() error: uninitialized symbol 'bucket'.
>>>
>>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>>
>> Hi Bart,
>>
>> Your change is OK to me, but the original code seems OK too. Can I say
>> this is a bug should be fixed from smatch ?
> 
> Hello Coly,
> 
> It would be great if smatch would be modified such that it does not complain
> about this code. But I think that this patch does not only make the code
> easier to analyze for static analyzers but also for humans. So please evaluate
> this patch from that perspective.

Hi Bart,

It's fair enough, I am convinced. Thanks.

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

Coly Li

[PATCH 13/16] bcache: Make bch_dump_read() fail if copying to user space fails

[Bart Van Assche]

copy_to_user() returns the number of remaining bytes. Avoid that
a larger value is returned than the number of bytes that have
been copied by returning -EFAULT if not all bytes have been copied.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/debug.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
index af89408befe8..376899cfcbf1 100644
--- a/drivers/md/bcache/debug.c
+++ b/drivers/md/bcache/debug.c
@@ -175,9 +175,8 @@ static ssize_t bch_dump_read(struct file *file, char __user *buf,
 		struct keybuf_key *w;
 		unsigned bytes = min(i->bytes, size);
 
-		int err = copy_to_user(buf, i->buf, bytes);
-		if (err)
-			return err;
+		if (copy_to_user(buf, i->buf, bytes))
+			return -EFAULT;
 
 		ret	 += bytes;
 		buf	 += bytes;
-- 
2.16.2

Re: [PATCH 13/16] bcache: Make bch_dump_read() fail if copying to user space fails

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> copy_to_user() returns the number of remaining bytes. Avoid that
> a larger value is returned than the number of bytes that have
> been copied by returning -EFAULT if not all bytes have been copied.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> ---
>  drivers/md/bcache/debug.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
> index af89408befe8..376899cfcbf1 100644
> --- a/drivers/md/bcache/debug.c
> +++ b/drivers/md/bcache/debug.c
> @@ -175,9 +175,8 @@ static ssize_t bch_dump_read(struct file *file, char __user *buf,
>  		struct keybuf_key *w;
>  		unsigned bytes = min(i->bytes, size);
>  
> -		int err = copy_to_user(buf, i->buf, bytes);
> -		if (err)
> -			return err;
> +		if (copy_to_user(buf, i->buf, bytes))
> +			return -EFAULT;
>  

Hi Bart,

I am not sure whether this change is correct. -EFAULT seems not an
expected return value of read(2), while -1 is the expected return value
when error occurs.

Maybe if copy_to_user() returns value in (0, size], "ret + (size - err)"
should be returned. An exception is when copy_to_user() returns 0 and
ret is 0 too, in this situation -1 should be returned.

Correct me if I am wrong.

Coly Li

>  		ret	 += bytes;
>  		buf	 += bytes;
> 

Re: [PATCH 13/16] bcache: Make bch_dump_read() fail if copying to user space fails

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTE2IGF0IDAxOjAwICswODAwLCBDb2x5IExpIHdyb3RlOg0KPiBPbiAx
NS8wMy8yMDE4IDExOjA4IFBNLCBCYXJ0IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gY29weV90b191
c2VyKCkgcmV0dXJucyB0aGUgbnVtYmVyIG9mIHJlbWFpbmluZyBieXRlcy4gQXZvaWQgdGhhdA0K
PiA+IGEgbGFyZ2VyIHZhbHVlIGlzIHJldHVybmVkIHRoYW4gdGhlIG51bWJlciBvZiBieXRlcyB0
aGF0IGhhdmUNCj4gPiBiZWVuIGNvcGllZCBieSByZXR1cm5pbmcgLUVGQVVMVCBpZiBub3QgYWxs
IGJ5dGVzIGhhdmUgYmVlbiBjb3BpZWQuDQo+ID4gDQo+ID4gU2lnbmVkLW9mZi1ieTogQmFydCBW
YW4gQXNzY2hlIDxiYXJ0LnZhbmFzc2NoZUB3ZGMuY29tPg0KPiA+IC0tLQ0KPiA+ICBkcml2ZXJz
L21kL2JjYWNoZS9kZWJ1Zy5jIHwgNSArKy0tLQ0KPiA+ICAxIGZpbGUgY2hhbmdlZCwgMiBpbnNl
cnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQ0KPiA+IA0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJz
L21kL2JjYWNoZS9kZWJ1Zy5jIGIvZHJpdmVycy9tZC9iY2FjaGUvZGVidWcuYw0KPiA+IGluZGV4
IGFmODk0MDhiZWZlOC4uMzc2ODk5Y2ZjYmYxIDEwMDY0NA0KPiA+IC0tLSBhL2RyaXZlcnMvbWQv
YmNhY2hlL2RlYnVnLmMNCj4gPiArKysgYi9kcml2ZXJzL21kL2JjYWNoZS9kZWJ1Zy5jDQo+ID4g
QEAgLTE3NSw5ICsxNzUsOCBAQCBzdGF0aWMgc3NpemVfdCBiY2hfZHVtcF9yZWFkKHN0cnVjdCBm
aWxlICpmaWxlLCBjaGFyIF9fdXNlciAqYnVmLA0KPiA+ICAJCXN0cnVjdCBrZXlidWZfa2V5ICp3
Ow0KPiA+ICAJCXVuc2lnbmVkIGJ5dGVzID0gbWluKGktPmJ5dGVzLCBzaXplKTsNCj4gPiAgDQo+
ID4gLQkJaW50IGVyciA9IGNvcHlfdG9fdXNlcihidWYsIGktPmJ1ZiwgYnl0ZXMpOw0KPiA+IC0J
CWlmIChlcnIpDQo+ID4gLQkJCXJldHVybiBlcnI7DQo+ID4gKwkJaWYgKGNvcHlfdG9fdXNlcihi
dWYsIGktPmJ1ZiwgYnl0ZXMpKQ0KPiA+ICsJCQlyZXR1cm4gLUVGQVVMVDsNCj4gPiAgDQo+IA0K
PiBIaSBCYXJ0LA0KPiANCj4gSSBhbSBub3Qgc3VyZSB3aGV0aGVyIHRoaXMgY2hhbmdlIGlzIGNv
cnJlY3QuIC1FRkFVTFQgc2VlbXMgbm90IGFuDQo+IGV4cGVjdGVkIHJldHVybiB2YWx1ZSBvZiBy
ZWFkKDIpLCB3aGlsZSAtMSBpcyB0aGUgZXhwZWN0ZWQgcmV0dXJuIHZhbHVlDQo+IHdoZW4gZXJy
b3Igb2NjdXJzLg0KPiANCj4gTWF5YmUgaWYgY29weV90b191c2VyKCkgcmV0dXJucyB2YWx1ZSBp
biAoMCwgc2l6ZV0sICJyZXQgKyAoc2l6ZSAtIGVycikiDQo+IHNob3VsZCBiZSByZXR1cm5lZC4g
QW4gZXhjZXB0aW9uIGlzIHdoZW4gY29weV90b191c2VyKCkgcmV0dXJucyAwIGFuZA0KPiByZXQg
aXMgMCB0b28sIGluIHRoaXMgc2l0dWF0aW9uIC0xIHNob3VsZCBiZSByZXR1cm5lZC4NCj4gDQo+
IENvcnJlY3QgbWUgaWYgSSBhbSB3cm9uZy4NCg0KSGVsbG8gQ29seSwNCg0KSSdtIG5vdCBmYW1p
bGlhciBlbm91Z2ggd2l0aCBiY2FjaGUgdG8gcHJvdmlkZSB0aGUgYW5zd2VyIHRvIHlvdXIgcXVl
c3Rpb24gc28NCkkgd2lsbCBkcm9wIHRoaXMgcGF0Y2ggZnJvbSB0aGlzIHNlcmllcy4NCg0KVGhh
bmtzLA0KDQpCYXJ0Lg0KDQoNCg==

Re: [PATCH 13/16] bcache: Make bch_dump_read() fail if copying to user space fails

[Coly Li]

On 16/03/2018 1:06 AM, Bart Van Assche wrote:
> On Fri, 2018-03-16 at 01:00 +0800, Coly Li wrote:
>> On 15/03/2018 11:08 PM, Bart Van Assche wrote:
>>> copy_to_user() returns the number of remaining bytes. Avoid that
>>> a larger value is returned than the number of bytes that have
>>> been copied by returning -EFAULT if not all bytes have been copied.
>>>
>>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
>>> ---
>>>  drivers/md/bcache/debug.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/md/bcache/debug.c b/drivers/md/bcache/debug.c
>>> index af89408befe8..376899cfcbf1 100644
>>> --- a/drivers/md/bcache/debug.c
>>> +++ b/drivers/md/bcache/debug.c
>>> @@ -175,9 +175,8 @@ static ssize_t bch_dump_read(struct file *file, char __user *buf,
>>>  		struct keybuf_key *w;
>>>  		unsigned bytes = min(i->bytes, size);
>>>  
>>> -		int err = copy_to_user(buf, i->buf, bytes);
>>> -		if (err)
>>> -			return err;
>>> +		if (copy_to_user(buf, i->buf, bytes))
>>> +			return -EFAULT;
>>>  
>>
>> Hi Bart,
>>
>> I am not sure whether this change is correct. -EFAULT seems not an
>> expected return value of read(2), while -1 is the expected return value
>> when error occurs.
>>
>> Maybe if copy_to_user() returns value in (0, size], "ret + (size - err)"
>> should be returned. An exception is when copy_to_user() returns 0 and
>> ret is 0 too, in this situation -1 should be returned.
>>
>> Correct me if I am wrong.
> 
> Hello Coly,
> 
> I'm not familiar enough with bcache to provide the answer to your question so
> I will drop this patch from this series.

Hi Bart,

It seems you catch a code bug here. I will look into this and handle it
here.

Thanks for the hint :-)

Coly Li

[PATCH 14/16] bcache: Make csum_set() implementation easier to read

[Bart Van Assche]

Introduce two temporary variables to avoid having to repeat
expressions. This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/bcache.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 12e5197f186c..72b1ea4576d9 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -777,10 +777,12 @@ static inline bool ptr_available(struct cache_set *c, const struct bkey *k,
  * This is used for various on disk data structures - cache_sb, prio_set, bset,
  * jset: The checksum is _always_ the first 8 bytes of these structs
  */
-#define csum_set(i)							\
-	bch_crc64(((void *) (i)) + sizeof(uint64_t),			\
-		  ((void *) bset_bkey_last(i)) -			\
-		  (((void *) (i)) + sizeof(uint64_t)))
+#define csum_set(i) ({						\
+	const void *p = (void *)(i) + sizeof(uint64_t);		\
+	const void *q = bset_bkey_last(i);			\
+								\
+	bch_crc64(p, q - p);					\
+})
 
 /* Error handling macros */
 
-- 
2.16.2

Re: [PATCH 14/16] bcache: Make csum_set() implementation easier to read

[Christoph Hellwig]

On Thu, Mar 15, 2018 at 08:08:12AM -0700, Bart Van Assche wrote:
> +#define csum_set(i) ({						\
> +	const void *p = (void *)(i) + sizeof(uint64_t);		\
> +	const void *q = bset_bkey_last(i);			\
> +								\
> +	bch_crc64(p, q - p);					\
> +})

Having this as a macro is really ugly.  Any chane to turn this into a
proper function?

[PATCH 15/16] bcache: Fix an endianness bug

[Bart Van Assche]

Ensure that byte swapping occurs on big endian architectures when
reading or writing the superblock.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/bcache.h | 12 ++++++++++++
 drivers/md/bcache/super.c  |  4 ++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 72b1ea4576d9..50ddc78596bf 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -784,6 +784,18 @@ static inline bool ptr_available(struct cache_set *c, const struct bkey *k,
 	bch_crc64(p, q - p);					\
 })
 
+/*
+ * Variant of csum_set() for data structures in which (i)->keys has type
+ * __le16.
+ */
+#define csum_set_le(i) ({						\
+	const void *p = (void *)(i) + sizeof(uint64_t);			\
+	const void *q = bkey_idx((struct bkey *)(i)->d,			\
+				 le16_to_cpu((i)->keys));		\
+									\
+	bch_crc64(p, q - p);						\
+})
+
 /* Error handling macros */
 
 #define btree_bug(b, ...)						\
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 39bec137f636..31d700aecd56 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -110,7 +110,7 @@ static const char *read_super(struct cache_sb *sb, struct block_device *bdev,
 		goto err;
 
 	err = "Bad checksum";
-	if (s->csum != csum_set(s))
+	if (s->csum != csum_set_le(s))
 		goto err;
 
 	err = "Bad UUID";
@@ -236,7 +236,7 @@ static void __write_super(struct cache_sb *sb, struct bio *bio)
 	for (i = 0; i < sb->keys; i++)
 		out->d[i] = cpu_to_le64(sb->d[i]);
 
-	out->csum = csum_set(out);
+	out->csum = csum_set_le(out);
 
 	pr_debug("ver %llu, flags %llu, seq %llu",
 		 sb->version, sb->flags, sb->seq);
-- 
2.16.2

Re: [PATCH 15/16] bcache: Fix an endianness bug

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> Ensure that byte swapping occurs on big endian architectures when
> reading or writing the superblock.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

Hi Bart,

The code still does not work with your patch on big endian machine (e.g.
s390x). When I fix the bcache s390x bug, I did something like this patch
in my first try. It turns out more things need to be fixed,
1, some cache_sb members only swapped to cpu endianness when read in,
not swapped to little endian when write out. For example data_offset,
nbuckets, nr_in_set, nr_this_dev ...
2, checksum is calculated in cpu endiannness, not explicitly in little
endian.

Also user space tools need to update to support the endianness issue.

How about leave the endianness problem to me? I will pick code piece
from your patch and set 'From: Bart Van Assche <bart.vanassche@wdc.com>'
on the patch(s), and continue my fix for bcache on s390x.

Thanks.

Coly Li

> ---
>  drivers/md/bcache/bcache.h | 12 ++++++++++++
>  drivers/md/bcache/super.c  |  4 ++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 72b1ea4576d9..50ddc78596bf 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -784,6 +784,18 @@ static inline bool ptr_available(struct cache_set *c, const struct bkey *k,
>  	bch_crc64(p, q - p);					\
>  })
>  
> +/*
> + * Variant of csum_set() for data structures in which (i)->keys has type
> + * __le16.
> + */
> +#define csum_set_le(i) ({						\
> +	const void *p = (void *)(i) + sizeof(uint64_t);			\
> +	const void *q = bkey_idx((struct bkey *)(i)->d,			\
> +				 le16_to_cpu((i)->keys));		\
> +									\
> +	bch_crc64(p, q - p);						\
> +})
> +
>  /* Error handling macros */
>  
>  #define btree_bug(b, ...)						\
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 39bec137f636..31d700aecd56 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -110,7 +110,7 @@ static const char *read_super(struct cache_sb *sb, struct block_device *bdev,
>  		goto err;
>  
>  	err = "Bad checksum";
> -	if (s->csum != csum_set(s))
> +	if (s->csum != csum_set_le(s))
>  		goto err;
>  
>  	err = "Bad UUID";
> @@ -236,7 +236,7 @@ static void __write_super(struct cache_sb *sb, struct bio *bio)
>  	for (i = 0; i < sb->keys; i++)
>  		out->d[i] = cpu_to_le64(sb->d[i]);
>  
> -	out->csum = csum_set(out);
> +	out->csum = csum_set_le(out);
>  
>  	pr_debug("ver %llu, flags %llu, seq %llu",
>  		 sb->version, sb->flags, sb->seq);
> 

Re: [PATCH 15/16] bcache: Fix an endianness bug

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTE2IGF0IDEyOjQ3ICswODAwLCBDb2x5IExpIHdyb3RlOg0KPiBUaGUg
Y29kZSBzdGlsbCBkb2VzIG5vdCB3b3JrIHdpdGggeW91ciBwYXRjaCBvbiBiaWcgZW5kaWFuIG1h
Y2hpbmUgKGUuZy4NCj4gczM5MHgpLiBXaGVuIEkgZml4IHRoZSBiY2FjaGUgczM5MHggYnVnLCBJ
IGRpZCBzb21ldGhpbmcgbGlrZSB0aGlzIHBhdGNoDQo+IGluIG15IGZpcnN0IHRyeS4gSXQgdHVy
bnMgb3V0IG1vcmUgdGhpbmdzIG5lZWQgdG8gYmUgZml4ZWQsDQo+IDEsIHNvbWUgY2FjaGVfc2Ig
bWVtYmVycyBvbmx5IHN3YXBwZWQgdG8gY3B1IGVuZGlhbm5lc3Mgd2hlbiByZWFkIGluLA0KPiBu
b3Qgc3dhcHBlZCB0byBsaXR0bGUgZW5kaWFuIHdoZW4gd3JpdGUgb3V0LiBGb3IgZXhhbXBsZSBk
YXRhX29mZnNldCwNCj4gbmJ1Y2tldHMsIG5yX2luX3NldCwgbnJfdGhpc19kZXYgLi4uDQo+IDIs
IGNoZWNrc3VtIGlzIGNhbGN1bGF0ZWQgaW4gY3B1IGVuZGlhbm5uZXNzLCBub3QgZXhwbGljaXRs
eSBpbiBsaXR0bGUNCj4gZW5kaWFuLg0KPiANCj4gQWxzbyB1c2VyIHNwYWNlIHRvb2xzIG5lZWQg
dG8gdXBkYXRlIHRvIHN1cHBvcnQgdGhlIGVuZGlhbm5lc3MgaXNzdWUuDQo+IA0KPiBIb3cgYWJv
dXQgbGVhdmUgdGhlIGVuZGlhbm5lc3MgcHJvYmxlbSB0byBtZT8gSSB3aWxsIHBpY2sgY29kZSBw
aWVjZQ0KPiBmcm9tIHlvdXIgcGF0Y2ggYW5kIHNldCAnRnJvbTogQmFydCBWYW4gQXNzY2hlIDxi
YXJ0LnZhbmFzc2NoZUB3ZGMuY29tPicNCj4gb24gdGhlIHBhdGNoKHMpLCBhbmQgY29udGludWUg
bXkgZml4IGZvciBiY2FjaGUgb24gczM5MHguDQoNCkhlbGxvIENvbHksDQoNClRoYXQgc291bmRz
IGZpbmUgdG8gbWUuIFBsZWFzZSBsZXQgbWUga25vdyBpZiB5b3Ugd291bGQgbGlrZSBtZSB0byBy
ZXBvc3QNCnRoaXMgc2VyaWVzIGFuZC9vciBhZGRyZXNzIHRoZSBmZWVkYmFjayB0aGF0IEkgcmVj
ZWl2ZWQgZm9yIHRoaXMgcGF0Y2ggc2VyaWVzLg0KDQpUaGFua3MsDQoNCkJhcnQuDQoNCg0K

Re: [PATCH 15/16] bcache: Fix an endianness bug

[Coly Li]

On 16/03/2018 11:00 PM, Bart Van Assche wrote:
> On Fri, 2018-03-16 at 12:47 +0800, Coly Li wrote:
>> The code still does not work with your patch on big endian machine (e.g.
>> s390x). When I fix the bcache s390x bug, I did something like this patch
>> in my first try. It turns out more things need to be fixed,
>> 1, some cache_sb members only swapped to cpu endianness when read in,
>> not swapped to little endian when write out. For example data_offset,
>> nbuckets, nr_in_set, nr_this_dev ...
>> 2, checksum is calculated in cpu endiannness, not explicitly in little
>> endian.
>>
>> Also user space tools need to update to support the endianness issue.
>>
>> How about leave the endianness problem to me? I will pick code piece
>> from your patch and set 'From: Bart Van Assche <bart.vanassche@wdc.com>'
>> on the patch(s), and continue my fix for bcache on s390x.
> 
> Hello Coly,
> 
> That sounds fine to me. Please let me know if you would like me to repost
> this series and/or address the feedback that I received for this patch series.

Hi Bart,

I see Mike already picks some of your patches, repost an updated version
for the rested patch should be a good idea.

Thanks.

Coly Li

[PATCH 16/16] bcache: Fix endianness annotations

[Bart Van Assche]

This patch avoids that sparse complains about using integer types
incorrectly (u32, __le32, __be32, ...) by splitting struct cache_sb
into two different structures:
- struct cache_sb in which all integer members except csum have
  CPU endianness.
- struct cache_sb_le in which all integer members except csum are
  declared as little endian.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
---
 drivers/md/bcache/super.c   |  10 ++--
 include/uapi/linux/bcache.h | 118 +++++++++++++++++++++++---------------------
 2 files changed, 68 insertions(+), 60 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 31d700aecd56..ef659c7d72f9 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -63,20 +63,22 @@ struct workqueue_struct *bcache_wq;
 /* limitation of bcache devices number on single system */
 #define BCACHE_DEVICE_IDX_MAX	((1U << MINORBITS)/BCACHE_MINORS)
 
+struct cache_sb_le STRUCT_CACHE_SB(le);
+
 /* Superblock */
 
 static const char *read_super(struct cache_sb *sb, struct block_device *bdev,
 			      struct page **res)
 {
 	const char *err;
-	struct cache_sb *s;
+	struct cache_sb_le *s;
 	struct buffer_head *bh = __bread(bdev, 1, SB_SIZE);
 	unsigned i;
 
 	if (!bh)
 		return "IO error";
 
-	s = (struct cache_sb *) bh->b_data;
+	s = (struct cache_sb_le *) bh->b_data;
 
 	sb->offset		= le64_to_cpu(s->offset);
 	sb->version		= le64_to_cpu(s->version);
@@ -211,7 +213,7 @@ static void write_bdev_super_endio(struct bio *bio)
 
 static void __write_super(struct cache_sb *sb, struct bio *bio)
 {
-	struct cache_sb *out = page_address(bio_first_page_all(bio));
+	struct cache_sb_le *out = page_address(bio_first_page_all(bio));
 	unsigned i;
 
 	bio->bi_iter.bi_sector	= SB_SECTOR;
@@ -959,7 +961,7 @@ void bch_cached_dev_detach(struct cached_dev *dc)
 int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 			  uint8_t *set_uuid)
 {
-	uint32_t rtime = cpu_to_le32(get_seconds());
+	__le32 rtime = cpu_to_le32(get_seconds());
 	struct uuid_entry *u;
 	char buf[BDEVNAME_SIZE];
 
diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h
index 821f71a2e48f..772787c47772 100644
--- a/include/uapi/linux/bcache.h
+++ b/include/uapi/linux/bcache.h
@@ -154,57 +154,63 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned nr_keys)
 
 #define BDEV_DATA_START_DEFAULT		16	/* sectors */
 
-struct cache_sb {
-	__u64			csum;
-	__u64			offset;	/* sector where this sb was written */
-	__u64			version;
-
-	__u8			magic[16];
-
-	__u8			uuid[16];
-	union {
-		__u8		set_uuid[16];
-		__u64		set_magic;
-	};
-	__u8			label[SB_LABEL_SIZE];
-
-	__u64			flags;
-	__u64			seq;
-	__u64			pad[8];
-
-	union {
-	struct {
-		/* Cache devices */
-		__u64		nbuckets;	/* device size */
-
-		__u16		block_size;	/* sectors */
-		__u16		bucket_size;	/* sectors */
-
-		__u16		nr_in_set;
-		__u16		nr_this_dev;
-	};
-	struct {
-		/* Backing devices */
-		__u64		data_offset;
-
-		/*
-		 * block_size from the cache device section is still used by
-		 * backing devices, so don't add anything here until we fix
-		 * things to not need it for backing devices anymore
-		 */
-	};
-	};
-
-	__u32			last_mount;	/* time_t */
-
-	__u16			first_bucket;
-	union {
-		__u16		njournal_buckets;
-		__u16		keys;
-	};
-	__u64			d[SB_JOURNAL_BUCKETS];	/* journal buckets */
+#define STRUCT_CACHE_SB(e)						\
+{									\
+	__u64			csum;					\
+	/* sector where this sb was written */				\
+	__##e##64		offset;					\
+	__##e##64		version;				\
+									\
+	__u8			magic[16];				\
+									\
+	__u8			uuid[16];				\
+	union {								\
+		__u8		set_uuid[16];				\
+		__##e##64	set_magic;				\
+	};								\
+	__u8			label[SB_LABEL_SIZE];			\
+									\
+	__##e##64		flags;					\
+	__##e##64		seq;					\
+	__##e##64		pad[8];					\
+									\
+	union {								\
+	struct {							\
+		/* Cache devices */					\
+		__##e##64	nbuckets;	/* device size */	\
+									\
+		__##e##16	block_size;	/* sectors */		\
+		__##e##16	bucket_size;	/* sectors */		\
+									\
+		__##e##16	nr_in_set;				\
+		__##e##16	nr_this_dev;				\
+	};								\
+	struct {							\
+		/* Backing devices */					\
+		__##e##64	data_offset;				\
+									\
+		/*							\
+		 * block_size from the cache device section is still	\
+		 * used by backing devices, so don't add anything here	\
+		 * until we fix things to not need it for backing	\
+		 * devices anymore					\
+		 */							\
+	};								\
+	};								\
+									\
+	__##e##32		last_mount;	/* time_t */		\
+									\
+	__##e##16		first_bucket;				\
+	union {								\
+		__##e##16	njournal_buckets;			\
+		__##e##16	keys;					\
+	};								\
+	/* journal buckets */						\
+	__##e##64		d[SB_JOURNAL_BUCKETS];			\
 };
 
+struct cache_sb STRUCT_CACHE_SB(u);
+
 static inline _Bool SB_IS_BDEV(const struct cache_sb *sb)
 {
 	return sb->version == BCACHE_SB_VERSION_BDEV
@@ -306,7 +312,7 @@ struct prio_set {
 	__u64			next_bucket;
 
 	struct bucket_disk {
-		__u16		prio;
+		__le16		prio;
 		__u8		gen;
 	} __attribute((packed)) data[];
 };
@@ -318,9 +324,9 @@ struct uuid_entry {
 		struct {
 			__u8	uuid[16];
 			__u8	label[32];
-			__u32	first_reg;
-			__u32	last_reg;
-			__u32	invalidated;
+			__le32	first_reg;
+			__le32	last_reg;
+			__le32	invalidated;
 
 			__u32	flags;
 			/* Size of flash only volumes */
@@ -366,9 +372,9 @@ struct bset {
 struct uuid_entry_v0 {
 	__u8		uuid[16];
 	__u8		label[32];
-	__u32		first_reg;
-	__u32		last_reg;
-	__u32		invalidated;
+	__le32		first_reg;
+	__le32		last_reg;
+	__le32		invalidated;
 	__u32		pad;
 };
 
-- 
2.16.2

Re: [PATCH 16/16] bcache: Fix endianness annotations

[Christoph Hellwig]

On Thu, Mar 15, 2018 at 08:08:14AM -0700, Bart Van Assche wrote:
> This patch avoids that sparse complains about using integer types
> incorrectly (u32, __le32, __be32, ...) by splitting struct cache_sb
> into two different structures:
> - struct cache_sb in which all integer members except csum have
>   CPU endianness.
> - struct cache_sb_le in which all integer members except csum are
>   declared as little endian.

Can you call this cache_sb_disk to name it after the purpose instead
of the implementation?

Except for that this looks like the right fix:

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

Re: [PATCH 16/16] bcache: Fix endianness annotations

[Coly Li]

On 15/03/2018 11:08 PM, Bart Van Assche wrote:
> This patch avoids that sparse complains about using integer types
> incorrectly (u32, __le32, __be32, ...) by splitting struct cache_sb
> into two different structures:
> - struct cache_sb in which all integer members except csum have
>   CPU endianness.
> - struct cache_sb_le in which all integer members except csum are
>   declared as little endian.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>

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

Hi Bart,

This patch is good, and I like the idea of STRUCT_CACHE_SB().

How about letting me to pick this patch into my bcache s390x fixes? I
will fix the structure name suggested by Christoph, and set you as the
patch author.

Thanks.

Coly Li

> ---
>  drivers/md/bcache/super.c   |  10 ++--
>  include/uapi/linux/bcache.h | 118 +++++++++++++++++++++++---------------------
>  2 files changed, 68 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 31d700aecd56..ef659c7d72f9 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -63,20 +63,22 @@ struct workqueue_struct *bcache_wq;
>  /* limitation of bcache devices number on single system */
>  #define BCACHE_DEVICE_IDX_MAX	((1U << MINORBITS)/BCACHE_MINORS)
>  
> +struct cache_sb_le STRUCT_CACHE_SB(le);
> +
>  /* Superblock */
>  
>  static const char *read_super(struct cache_sb *sb, struct block_device *bdev,
>  			      struct page **res)
>  {
>  	const char *err;
> -	struct cache_sb *s;
> +	struct cache_sb_le *s;
>  	struct buffer_head *bh = __bread(bdev, 1, SB_SIZE);
>  	unsigned i;
>  
>  	if (!bh)
>  		return "IO error";
>  
> -	s = (struct cache_sb *) bh->b_data;
> +	s = (struct cache_sb_le *) bh->b_data;
>  
>  	sb->offset		= le64_to_cpu(s->offset);
>  	sb->version		= le64_to_cpu(s->version);
> @@ -211,7 +213,7 @@ static void write_bdev_super_endio(struct bio *bio)
>  
>  static void __write_super(struct cache_sb *sb, struct bio *bio)
>  {
> -	struct cache_sb *out = page_address(bio_first_page_all(bio));
> +	struct cache_sb_le *out = page_address(bio_first_page_all(bio));
>  	unsigned i;
>  
>  	bio->bi_iter.bi_sector	= SB_SECTOR;
> @@ -959,7 +961,7 @@ void bch_cached_dev_detach(struct cached_dev *dc)
>  int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
>  			  uint8_t *set_uuid)
>  {
> -	uint32_t rtime = cpu_to_le32(get_seconds());
> +	__le32 rtime = cpu_to_le32(get_seconds());
>  	struct uuid_entry *u;
>  	char buf[BDEVNAME_SIZE];
>  
> diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h
> index 821f71a2e48f..772787c47772 100644
> --- a/include/uapi/linux/bcache.h
> +++ b/include/uapi/linux/bcache.h
> @@ -154,57 +154,63 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned nr_keys)
>  
>  #define BDEV_DATA_START_DEFAULT		16	/* sectors */
>  
> -struct cache_sb {
> -	__u64			csum;
> -	__u64			offset;	/* sector where this sb was written */
> -	__u64			version;
> -
> -	__u8			magic[16];
> -
> -	__u8			uuid[16];
> -	union {
> -		__u8		set_uuid[16];
> -		__u64		set_magic;
> -	};
> -	__u8			label[SB_LABEL_SIZE];
> -
> -	__u64			flags;
> -	__u64			seq;
> -	__u64			pad[8];
> -
> -	union {
> -	struct {
> -		/* Cache devices */
> -		__u64		nbuckets;	/* device size */
> -
> -		__u16		block_size;	/* sectors */
> -		__u16		bucket_size;	/* sectors */
> -
> -		__u16		nr_in_set;
> -		__u16		nr_this_dev;
> -	};
> -	struct {
> -		/* Backing devices */
> -		__u64		data_offset;
> -
> -		/*
> -		 * block_size from the cache device section is still used by
> -		 * backing devices, so don't add anything here until we fix
> -		 * things to not need it for backing devices anymore
> -		 */
> -	};
> -	};
> -
> -	__u32			last_mount;	/* time_t */
> -
> -	__u16			first_bucket;
> -	union {
> -		__u16		njournal_buckets;
> -		__u16		keys;
> -	};
> -	__u64			d[SB_JOURNAL_BUCKETS];	/* journal buckets */
> +#define STRUCT_CACHE_SB(e)						\
> +{									\
> +	__u64			csum;					\
> +	/* sector where this sb was written */				\
> +	__##e##64		offset;					\
> +	__##e##64		version;				\
> +									\
> +	__u8			magic[16];				\
> +									\
> +	__u8			uuid[16];				\
> +	union {								\
> +		__u8		set_uuid[16];				\
> +		__##e##64	set_magic;				\
> +	};								\
> +	__u8			label[SB_LABEL_SIZE];			\
> +									\
> +	__##e##64		flags;					\
> +	__##e##64		seq;					\
> +	__##e##64		pad[8];					\
> +									\
> +	union {								\
> +	struct {							\
> +		/* Cache devices */					\
> +		__##e##64	nbuckets;	/* device size */	\
> +									\
> +		__##e##16	block_size;	/* sectors */		\
> +		__##e##16	bucket_size;	/* sectors */		\
> +									\
> +		__##e##16	nr_in_set;				\
> +		__##e##16	nr_this_dev;				\
> +	};								\
> +	struct {							\
> +		/* Backing devices */					\
> +		__##e##64	data_offset;				\
> +									\
> +		/*							\
> +		 * block_size from the cache device section is still	\
> +		 * used by backing devices, so don't add anything here	\
> +		 * until we fix things to not need it for backing	\
> +		 * devices anymore					\
> +		 */							\
> +	};								\
> +	};								\
> +									\
> +	__##e##32		last_mount;	/* time_t */		\
> +									\
> +	__##e##16		first_bucket;				\
> +	union {								\
> +		__##e##16	njournal_buckets;			\
> +		__##e##16	keys;					\
> +	};								\
> +	/* journal buckets */						\
> +	__##e##64		d[SB_JOURNAL_BUCKETS];			\
>  };
>  
> +struct cache_sb STRUCT_CACHE_SB(u);
> +
>  static inline _Bool SB_IS_BDEV(const struct cache_sb *sb)
>  {
>  	return sb->version == BCACHE_SB_VERSION_BDEV
> @@ -306,7 +312,7 @@ struct prio_set {
>  	__u64			next_bucket;
>  
>  	struct bucket_disk {
> -		__u16		prio;
> +		__le16		prio;
>  		__u8		gen;
>  	} __attribute((packed)) data[];
>  };
> @@ -318,9 +324,9 @@ struct uuid_entry {
>  		struct {
>  			__u8	uuid[16];
>  			__u8	label[32];
> -			__u32	first_reg;
> -			__u32	last_reg;
> -			__u32	invalidated;
> +			__le32	first_reg;
> +			__le32	last_reg;
> +			__le32	invalidated;
>  
>  			__u32	flags;
>  			/* Size of flash only volumes */
> @@ -366,9 +372,9 @@ struct bset {
>  struct uuid_entry_v0 {
>  	__u8		uuid[16];
>  	__u8		label[32];
> -	__u32		first_reg;
> -	__u32		last_reg;
> -	__u32		invalidated;
> +	__le32		first_reg;
> +	__le32		last_reg;
> +	__le32		invalidated;
>  	__u32		pad;
>  };
>  
> 

Re: [PATCH 00/16] bcache: Compiler, sparse and smatch fixes

[Michael Lyle]

Hey Bart & everyone,

I'll dig through these.  In the future can you please send things off
the linux-bcache list?  I'm a bit off my normal workflow this way.

Thanks,

Mike

On Thu, Mar 15, 2018 at 8:07 AM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> Hello Michael, Coly and Christoph,
>
> A few weeks ago I analyzed the compiler (W=1), sparse and smatch warnings
> triggered by the bcache driver. This patch series is what I came up with.
> Although none of these patches have been tested I think these patches are
> worth a closer look. Feedback is welcome.
>
> Thanks,
>
> Bart.
>
> Bart Van Assche (16):
>   bcache: Fix indentation
>   bcache: Add __printf annotation to __bch_check_keys()
>   bcache: Annotate switch fall-through
>   bcache: Fix kernel-doc warnings
>   bcache: Remove an unused variable
>   bcache: Suppress more warnings about set-but-not-used variables
>   bcache: Reduce the number of sparse complaints about lock imbalances
>   bcache: Fix a compiler warning in bcache_device_init()
>   bcache: Remove a redundant assignment
>   bcache: Suppress a compiler warning in bch_##name##_h()
>   bcache: Check the d->disk pointer before using it
>   bcache: Make it easier for static analyzers to analyze
>     bch_allocator_thread()
>   bcache: Make bch_dump_read() fail if copying to user space fails
>   bcache: Make csum_set() implementation easier to read
>   bcache: Fix an endianness bug
>   bcache: Fix endianness annotations
>
>  drivers/md/bcache/alloc.c     |   7 +--
>  drivers/md/bcache/bcache.h    |  22 ++++++--
>  drivers/md/bcache/bset.c      |   4 +-
>  drivers/md/bcache/bset.h      |   5 +-
>  drivers/md/bcache/btree.c     |   4 +-
>  drivers/md/bcache/closure.c   |   8 +--
>  drivers/md/bcache/debug.c     |   5 +-
>  drivers/md/bcache/extents.c   |   2 -
>  drivers/md/bcache/journal.c   |   4 +-
>  drivers/md/bcache/request.c   |   1 +
>  drivers/md/bcache/super.c     |  23 ++++----
>  drivers/md/bcache/util.c      |  33 +++++++-----
>  drivers/md/bcache/writeback.h |   2 +-
>  include/uapi/linux/bcache.h   | 118 ++++++++++++++++++++++--------------------
>  14 files changed, 131 insertions(+), 107 deletions(-)
>
> --
> 2.16.2
>

Re: [PATCH 00/16] bcache: Compiler, sparse and smatch fixes

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAzLTE2IGF0IDExOjE2IC0wNzAwLCBNaWNoYWVsIEx5bGUgd3JvdGU6DQo+
IEknbGwgZGlnIHRocm91Z2ggdGhlc2UuICBJbiB0aGUgZnV0dXJlIGNhbiB5b3UgcGxlYXNlIHNl
bmQgdGhpbmdzIG9mZg0KPiB0aGUgbGludXgtYmNhY2hlIGxpc3Q/ICBJJ20gYSBiaXQgb2ZmIG15
IG5vcm1hbCB3b3JrZmxvdyB0aGlzIHdheS4NCg0KSSB3aWxsIGRvIHRoYXQuIFRoYW5rcyBmb3Ig
dGhlIHJldmlld3MhDQoNCkJhcnQuDQoNCg0KDQo=