[PATCH] md: Fix bitmap offset type in sb writer

[PATCH] md: Fix bitmap offset type in sb writer

[Jonathan Derrick]

Bitmap offset is allowed to be negative, indicating that bitmap precedes
metadata. Change the type back from sector_t to loff_t to satisfy
conditionals and calculations.

Signed-off-by: Jonathan Derrick <jonathan.derrick@linux.dev>
---
 drivers/md/md-bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 920bb68156d2..29ae7f7015e4 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -237,8 +237,8 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap,
 	struct block_device *bdev;
 	struct mddev *mddev = bitmap->mddev;
 	struct bitmap_storage *store = &bitmap->storage;
-	sector_t offset = mddev->bitmap_info.offset;
-	sector_t ps, sboff, doff;
+	loff_t sboff, offset = mddev->bitmap_info.offset;
+	sector_t ps, doff;
 	unsigned int size = PAGE_SIZE;
 	unsigned int opt_size = PAGE_SIZE;
 
-- 
2.40.0

Re: [PATCH] md: Fix bitmap offset type in sb writer

[Song Liu]

On Mon, Apr 24, 2023 at 6:16 PM Jonathan Derrick
<jonathan.derrick@linux.dev> wrote:
>
> Bitmap offset is allowed to be negative, indicating that bitmap precedes
> metadata. Change the type back from sector_t to loff_t to satisfy
> conditionals and calculations.
>
> Signed-off-by: Jonathan Derrick <jonathan.derrick@linux.dev>

I added the following to the patch and applied it to md-next.

Thanks,
Song

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 10172f200b67 ("md: Fix types in sb writer")

> ---
>  drivers/md/md-bitmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 920bb68156d2..29ae7f7015e4 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -237,8 +237,8 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap,
>         struct block_device *bdev;
>         struct mddev *mddev = bitmap->mddev;
>         struct bitmap_storage *store = &bitmap->storage;
> -       sector_t offset = mddev->bitmap_info.offset;
> -       sector_t ps, sboff, doff;
> +       loff_t sboff, offset = mddev->bitmap_info.offset;
> +       sector_t ps, doff;
>         unsigned int size = PAGE_SIZE;
>         unsigned int opt_size = PAGE_SIZE;
>
> --
> 2.40.0
>

Re: [PATCH] md: Fix bitmap offset type in sb writer

[Song Liu]

Hi Jonathan,

On Tue, Apr 25, 2023 at 8:44 PM Song Liu <song@kernel.org> wrote:
>
> On Mon, Apr 24, 2023 at 6:16 PM Jonathan Derrick
> <jonathan.derrick@linux.dev> wrote:
> >
> > Bitmap offset is allowed to be negative, indicating that bitmap precedes
> > metadata. Change the type back from sector_t to loff_t to satisfy
> > conditionals and calculations.

This actually breaks the following tests from mdadm:

05r1-add-internalbitmap-v1a
05r1-internalbitmap-v1a
05r1-remove-internalbitmap-v1a

Please look into these.

Thanks,
Song

> >
> > Signed-off-by: Jonathan Derrick <jonathan.derrick@linux.dev>
>
> I added the following to the patch and applied it to md-next.
>
> Thanks,
> Song
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 10172f200b67 ("md: Fix types in sb writer")
>
> > ---
> >  drivers/md/md-bitmap.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> > index 920bb68156d2..29ae7f7015e4 100644
> > --- a/drivers/md/md-bitmap.c
> > +++ b/drivers/md/md-bitmap.c
> > @@ -237,8 +237,8 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap,
> >         struct block_device *bdev;
> >         struct mddev *mddev = bitmap->mddev;
> >         struct bitmap_storage *store = &bitmap->storage;
> > -       sector_t offset = mddev->bitmap_info.offset;
> > -       sector_t ps, sboff, doff;
> > +       loff_t sboff, offset = mddev->bitmap_info.offset;
> > +       sector_t ps, doff;
> >         unsigned int size = PAGE_SIZE;
> >         unsigned int opt_size = PAGE_SIZE;
> >
> > --
> > 2.40.0
> >

Re: [PATCH] md: Fix bitmap offset type in sb writer

[Jonathan Derrick]

Hi Song,

On 4/26/2023 11:58 AM, Song Liu wrote:
> Hi Jonathan,
> 
> On Tue, Apr 25, 2023 at 8:44 PM Song Liu <song@kernel.org> wrote:
>>
>> On Mon, Apr 24, 2023 at 6:16 PM Jonathan Derrick
>> <jonathan.derrick@linux.dev> wrote:
>>>
>>> Bitmap offset is allowed to be negative, indicating that bitmap precedes
>>> metadata. Change the type back from sector_t to loff_t to satisfy
>>> conditionals and calculations.
> 
> This actually breaks the following tests from mdadm:
> 
> 05r1-add-internalbitmap-v1a
> 05r1-internalbitmap-v1a
> 05r1-remove-internalbitmap-v1a
> 
> Please look into these.
> 
I no longer work for Solidigm and don't have access to equipment that 
can test this. I'm CCing Sushma and Michael who should be able to test 
and report back.

