Re: [dm-devel] [PATCH 0/6] Overhaul memalloc_no*

2020-06-28 Thread Dave Chinner
On Sat, Jun 27, 2020 at 09:09:09AM -0400, Mikulas Patocka wrote:
> 
> 
> On Sat, 27 Jun 2020, Dave Chinner wrote:
> 
> > On Fri, Jun 26, 2020 at 11:02:19AM -0400, Mikulas Patocka wrote:
> > > Hi
> > > 
> > > I suggest to join memalloc_noio and memalloc_nofs into just one flag that 
> > > prevents both filesystem recursion and i/o recursion.
> > > 
> > > Note that any I/O can recurse into a filesystem via the loop device, thus 
> > > it doesn't make much sense to have a context where PF_MEMALLOC_NOFS is 
> > > set 
> > > and PF_MEMALLOC_NOIO is not set.
> > 
> > Correct me if I'm wrong, but I think that will prevent swapping from
> > GFP_NOFS memory reclaim contexts.
> 
> Yes.
> 
> > IOWs, this will substantially
> > change the behaviour of the memory reclaim system under sustained
> > GFP_NOFS memory pressure. Sustained GFP_NOFS memory pressure is
> > quite common, so I really don't think we want to telling memory
> > reclaim "you can't do IO at all" when all we are trying to do is
> > prevent recursion back into the same filesystem.
> 
> So, we can define __GFP_ONLY_SWAP_IO and __GFP_IO.

Uh, why?

Exactly what problem are you trying to solve here?

> > Given that the loop device IO path already operates under
> > memalloc_noio context, (i.e. the recursion restriction is applied in
> > only the context that needs is) I see no reason for making that a
> > global reclaim limitation
> 
> I think this is a problem.
> 
> Suppose that a filesystem does GFP_NOFS allocation, the allocation 
> triggers an IO and waits for it to finish, the loop device driver 
> redirects the IO to the same filesystem that did the GFP_NOFS allocation.

The loop device IO path is under memalloc_noio. By -definition-,
allocations in that context cannot recurse back into filesystem
level reclaim.

So either your aren't explaining the problem you are trying to solve
clearly, or you're talking about allocations in the IO path that are
broken because they don't use GFP_NOIO correctly...

> I saw this deadlock in the past in the dm-bufio subsystem - see the commit 
> 9d28eb12447ee08bb5d1e8bb3195cf20e1ecd1c0 that fixed it.

2014?

/me looks closer.

Hmmm. Only sent to dm-devel, no comments, no review, just merged.
No surprise that nobody else actually knows about this commit. Well,
time to review it ~6 years after it was merged

| dm-bufio tested for __GFP_IO. However, dm-bufio can run on a loop block
| device that makes calls into the filesystem. If __GFP_IO is present and
| __GFP_FS isn't, dm-bufio could still block on filesystem operations if it
| runs on a loop block device.

OK, so from an architectural POV, this commit is fundamentally
broken - block/device layer allocation should not allow relcaim
recursion into filesystems because filesystems are dependent on
the block layer making forwards progress. This commit is trying to
work around the loop device doing GFP_KERNEL/GFP_NOFS context
allocation back end IO path of the loop device. This part of the
loop device is a block device, so needs to run under GFP_NOIO
context.

IOWs, this commit just papered over the reclaim context layering
violation in the loop device by trying to avoid blocking filesystem
IO in the dm-bufio shrinker context just in case it was IO from a
loop device that was incorrectly tagged as GFP_KERNEL.

So, step forward 5 years to 2019, and this change was made:

commit d0a255e795ab976481565f6ac178314b34fbf891
Author: Mikulas Patocka 
Date:   Thu Aug 8 11:17:01 2019 -0400

loop: set PF_MEMALLOC_NOIO for the worker thread

A deadlock with this stacktrace was observed.

