Re: [PATCH] md: don't account io stat for split bio

From: Song Liu <song@kernel.org>
To: Guoqing Jiang <jgq516@gmail.com>
Cc: "Artur Paszkiewicz" <artur.paszkiewicz@intel.com>,
	"Christoph Hellwig" <hch@infradead.org>,
	linux-raid <linux-raid@vger.kernel.org>,
	linux-block@vger.kernel.org,
	"Paweł Wiejacha" <pawel.wiejacha@rtbhouse.com>
Subject: Re: [PATCH] md: don't account io stat for split bio
Date: Mon, 10 May 2021 23:58:32 -0700	[thread overview]
Message-ID: <CAPhsuW44cc2p+29_rLqrq7i3R0d03sjtwRQtbLRkta+jzsdYsw@mail.gmail.com> (raw)
In-Reply-To: <c1bc42ff-eae7-d0ba-505d-9c6a19d60e93@gmail.com>

On Mon, May 10, 2021 at 7:13 PM Guoqing Jiang <jgq516@gmail.com> wrote:
>
>
>
> On 5/11/21 3:49 AM, Artur Paszkiewicz wrote:
> > On 5/10/21 9:46 AM, Guoqing Jiang wrote:
> >> On 5/10/21 2:00 PM, Christoph Hellwig wrote:
> >>> On Sat, May 08, 2021 at 11:48:15AM +0800, Guoqing Jiang wrote:
> >>>> It looks like stack overflow happened for split bio, to fix this,
> >>>> let's keep split bio untouched in md_submit_bio.
> >>>>
> >>>> As a side effect, we need to export bio_chain_endio.
> >>> Err, no.  The right answer is to not change ->bi_end_io of bios that
> >>> you do not own instead of using a horrible hack to skip accounting for
> >>> bios that have no more or less reason to be accounted than others bios.
> >> Thanks for the reply. I suppose that md needs to revert current
> >> implementation of accounting io stats, then re-implement it.
> >>
> >> Song and Artur, what are your opinion?
> > In the initial version of the io accounting patch the bio was cloned instead
> > of just overriding bi_end_io and bi_private. Would this be the right approach?
> >
> > https://lore.kernel.org/linux-raid/20200601161256.27718-1-artur.paszkiewicz@intel.com/
>
> Maybe we can have different approach for different personality layers.
>
> 1. raid1 and raid10 can do the accounting in their own layer since they
> already
>      clone bio here.
> 2. make the initial version handles other personality such as raid0 and
> raid5
>      in the md layer.
>
> Also a sysfs node which can enable/disable the accounting could be helpful.

IIUC, the sysfs node is needed to get better performance (by disabling
accounting)?
And splitting 1 and 2 above is also for better performance? If we add
the sysfs node,
I would prefer we use the same approach for all personalities (clone
in md.c). This
should simplify the code. If the user do not need extreme performance, we should
keep the stats on (default). If the user do need extreme performance, s/he could
disable stats via sysfs.

Thoughts?

Thanks,
Song