Re: [dm-devel] Processes hung in "D" state in ext4, mm, md and dmcrypt

[Andrew Morton]

On Wed, 26 Jul 2023 23:29:51 +0800 Ming Lei  wrote:

> On Wed, Jul 26, 2023 at 6:02 PM David Howells  wrote:
> >
> > Hi,
> >
> > With 6.5-rc2 (6.5.0-0.rc2.20230721gitf7e3a1bafdea.20.fc39.x86_64), I'm 
> > seeing
> > a bunch of processes getting stuck in the D state on my desktop after a few
> > hours of reading email and compiling stuff.  It's happened every day this 
> > week
> > so far and I managed to grab stack traces of the stuck processes this 
> > morning
> > (see attached).
> >
> > There are two blockdevs involved below, /dev/md2 and /dev/md3.  md3 is a 
> > raid1
> > array with two partitions with an ext4 partition on it.  md2 is similar but
> > it's dm-crypted and ext4 is on top of that.
> >
> ...
> 
> > ===117547===
> > PID TTY  STAT   TIME COMMAND
> >  117547 ?D  5:12 [kworker/u16:8+flush-9:3]
> > [<0>] blk_mq_get_tag+0x11e/0x2b0
> > [<0>] __blk_mq_alloc_requests+0x1bc/0x350
> > [<0>] blk_mq_submit_bio+0x2c7/0x680
> > [<0>] __submit_bio+0x8b/0x170
> > [<0>] submit_bio_noacct_nocheck+0x159/0x370
> > [<0>] __block_write_full_folio+0x1e1/0x400
> > [<0>] writepage_cb+0x1a/0x70
> > [<0>] write_cache_pages+0x144/0x3b0
> > [<0>] do_writepages+0x164/0x1e0
> > [<0>] __writeback_single_inode+0x3d/0x360
> > [<0>] writeback_sb_inodes+0x1ed/0x4b0
> > [<0>] __writeback_inodes_wb+0x4c/0xf0
> > [<0>] wb_writeback+0x298/0x310
> > [<0>] wb_workfn+0x35b/0x510
> > [<0>] process_one_work+0x1de/0x3f0
> > [<0>] worker_thread+0x51/0x390
> > [<0>] kthread+0xe5/0x120
> > [<0>] ret_from_fork+0x31/0x50
> > [<0>] ret_from_fork_asm+0x1b/0x30
> 
> BTW, -rc3 fixes one similar issue on the above code path, so please try -rc3.
> 
> 106397376c03 sbitmap: fix batching wakeup

That patch really needs a Fixes:, please.  And consideration for a
-stable backport.

Looking at what has changed recently in sbitmap, it seems unlikely that
106397376c03 fixes an issue that just appeared in 6.5-rcX.  But maybe
the issue you have identified has recently become easier to hit; we'll
see.

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

Re: [PATCH] lib/memweight.c: optimize by inlining bitmap_weight()

[Andrew Morton]

On Wed, 21 Aug 2019 10:42:00 +0300 Denis Efremov  wrote:

> This patch inlines bitmap_weight() call.

It is better to say the patch "open codes" the bitmap_weight() call.

> Thus, removing the BUG_ON,

Why is that OK to do?

I expect all the code size improvements are from doing this?

> and 'longs to bits -> bits to longs' conversion by directly calling
> hweight_long().
> 
> ./scripts/bloat-o-meter lib/memweight.o.old lib/memweight.o.new
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-10 (-10)
> Function old new   delta
> memweight162 152 -10
> 

Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

[Andrew Morton]

On Mon, 18 Jun 2018 15:01:43 -0700 Dmitry Torokhov  
wrote:

> > > +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
> > > +{
> > > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), 
> > > flags);
> > > +}
> > > +EXPORT_SYMBOL(bitmap_alloc);
> > > +
> > > +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
> > > +{
> > > + return bitmap_alloc(nbits, flags | __GFP_ZERO);
> > > +}
> > > +EXPORT_SYMBOL(bitmap_zalloc);
> > > +
> > > +void bitmap_free(const unsigned long *bitmap)
> > > +{
> > > + kfree(bitmap);
> > > +}
> > > +EXPORT_SYMBOL(bitmap_free);
> > > +
> >
> > I suggest these functions are small and simple enough to justify
> > inlining them.
> >
> 
> We can't as we end up including bitmap.h (by the way of cpumask.h)
> form slab.h, so we gen circular dependency.