The loop thread does a GFP_KERNEL allocation, it calls into dm-bufio
shrinker and the shrinker depends on I/O completion in the dm-bufio
subsystem.

In order to fix the deadlock (and other similar ones), we set the flag
PF_MEMALLOC_NOIO at loop thread entry.

PID: 474TASK: 8813e11f4600  CPU: 10  COMMAND: "kswapd0"
   #0 [8813dedfb938] __schedule at 8173f405
   #1 [8813dedfb990] schedule at 8173fa27
   #2 [8813dedfb9b0] schedule_timeout at 81742fec
   #3 [8813dedfba60] io_schedule_timeout at 8173f186
   #4 [8813dedfbaa0] bit_wait_io at 8174034f
   #5 [8813dedfbac0] __wait_on_bit at 8173fec8
   #6 [8813dedfbb10] out_of_line_wait_on_bit at 8173ff81
   #7 [8813dedfbb90] __make_buffer_clean at a038736f [dm_bufio]
   #8 [8813dedfbbb0] __try_evict_buffer at a0387bb8 [dm_bufio]
   #9 [8813dedfbbd0] dm_bufio_shrink_scan at a0387cc3 [dm_bufio]
  #10 [8813dedfbc40] shrink_slab at 811a87ce
  #11 [8813dedfbd30] shrink_zone at 811ad778
  #12 [8813dedfbdc0] kswapd at 811ae92f
  #13 [8813dedfbec0] kthread at 810a8428
  #14 [8813dedfbf50] ret_from_fork at 81745242

  PID: 14127  TASK: 881455749c00  CPU: 11  

Re: [dm-devel] [PATCH 1/3 v2] crypto: introduce the flag CRYPTO_ALG_ALLOCATES_MEMORY

2020-06-28 Thread Eric Biggers
On Sun, Jun 28, 2020 at 03:07:49PM -0400, Mikulas Patocka wrote:
> > 
> > cryptd_create_skcipher(), cryptd_create_hash(), cryptd_create_aead(), and
> > crypto_rfc4309_create() are also missing setting the mask.
> > 
> > pcrypt_create_aead() is missing both setting the mask and inheriting the 
> > flags.
> 
> I added CRYPTO_ALG_ALLOCATES_MEMORY there.

I don't see where the cryptd request processing functions allocate memory.

It seems that cryptd should just inherit the flag, like most other templates.

Likewise for pcrypt.

And also likewise for rfc4309.

Where are you seeing the memory allocations that would require
CRYPTO_ALG_ALLOCATES_MEMORY to always be enabled for these?

- Eric

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] [PATCH 1/3 v2] crypto: introduce the flag CRYPTO_ALG_ALLOCATES_MEMORY

2020-06-28 Thread Eric Biggers
On Sun, Jun 28, 2020 at 03:07:49PM -0400, Mikulas Patocka wrote:
> 
> > Also, "seqiv" instances can be created without CRYPTO_ALG_ALLOCATES_MEMORY 
> > set,
> > despite seqiv_aead_encrypt() allocating memory.
> > 

This comment wasn't addressed.

- Eric

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] [PATCH 1/3 v2] crypto: introduce the flag CRYPTO_ALG_ALLOCATES_MEMORY

