[PATCH 0/2] Solve problem adding internal bitmaps to 1.0 arrays

[PATCH 0/2] Solve problem adding internal bitmaps to 1.0 arrays

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

This set solves the problem with adding internal bitmaps to 1.0
arrays. Basically there were two problems, one the sign extension from
u32 to long was done incorrectly, and second we also tried to convert
it to an 'unsigned long long' when passing it to the kernel.

With these two applied I can succesfully add bitmaps to 1.0 arrays.

Please hollor if you see any problems with this patchset.

Cheers,
Jes

Jes Sorensen (2):
  Fix sign extension of bitmap_offset in super1.c
  Introduce sysfs_set_num_signed() and use it to set bitmap/offset

 Grow.c   |    4 ++--
 mdadm.h  |    2 ++
 super1.c |    4 ++--
 sysfs.c  |    8 ++++++++
 4 files changed, 14 insertions(+), 4 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

fbdef49811c9e2b54e2064d9af68cfffa77c6e77 incorrectly tried to fix sign
extension of the bitmap offset. However mdinfo->bitmap_offset is a u32
and needs to be converted to a 32 bit signed integer before the sign
extension.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 super1.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/super1.c b/super1.c
index 36369d8..be77c33 100644
--- a/super1.c
+++ b/super1.c
@@ -620,7 +620,7 @@ static void getinfo_super1(struct supertype *st, struct mdinfo *info, char *map)
 	info->data_offset = __le64_to_cpu(sb->data_offset);
 	info->component_size = __le64_to_cpu(sb->size);
 	if (sb->feature_map & __le32_to_cpu(MD_FEATURE_BITMAP_OFFSET))
-		info->bitmap_offset = (long)__le32_to_cpu(sb->bitmap_offset);
+		info->bitmap_offset = (int32_t)__le32_to_cpu(sb->bitmap_offset);
 
 	info->disk.major = 0;
 	info->disk.minor = 0;
@@ -1651,7 +1651,7 @@ add_internal_bitmap1(struct supertype *st,
 		offset = -room;
 	}
 
-	sb->bitmap_offset = (long)__cpu_to_le32(offset);
+	sb->bitmap_offset = (int32_t)__cpu_to_le32(offset);
 
 	sb->feature_map = __cpu_to_le32(__le32_to_cpu(sb->feature_map)
 					| MD_FEATURE_BITMAP_OFFSET);
-- 
1.7.7.6

Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[Doug Ledford]

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

On 04/26/2012 11:12 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> fbdef49811c9e2b54e2064d9af68cfffa77c6e77 incorrectly tried to fix sign
> extension of the bitmap offset. However mdinfo->bitmap_offset is a u32
> and needs to be converted to a 32 bit signed integer before the sign
> extension.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

I was scratching my head over this patch, saying to myself "But won't
that cause us to truncate large values of bitmap_offset?"  And it will,
but I see your point now, that's *exactly* the problem if we don't do
the sign conversion before the extension, the actual bitmap_offset
should really be signed in order to support negative offsets, but since
it isn't, when we save a negative offset into bitmap_offset it appears
as a really large positive offset, and then when we sign extend to long,
it keeps the large size positive offset instead of picking up the
negative offset.  Gotcha.  So, I see why this works, but do you think it
should be fixed this way, or by converting bitmap_offset to type int32
instead of uint32?

> ---
>  super1.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/super1.c b/super1.c
> index 36369d8..be77c33 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -620,7 +620,7 @@ static void getinfo_super1(struct supertype *st, struct mdinfo *info, char *map)
>  	info->data_offset = __le64_to_cpu(sb->data_offset);
>  	info->component_size = __le64_to_cpu(sb->size);
>  	if (sb->feature_map & __le32_to_cpu(MD_FEATURE_BITMAP_OFFSET))
> -		info->bitmap_offset = (long)__le32_to_cpu(sb->bitmap_offset);
> +		info->bitmap_offset = (int32_t)__le32_to_cpu(sb->bitmap_offset);
>  
>  	info->disk.major = 0;
>  	info->disk.minor = 0;
> @@ -1651,7 +1651,7 @@ add_internal_bitmap1(struct supertype *st,
>  		offset = -room;
>  	}
>  
> -	sb->bitmap_offset = (long)__cpu_to_le32(offset);
> +	sb->bitmap_offset = (int32_t)__cpu_to_le32(offset);
>  
>  	sb->feature_map = __cpu_to_le32(__le32_to_cpu(sb->feature_map)
>  					| MD_FEATURE_BITMAP_OFFSET);


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[Jes Sorensen]

