Btrfs devel - Nov 2011 - [PATCH] Btrfs: fix num_start_workers count if we fail to make an alloc

Al pointed out that if we fail to start a worker for whatever reason (ENOMEM
basically), we could leak our count for num_start_workers, and so we''d
think we
had more workers than we actually do.  This could cause us to shrink workers
when we shouldn''t or not start workers when we should.  So check the
return
value and if we failed fix num_start_workers and fallback.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/btrfs/async-thread.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
index 7ec1409..09ef1b0 100644
--- a/fs/btrfs/async-thread.c
+++ b/fs/btrfs/async-thread.c
@@ -568,6 +568,7 @@ static struct btrfs_worker_thread *find_worker(struct
btrfs_workers *workers)
 	struct btrfs_worker_thread *worker;
 	unsigned long flags;
 	struct list_head *fallback;
+	int ret;
 
 again:
 	spin_lock_irqsave(&workers->lock, flags);
@@ -584,7 +585,13 @@ again:
 			workers->num_workers_starting++;
 			spin_unlock_irqrestore(&workers->lock, flags);
 			/* we''re below the limit, start another worker */
-			__btrfs_start_workers(workers, 1);
+			ret = __btrfs_start_workers(workers, 1);
+			if (ret) {
+				spin_lock_irqsave(&workers->lock, flags);
+				workers->num_workers_starting--;
+				spin_unlock_irqrestore(&workers->lock, flags);
+				goto fallback;
+			}
 			goto again;
 		}
 	}
-- 
1.7.5.2

--
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

On Fri, Nov 18, 2011 at 02:38:54PM -0500, Josef Bacik
wrote:
> Al pointed out that if we fail to start a worker for whatever reason
(ENOMEM
> basically), we could leak our count for num_start_workers, and so
we''d think we
> had more workers than we actually do.  This could cause us to shrink
workers
> when we shouldn''t or not start workers when we should.  So check
the return
> value and if we failed fix num_start_workers and fallback.  Thanks,
It''s actually uglier than that; consider check_pending_workers_create()
where we
	* bump the num_start_workers
	* call start_new_worker(), which can fail, and then we have the same
leak; if it doesn''t fail, it schedules a call of
start_new_worker_func()
	* when start_new_worker_func() runs, it does btrfs_start_workers(),
which can run into the same leak again (this time on another pool - one
we have as ->async_helper).

Worse, __btrfs_start_workers() does btrfs_stop_workers() on failure.  That,
to put it mildly, is using excessive force.  As far as I can see, it''s
_never_ the right thing to do - __btrfs_start_workers() is always getting
1 as the second argument, so even calls from mount path don''t need that
kind of "kill ones we''d already created if we fail halfway
through".  It
used to make sense when they had all been started at mount time, but now
it''s useless in the best case (mount) and destructive elsewhere (when
pool had already been non-empty).

So I''d suggest killing that call of btrfs_stop_workers() completely,
losing
the num_workers argument (along with a loop in __btrfs_start_workers())
and looking into check_pending_workers_create() path.

Probably I''d put decrement on ->num_workers_starting into failure
exits of
__btrfs_start_workers() and start_new_worker(), but... can btrfs_queue_worker()
ever return non-zero?  AFAICS it can''t and we could just merge
start_new_worker() into check_pending_workers() and pull allocation before
incrementing the ->num_workers_starting...
--
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

On Fri, Nov 18, 2011 at 08:20:56PM +0000, Al Viro wrote:
> On Fri, Nov 18, 2011 at 02:38:54PM -0500, Josef Bacik wrote:
> > Al pointed out that if we fail to start a worker for whatever reason
(ENOMEM
> > basically), we could leak our count for num_start_workers, and so
we''d think we
> > had more workers than we actually do.  This could cause us to shrink
workers
> > when we shouldn''t or not start workers when we should.  So
check the return
> > value and if we failed fix num_start_workers and fallback.  Thanks,
> 
> It''s actually uglier than that; consider
check_pending_workers_create()
> where we
> 	* bump the num_start_workers
> 	* call start_new_worker(), which can fail, and then we have the same
> leak; if it doesn''t fail, it schedules a call of
start_new_worker_func()
> 	* when start_new_worker_func() runs, it does btrfs_start_workers(),
> which can run into the same leak again (this time on another pool - one
> we have as ->async_helper).
Nuts...  AFAICS, we _always_ leak ->num_start_workers here (i.e. when
check_pending_workers_create() finds ->atomic_start_pending set).  In
effect, we bump it once in check_pending_workers_create() itself, then
another time (on the same pool) when start_new_worker_func() calls
btrfs_start_workers().  That one will be dropped when we manage to 
start the thread, but the first one won''t.

Shouldn''t we use __btrfs_start_workers() instead here?
--
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

On Sat, Nov 19, 2011 at 01:37:39AM +0000, Al Viro wrote:
> On Fri, Nov 18, 2011 at 08:20:56PM +0000, Al Viro wrote:
> > On Fri, Nov 18, 2011 at 02:38:54PM -0500, Josef Bacik wrote:
> > > Al pointed out that if we fail to start a worker for whatever
reason (ENOMEM
> > > basically), we could leak our count for num_start_workers, and so
we''d think we
> > > had more workers than we actually do.  This could cause us to
shrink workers
> > > when we shouldn''t or not start workers when we should. 
So check the return
> > > value and if we failed fix num_start_workers and fallback. 
Thanks,
> > 
> > It''s actually uglier than that; consider
check_pending_workers_create()
> > where we
> > 	* bump the num_start_workers
> > 	* call start_new_worker(), which can fail, and then we have the same
> > leak; if it doesn''t fail, it schedules a call of
start_new_worker_func()
> > 	* when start_new_worker_func() runs, it does btrfs_start_workers(),
> > which can run into the same leak again (this time on another pool -
one
> > we have as ->async_helper).
> 
> Nuts...  AFAICS, we _always_ leak ->num_start_workers here (i.e. when
> check_pending_workers_create() finds ->atomic_start_pending set).  In
> effect, we bump it once in check_pending_workers_create() itself, then
> another time (on the same pool) when start_new_worker_func() calls
> btrfs_start_workers().  That one will be dropped when we manage to 
> start the thread, but the first one won''t.
> 
> Shouldn''t we use __btrfs_start_workers() instead here?
OK, tentative fixes for that stuff are pushed into #btrfs in vfs.git; comments
would be welcome...
--
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