md/raid5: Convert stripe_head's "dev" to flexible array member
PATCH 1/1 md/raid5: Convert stripe_head's "dev" to flexible array member
Replace old-style 1-element array of "dev" in struct stripe_head with modern C99 flexible array. In the future, we can additionally annotate it with the run-time size, found in the "disks" member.
---
It looks like this memory calculation:
memory = conf->min_nr_stripes * (sizeof(struct stripe_head) +
max_disks * ((sizeof(struct bio) + PAGE_SIZE))) / 1024;... was already buggy (i.e. it included the single "dev" bytes in the result). However, I'm not entirely sure if that is the right analysis, since "dev" is not related to struct bio nor PAGE_SIZE?
--- drivers/md/raid5.c | 4 ++-- drivers/md/raid5.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/md/raid5.c +2 -2
@@ -2433,7 +2433,7 @@ static int grow_stripes(struct r5conf *conf, int num) conf->active_name =0 ; sc =kmem_cache_create (conf->cache_name[conf->active_name], -sizeof (struct stripe_head )+(devs-1 )*sizeof (struct r5dev ), +struct_size ((struct stripe_head *)0 , dev, devs),0 ,0 ,NULL );if (!sc)return 1 ;
@@ -2559,7 +2559,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)/* Step 1 */ sc =kmem_cache_create (conf->cache_name[1 -conf->active_name], -sizeof (struct stripe_head )+(newsize-1 )*sizeof (struct r5dev ), +struct_size ((struct stripe_head *)0 , dev, newsize),0 ,0 ,NULL );if (!sc)return -ENOMEM ;
drivers/md/raid5.h +1 -1
@@ -268,7 +268,7 @@ struct stripe_head {unsigned long flags;u32 log_checksum;unsigned short write_hint; - } dev[1 ];/* allocated with extra space depending of RAID geometry */ + } dev[]; /* allocated with extra space depending of RAID geometry */ };/* stripe_head_state - collects and tracks the dynamic state of a stripe_head
Signature · 1 line
2.34.1
drivers/md/raid5.c static int resize_stripes(struct r5conf *conf, int newsize) · line 2562
Quoted 12 lines · Kees Cook
drivers/md/raid5.c +2
-2sc =kmem_cache_create (conf->cache_name[conf->active_name], -sizeof (struct stripe_head )+(devs-1 )*sizeof (struct r5dev ), +struct_size ((struct stripe_head *)0 , dev, devs),0 ,0 ,NULL );if (!sc)return 1 ;@@ -2559,7 +2559,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)/* Step 1 */ sc = kmem_cache_create(conf->cache_name[1-conf->active_name], - sizeof(struct stripe_head)+(newsize-1)*sizeof(struct r5dev), + struct_size((struct stripe_head *)0, dev, newsize),
The constant you're casting here should be NULL, not 0. Also given that this expression is duplicated, I'd suggest a little helper for it…
drivers/md/raid5.h struct stripe_head { · line 271
Quoted 2 lines
drivers/md/raid5.h +1
-1- } dev[1 ];/* allocated with extra space depending of RAID geometry */ + } dev[]; /* allocated with extra space depending of RAID geometry */
And this isn't extra space over the single entry anymore, so I'd change this to:
/* allocated depending of RAID geometry */
Hi, Christoph
在 2023/05/18 12:27, Christoph Hellwig 写道:
It's not related to this patch, just I think I found a problem while reviewing raid5 code, commit e82ed3a4fbb5 ("md/raid6: refactor raid5_read_one_chunk") changes the caculation of 'end_sector', 'end_sector' is compared to 'rdev->recovery_offset', so it should be offset to rdev, but this commit change it to offset to the array.
Perhaps following change will make sense:
drivers/md/raid5.c +1 -1
@@ -5516,7 +5516,7 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio) sector =raid5_compute_sector (conf, raid_bio->bi_iter.bi_sector,0 , &dd_idx,NULL ); - end_sector =bio_end_sector (raid_bio); + end_sector = sector +bio_sectors (raid_bio);rcu_read_lock ();if (r5c_big_stripe_cached (conf, sector))
Thanks,
Kuai
Quoted 27 lines · Kees Cook
Review static int resize_stripes(struct r5conf *conf, int newsize) · line 2562
Quoted 12 lines
sc = kmem_cache_create(conf->cache_name[conf->active_name],
(quoted diff) +2
-2- sizeof(struct stripe_head)+(devs-1)*sizeof(struct r5dev), + struct_size((struct stripe_head *)0, dev, devs), 0, 0, NULL); if (!sc) return 1;@@ -2559,7 +2559,7 @@ static int resize_stripes(struct r5conf *conf, int newsize) /* Step 1 */ sc = kmem_cache_create(conf->cache_name[1-conf->active_name], - sizeof(struct stripe_head)+(newsize-1)*sizeof(struct r5dev), + struct_size((struct stripe_head *)0, dev, newsize),The constant you're casting here should be NULL, not 0. Also given that this expression is duplicated, I'd suggest a little helper for it…
Review static int resize_stripes(struct r5conf *conf, int newsize) · line 2563
Quoted 2 lines
(quoted diff) +1
-1- } dev[1]; /* allocated with extra space depending of RAID geometry */ + } dev[]; /* allocated with extra space depending of RAID geometry */And this isn't extra space over the single entry anymore, so I'd change this to:
/* allocated depending of RAID geometry */.
Quoted 11 lines · Yu Kuai
Hi, Christoph
在 2023/05/18 12:27, Christoph Hellwig 写道:
It's not related to this patch, just I think I found a problem while reviewing raid5 code, commit
e82ed3a4fbb5("md/raid6: refactorraid5_read_one_chunk") changes the caculation of 'end_sector', 'end_sector' is compared to 'rdev->recovery_offset', so it should be offset to rdev, but this commit change it to offset to the array.Perhaps following change will make sense:
Yes, I think it does. Can you send a patch?
Hi,
Quoted 11 lines · Christoph Hellwig
Quoted 10 lines · Yu Kuai
Hi, Christoph
在 2023/05/18 12:27, Christoph Hellwig 写道:
It's not related to this patch, just I think I found a problem while reviewing raid5 code, commit
e82ed3a4fbb5("md/raid6: refactorraid5_read_one_chunk") changes the caculation of 'end_sector', 'end_sector' is compared to 'rdev->recovery_offset', so it should be offset to rdev, but this commit change it to offset to the array.
Quoted 5 lines
Quoted 1 line
Perhaps following change will make sense:
Yes, I think it does. Can you send a patch?
.
Of course, thanks for your response.
Kuai