[bug report] writeback: Avoid contention on wb->list_lock when switching inodes

[bug report] writeback: Avoid contention on wb->list_lock when switching inodes

[Dan Carpenter]

Hello Jan Kara,

Commit 67c312b4e9bf ("writeback: Avoid contention on wb->list_lock
when switching inodes") from Sep 12, 2025 (linux-next), leads to the
following Smatch static checker warning:

	fs/fs-writeback.c:730 cleanup_offline_cgwb()
	error: uninitialized symbol 'new_wb'.

fs/fs-writeback.c
    709 bool cleanup_offline_cgwb(struct bdi_writeback *wb)
    710 {
    711         struct cgroup_subsys_state *memcg_css;
    712         struct inode_switch_wbs_context *isw;
    713         struct bdi_writeback *new_wb;
    714         int nr;
    715         bool restart = false;
    716 
    717         isw = kzalloc(struct_size(isw, inodes, WB_MAX_INODES_PER_ISW),
    718                       GFP_KERNEL);
    719         if (!isw)
    720                 return restart;
    721 
    722         atomic_inc(&isw_nr_in_flight);
    723 
    724         for (memcg_css = wb->memcg_css->parent; memcg_css;
    725              memcg_css = memcg_css->parent) {

The concern here is that do we know for sure that we enter the loop?

    726                 new_wb = wb_get_create(wb->bdi, memcg_css, GFP_KERNEL);
    727                 if (new_wb)
    728                         break;
    729         }
--> 730         if (unlikely(!new_wb))
                              ^^^^^^
These are a common source of false positives, but I just wanted to be
sure.  Thanks!

    731                 new_wb = &wb->bdi->wb; /* wb_get() is noop for bdi's wb */
    732 
    733         nr = 0;

regards,
dan carpenter

Re: [bug report] writeback: Avoid contention on wb->list_lock when switching inodes

[Jan Kara]

Hello Dan!

On Thu 18-09-25 12:52:54, Dan Carpenter wrote:
> Commit 67c312b4e9bf ("writeback: Avoid contention on wb->list_lock
> when switching inodes") from Sep 12, 2025 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	fs/fs-writeback.c:730 cleanup_offline_cgwb()
> 	error: uninitialized symbol 'new_wb'.
> 
> fs/fs-writeback.c
>     709 bool cleanup_offline_cgwb(struct bdi_writeback *wb)
>     710 {
>     711         struct cgroup_subsys_state *memcg_css;
>     712         struct inode_switch_wbs_context *isw;
>     713         struct bdi_writeback *new_wb;
>     714         int nr;
>     715         bool restart = false;
>     716 
>     717         isw = kzalloc(struct_size(isw, inodes, WB_MAX_INODES_PER_ISW),
>     718                       GFP_KERNEL);
>     719         if (!isw)
>     720                 return restart;
>     721 
>     722         atomic_inc(&isw_nr_in_flight);
>     723 
>     724         for (memcg_css = wb->memcg_css->parent; memcg_css;
>     725              memcg_css = memcg_css->parent) {
> 
> The concern here is that do we know for sure that we enter the loop?

Yes. The root memcg never gets offlined and all the others have non-zero
parent.

> 
>     726                 new_wb = wb_get_create(wb->bdi, memcg_css, GFP_KERNEL);
>     727                 if (new_wb)
>     728                         break;
>     729         }
> --> 730         if (unlikely(!new_wb))
>                               ^^^^^^
> These are a common source of false positives, but I just wanted to be
> sure.  Thanks!

But initializing new_wb to NULL is still a reasonable defensive
programming. I'll send a fixup.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR