Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp4203250pxj; Tue, 25 May 2021 02:40:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJynnXSWobcup4DXRJEeHffqGMKYzEr43PPWJg/c5gPmWvAweViO8S+kC8kV5vO/lXORbc0V X-Received: by 2002:a17:907:dab:: with SMTP id go43mr26842254ejc.164.1621935649913; Tue, 25 May 2021 02:40:49 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1621935649; cv=none; d=google.com; s=arc-20160816; b=dIUVkDzF0N9yAbGYkGM9z71IKgzZL7Zh3MnKW+Jkfvm7Nen9A1ABzDGYvEhoEsLuq5 uudj5aIKlZhUp1guKIdgxPbuiYiZlph1qqkd2DH6Jh3QO9QDa0aLYqLrJRbjByIeQesf l7pcj7tg9M+lkHxhhyHiJfrXDZW/qhetseFEyqOCR9sKNjvsXB7pOQu1yYDc4wLk+y2J NT8g2N3qfv4xb4F7S8lOUF6IEE7DSc8FUbSIjo/fP6EF6A4PSHUm0T0ghdUNSoi2VLK6 VdUDsvPFsD/t8tTjZhCiBKw5zoED8QNlA/Y+uWaJvrtYpwfajw6ARtSkTKBCR8uA1p6v JaCw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:date:message-id:references:cc:to:from :dkim-signature:subject; bh=OCbZUSYO/Qjjh9jyQG2jXXzYuAdYm8EOqTaCwN/XrUQ=; b=JXnPpGtPalTV897bdVrIukKigdrQQNrD79JYiaNQEuEO0EaAD1oQXp1xQGGBua0vnz IP6+Yyj+sy+ED0RI2YgfLVYYHGIEnIAUccdcOaerJ/Ke8ihz6wU+NWbP8meTSF6bmOYb NdWXY0TgCc6v2kbtJBTUmJujKXVDOkJdkoTXKiS0VyZwu8VZ+h4uIauRnkQyxYXLSQYL lek3rUCcwivlBRUc5ImYactogRtaR9P6Ip9VkOKxr4OfjkK/01/H0rnRFDzWlZR4csJh N3iKFoWzZgQb6uPYDszPplg1TGaJvYEPQhxNc5xeKo2Zaq1ekLN3mjDNIpoYdChGENwA OjHA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@synology.com header.s=123 header.b="MJiUB/Yf"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=synology.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id i2si145236ejp.181.2021.05.25.02.40.26; Tue, 25 May 2021 02:40:49 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@synology.com header.s=123 header.b="MJiUB/Yf"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=synology.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232508AbhEYJjf (ORCPT + 99 others); Tue, 25 May 2021 05:39:35 -0400 Received: from mail.synology.com ([211.23.38.101]:53532 "EHLO synology.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S232563AbhEYJjc (ORCPT ); Tue, 25 May 2021 05:39:32 -0400 Subject: Re: [PATCH v2] block: fix trace completion for chained bio DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=synology.com; s=123; t=1621935449; bh=pyh7Wvy0eo8qDjfoSG3IQUSH3Z/keP/gcb0hzR7DpoU=; h=Subject:From:To:Cc:References:Date:In-Reply-To; b=MJiUB/Yf+sXio24HtfILVjS+nsMDr6KoYJgHhDXEt2cjCbQp8dm/XWLkIXxvlW5DP 7diC2grtpkywKzRaP6yyws3/Eu6DpakCxpPB93cGIhFtyDL+9lqIt3uoZUo2yoNYFz 8Yjm+lLT7504ZRpiQNSnT/lgiK5G2AAeLQMvCGKc= From: Edward Hsieh To: NeilBrown , 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 References: <1614741726-28074-1-git-send-email-edwardh@synology.com> <87zgyudgss.fsf@notabene.neil.brown.name> <10f3e198-f34c-47e9-608a-e5f84e3379a1@synology.com> Message-ID: <3600b6c0-f83d-f375-bebc-cd5ac811c3d5@synology.com> Date: Tue, 25 May 2021 17:37:29 +0800 MIME-Version: 1.0 In-Reply-To: <10f3e198-f34c-47e9-608a-e5f84e3379a1@synology.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Synology-MCP-Status: no X-Synology-Spam-Flag: no X-Synology-Spam-Status: score=0, required 6, WHITELIST_FROM_ADDRESS 0 X-Synology-Virus-Status: no Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 5/10/2021 10:06 AM, Edward Hsieh wrote: > > On 4/23/2021 4:04 PM, Edward Hsieh wrote: >> On 3/23/2021 5:22 AM, NeilBrown wrote: >>> On Wed, Mar 03 2021, edwardh wrote: >>> >>>> From: Edward Hsieh >>>> >>>> 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 ("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 > > Hi Neil and Jens, > > Any feedback on this one? > > Thank you, > Edward Hsieh > Hi Neil and Jens, Any comments? Thank you, Edward Hsieh