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

On Mon, May 10, 2021 at 7:13 PM Guoqing Jiang [off-list ref] wrote:

On 5/11/21 3:49 AM, Artur Paszkiewicz wrote:

quoted

On 5/10/21 9:46 AM, Guoqing Jiang wrote:

quoted

On 5/10/21 2:00 PM, Christoph Hellwig wrote:

quoted

On Sat, May 08, 2021 at 11:48:15AM +0800, Guoqing Jiang wrote:

quoted

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/ (local)

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