Thanks,
Jon

> Thanks,
> Song
> 
>>>
>>> Signed-off-by: Jonathan Derrick <jonathan.derrick@linux.dev>
>>
>> I added the following to the patch and applied it to md-next.
>>
>> Thanks,
>> Song
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Fixes: 10172f200b67 ("md: Fix types in sb writer")
>>
>>> ---
>>>   drivers/md/md-bitmap.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>>> index 920bb68156d2..29ae7f7015e4 100644
>>> --- a/drivers/md/md-bitmap.c
>>> +++ b/drivers/md/md-bitmap.c
>>> @@ -237,8 +237,8 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap,
>>>          struct block_device *bdev;
>>>          struct mddev *mddev = bitmap->mddev;
>>>          struct bitmap_storage *store = &bitmap->storage;
>>> -       sector_t offset = mddev->bitmap_info.offset;
>>> -       sector_t ps, sboff, doff;
>>> +       loff_t sboff, offset = mddev->bitmap_info.offset;
>>> +       sector_t ps, doff;
>>>          unsigned int size = PAGE_SIZE;
>>>          unsigned int opt_size = PAGE_SIZE;
>>>
>>> --
>>> 2.40.0
>>>

Re: [PATCH] md: Fix bitmap offset type in sb writer

[Yu Kuai]

Hi,

在 2023/04/27 1:58, Song Liu 写道:
> Hi Jonathan,
> 
> On Tue, Apr 25, 2023 at 8:44 PM Song Liu <song@kernel.org> wrote:
>>
>> On Mon, Apr 24, 2023 at 6:16 PM Jonathan Derrick
>> <jonathan.derrick@linux.dev> wrote:
>>>
>>> Bitmap offset is allowed to be negative, indicating that bitmap precedes
>>> metadata. Change the type back from sector_t to loff_t to satisfy
>>> conditionals and calculations.
> 
> This actually breaks the following tests from mdadm:
> 
> 05r1-add-internalbitmap-v1a

After a quick look of this test, I think the root cause is another
patch:

commit 8745faa95611 ("md: Use optimal I/O size for last bitmap page")

This patch add a new helper bitmap_io_size(), which breaks the condition
that 'negative value < 0'.

