diff mbox series

md/raid5: Convert stripe_head's "dev" to flexible array member

Message ID 20230517233313.never.130-kees@kernel.org
State New
Headers
Series md/raid5: Convert stripe_head's "dev" to flexible array member |

Commit Message

Kees Cook May 17, 2023, 11:33 p.m. UTC 
  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.

Cc: Song Liu <song@kernel.org>
Cc: linux-raid@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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(-)

Comments

Christoph Hellwig May 18, 2023, 4:27 a.m. UTC | #1 
On Wed, May 17, 2023 at 04:33:14PM -0700, Kees Cook wrote:
>  	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),

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…

> -	} 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 */
Yu Kuai May 22, 2023, 11:49 a.m. UTC | #2 
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:

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7e2bbcfef325..8686d629e3f2 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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
> On Wed, May 17, 2023 at 04:33:14PM -0700, Kees Cook wrote:
>>   	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),
> 
> 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…
> 
>> -	} 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 */
> .
>
Christoph Hellwig May 23, 2023, 5:54 a.m. UTC | #3 
On Mon, May 22, 2023 at 07:49:38PM +0800, Yu Kuai wrote:
> 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:

Yes, I think it does.  Can you send a patch?
Yu Kuai May 23, 2023, 1:46 p.m. UTC | #4 
Hi,

在 2023/05/23 13:54, Christoph Hellwig 写道:
> On Mon, May 22, 2023 at 07:49:38PM +0800, Yu Kuai wrote:
>> 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:
> 
> Yes, I think it does.  Can you send a patch?
> .
> 

Of course, thanks for your response.

Kuai
diff mbox series

Patch

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 4739ed891e75..95aa74aea076 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index e873938a6125..6cfc74162b41 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -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