* re: Btrfs: fix num_workers_starting bug and other bugs in async thread
@ 2011-12-23 10:44 Dan Carpenter
2011-12-23 13:06 ` Chris Mason
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2011-12-23 10:44 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs
Hi Josef,
Smatch complains about this change introduces a double unlock.
fs/btrfs/async-thread.c +608 find_worker(49) error: double unlock 'spin_lock:&workers->lock'
579 spin_unlock_irqrestore(&workers->lock, flags);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We unlock here.
580 /* we're below the limit, start another worker */
581 ret = __btrfs_start_workers(workers);
582 if (ret)
583 goto fallback;
584 goto again;
585 }
586 }
587 goto found;
588
589 fallback:
590 fallback = NULL;
591 /*
592 * we have failed to find any workers, just
593 * return the first one we can find.
594 */
595 if (!list_empty(&workers->worker_list))
596 fallback = workers->worker_list.next;
597 if (!list_empty(&workers->idle_list))
598 fallback = workers->idle_list.next;
599 BUG_ON(!fallback);
600 worker = list_entry(fallback,
601 struct btrfs_worker_thread, worker_list);
602 found:
603 /*
604 * this makes sure the worker doesn't exit before it is placed
605 * onto a busy/idle list
606 */
607 atomic_inc(&worker->num_pending);
608 spin_unlock_irqrestore(&workers->lock, flags);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
And again here.
Btw, does find_worker() ever get called with IRQs disabled? If so then
__btrfs_start_workers() enables them. Maybe that function should use
spin_lock_irqsave() instead of spin_lock_irq().
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Btrfs: fix num_workers_starting bug and other bugs in async thread
2011-12-23 10:44 Btrfs: fix num_workers_starting bug and other bugs in async thread Dan Carpenter
@ 2011-12-23 13:06 ` Chris Mason
2011-12-23 13:21 ` Chris Mason
0 siblings, 1 reply; 3+ messages in thread
From: Chris Mason @ 2011-12-23 13:06 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Josef Bacik, linux-btrfs
On Fri, Dec 23, 2011 at 01:44:53PM +0300, Dan Carpenter wrote:
> Hi Josef,
>
> Smatch complains about this change introduces a double unlock.
>
> fs/btrfs/async-thread.c +608 find_worker(49) error: double unlock 'spin_lock:&workers->lock'
>
> 579 spin_unlock_irqrestore(&workers->lock, flags);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Thanks Dan, fixing.
> We unlock here.
>
> 580 /* we're below the limit, start another worker */
> 581 ret = __btrfs_start_workers(workers);
> 582 if (ret)
> 583 goto fallback;
> 584 goto again;
> 585 }
> 586 }
> 587 goto found;
> 588
> 589 fallback:
> 590 fallback = NULL;
> 591 /*
> 592 * we have failed to find any workers, just
> 593 * return the first one we can find.
> 594 */
> 595 if (!list_empty(&workers->worker_list))
> 596 fallback = workers->worker_list.next;
> 597 if (!list_empty(&workers->idle_list))
> 598 fallback = workers->idle_list.next;
> 599 BUG_ON(!fallback);
> 600 worker = list_entry(fallback,
> 601 struct btrfs_worker_thread, worker_list);
> 602 found:
> 603 /*
> 604 * this makes sure the worker doesn't exit before it is placed
> 605 * onto a busy/idle list
> 606 */
> 607 atomic_inc(&worker->num_pending);
> 608 spin_unlock_irqrestore(&workers->lock, flags);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> And again here.
>
> Btw, does find_worker() ever get called with IRQs disabled? If so then
> __btrfs_start_workers() enables them. Maybe that function should use
> spin_lock_irqsave() instead of spin_lock_irq().
Patching this too.
-chris
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Btrfs: fix num_workers_starting bug and other bugs in async thread
2011-12-23 13:06 ` Chris Mason
@ 2011-12-23 13:21 ` Chris Mason
0 siblings, 0 replies; 3+ messages in thread
From: Chris Mason @ 2011-12-23 13:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Josef Bacik, linux-btrfs
On Fri, Dec 23, 2011 at 08:06:03AM -0500, Chris Mason wrote:
> On Fri, Dec 23, 2011 at 01:44:53PM +0300, Dan Carpenter wrote:
> > Hi Josef,
> >
> > Smatch complains about this change introduces a double unlock.
> >
> > fs/btrfs/async-thread.c +608 find_worker(49) error: double unlock 'spin_lock:&workers->lock'
> >
> > 579 spin_unlock_irqrestore(&workers->lock, flags);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Thanks Dan, fixing.
> > 602 found:
> > 603 /*
> > 604 * this makes sure the worker doesn't exit before it is placed
> > 605 * onto a busy/idle list
> > 606 */
> > 607 atomic_inc(&worker->num_pending);
> > 608 spin_unlock_irqrestore(&workers->lock, flags);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > And again here.
> >
> > Btw, does find_worker() ever get called with IRQs disabled? If so then
> > __btrfs_start_workers() enables them. Maybe that function should use
> > spin_lock_irqsave() instead of spin_lock_irq().
>
> Patching this too.
Read that too quickly. __btrfs_start_workers() can't be called with
irqs off, kthread_run schedules.
-chris
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-12-23 13:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-23 10:44 Btrfs: fix num_workers_starting bug and other bugs in async thread Dan Carpenter
2011-12-23 13:06 ` Chris Mason
2011-12-23 13:21 ` Chris Mason
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).