2020-06-28 Thread Eric Biggers
On Sun, Jun 28, 2020 at 03:04:22PM -0400, Mikulas Patocka wrote:
> > > Index: linux-2.6/crypto/authenc.c
> > > ===
> > > --- linux-2.6.orig/crypto/authenc.c   2020-06-26 17:24:03.566417000 
> > > +0200
> > > +++ linux-2.6/crypto/authenc.c2020-06-26 17:24:03.566417000 +0200
> > > @@ -388,7 +388,8 @@ static int crypto_authenc_create(struct
> > >   if ((algt->type ^ CRYPTO_ALG_TYPE_AEAD) & algt->mask)
> > >   return -EINVAL;
> > >  
> > > - mask = crypto_requires_sync(algt->type, algt->mask);
> > > + mask = crypto_requires_sync(algt->type, algt->mask) |
> > > +crypto_requires_nomem(algt->type, algt->mask);
> > 
> > As I suggested earlier, shouldn't there be a function that returns the mask 
> > for
> > all inherited flags, rather than handling each flag individually?
> 
> Yes - I've created crypto_requires_inherited for this purpose.

Since all callers pass in 'struct crypto_attr_type', a better helper might be:

static inline int crypto_algt_inherited_mask(struct crypto_attr_type *algt)
{
return crypto_requires_off(algt->type, algt->mask,
   CRYPTO_ALG_INHERITED_FLAGS);
}

> > > @@ -424,7 +425,7 @@ static int crypto_authenc_create(struct
> > >   goto err_free_inst;
> > >  
> > >   inst->alg.base.cra_flags = (auth_base->cra_flags |
> > > - enc->base.cra_flags) & CRYPTO_ALG_ASYNC;
> > > + enc->base.cra_flags) & CRYPTO_ALG_INHERITED_FLAGS;
> > 
> > Strange indentation here.  Likewise in most of the other files.
> 
> I was told that the code should be 80-characters wide.

You could use:

inst->alg.base.cra_flags =
(auth_base->cra_flags | enc->base.cra_flags) &
CRYPTO_ALG_INHERITED_FLAGS;

Just a suggestion, it's not a big deal...  Your indentation of the continuation
line just seems weird.

> > > --- linux-2.6.orig/crypto/xts.c   2020-06-26 17:24:03.566417000 +0200
> > > +++ linux-2.6/crypto/xts.c2020-06-26 17:24:03.566417000 +0200
> > > @@ -415,7 +415,7 @@ static int create(struct crypto_template
> > >   } else
> > >   goto err_free_inst;
> > >  
> > > - inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
> > > + inst->alg.base.cra_flags = alg->base.cra_flags & 
> > > CRYPTO_ALG_INHERITED_FLAGS;
> > >   inst->alg.base.cra_priority = alg->base.cra_priority;
> > >   inst->alg.base.cra_blocksize = XTS_BLOCK_SIZE;
> > >   inst->alg.base.cra_alignmask = alg->base.cra_alignmask |
> > 
> > Need to set the mask correctly in this file.
> 
> I don't know what do you mean.

I mean that the CRYPTO_ALG_ALLOCATES_MEMORY flag is not handled when the 'mask'
variable is assigned to earlier in this function.

It should use your new helper function, like all the other places in this patch.

- Eric

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] [PATCH 1/3 v2] crypto: introduce the flag CRYPTO_ALG_ALLOCATES_MEMORY

2020-06-28 Thread Mikulas Patocka



On Fri, 26 Jun 2020, Eric Biggers wrote:

> On Fri, Jun 26, 2020 at 09:46:17AM -0700, Eric Biggers wrote:
> > On Fri, Jun 26, 2020 at 12:16:33PM -0400, Mikulas Patocka wrote:
> > > +/*
> > > + * Pass these flags down through the crypto API.
> > > + */
> > > +#define CRYPTO_ALG_INHERITED_FLAGS   (CRYPTO_ALG_ASYNC | 
> > > CRYPTO_ALG_ALLOCATES_MEMORY)
> > 
> > This comment is useless.  How about:
> > 
> > /*
> >  * When an algorithm uses another algorithm (e.g., if it's an instance of a
> >  * template), these are the flags that always get set on the "outer" 
> > algorithm
> >  * if any "inner" algorithm has them set.  In some cases other flags are
> >  * inherited too; these are just the flags that are *always* inherited.
> >  */
> > #define CRYPTO_ALG_INHERITED_FLAGS  (CRYPTO_ALG_ASYNC | 
> > CRYPTO_ALG_ALLOCATES_MEMORY)
> > 
> > Also I wonder about the case where the inner algorithm is a fallback rather 
> > than
> > part of a template instance.  This patch only handles templates, not 
> > fallbacks.
> > Is that intentional?  Isn't that technically a bug?
> 
> Also is CRYPTO_ALG_ALLOCATES_MEMORY meant to apply for algorithms of type
> "cipher" and "shash"?  The code doesn't handle those, so presumably not?
> 
> What about "akcipher"?

