linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Edward Hsieh <edwardh@synology.com>
To: NeilBrown <neilb@suse.de>, axboe@kernel.dk, neilb@suse.com
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	s3t@synology.com, bingjingc@synology.com, cccheng@synology.com
Subject: Re: [PATCH v2] block: fix trace completion for chained bio
Date: Fri, 23 Apr 2021 16:04:49 +0800	[thread overview]
Message-ID: <fb8620bf-f4e9-5787-ab79-6e0a5d79d26d@synology.com> (raw)
In-Reply-To: <87zgyudgss.fsf@notabene.neil.brown.name>

On 3/23/2021 5:22 AM, NeilBrown wrote:
> On Wed, Mar 03 2021, edwardh wrote:
> 
>> From: Edward Hsieh <edwardh@synology.com>
>>
>> For chained bio, trace_block_bio_complete in bio_endio is currently called
>> only by the parent bio once upon all chained bio completed.
>> However, the sector and size for the parent bio are modified in bio_split.
>> Therefore, the size and sector of the complete events might not match the
>> queue events in blktrace.
>>
>> The original fix of bio completion trace <fbbaf700e7b1> ("block: trace
>> completion of all bios.") wants multiple complete events to correspond
>> to one queue event but missed this.
>>
>> md/raid5 read with bio cross chunks can reproduce this issue.
>>
>> To fix, move trace completion into the loop for every chained bio to call.
> 
> Thanks.  I think this is correct as far as tracing goes.
> However the code still looks a bit odd.
> 
> The comment for the handling of bio_chain_endio suggests that the *only*
> purpose for that is to avoid deep recursion.  That suggests it should be
> at the end of the function.
> As it is blk_throtl_bio_endio() and bio_unint() are only called on the
> last bio in a chain.
> That seems wrong.
> 
> I'd be more comfortable if the patch moved the bio_chain_endio()
> handling to the end, after all of that.
> So the function would end.
> 
> if (bio->bi_end_io == bio_chain_endio) {
>     bio = __bio_chain_endio(bio);
>     goto again;
> } else if (bio->bi_end_io)
>     bio->bi_end_io(bio);
> 
> Jens:  can you see any reason why that functions must only be called on
> the last bio in the chain?
> 
> Thanks,
> NeilBrown
> 

Hi Neil and Jens,

 From the commit message, bio_uninit is put here for bio allocated in 
special ways (e.g., on stack), that will not be release by bio_free. For 
chained bio, __bio_chain_endio invokes bio_put and release the 
resources, so it seems that we don't need to call bio_uninit for chained 
bio.

The blk_throtl_bio_endio is used to update the latency for the throttle 
group. I think the latency should only be updated after the whole bio is 
finished?

To make sense for the "tail call optimization" in the comment, I'll 
suggest to wrap the whole statement with an else. What do you think?

if (bio->bi_end_io == bio_chain_endio) {
	bio = __bio_chain_endio(bio);
	goto again;
} else {
	blk_throtl_bio_endio(bio);
	/* release cgroup info */
	bio_uninit(bio);
	if (bio->bi_end_io)
		bio->bi_end_io(bio);
}

Thanks,
Edward Hsieh

  reply	other threads:[~2021-04-23  8:04 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-03  3:22 [PATCH v2] block: fix trace completion for chained bio edwardh
2021-03-16 10:30 ` Edward Hsieh
2021-03-22 21:22 ` NeilBrown
2021-04-23  8:04   ` Edward Hsieh [this message]
2021-05-10  2:06     ` Edward Hsieh
2021-05-25  9:37       ` Edward Hsieh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fb8620bf-f4e9-5787-ab79-6e0a5d79d26d@synology.com \
    --to=edwardh@synology.com \
    --cc=axboe@kernel.dk \
    --cc=bingjingc@synology.com \
    --cc=cccheng@synology.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=neilb@suse.com \
    --cc=neilb@suse.de \
    --cc=s3t@synology.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).