That info should have been in the changelog, and probably a code
comment.

> Maybe if we removed memcg
> stuff from slab.h so we do not need to include workqueue.h...

Or move the basic slab API stuff out of slab.h into a new header.  Or
create a new, standalone work_struct.h - that looks pretty simple.

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

Re: [dm-devel] [PATCH v3 3/5] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()

[Andrew Morton]

On Mon, 18 Jun 2018 16:10:01 +0300 Andy Shevchenko 
 wrote:

> A lot of code become ugly because of open coding allocations for bitmaps.
> 
> Introduce three helpers to allow users be more clear of intention
> and keep their code neat.
> 
> ...
>
> +unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)
> +{
> + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), 
> flags);
> +}
> +EXPORT_SYMBOL(bitmap_alloc);
> +
> +unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
> +{
> + return bitmap_alloc(nbits, flags | __GFP_ZERO);
> +}
> +EXPORT_SYMBOL(bitmap_zalloc);
> +
> +void bitmap_free(const unsigned long *bitmap)
> +{
> + kfree(bitmap);
> +}
> +EXPORT_SYMBOL(bitmap_free);
> +

I suggest these functions are small and simple enough to justify
inlining them.

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

Re: [dm-devel] [PATCH] kvmalloc: always use vmalloc if CONFIG_DEBUG_VM

[Andrew Morton]

On Tue, 24 Apr 2018 12:33:01 -0400 (EDT) Mikulas Patocka  
wrote:

> 
> 
> On Tue, 24 Apr 2018, Michal Hocko wrote:
> 
> > On Tue 24-04-18 11:30:40, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Tue, 24 Apr 2018, Michal Hocko wrote:
> > > 
> > > > On Mon 23-04-18 20:25:15, Mikulas Patocka wrote:
> > > > 
> > > > > Fixing __vmalloc code 
> > > > > is easy and it doesn't require cooperation with maintainers.
> > > > 
> > > > But it is a hack against the intention of the scope api.
> > > 
> > > It is not!
> > 
> > This discussion simply doesn't make much sense it seems. The scope API
> > is to document the scope of the reclaim recursion critical section. That
> > certainly is not a utility function like vmalloc.
> 
> That 15-line __vmalloc bugfix doesn't prevent you (or any other kernel 
> developer) from converting the code to the scope API. You make nonsensical 
> excuses.
> 

Fun thread!

Winding back to the original problem, I'd state it as

- Caller uses kvmalloc() but passes the address into vmalloc-naive
  DMA API and

- Caller uses kvmalloc() but passes the address into kfree()

Yes?

If so, then...

Is there a way in which, in the kvmalloc-called-kmalloc path, we can
tag the slab-allocated memory with a "this memory was allocated with
kvmalloc()" flag?  I *think* there's extra per-object storage available
with suitable slab/slub debugging options?  Perhaps we could steal one
bit from the redzone, dunno.

If so then we can

a) set that flag in kvmalloc() if the kmalloc() call succeeded

b) check for that flag in the DMA code, WARN if it is set.

c) in kvfree(), clear that flag before calling kfree()

d) in kfree(), check for that flag and go WARN() if set.

So both potential bugs are detected all the time, dependent upon
CONFIG_SLUB_DEBUG (and perhaps other slub config options).

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

Re: [dm-devel] [PATCH] kvmalloc: always use vmalloc if CONFIG_DEBUG_VM

[Andrew Morton]

On Thu, 19 Apr 2018 17:19:20 -0400 (EDT) Mikulas Patocka  
wrote:

