lkml.org 
[lkml]   [2022]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 12/12] md/raid5: Pivot raid5_make_request()
On Wed, Apr 20, 2022 at 01:54:25PM -0600, Logan Gunthorpe wrote:
> struct stripe_request_ctx {
> bool do_flush;
> struct stripe_head *batch_last;
> + sector_t disk_sector_done;
> + sector_t start_disk_sector;

Very nitpicky, but why use two different naming styles for the sectors
here?

> + bool first_wrap;
> + sector_t last_sector;

And especially with the last_sector here a few comments explaining
what each of the sector values mean might be useful.

I'd also keep the two bool variables together for a better structure
layout.

> + * if new_sector is less than the starting sector. Clear the
> + * boolean once the start sector is hit for the second time.
> + * When first_wrap is set, ignore the disk_sector_done.
> + */
> + if (ctx->start_disk_sector == MaxSector) {
> + ctx->start_disk_sector = new_sector;
> + } else if (new_sector < ctx->start_disk_sector) {
> + ctx->first_wrap = true;
> + } else if (new_sector == ctx->start_disk_sector) {
> + ctx->first_wrap = false;
> + ctx->start_disk_sector = 0;
> + return STRIPE_SUCCESS;
> + } else if (!ctx->first_wrap && new_sector <= ctx->disk_sector_done) {
> + return STRIPE_SUCCESS;
> + }
> +

I find this a bit confusing to read. While trying to mentally untangle
it I came up with this version instead, but it could really use some
good comments explaining each of the checks as I found your big comment
to not quite match the logic easily.

if (ctx->start_disk_sector == MaxSector) {
/*
* First loop iteration, start our state machine.
*
ctx->start_disk_sector = new_sector;
} else {
/*
* We are done if we wrapped around to the same sector.
* (???)
*/
if (new_sector == ctx->start_disk_sector) {
ctx->first_wrap = false;
ctx->start_disk_sector = 0;
return STRIPE_SUCCESS;
}

/*
* Sector before the start sector? Keep going and wrap
* around.
*/
if (new_sector < ctx->start_disk_sector) {
ctx->first_wrap = true;
} else {
// ???
if (new_sector <= ctx->disk_sector_done &&
!ctx->first_wrap)
return STRIPE_SUCCESS;
}
}

\
 
 \ /
  Last update: 2022-04-21 08:47    [W:0.481 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site