should bdi_init() initialize congested_fn?

should bdi_init() initialize congested_fn?

[Bernd Schubert]

Hello,

while testing FhGFS I got a stack trace that seems to point out that 
bdi_congested() tries to call an uninitialized bdi->congested_fn().

So I checked where ->congested_fn() is initialized - with exception of 
btrfs I do not find a file system that does that at all. But then 
bdi_init(), which is called by most file systems for initialization also 
does *not* initialize it. So either I'm missing something, or almost all 
bdi users have a bug?


Thanks,
Bernd

> [  351.110903] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[...]
> [  351.112022] Call Trace:
> [  351.112022]  [<ffffffff8139a86d>] ? _raw_spin_lock_irq+0x1d/0x60
> [  351.112022]  [<ffffffff8139b0f0>] ? _raw_spin_unlock_irq+0x30/0x50
> [  351.112022]  [<ffffffff81112af3>] shrink_inactive_list+0x193/0x430
> [  351.112022]  [<ffffffff8110871a>] ? determine_dirtyable_memory+0x1a/0x30
> [  351.112022]  [<ffffffff811131fe>] shrink_zone+0x46e/0x5e0
> [  351.112022]  [<ffffffff81114689>] try_to_free_pages+0x1b9/0x560
> [  351.112022]  [<ffffffff8114c395>] ? __slab_alloc+0x405/0x5c0
> [  351.112022]  [<ffffffff811075af>] __alloc_pages_nodemask+0x4af/0x860
> [  351.112022]  [<ffffffff8114c8a5>] ? __kmalloc_node+0x145/0x210
> [  351.112022]  [<ffffffff81140933>] alloc_pages_current+0x83/0x100
> [  351.112022]  [<ffffffff811336da>] __vmalloc_node_range+0x16a/0x210
[...]
> [  351.112022] RIP  [<ffffffff8111247f>] shrink_page_list+0x47f/0x960


> (gdb) l *(shrink_page_list+0x47f)
> 0xffffffff8111247f is in shrink_page_list (include/linux/backing-dev.h:266).
> 261     int writeback_in_progress(struct backing_dev_info *bdi);
> 262
> 263     static inline int bdi_congested(struct backing_dev_info *bdi, int bdi_bits)
> 264     {
> 265             if (bdi->congested_fn)
> 266                     return bdi->congested_fn(bdi->congested_data, bdi_bits);
> 267             return (bdi->state & bdi_bits);
> 268     }
> 269

Re: should bdi_init() initialize congested_fn?

[Dave Chinner]

On Thu, Oct 13, 2011 at 05:12:29PM +0200, Bernd Schubert wrote:
> Hello,
> 
> while testing FhGFS I got a stack trace that seems to point out that
> bdi_congested() tries to call an uninitialized bdi->congested_fn().

Right. bdi_init() assumes you've zero'd the structure before
calling - it doesn't initialise any fields have a zero value.

> So I checked where ->congested_fn() is initialized - with exception
> of btrfs I do not find a file system that does that at all.

the bdi congestion function is filled out by the underlying block
device when the block device is initialised. Most filesystems get
their bdi from the underlying device when they are mounted. See
mount_bdev/set_bdev_super.

btrfs is a special case. You'd do best to ignore the games btrfs
plays while trying to understand how the generic infrastructure
works. ;)

> But then
> bdi_init(), which is called by most file systems for initialization
> also does *not* initialize it. So either I'm missing something, or
> almost all bdi users have a bug?

bdi_init() is only used by filesystems that done not operate
directly on a block device with that a bdi can be taken from. e.g.
filesystems without a backing store (ramfs, /proc, /sys, etc)Da, or
have special needs (e.g. ubifs). They are all ensuring that the
unused fields of the bdi are zero before calling bdi_init().

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com