> > > In order to detect these bugs reliably I submit this patch that changes
> > > kvmalloc to always use vmalloc if CONFIG_DEBUG_VM is turned on.
> > > 
> > > ...
> > >
> > > --- linux-2.6.orig/mm/util.c  2018-04-18 15:46:23.0 +0200
> > > +++ linux-2.6/mm/util.c   2018-04-18 16:00:43.0 +0200
> > > @@ -395,6 +395,7 @@ EXPORT_SYMBOL(vm_mmap);
> > >   */
> > >  void *kvmalloc_node(size_t size, gfp_t flags, int node)
> > >  {
> > > +#ifndef CONFIG_DEBUG_VM
> > >   gfp_t kmalloc_flags = flags;
> > >   void *ret;
> > >  
> > > @@ -426,6 +427,7 @@ void *kvmalloc_node(size_t size, gfp_t f
> > >*/
> > >   if (ret || size <= PAGE_SIZE)
> > >   return ret;
> > > +#endif
> > >  
> > >   return __vmalloc_node_flags_caller(size, node, flags,
> > >   __builtin_return_address(0));
> > 
> > Well, it doesn't have to be done at compile-time, does it?  We could
> > add a knob (in debugfs, presumably) which enables this at runtime. 
> > That's far more user-friendly.
> 
> But who will turn it on in debugfs?

But who will turn it on in Kconfig?  Just a handful of developers.  We
could add SONFIG_DEBUG_SG to the list in
Documentation/process/submit-checklist.rst, but nobody reads that.

Also, a whole bunch of defconfigs set CONFIG_DEBUG_SG=y and some
googling indicates that they aren't the only ones...

> It should be default for debugging 
> kernels, so that users using them would report the error.

Well.  This isn't the first time we've wanted to enable expensive (or
noisy) debugging things in debug kernels, by any means.

So how could we define a debug kernel in which it's OK to enable such
things?

- Could be "it's an -rc kernel".  But then we'd be enabling a bunch of
  untested code when Linus cuts a release.

- Could be "it's an -rc kernel with SUBLEVEL <= 5".  But then we risk
  unexpected things happening when Linux cuts -rc6, which still isn't
  good.

- How about "it's an -rc kernel with odd-numbered SUBLEVEL and
  SUBLEVEL <= 5".  That way everybody who runs -rc1, -rc3 and -rc5 will
  have kvmalloc debugging enabled.  That's potentially nasty because
  vmalloc is much slower than kmalloc.  But kvmalloc() is only used for
  large and probably infrequent allocations, so it's probably OK.

I wonder how we get at SUBLEVEL from within .c.  

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

Re: [dm-devel] [PATCH] kvmalloc: always use vmalloc if CONFIG_DEBUG_VM

[Andrew Morton]

On Thu, 19 Apr 2018 12:12:38 -0400 (EDT) Mikulas Patocka  
wrote:

> The kvmalloc function tries to use kmalloc and falls back to vmalloc if
> kmalloc fails.
> 
> Unfortunatelly, some kernel code has bugs - it uses kvmalloc and then
> uses DMA-API on the returned memory or frees it with kfree. Such bugs were
> found in the virtio-net driver, dm-integrity or RHEL7 powerpc-specific
> code.
> 
> These bugs are hard to reproduce because vmalloc falls back to kmalloc
> only if memory is fragmented.

Yes, that's nasty.

> In order to detect these bugs reliably I submit this patch that changes
> kvmalloc to always use vmalloc if CONFIG_DEBUG_VM is turned on.
> 
> ...
>
> --- linux-2.6.orig/mm/util.c  2018-04-18 15:46:23.0 +0200
> +++ linux-2.6/mm/util.c   2018-04-18 16:00:43.0 +0200
> @@ -395,6 +395,7 @@ EXPORT_SYMBOL(vm_mmap);
>   */
>  void *kvmalloc_node(size_t size, gfp_t flags, int node)
>  {
> +#ifndef CONFIG_DEBUG_VM
>   gfp_t kmalloc_flags = flags;
>   void *ret;
>  
> @@ -426,6 +427,7 @@ void *kvmalloc_node(size_t size, gfp_t f
>*/
>   if (ret || size <= PAGE_SIZE)
>   return ret;
> +#endif
>  
>   return __vmalloc_node_flags_caller(size, node, flags,
>   __builtin_return_address(0));

Well, it doesn't have to be done at compile-time, does it?  We could
add a knob (in debugfs, presumably) which enables this at runtime. 
That's far more user-friendly.

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

Re: [dm-devel] [RFC PATCH] mm: introduce kv[mz]alloc helpers

[Andrew Morton]

On Tue, 20 Dec 2016 09:38:22 -0800 Joe Perches  wrote:

> > So what are we going to do about this patch?
> 
> Well if Andrew doesn't object again, it should probably be applied.
> Unless his silence here acts like a pocket-veto.
> 
> Andrew?  Anything to add?

I guess we should give in to reality and do this, or something like it.
But Al said he was going to dig out some patches for us to consider?


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