Dan Carpenter
2011-Dec-23 10:44 UTC
re: Btrfs: fix num_workers_starting bug and other bugs in async thread
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Chris Mason
2011-Dec-23 13:06 UTC
Re: Btrfs: fix num_workers_starting bug and other bugs in async thread
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Chris Mason
2011-Dec-23 13:21 UTC
Re: Btrfs: fix num_workers_starting bug and other bugs in async thread
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html