Yes - the patch should apply for these cases, but I don't know how to do 
it. Please, do it.

> > > Index: linux-2.6/crypto/xts.c
> > > ===
> > > --- linux-2.6.orig/crypto/xts.c   2020-06-26 17:24:03.566417000 +0200
> > > +++ linux-2.6/crypto/xts.c2020-06-26 17:24:03.566417000 +0200
> > > @@ -415,7 +415,7 @@ static int create(struct crypto_template
> > >   } else
> > >   goto err_free_inst;
> > >  
> > > - inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
> > > + inst->alg.base.cra_flags = alg->base.cra_flags & 
> > > CRYPTO_ALG_INHERITED_FLAGS;
> > >   inst->alg.base.cra_priority = alg->base.cra_priority;
> > >   inst->alg.base.cra_blocksize = XTS_BLOCK_SIZE;
> > >   inst->alg.base.cra_alignmask = alg->base.cra_alignmask |
> > 
> > Need to set the mask correctly in this file.
> 
> cryptd_create_skcipher(), cryptd_create_hash(), cryptd_create_aead(), and
> crypto_rfc4309_create() are also missing setting the mask.
> 
> pcrypt_create_aead() is missing both setting the mask and inheriting the 
> flags.

I added CRYPTO_ALG_ALLOCATES_MEMORY there.

> Also, "seqiv" instances can be created without CRYPTO_ALG_ALLOCATES_MEMORY 
> set,
> despite seqiv_aead_encrypt() allocating memory.
> 
> - Eric

Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



[dm-devel] [PATCH 1/3 v3] crypto: introduce the flag CRYPTO_ALG_ALLOCATES_MEMORY

2020-06-28 Thread Mikulas Patocka
Introduce a new flag CRYPTO_ALG_ALLOCATES_MEMORY and pass it down the
crypto stack.

If the flag is set, then the crypto driver allocates memory in its request
routine. Such drivers are not suitable for disk encryption because
GFP_ATOMIC allocation can fail anytime (causing random I/O errors) and
GFP_KERNEL allocation can recurse into the block layer, causing a
deadlock.

Pass the flag CRYPTO_ALG_ALLOCATES_MEMORY down through the crypto API.

Signed-off-by: Mikulas Patocka 

---
 crypto/adiantum.c |2 +-
 crypto/authenc.c  |4 ++--
 crypto/authencesn.c   |4 ++--
 crypto/ccm.c  |9 +
 crypto/chacha20poly1305.c |4 ++--
 crypto/cryptd.c   |   12 +---
 crypto/ctr.c  |4 ++--
 crypto/cts.c  |4 ++--
 crypto/essiv.c|4 ++--
 crypto/gcm.c  |   12 ++--
 crypto/geniv.c|4 ++--
 crypto/lrw.c  |4 ++--
 crypto/pcrypt.c   |4 +++-
 crypto/rsa-pkcs1pad.c |4 ++--
 crypto/xts.c  |2 +-
 include/crypto/algapi.h   |   10 ++
 include/linux/crypto.h|   15 +++
 17 files changed, 64 insertions(+), 38 deletions(-)