And following patch should fix this problem:

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index adbe95e03852..b1b521837156 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -219,8 +219,9 @@ static unsigned int optimal_io_size(struct 
block_device *bdev,
  }

  static unsigned int bitmap_io_size(unsigned int io_size, unsigned int 
opt_size,
-                                  sector_t start, sector_t boundary)
+                                  loff_t start, loff_t boundary)
  {

> 05r1-internalbitmap-v1a
> 05r1-remove-internalbitmap-v1a
> 

The patch is not tested yet, and I don't have time to look other tests
yet...

Thanks,
Kuai
> Please look into these.
> 
> Thanks,
> Song

Re: [PATCH] md: Fix bitmap offset type in sb writer

[Song Liu]

On Thu, Apr 27, 2023 at 2:35 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2023/04/27 1:58, Song Liu 写道:
> > Hi Jonathan,
> >
> > On Tue, Apr 25, 2023 at 8:44 PM Song Liu <song@kernel.org> wrote:
> >>
> >> On Mon, Apr 24, 2023 at 6:16 PM Jonathan Derrick
> >> <jonathan.derrick@linux.dev> wrote:
> >>>
> >>> Bitmap offset is allowed to be negative, indicating that bitmap precedes
> >>> metadata. Change the type back from sector_t to loff_t to satisfy
> >>> conditionals and calculations.
> >
> > This actually breaks the following tests from mdadm:
> >
> > 05r1-add-internalbitmap-v1a
>
> After a quick look of this test, I think the root cause is another
> patch:
>
> commit 8745faa95611 ("md: Use optimal I/O size for last bitmap page")
>
> This patch add a new helper bitmap_io_size(), which breaks the condition
> that 'negative value < 0'.
>
> And following patch should fix this problem:
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index adbe95e03852..b1b521837156 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -219,8 +219,9 @@ static unsigned int optimal_io_size(struct
> block_device *bdev,
>   }
>
>   static unsigned int bitmap_io_size(unsigned int io_size, unsigned int
> opt_size,
> -                                  sector_t start, sector_t boundary)
> +                                  loff_t start, loff_t boundary)
>   {
>
> > 05r1-internalbitmap-v1a
> > 05r1-remove-internalbitmap-v1a
> >
>
> The patch is not tested yet, and I don't have time to look other tests
> yet...

Thanks Kuai! This fixed the test.

I will add your Signed-off-by to the patch. Please let me know if you
prefer not to have it.

Song

Re: [PATCH] md: Fix bitmap offset type in sb writer

[Yu Kuai]

Hi,

在 2023/04/28 1:45, Song Liu 写道:
> On Thu, Apr 27, 2023 at 2:35 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> Hi,
>>
>> 在 2023/04/27 1:58, Song Liu 写道:
>>> Hi Jonathan,
>>>
>>> On Tue, Apr 25, 2023 at 8:44 PM Song Liu <song@kernel.org> wrote:
>>>>
>>>> On Mon, Apr 24, 2023 at 6:16 PM Jonathan Derrick
>>>> <jonathan.derrick@linux.dev> wrote:
>>>>>
>>>>> Bitmap offset is allowed to be negative, indicating that bitmap precedes
>>>>> metadata. Change the type back from sector_t to loff_t to satisfy
>>>>> conditionals and calculations.
>>>
>>> This actually breaks the following tests from mdadm:
>>>
>>> 05r1-add-internalbitmap-v1a
>>
>> After a quick look of this test, I think the root cause is another
>> patch:
>>
>> commit 8745faa95611 ("md: Use optimal I/O size for last bitmap page")
>>
>> This patch add a new helper bitmap_io_size(), which breaks the condition
>> that 'negative value < 0'.
>>
>> And following patch should fix this problem:
>>
>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> index adbe95e03852..b1b521837156 100644
>> --- a/drivers/md/md-bitmap.c
>> +++ b/drivers/md/md-bitmap.c
>> @@ -219,8 +219,9 @@ static unsigned int optimal_io_size(struct
>> block_device *bdev,
>>    }
>>
>>    static unsigned int bitmap_io_size(unsigned int io_size, unsigned int
>> opt_size,
>> -                                  sector_t start, sector_t boundary)
>> +                                  loff_t start, loff_t boundary)
>>    {
>>
>>> 05r1-internalbitmap-v1a
>>> 05r1-remove-internalbitmap-v1a
>>>
>>
>> The patch is not tested yet, and I don't have time to look other tests
>> yet...
> 
> Thanks Kuai! This fixed the test.

Thanks for the test, I'll check more details and try tests myself later.

Kuai
> 
> I will add your Signed-off-by to the patch. Please let me know if you
> prefer not to have it.
> 
> Song
> .
> 

Re: [PATCH] md: Fix bitmap offset type in sb writer

[Yu Kuai]

Hi,

在 2023/04/28 10:04, Yu Kuai 写道:
> Hi,
> 
> 在 2023/04/28 1:45, Song Liu 写道:
>> On Thu, Apr 27, 2023 at 2:35 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>
>>> Hi,
>>>
>>> 在 2023/04/27 1:58, Song Liu 写道:
>>>> Hi Jonathan,
>>>>
>>>> On Tue, Apr 25, 2023 at 8:44 PM Song Liu <song@kernel.org> wrote:
>>>>>
>>>>> On Mon, Apr 24, 2023 at 6:16 PM Jonathan Derrick
>>>>> <jonathan.derrick@linux.dev> wrote:
>>>>>>
>>>>>> Bitmap offset is allowed to be negative, indicating that bitmap 
>>>>>> precedes
>>>>>> metadata. Change the type back from sector_t to loff_t to satisfy
>>>>>> conditionals and calculations.
>>>>
>>>> This actually breaks the following tests from mdadm:
>>>>
>>>> 05r1-add-internalbitmap-v1a
>>>
>>> After a quick look of this test, I think the root cause is another
>>> patch:
>>>
>>> commit 8745faa95611 ("md: Use optimal I/O size for last bitmap page")
>>>
>>> This patch add a new helper bitmap_io_size(), which breaks the condition
>>> that 'negative value < 0'.
>>>
>>> And following patch should fix this problem:
>>>
>>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>>> index adbe95e03852..b1b521837156 100644
>>> --- a/drivers/md/md-bitmap.c
>>> +++ b/drivers/md/md-bitmap.c
>>> @@ -219,8 +219,9 @@ static unsigned int optimal_io_size(struct
>>> block_device *bdev,
>>>    }
>>>
>>>    static unsigned int bitmap_io_size(unsigned int io_size, unsigned int
>>> opt_size,
>>> -                                  sector_t start, sector_t boundary)
>>> +                                  loff_t start, loff_t boundary)
>>>    {
>>>
>>>> 05r1-internalbitmap-v1a
>>>> 05r1-remove-internalbitmap-v1a
>>>>
>>>
>>> The patch is not tested yet, and I don't have time to look other tests
>>> yet...
>>
>> Thanks Kuai! This fixed the test.
> 
> Thanks for the test, I'll check more details and try tests myself later.
> 
> Kuai
>>
>> I will add your Signed-off-by to the patch. Please let me know if you
>> prefer not to have it.

I just reviewed realted patches and tested myself, perhaps it'll be
better to use Suggested-and-reviewed-by, anyway, I'm good with either
way.

Thanks,
Kuai
>>
>> Song
>> .
>>
> 
> .
>