linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Artem Bityutskiy <dedekind1@gmail.com>
To: Jens Axboe <axboe@kernel.dk>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [RFC][PATCH 04/16] writeback: fix possible race when shutting down bdi
Date: Fri, 16 Jul 2010 15:45:00 +0300	[thread overview]
Message-ID: <1279284312-2411-5-git-send-email-dedekind1@gmail.com> (raw)
In-Reply-To: <1279284312-2411-1-git-send-email-dedekind1@gmail.com>

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Current bdi code has the following race between 'bdi_wb_shutdown()'
and 'bdi_forker_thread()'.

Initial condition: BDI_pending is cleaned, bdi has no writeback thread,
because it was inactive and exited, 'bdi_wb_shutdown()' and
'bdi_forker_thread()' are executed concurrently.

1. bdi_wb_shutdown() executes  wait_on_bit(), tests the BDI_pending bit,
   it is clean, so it does not wait for anything.

2. 'bdi_forker_thread()' takes the 'bdi_lock', finds out that bdi has
   work to do, takes it out of the 'bdi_list', sets the BDI_pending flag,
   unlocks the 'bdi_lock' lock

3. 'bdi_wb_shutdown()' takes the lock, and nasty things start happening:
   a) it removes the bdi from bdi->bdi_list, but the bdi is not in any
      list
   b) it starts deleting the bdi, but 'bdi_forker_thread()' is still working
      with it.

Note, it is very difficult to hit this race, and I never observed it, so it
is quite theoretical, but it is still a race. Also note, this race exist
without my previous clean-ups as well.

This patch fixes this race by making 'bdi_wb_shutdown()' first search for
the bdi in the 'bdi_list', and only if it is there, remove it from 'bdi_list'
and destroy. But if it is not there, assume it is in transit and re-try
waiting on the BDI_pending bit.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 mm/backing-dev.c |   41 ++++++++++++++++++++++++++++-------------
 1 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index b34c12a..a445ff0 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -456,15 +456,26 @@ void static bdi_add_default_flusher_thread(struct backing_dev_info *bdi)
 }
 
 /*
- * Remove bdi from bdi_list, and ensure that it is no longer visible
+ * Look up for bdi in the bdi_list. If found, remove it, ensure that it is
+ * no longer visible, and return 0. If not found, return 1.
  */
