| Message ID | 20230425011438.71046-1-jonathan.derrick@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Song Liu |
| Headers | show |
| Series | md: Fix bitmap offset type in sb writer | expand |
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 >
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 > >
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 >>>
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
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
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 > . >
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 >> . >> > > . >
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;
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(-)