NACK/Cmnt: [SRU][B][F][G][PATCH 0/1] dm crypt: add flags to optionally

[Stefan Bader]

On 28.06.21 11:37, Gerald Yang wrote:
> Hi Stefan,
> 
> I checked and also tried to backport these commits:
> git log --oneline --grep "Fixes: 39d42fa96ba1"
> c87a95dc28b1 dm crypt: defer decryption to a tasklet if interrupts disabled
> 8e14f610159d dm crypt: do not call bio_endio() from the dm-crypt tasklet
> d68b29584c25 dm crypt: use GFP_ATOMIC when allocating crypto requests from softirq
> 8abec36d1274 dm crypt: do not wait for backlogged crypto request completion in
> softirq
> 4a5caa4af0df dm crypt: document new no_workqueue flags
> 
> But it failed to build the kernel, I found there are more dependencies need to 
> be backported
> Take focal kernel (5.4) for instance, the build error is:
> /home/ubuntu/focal/drivers/md/dm-crypt.c:2768:8: error: ‘dm_report_zones_cb’ 
> undeclared (first use in this function); did you mean ‘dm_report_zones_fn’?
> 2768 | dm_report_zones_cb, args);
> 
> This function "dm_report_zones_cb" was introduced in this commit in 5.5 kernel:
> git log -L :dm_report_zones_cb:drivers/md/dm.c
> commit d41003513e61dd9d4974cb441d30b63650b85654
> git describe --contains d41003513e61dd9d4974cb441d30b63650b85654
> v5.5-rc1~199^2~1
> 
> It reworked "zone reporting"
> d41003513e61 block: rework zone reporting
> 
> And modify some other files in block layer
> drivers/md/dm-flakey.c
> drivers/md/dm-linear.c
> drivers/scsi/sd.h
> fs/f2fs/super.c
> include/linux/device-mapper.h
> block/blk-zoned.c
> drivers/block/null_blk.h
> drivers/block/null_blk_zoned.c
> drivers/md/dm-zoned-metadata.c
> drivers/md/dm.c
> drivers/scsi/sd_zbc.c
> include/linux/blkdev.h
> 
> I think it could get more dependencies involved for backporting this commit
> 
> And the original commit I wanted to backport 
> (39d42fa96ba1b7d2544db3f8ed5da8fb0d5cb877)
> changes dm-crypt version from 1.19.0 to 1.22.0 for focal kernel (5.4)
> 
> Because this is a request from our customer, I would like to ask for advice if 
> it's still proper to backport this one and all related commits?

The amount of follow-up changes which were done upstream to a seemingly simple 
functional tweak suggest this change had more complications than even upstream 
anticipated. At some point we have to make a decision and for LTS release 
kernels this would be rather in favour of stability over feature backports. Even 
more so the older a release is (I would acutally never consider feature changes 
to Bionic anymore). Groovy nears its end of life and I assume Hirsute already 
has this. And somehow the errors of the Focal 5.4 backport sound like a lot of 
generic device-mapper infrastructure would have to be backported and that would 
put the stability of the kernel at risk.
So I would rather point that customer at the hwe-5.11 kernel which is about to 
become the default hwe kernel for Focal.

-Stefan

> or it's better to wait for a newer hwe kernel that has this commit included?
> 
> Thanks,
> Gerald
> 
> On Wed, May 19, 2021 at 3:26 PM Gerald Yang <gerald.yang at canonical.com 
> <mailto:gerald.yang at canonical.com>> wrote:
> 
>     Thanks Stefan, I will update the SRU with all related commits
> 
>     On Tue, May 18, 2021 at 2:42 PM Stefan Bader <stefan.bader at canonical.com
>     <mailto:stefan.bader at canonical.com>> wrote:
> 
>         On 18.05.21 04:56, Gerald Yang wrote:
>          > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1910976
>         <https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1910976>
> 
>         Above should be https://bugs.launchpad.net/bugs/1910976
>         <https://bugs.launchpad.net/bugs/1910976> but that could be done
>         when applying. However there seem to be multiple references to this patch
>         following its introduction upstream (see below). That does not sound
>         like just
>         picking this one patch is a good idea.
> 
>         -Stefan
> 
>          >
>          > === SRU Justification ===
>          > [Impact]
>          > To get better performance for dm-crypt in some cases, bypass kcryptd
>          > workqueue can reduce the overhead in context switch between kworkers,
>          > cherry-pick commit 39d42fa96ba1b7d2544db3f8ed5da8fb0d5cb877 from mainline
>          > kernel, and bypass kcryptd workqueue is not enabled by default
>          >
>          > [Fix]
>          > Add flags to bypass kcryptd workqueue
>          >
>          > [Test]
>          > create dm-crypt and setup DM_CRYPT_NO_READ_WORKQUEUE and
>          > DM_CRYPT_NO_WRITE_WORKQUEUE, read/write data and run perf record to see
>          > if read/write to the encrypted device will bypass kcryptd workqueue
>          >
>          > [Regression Potential]
>          > Low, this feature is disabled by default, need to enable manually
>          >
>          > Ignat Korchagin (1):
>          >    dm crypt: add flags to optionally bypass kcryptd workqueues
>          >
>          >   drivers/md/dm-crypt.c | 50 ++++++++++++++++++++++++++++++++++++-------
>          >   1 file changed, 42 insertions(+), 8 deletions(-)
>          >
> 
>           > git log --oneline --grep "Fixes: 39d42fa96ba1"
>         c87a95dc28b1 dm crypt: defer decryption to a tasklet if interrupts disabled
>         8e14f610159d dm crypt: do not call bio_endio() from the dm-crypt tasklet
>         d68b29584c25 dm crypt: use GFP_ATOMIC when allocating crypto requests
>         from softirq
>         8abec36d1274 dm crypt: do not wait for backlogged crypto request
>         completion in
>         softirq
>         4a5caa4af0df dm crypt: document new no_workqueue flags
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210629/8634eb62/attachment.sig>