Index: linux-2.6/include/linux/crypto.h
===
--- linux-2.6.orig/include/linux/crypto.h   2020-06-26 17:24:03.566417000 
+0200
+++ linux-2.6/include/linux/crypto.h2020-06-28 17:55:21.976417000 +0200
@@ -102,6 +102,21 @@
 #define CRYPTO_NOLOAD  0x8000
 
 /*
+ * The driver may allocate memory during request processing, so it shouldn't be
+ * used in cases where memory allocation failures aren't acceptable, such as
+ * during block device encryption.
+ */
+#define CRYPTO_ALG_ALLOCATES_MEMORY0x0001
+
+/*
+ * When an algorithm uses another algorithm (e.g., if it's an instance of a
+ * template), these are the flags that always get set on the "outer" algorithm
+ * if any "inner" algorithm has them set.  In some cases other flags are
+ * inherited too; these are just the flags that are *always* inherited.
+ */
+#define CRYPTO_ALG_INHERITED_FLAGS (CRYPTO_ALG_ASYNC | 
CRYPTO_ALG_ALLOCATES_MEMORY)
+
+/*
  * Transform masks and values (for crt_flags).
  */
 #define CRYPTO_TFM_NEED_KEY0x0001
Index: linux-2.6/crypto/authenc.c
===
--- linux-2.6.orig/crypto/authenc.c 2020-06-26 17:24:03.566417000 +0200
+++ linux-2.6/crypto/authenc.c  2020-06-28 20:33:29.136417000 +0200
@@ -388,7 +388,7 @@ static int crypto_authenc_create(struct
if ((algt->type ^ CRYPTO_ALG_TYPE_AEAD) & algt->mask)
return -EINVAL;
 
-   mask = crypto_requires_sync(algt->type, algt->mask);
+   mask = crypto_requires_inherited(algt->type, algt->mask);
 
inst = kzalloc(sizeof(*inst) + sizeof(*ctx), GFP_KERNEL);
if (!inst)
@@ -424,7 +424,7 @@ static int crypto_authenc_create(struct
goto err_free_inst;
 
inst->alg.base.cra_flags = (auth_base->cra_flags |
-   enc->base.cra_flags) & CRYPTO_ALG_ASYNC;
+   enc->base.cra_flags) & CRYPTO_ALG_INHERITED_FLAGS;
inst->alg.base.cra_priority = enc->base.cra_priority * 10 +
  auth_base->cra_priority;
inst->alg.base.cra_blocksize = enc->base.cra_blocksize;
Index: linux-2.6/crypto/authencesn.c
===
--- linux-2.6.orig/crypto/authencesn.c  2020-06-26 17:24:03.566417000 +0200
+++ linux-2.6/crypto/authencesn.c   2020-06-28 20:33:32.556417000 +0200
@@ -406,7 +406,7 @@ static int crypto_authenc_esn_create(str
if ((algt->type ^ CRYPTO_ALG_TYPE_AEAD) & algt->mask)
return -EINVAL;
 
-   mask = crypto_requires_sync(algt->type, algt->mask);
+   mask = crypto_requires_inherited(algt->type, algt->mask);
 
inst = kzalloc(sizeof(*inst) + sizeof(*ctx), GFP_KERNEL);
if (!inst)
@@ -438,7 +438,7 @@ static int crypto_authenc_esn_create(str
goto err_free_inst;
 
inst->alg.base.cra_flags = (auth_base->cra_flags |
-   enc->base.cra_flags) & CRYPTO_ALG_ASYNC;
+   enc->base.cra_flags) & CRYPTO_ALG_INHERITED_FLAGS;
inst->alg.base.cra_priority = enc->base.cra_priority * 10 +
  auth_base->cra_priority;
inst->alg.base.cra_blocksize = enc->base.cra_blocksize;
Index: linux-2.6/crypto/ccm.c
===
--- linux-2.6.orig/crypto/ccm.c 2020-06-26 17:24:03.566417000 +0200
+++ linux-2.6/crypto/ccm.c  2020-06-28 20:43:42.456417000 +0200
@@ -462,7 +462,7 @@ static int crypto_ccm_create_common(stru
if ((algt->type ^ CRYPTO_ALG_TYPE_AEAD) & algt->mask)
return -EINVAL;
 
-  

Re: [dm-devel] [PATCH 1/3 v2] crypto: introduce the flag CRYPTO_ALG_ALLOCATES_MEMORY

2020-06-28 Thread Mikulas Patocka



On Fri, 26 Jun 2020, Eric Biggers wrote:

> On Fri, Jun 26, 2020 at 12:16:33PM -0400, Mikulas Patocka wrote:
> > +/*
> > + * Pass these flags down through the crypto API.
> > + */
> > +#define CRYPTO_ALG_INHERITED_FLAGS (CRYPTO_ALG_ASYNC | 
> > CRYPTO_ALG_ALLOCATES_MEMORY)
> 
> This comment is useless.  How about:
> 
> /*
>  * When an algorithm uses another algorithm (e.g., if it's an instance of a
>  * template), these are the flags that always get set on the "outer" algorithm
>  * if any "inner" algorithm has them set.  In some cases other flags are
>  * inherited too; these are just the flags that are *always* inherited.
>  */
> #define CRYPTO_ALG_INHERITED_FLAGS(CRYPTO_ALG_ASYNC | 
> CRYPTO_ALG_ALLOCATES_MEMORY)
> 
> Also I wonder about the case where the inner algorithm is a fallback rather 
> than
> part of a template instance.  This patch only handles templates, not 
> fallbacks.
> Is that intentional?  Isn't that technically a bug?

I'm not an expert in crypto internals, so I don't know. I'll send version 
3 of this patch and I'd like to ask you or Herbert to fix it.

> > +
> > +/*
> >   * Transform masks and values (for crt_flags).
> >   */
> >  #define CRYPTO_TFM_NEED_KEY0x0001
> > Index: linux-2.6/crypto/authenc.c
> > ===
> > --- linux-2.6.orig/crypto/authenc.c 2020-06-26 17:24:03.566417000 +0200
> > +++ linux-2.6/crypto/authenc.c  2020-06-26 17:24:03.566417000 +0200
> > @@ -388,7 +388,8 @@ static int crypto_authenc_create(struct
> > if ((algt->type ^ CRYPTO_ALG_TYPE_AEAD) & algt->mask)
> > return -EINVAL;
> >  
> > -   mask = crypto_requires_sync(algt->type, algt->mask);
> > +   mask = crypto_requires_sync(algt->type, algt->mask) |
> > +  crypto_requires_nomem(algt->type, algt->mask);
> 
> As I suggested earlier, shouldn't there be a function that returns the mask 
> for
> all inherited flags, rather than handling each flag individually?

Yes - I've created crypto_requires_inherited for this purpose.

> >  
> > inst = kzalloc(sizeof(*inst) + sizeof(*ctx), GFP_KERNEL);
> > if (!inst)
> > @@ -424,7 +425,7 @@ static int crypto_authenc_create(struct
> > goto err_free_inst;
> >  
> > inst->alg.base.cra_flags = (auth_base->cra_flags |
> > -   enc->base.cra_flags) & CRYPTO_ALG_ASYNC;
> > +   enc->base.cra_flags) & CRYPTO_ALG_INHERITED_FLAGS;
> 
> Strange indentation here.  Likewise in most of the other files.

I was told that the code should be 80-characters wide.

> > Index: linux-2.6/crypto/xts.c
> > ===
> > --- linux-2.6.orig/crypto/xts.c 2020-06-26 17:24:03.566417000 +0200
> > +++ linux-2.6/crypto/xts.c  2020-06-26 17:24:03.566417000 +0200
> > @@ -415,7 +415,7 @@ static int create(struct crypto_template
> > } else
> > goto err_free_inst;
> >  
> > -   inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
> > +   inst->alg.base.cra_flags = alg->base.cra_flags & 
> > CRYPTO_ALG_INHERITED_FLAGS;
> > inst->alg.base.cra_priority = alg->base.cra_priority;
> > inst->alg.base.cra_blocksize = XTS_BLOCK_SIZE;
> > inst->alg.base.cra_alignmask = alg->base.cra_alignmask |
> 
> Need to set the mask correctly in this file.

I don't know what do you mean.

> > Index: linux-2.6/crypto/adiantum.c
> > ===
> > --- linux-2.6.orig/crypto/adiantum.c2020-06-26 17:24:03.566417000 
> > +0200
> > +++ linux-2.6/crypto/adiantum.c 2020-06-26 17:24:03.566417000 +0200
> > @@ -507,7 +507,8 @@ static int adiantum_create(struct crypto
> > if ((algt->type ^ CRYPTO_ALG_TYPE_SKCIPHER) & algt->mask)
> > return -EINVAL;
> >  
> > -   mask = crypto_requires_sync(algt->type, algt->mask);
> > +   mask = crypto_requires_sync(algt->type, algt->mask) |
> > +  crypto_requires_nomem(algt->type, algt->mask);
> >  
> > inst = kzalloc(sizeof(*inst) + sizeof(*ictx), GFP_KERNEL);
> > if (!inst)
> 
> Need to use CRYPTO_ALG_INHERITED_FLAGS in this file.

OK.

> - Eric

Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] dm-crypt hard lockup

2020-06-28 Thread Bart Van Assche
On 2020-06-26 02:07, Artur Paszkiewicz wrote:
> I'm getting regular lockups which seem to be caused by dm-crypt. I
> reproduced it on vanilla v5.8-rc2, but I started regularly seeing this
> some time ago on openSUSE Tumbleweed kernels. It's easily reproducible
> (every time, after about a minute) when I run "make -j" on the linux
> kernel sources, sometimes it occurs also when doing other IO intensive
> tasks on multiple CPUs. I'm using LVM and ext4 on dm-crypt devices,
> Intel SSDSC2KW010X6 and SSDSC2BA200G3 SSDs.

Since considerable time I'm doing kernel builds (make -j8) on an
openSUSE Tumbleweed system on top of dm-crypt and an NVMe SSD but I have
not yet encountered any kind of lockup. Maybe another driver, e.g. an
I/O scheduler, is responsible for the lockups?

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] [PATCH v4 1/4] dm dust: report some message results back to user directly

2020-06-28 Thread Mike Snitzer
It is on the dm-devel patchwork list to pick up for the 5.9 merge
window.  I've just been busy with other work.

Thanks,
Mike

On Sat, Jun 27 2020 at  9:08pm -0400,
yangerkun  wrote:

> Hi Mike, does there any advice for this patchset?
> 
> Thanks,
> Kun.
> 
> 在 2020/6/20 5:10, Bryan Gurney 写道:
> >From: yangerkun 
> >
> >From: yangerkun 
> >
> >Some type of message(queryblock/countbadblocks/removebadblock) may better
> >report results to user directly. Do it with DMEMIT.
> >
> >[Bryan: maintain __func__ output in DMEMIT messages]
> >
> >Signed-off-by: yangerkun 
> >Signed-off-by: Bryan Gurney 
> >---
> >  drivers/md/dm-dust.c | 31 ++-
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> >
> >diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
> >index ff03b90072c5..f1f2dd6a4e84 100644
> >--- a/drivers/md/dm-dust.c
> >+++ b/drivers/md/dm-dust.c
> >@@ -138,20 +138,22 @@ static int dust_add_block(struct dust_device *dd, 
> >unsigned long long block,
> > return 0;
> >  }
> >-static int dust_query_block(struct dust_device *dd, unsigned long long 
> >block)
> >+static int dust_query_block(struct dust_device *dd, unsigned long long 
> >block, char *result,
> >+unsigned int maxlen, unsigned int *sz_ptr)
> >  {
> > struct badblock *bblock;
> > unsigned long flags;
> >+unsigned int sz = *sz_ptr;
> > spin_lock_irqsave(>dust_lock, flags);
> > bblock = dust_rb_search(>badblocklist, block);
> > if (bblock != NULL)
> >-DMINFO("%s: block %llu found in badblocklist", __func__, block);
> >+DMEMIT("%s: block %llu found in badblocklist", __func__, block);
> > else
> >-DMINFO("%s: block %llu not found in badblocklist", __func__, 
> >block);
> >+DMEMIT("%s: block %llu not found in badblocklist", __func__, 
> >block);
> > spin_unlock_irqrestore(>dust_lock, flags);
> >-return 0;
> >+return 1;
> >  }
> >  static int __dust_map_read(struct dust_device *dd, sector_t thisblock)
> >@@ -259,11 +261,13 @@ static bool __dust_clear_badblocks(struct rb_root 
> >*tree,
> > return true;
> >  }
> >-static int dust_clear_badblocks(struct dust_device *dd)
> >+static int dust_clear_badblocks(struct dust_device *dd, char *result, 
> >unsigned int maxlen,
> >+unsigned int *sz_ptr)
> >  {
> > unsigned long flags;
> > struct rb_root badblocklist;
> > unsigned long long badblock_count;
> >+unsigned int sz = *sz_ptr;
> > spin_lock_irqsave(>dust_lock, flags);
> > badblocklist = dd->badblocklist;
> >@@ -273,11 +277,11 @@ static int dust_clear_badblocks(struct dust_device *dd)
> > spin_unlock_irqrestore(>dust_lock, flags);
> > if (!__dust_clear_badblocks(, badblock_count))
> >-DMINFO("%s: no badblocks found", __func__);
> >+DMEMIT("%s: no badblocks found", __func__);
> > else
> >-DMINFO("%s: badblocks cleared", __func__);
> >+DMEMIT("%s: badblocks cleared", __func__);
> >-return 0;
> >+return 1;
> >  }
> >  /*
> >@@ -383,7 +387,7 @@ static void dust_dtr(struct dm_target *ti)
> >  }
> >  static int dust_message(struct dm_target *ti, unsigned int argc, char 
> > **argv,
> >-char *result_buf, unsigned int maxlen)
> >+char *result, unsigned int maxlen)
> >  {
> > struct dust_device *dd = ti->private;
> > sector_t size = i_size_read(dd->dev->bdev->bd_inode) >> SECTOR_SHIFT;
> >@@ -393,6 +397,7 @@ static int dust_message(struct dm_target *ti, unsigned 
> >int argc, char **argv,
> > unsigned char wr_fail_cnt;
> > unsigned int tmp_ui;
> > unsigned long flags;
> >+unsigned int sz = 0;
> > char dummy;
> > if (argc == 1) {
> >@@ -410,12 +415,12 @@ static int dust_message(struct dm_target *ti, unsigned 
> >int argc, char **argv,
> > r = 0;
> > } else if (!strcasecmp(argv[0], "countbadblocks")) {
> > spin_lock_irqsave(>dust_lock, flags);
> >-DMINFO("countbadblocks: %llu badblock(s) found",
> >+DMEMIT("countbadblocks: %llu badblock(s) found",
> >dd->badblock_count);
> > spin_unlock_irqrestore(>dust_lock, flags);
> >-r = 0;
> >+r = 1;
> > } else if (!strcasecmp(argv[0], "clearbadblocks")) {
> >-r = dust_clear_badblocks(dd);
> >+r = dust_clear_badblocks(dd, result, maxlen, );
> > } else if (!strcasecmp(argv[0], "quiet")) {
> > if (!dd->quiet_mode)
> > dd->quiet_mode = true;
> >@@ -441,7 +446,7 @@ static int dust_message(struct dm_target *ti, unsigned 
> >int argc, char **argv,
> > else if (!strcasecmp(argv[0], "removebadblock"))
> > r = dust_remove_block(dd, block);
> > else if