[SRU][B][F][G][PATCH 0/7] raid10: Block discard is very slow, causing severe delays for mkfs and fstrim operations

[Matthew Ruffell]

On 29/10/20 9:46 pm, Stefan Bader wrote:
>> There is also some additional commits which is required, and was merged after 
>> "md/raid10: improve raid10 discard request" was merged. The following commits 
>> enables Radid10 to use large discards, instead of splitting into many bios, 
>> since the technical hurdles have now been removed.
>
> The below two patches are marked up as only needed for F and G. What about
> Bionic? If the changes they refer to were in 4.12, then those would have to go
> to Bionic as well.

Sorry, it seems I have confused you in my SRU template. See backport note #3
about the two commits for F and G:

> 3) The 4.15 kernel does not need "dm raid: fix discard limits for raid1 and raid10"
> and "dm raid: remove unnecessary discard limits for raid10" due to not having
> the following commit, which was merged in 5.1-rc1:
> 
> commit 61697a6abd24acba941359c6268a94f4afe4a53d
> Author: Mike Snitzer <snitzer at redhat.com>
> Date:   Fri Jan 18 14:19:26 2019 -0500
> Subject: dm: eliminate 'split_discard_bios' flag from DM target interface
> Link: https://github.com/torvalds/linux/commit/61697a6abd24acba941359c6268a94f4afe4a53d

Now, "dm: eliminate 'split_discard_bios' flag from DM target interface" is a
really messy backport to Bionic, it also changes the DM API and version
requirements, and it also needs a bunch of dependency commits to enable
blk_queue_split() to handle splitting discards based on queue_limits. In 4.15,
this is all handled by DM core.

The two commits marked for F and G but not B, do not actually modify performance
in any way, and technically aren't needed to solve the problem. I included them
for F and G to provide a more complete fix, but they can be omitted if necessary.

I decided that making the two commits marked for F and G work in Bionic is too
much of a regression risk, and did not include them in the SRU. The benefits for
the 4.15 kernel do not outweigh the risks, while for the 5.4 and 5.8 kernels,
they will get one of the commits via upstream -stable anyway, so we may as well
ship a complete solution.

As for the paragraph that mentions the changes in 4.12, it is referencing the
overall architecture changes required for raid0 to gain better performance, as
it was used as a design template for the changes to raid10. I included it more
or less as a statement that the new code follows a design implemented in 4.12,
and that the design still holds up today, in order to try lend the new patches
some credibility, since they are more or less the same refactor as introduced
in:

> All the commits mentioned follow a similar strategy which was implemented in 
> Raid0 in the below commit, which was merged in 4.12-rc2, which fixed block 
> discard performance issues in Raid0:
> 
> commit 29efc390b9462582ae95eb9a0b8cd17ab956afc0
> Author: Shaohua Li <shli at fb.com>
> Date: Sun May 7 17:36:24 2017 -0700
> Subject: md/md0: optimize raid0 discard handling
> Link: https://github.com/torvalds/linux/commit/29efc390b9462582ae95eb9a0b8cd17ab956afc0 

"follow a similar strategy which was implemented in Raid0" was a poor choice of
words by me to say that the whole patchset is derived from the refactor of raid0
to fix similar problems back in 4.12.

Sorry for the confusion.

> Beside that, I am not sure how exactly that might be better phrased, but
> personally I stumbled over "remove 'address of' pointer for...". Mabye "do
> not use a pointer for one of the arguments to ..." but not sure.

Possibly we could change the wording to "remove pointer reference for parameter
in ...".

e.g. for "md/raid10: improve raid10 discard request":

(backported from commit bcc90d280465ebd51ab8688be86e1f00c62dccf9)
[mruffell: change submit_bio_noacct() to generic_make_request(), remove pointer
reference for parameter in bio_split(), mempool_alloc(), bio_clone_fast()]
Signed-off-by: Matthew Ruffell <matthew.ruffell at canonical.com>

e.g. for "md/raid10: improve discard request for far layout":

(backported from commit d3ee2d8415a6256c1c41e1be36e80e640c3e6359)
[mruffell: remove pointer reference for parameter in mempool_alloc()]
Signed-off-by: Matthew Ruffell <matthew.ruffell at canonical.com>

Would you like me to send a V2? Or is it possible for you to change in place?

Let me know if you have any more concerns. I know this is a big SRU request, so
I have tried to document it the best I can. A customer would also really
like this SRU to go through, since they are avoiding all use of raid10 due to
the severe delays with block discard, but they want to use raid10 on NVMe
devices, and are frustrated that they can't. At the same time, I also understand
if you have regression concerns due to the amount of code changing.

Thanks,
Matthew