diff mbox

[15/16] bcache: Fix an endianness bug

Message ID 20180315150814.9412-16-bart.vanassche@wdc.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bart Van Assche March 15, 2018, 3:08 p.m. UTC
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(-)

Comments

Coly Li March 16, 2018, 4:47 a.m. UTC | #1
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);
>
Bart Van Assche March 16, 2018, 3 p.m. UTC | #2
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.

Thanks,

Bart.
Coly Li March 17, 2018, 10:05 a.m. UTC | #3
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
diff mbox

Patch

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);