On 04/26/12 17:18, Doug Ledford wrote:
> On 04/26/2012 11:12 AM, Jes.Sorensen@redhat.com wrote:
>> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> > 
>> > fbdef49811c9e2b54e2064d9af68cfffa77c6e77 incorrectly tried to fix sign
>> > extension of the bitmap offset. However mdinfo->bitmap_offset is a u32
>> > and needs to be converted to a 32 bit signed integer before the sign
>> > extension.
>> > 
>> > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> I was scratching my head over this patch, saying to myself "But won't
> that cause us to truncate large values of bitmap_offset?"  And it will,
> but I see your point now, that's *exactly* the problem if we don't do
> the sign conversion before the extension, the actual bitmap_offset
> should really be signed in order to support negative offsets, but since
> it isn't, when we save a negative offset into bitmap_offset it appears
> as a really large positive offset, and then when we sign extend to long,
> it keeps the large size positive offset instead of picking up the
> negative offset.  Gotcha.  So, I see why this works, but do you think it
> should be fixed this way, or by converting bitmap_offset to type int32
> instead of uint32?

Heh, I have to admit I cheated too and asked Richard Henderson for help
as I couldn't figure out why the sign conversion failed. Otherwise I
would probably still have been scratching my head over it :)

I noticed other parts of the code already handled it this way, so my fix
is consistent with that, but we could do both. Neil?

Cheers,
Jes

Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[Jes Sorensen]

On 04/26/12 17:21, Jes Sorensen wrote:
> On 04/26/12 17:18, Doug Ledford wrote:
>> I was scratching my head over this patch, saying to myself "But won't
>> that cause us to truncate large values of bitmap_offset?"  And it will,
>> but I see your point now, that's *exactly* the problem if we don't do
>> the sign conversion before the extension, the actual bitmap_offset
>> should really be signed in order to support negative offsets, but since
>> it isn't, when we save a negative offset into bitmap_offset it appears
>> as a really large positive offset, and then when we sign extend to long,
>> it keeps the large size positive offset instead of picking up the
>> negative offset.  Gotcha.  So, I see why this works, but do you think it
>> should be fixed this way, or by converting bitmap_offset to type int32
>> instead of uint32?
> 
> Heh, I have to admit I cheated too and asked Richard Henderson for help
> as I couldn't figure out why the sign conversion failed. Otherwise I
> would probably still have been scratching my head over it :)
> 
> I noticed other parts of the code already handled it this way, so my fix
> is consistent with that, but we could do both. Neil?

Just checking mdadm.h and bswap32() is defined like this:

#define bswap_32(x) (((x) & 0x000000ffU) << 24 | \
                     ((x) & 0xff000000U) >> 24 | \
                     ((x) & 0x0000ff00U) << 8  | \
                     ((x) & 0x00ff0000U) >> 8)

so I am not 100% sure just swapping to an s32 in the struct will work on
big endian systems? Will the 0x000000ffU not force the conversion back
to unsigned or what happens in this case?

Cheers,
Jes

Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[Richard Henderson]

On 04/26/12 08:25, Jes Sorensen wrote:
> Just checking mdadm.h and bswap32() is defined like this:
> 
> #define bswap_32(x) (((x) & 0x000000ffU) << 24 | \
>                      ((x) & 0xff000000U) >> 24 | \
>                      ((x) & 0x0000ff00U) << 8  | \
>                      ((x) & 0x00ff0000U) >> 8)
> 
> so I am not 100% sure just swapping to an s32 in the struct will work on
> big endian systems? Will the 0x000000ffU not force the conversion back
> to unsigned or what happens in this case?

This is actually semi-complicated.  c89 or c99 rules?  X already of a
type larger than unsigned int?

But if X is signed int, this entire expression will always be unsigned.

You're certainly better off with a cast as we discussed on irc.


r~

Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[Doug Ledford]

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

On 04/26/2012 11:32 AM, Richard Henderson wrote:
> On 04/26/12 08:25, Jes Sorensen wrote:
>> Just checking mdadm.h and bswap32() is defined like this:
>>
>> #define bswap_32(x) (((x) & 0x000000ffU) << 24 | \
>>                      ((x) & 0xff000000U) >> 24 | \
>>                      ((x) & 0x0000ff00U) << 8  | \
>>                      ((x) & 0x00ff0000U) >> 8)
>>
>> so I am not 100% sure just swapping to an s32 in the struct will work on
>> big endian systems? Will the 0x000000ffU not force the conversion back
>> to unsigned or what happens in this case?
> 
> This is actually semi-complicated.  c89 or c99 rules?  X already of a
> type larger than unsigned int?
> 
> But if X is signed int, this entire expression will always be unsigned.
> 
> You're certainly better off with a cast as we discussed on irc.

Answering this question is beyond my bitfield/bigendian/littleendian
foo.  So nice to have Richard around ;-)


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[Jes Sorensen]

On 04/26/12 17:32, Richard Henderson wrote:
> On 04/26/12 08:25, Jes Sorensen wrote:
>> Just checking mdadm.h and bswap32() is defined like this:
>>
>> #define bswap_32(x) (((x) & 0x000000ffU) << 24 | \
>>                      ((x) & 0xff000000U) >> 24 | \
>>                      ((x) & 0x0000ff00U) << 8  | \
>>                      ((x) & 0x00ff0000U) >> 8)
>>
>> so I am not 100% sure just swapping to an s32 in the struct will work on
>> big endian systems? Will the 0x000000ffU not force the conversion back
>> to unsigned or what happens in this case?
> 
> This is actually semi-complicated.  c89 or c99 rules?  X already of a
> type larger than unsigned int?