-static void bdi_remove_from_list(struct backing_dev_info *bdi)
+static int bdi_remove_from_list(struct backing_dev_info *me)
 {
+	struct backing_dev_info *bdi;
+
 	spin_lock_bh(&bdi_lock);
-	list_del_rcu(&bdi->bdi_list);
+	list_for_each_entry(bdi, &bdi_list, bdi_list) {
+		if (bdi == me) {
+			list_del_rcu(&me->bdi_list);
+			spin_unlock_bh(&bdi_lock);
+			synchronize_rcu();
+			return 0;
+		}
+
+	}
 	spin_unlock_bh(&bdi_lock);
+	return 1;
 
-	synchronize_rcu();
 }
 
 int bdi_register(struct backing_dev_info *bdi, struct device *parent,
@@ -532,16 +543,20 @@ static void bdi_wb_shutdown(struct backing_dev_info *bdi)
 	if (!bdi_cap_writeback_dirty(bdi))
 		return;
 
-	/*
-	 * If setup is pending, wait for that to complete first
-	 */
-	wait_on_bit(&bdi->state, BDI_pending, bdi_sched_wait,
-			TASK_UNINTERRUPTIBLE);
+	do {
+		/*
+		 * If setup is pending, wait for that to complete first
+		 */
+		wait_on_bit(&bdi->state, BDI_pending, bdi_sched_wait,
+				TASK_UNINTERRUPTIBLE);
 
-	/*
-	 * Make sure nobody finds us on the bdi_list anymore
-	 */
-	bdi_remove_from_list(bdi);
+		/*
+		 * Make sure nobody finds us on the bdi_list anymore. However,
+		 * bdi may be temporary be not in the bdi_list but be in transit
+		 * in bdi_forker_thread. Namely, this may happen if we race
+		 * with the forker thread.
+		 */
+	} while (bdi_remove_from_list(bdi));
 
 	/*
 	 * Finally, kill the kernel thread. We don't need to be RCU
-- 
1.7.1.1


  parent reply	other threads:[~2010-07-16 12:49 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-07-16 12:44 [RFC][PATCH 00/16] kill unnecessary bdi wakeups + cleanups Artem Bityutskiy
2010-07-16 12:44 ` [RFC][PATCH 01/16] writeback: do not self-wakeup Artem Bityutskiy
2010-07-18  6:44   ` Christoph Hellwig
2010-07-18  9:43     ` Artem Bityutskiy
2010-07-16 12:44 ` [RFC][PATCH 02/16] writeback: remove redundant list initialization Artem Bityutskiy
2010-07-18  6:44   ` Christoph Hellwig
2010-07-16 12:44 ` [RFC][PATCH 03/16] writeback: harmonize writeback threads naming Artem Bityutskiy
2010-07-18  6:45   ` Christoph Hellwig
2010-07-16 12:45 ` Artem Bityutskiy [this message]
2010-07-18  6:47   ` [RFC][PATCH 04/16] writeback: fix possible race when shutting down bdi Christoph Hellwig
2010-07-20  8:58     ` Artem Bityutskiy
2010-07-16 12:45 ` [RFC][PATCH 05/16] writeback: fix possible race when creating bdi threads Artem Bityutskiy
2010-07-16 12:45 ` [RFC][PATCH 06/16] writeback: improve bdi_has_dirty_io Artem Bityutskiy
2010-07-18  6:49   ` Christoph Hellwig
2010-07-16 12:45 ` [RFC][PATCH 07/16] writeback: do not lose wake-ups in the forker thread Artem Bityutskiy
2010-07-18  6:49   ` Christoph Hellwig
2010-07-16 12:45 ` [RFC][PATCH 08/16] writeback: do not lose default bdi wake-ups Artem Bityutskiy
2010-07-18  6:52   ` Christoph Hellwig
2010-07-16 12:45 ` [RFC][PATCH 09/16] writeback: do not lose wake-ups in bdi threads Artem Bityutskiy
2010-07-18  6:52   ` Christoph Hellwig
2010-07-16 12:45 ` [RFC][PATCH 10/16] writeback: simplify bdi code a little Artem Bityutskiy
2010-07-18  6:56   ` Christoph Hellwig
2010-07-20 10:34     ` Artem Bityutskiy
2010-07-16 12:45 ` [RFC][PATCH 11/16] writeback: move last_active to bdi Artem Bityutskiy
2010-07-18  7:03   ` Christoph Hellwig
2010-07-16 12:45 ` [RFC][PATCH 12/16] writeback: add to bdi_list in the forker thread Artem Bityutskiy
2010-07-18  6:58   ` Christoph Hellwig
2010-07-20 11:07     ` Artem Bityutskiy
2010-07-20 11:32     ` Artem Bityutskiy
2010-07-16 12:45 ` [RFC][PATCH 13/16] writeback: restructure bdi forker loop a little Artem Bityutskiy
2010-07-16 12:45 ` [RFC][PATCH 14/16] writeback: move bdi threads exiting logic to the forker thread Artem Bityutskiy
2010-07-18  7:02   ` Christoph Hellwig
2010-07-20 12:23     ` Artem Bityutskiy
2010-07-20 12:54     ` Artem Bityutskiy
2010-07-16 12:45 ` [RFC][PATCH 15/16] writeback: clean-up the warning about non-registered bdi Artem Bityutskiy
2010-07-18  7:03   ` Christoph Hellwig
2010-07-16 12:45 ` [RFC][PATCH 16/16] writeback: prevent unnecessary bdi threads wakeups Artem Bityutskiy
2010-07-18  7:45   ` Christoph Hellwig
2010-07-20 13:13     ` Artem Bityutskiy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1279284312-2411-5-git-send-email-dedekind1@gmail.com \
    --to=dedekind1@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).