mbox series

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

Message ID 20210518025642.740082-1-gerald.yang@canonical.com
Headers show
Series dm crypt: add flags to optionally | expand

Message

Gerald Yang May 18, 2021, 2:56 a.m. UTC 
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1910976

=== 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(-)

Comments

Stefan Bader May 18, 2021, 6:42 a.m. UTC | #1 
On 18.05.21 04:56, Gerald Yang wrote:
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1910976

Above should be 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
Gerald Yang May 19, 2021, 7:26 a.m. UTC | #2 
Thanks Stefan, I will update the SRU with all related commits

On Tue, May 18, 2021 at 2:42 PM Stefan Bader <stefan.bader@canonical.com>
wrote:

> On 18.05.21 04:56, Gerald Yang wrote:
> > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1910976
>
> Above should be 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
>
>
Gerald Yang June 28, 2021, 9:37 a.m. UTC | #3 
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?
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@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@canonical.com>
> wrote:
>
>> On 18.05.21 04:56, Gerald Yang wrote:
>> > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1910976
>>
>> Above should be 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
>>
>>
Stefan Bader June 29, 2021, 7:26 a.m. UTC | #4 
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@canonical.com 
> <mailto:gerald.yang@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@canonical.com
>     <mailto:stefan.bader@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
>
Gerald Yang June 29, 2021, 10:01 a.m. UTC | #5 
Understood, thank you Stefan

On Tue, Jun 29, 2021 at 3:26 PM Stefan Bader <stefan.bader@canonical.com>
wrote:

> 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@canonical.com
> > <mailto:gerald.yang@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@canonical.com
> >     <mailto:stefan.bader@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
> >
>
>
>