This is what I was afraid of :)

> But if X is signed int, this entire expression will always be unsigned.
> 
> You're certainly better off with a cast as we discussed on irc.

In this particular case X would be a signed int, so we would end up with
unsigned and still need the cast.

I will opt for the cast in this case - thanks a lot for the clarification.

Cheers,
Jes

Re: [PATCH 1/2] Fix sign extension of bitmap_offset in super1.c

[NeilBrown]

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

On Thu, 26 Apr 2012 17:21:36 +0200 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:

> On 04/26/12 17:18, Doug Ledford wrote:
> > On 04/26/2012 11:12 AM, Jes.Sorensen@redhat.com wrote:
> >> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> > 
> >> > fbdef49811c9e2b54e2064d9af68cfffa77c6e77 incorrectly tried to fix sign
> >> > extension of the bitmap offset. However mdinfo->bitmap_offset is a u32
> >> > and needs to be converted to a 32 bit signed integer before the sign
> >> > extension.
> >> > 
> >> > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> > I was scratching my head over this patch, saying to myself "But won't
> > that cause us to truncate large values of bitmap_offset?"  And it will,
> > but I see your point now, that's *exactly* the problem if we don't do
> > the sign conversion before the extension, the actual bitmap_offset
> > should really be signed in order to support negative offsets, but since
> > it isn't, when we save a negative offset into bitmap_offset it appears
> > as a really large positive offset, and then when we sign extend to long,
> > it keeps the large size positive offset instead of picking up the
> > negative offset.  Gotcha.  So, I see why this works, but do you think it
> > should be fixed this way, or by converting bitmap_offset to type int32
> > instead of uint32?
> 
> Heh, I have to admit I cheated too and asked Richard Henderson for help
> as I couldn't figure out why the sign conversion failed. Otherwise I
> would probably still have been scratching my head over it :)
> 
> I noticed other parts of the code already handled it this way, so my fix
> is consistent with that, but we could do both. Neil?

The reason that "bitmap_offset" in 'mdp_superblock_1' is '__u32' is simply
that there is no '__s32'.
I wouldn't be against changing it, but I think you've concluded that it keeps
the byte swapping simpler if we don't.

But then I read recently that Rob Pike thinks byte swapping is always wrong

http://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

so maybe we shouldn't be todo that....

I'll just take your patches as they are I think - they look good.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 2/2] Introduce sysfs_set_num_signed() and use it to set bitmap/offset

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

mdinfo->bitmap_offset is a signed long and needs to be treated as
such when passed to the kernel.

This resolves the problem with adding internal bitmaps to a 1.0 array.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Grow.c  |    4 ++--
 mdadm.h |    2 ++
 sysfs.c |    8 ++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Grow.c b/Grow.c
index b4b9ff2..0b0d718 100644
--- a/Grow.c
+++ b/Grow.c
@@ -424,8 +424,8 @@ int Grow_addbitmap(char *devname, int fd, char *file, int chunk, int delay, int
 		if (offset_setable) {
 			st->ss->getinfo_super(st, mdi, NULL);
 			sysfs_init(mdi, fd, -1);
-			rv = sysfs_set_num(mdi, NULL, "bitmap/location",
-					   mdi->bitmap_offset);
+			rv = sysfs_set_num_signed(mdi, NULL, "bitmap/location",
+						  mdi->bitmap_offset);
 		} else {
 			array.state |= (1<<MD_SB_BITMAP_PRESENT);
 			rv = ioctl(fd, SET_ARRAY_INFO, &array);
diff --git a/mdadm.h b/mdadm.h
index e60a706..0c91a34 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -473,6 +473,8 @@ extern int sysfs_set_str(struct mdinfo *sra, struct mdinfo *dev,
 			 char *name, char *val);
 extern int sysfs_set_num(struct mdinfo *sra, struct mdinfo *dev,
 			 char *name, unsigned long long val);
+extern int sysfs_set_num_signed(struct mdinfo *sra, struct mdinfo *dev,
+				char *name, long long val);
 extern int sysfs_uevent(struct mdinfo *sra, char *event);
 extern int sysfs_get_fd(struct mdinfo *sra, struct mdinfo *dev,
 			char *name);
diff --git a/sysfs.c b/sysfs.c
index a1007cf..8e9d0c5 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -428,6 +428,14 @@ int sysfs_set_num(struct mdinfo *sra, struct mdinfo *dev,
 	return sysfs_set_str(sra, dev, name, valstr);
 }
 
+int sysfs_set_num_signed(struct mdinfo *sra, struct mdinfo *dev,
+			 char *name, long long val)
+{
+	char valstr[50];
+	sprintf(valstr, "%lli", val);
+	return sysfs_set_str(sra, dev, name, valstr);
+}
+
 int sysfs_uevent(struct mdinfo *sra, char *event)
 {
 	char fname[50];
-- 
1.7.7.6