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;
}
}
On 2022-04-21 00:43, Christoph Hellwig wrote:
> 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.
Yeah, this logic has been very tricky to figure out. Thus explaining it
was quite difficult. It was a consistent source of bugs for a me for a
while until I figured out this little state machine. I'll follow your
rough template and rework the comments and try to make a large example
comment or something to explain this, and maybe factor it into a helper
function.
>> + * 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 {
> // ???
This is actually the common most important branch that says if we've
already done this disk sector to stop and move on. So I'll probably
rework it some so that it is not so deeply nested.
> if (new_sector <= ctx->disk_sector_done &&
> !ctx->first_wrap)
> return STRIPE_SUCCESS;
> }
> }
No problem cleaning up your other nits. I'll send a v3 in due course.
Thanks,
Logan