| Message ID | 20230222215828.225-3-jonathan.derrick@linux.dev (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | md/bitmap: Optimal last page size | expand |
On Wed, Feb 22, 2023 at 02:58:27PM -0700, Jonathan Derrick wrote: > From: Jon Derrick <jonathan.derrick@linux.dev> > > Page->index is a pgoff_t and multiplying could cause overflows on a > 32-bit architecture. In the sb writer, this is used to calculate and > verify the sector being used, and is multiplied by a sector value. Using > sector_t will cast it to a u64 type and is the more appropriate type for > the unit. Additionally, the integer size unit is converted to a sector > unit in later calculations, and is now corrected to be an unsigned type. > > Finally, clean up the calculations using variable aliases to improve > readabiliy. > > Signed-off-by: Jon Derrick <jonathan.derrick@linux.dev> > --- > drivers/md/md-bitmap.c | 36 +++++++++++++++--------------------- > 1 file changed, 15 insertions(+), 21 deletions(-) > > diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c > index 5c65268a2d97..11f4453775ee 100644 > --- a/drivers/md/md-bitmap.c > +++ b/drivers/md/md-bitmap.c > @@ -215,55 +215,49 @@ 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; > - loff_t offset = mddev->bitmap_info.offset; > - int size = PAGE_SIZE; > + sector_t offset = mddev->bitmap_info.offset; > + sector_t ps, sboff, doff; > + unsigned int size = PAGE_SIZE; > > bdev = (rdev->meta_bdev) ? rdev->meta_bdev : rdev->bdev; > if (page->index == store->file_pages - 1) { > - int last_page_size = store->bytes & (PAGE_SIZE - 1); > + unsigned int last_page_size = store->bytes & (PAGE_SIZE - 1); > if (last_page_size == 0) This should probably grow an empty line after the variable declaration (here or in the previous patch). > + sboff < (doff + mddev->dev_sectors + (PAGE_SIZE / SECTOR_SIZE))) No need for either pair of braces here. > - rdev->sb_start + offset > - + page->index * (PAGE_SIZE / SECTOR_SIZE), > - size, page); > + md_super_write(mddev, rdev, sboff + ps, > + (int) size, page); This easily fits into a single line now. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c index 5c65268a2d97..11f4453775ee 100644 --- a/drivers/md/md-bitmap.c +++ b/drivers/md/md-bitmap.c @@ -215,55 +215,49 @@ 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; - loff_t offset = mddev->bitmap_info.offset; - int size = PAGE_SIZE; + sector_t offset = mddev->bitmap_info.offset; + sector_t ps, sboff, doff; + unsigned int size = PAGE_SIZE; bdev = (rdev->meta_bdev) ? rdev->meta_bdev : rdev->bdev; if (page->index == store->file_pages - 1) { - int last_page_size = store->bytes & (PAGE_SIZE - 1); + unsigned int last_page_size = store->bytes & (PAGE_SIZE - 1); if (last_page_size == 0) last_page_size = PAGE_SIZE; size = roundup(last_page_size, bdev_logical_block_size(bdev)); } + ps = page->index * (PAGE_SIZE / SECTOR_SIZE); + sboff = rdev->sb_start + offset; + doff = rdev->data_offset; + /* Just make sure we aren't corrupting data or metadata */ if (mddev->external) { /* Bitmap could be anywhere. */ - if (rdev->sb_start + offset - + (page->index * (PAGE_SIZE / SECTOR_SIZE)) - > rdev->data_offset && - rdev->sb_start + offset - < (rdev->data_offset + mddev->dev_sectors - + (PAGE_SIZE / SECTOR_SIZE))) + if (sboff + ps > doff && + sboff < (doff + mddev->dev_sectors + (PAGE_SIZE / SECTOR_SIZE))) return -EINVAL; } else if (offset < 0) { /* DATA BITMAP METADATA */ - if (offset - + (long)(page->index * (PAGE_SIZE / SECTOR_SIZE)) - + size / SECTOR_SIZE > 0) + if (offset + ps + size / SECTOR_SIZE > 0) /* bitmap runs in to metadata */ return -EINVAL; - if (rdev->data_offset + mddev->dev_sectors - > rdev->sb_start + offset) + if (doff + mddev->dev_sectors > sboff) /* data runs in to bitmap */ return -EINVAL; } else if (rdev->sb_start < rdev->data_offset) { /* METADATA BITMAP DATA */ - if (rdev->sb_start + offset - + page->index * (PAGE_SIZE / SECTOR_SIZE) - + size / SECTOR_SIZE > rdev->data_offset) + if (sboff + ps + size / SECTOR_SIZE > doff) /* bitmap runs in to data */ return -EINVAL; } else { /* DATA METADATA BITMAP - no problems */ } - md_super_write(mddev, rdev, - rdev->sb_start + offset - + page->index * (PAGE_SIZE / SECTOR_SIZE), - size, page); + md_super_write(mddev, rdev, sboff + ps, + (int) size, page); return 0; }