Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <d49e2bb9-c7ae-8192-dbca-5c0a0e18359c@deltatee.com>
Date:   Thu, 21 Apr 2022 09:54:23 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Content-Language: en-CA
To:     Christoph Hellwig <hch@infradead.org>
Cc:     linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org,
        Song Liu <song@kernel.org>,
        Guoqing Jiang <guoqing.jiang@linux.dev>,
        Stephen Bates <sbates@raithlin.com>,
        Martin Oliveira <Martin.Oliveira@eideticom.com>,
        David Sloan <David.Sloan@eideticom.com>
References: <20220420195425.34911-1-logang@deltatee.com>
 <20220420195425.34911-13-logang@deltatee.com>
 <YmD9KBJtOjV+J5We@infradead.org>
From:   Logan Gunthorpe <logang@deltatee.com>
In-Reply-To: <YmD9KBJtOjV+J5We@infradead.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH v2 12/12] md/raid5: Pivot raid5_make_request()
Precedence: bulk


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