From: Namhyung Kim <namhyung@gmail.com>
To: NeilBrown <neilb@suse.de>
Cc: linux-raid@vger.kernel.org
Subject: Re: [md PATCH 08/34] md/raid5: replace sh->lock with an 'active' flag.
Date: Fri, 22 Jul 2011 13:27:36 +0900 [thread overview]
Message-ID: <87hb6fdjnr.fsf@gmail.com> (raw)
In-Reply-To: <20110721023225.6728.96772.stgit@notabene.brown> (NeilBrown's message of "Thu, 21 Jul 2011 12:32:25 +1000")
NeilBrown <neilb@suse.de> writes:
> sh->lock is now mainly used to ensure that two threads aren't running
> in the locked part of handle_stripe[56] at the same time.
>
> That can more neatly be achieved with an 'active' flag which we set
> while running handle_stripe. If we find the flag is set, we simply
> requeue the stripe for later by setting STRIPE_HANDLE.
>
> For safety we take ->device_lock while examining the state of the
> stripe and creating a summary in 'stripe_head_state / r6_state'.
> This possibly isn't needed but as shared fields like ->toread,
> ->towrite are checked it is safer for now at least.
>
> We leave the label after the old 'unlock' called "unlock" because it
> will disappear in a few patches, so renaming seems pointless.
>
> This leaves the stripe 'locked' for longer as we clear STRIPE_ACTIVE
> later, but that is not a problem.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
Reviewed-by: Namhyung Kim <namhyung@gmail.com>
But I have a question, please see below.
> ---
>
> drivers/md/raid5.c | 26 +++++++++++++-------------
> drivers/md/raid5.h | 2 +-
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 9985138..f8275b5 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -1020,14 +1020,12 @@ ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
> if (test_and_clear_bit(R5_Wantdrain, &dev->flags)) {
> struct bio *wbi;
>
> - spin_lock(&sh->lock);
> spin_lock_irq(&sh->raid_conf->device_lock);
> chosen = dev->towrite;
> dev->towrite = NULL;
> BUG_ON(dev->written);
> wbi = dev->written = chosen;
> spin_unlock_irq(&sh->raid_conf->device_lock);
> - spin_unlock(&sh->lock);
>
> while (wbi && wbi->bi_sector <
> dev->sector + STRIPE_SECTORS) {
> @@ -1322,7 +1320,6 @@ static int grow_one_stripe(raid5_conf_t *conf)
> return 0;
>
> sh->raid_conf = conf;
> - spin_lock_init(&sh->lock);
> #ifdef CONFIG_MULTICORE_RAID456
> init_waitqueue_head(&sh->ops.wait_for_ops);
> #endif
> @@ -1442,7 +1439,6 @@ static int resize_stripes(raid5_conf_t *conf, int newsize)
> break;
>
> nsh->raid_conf = conf;
> - spin_lock_init(&nsh->lock);
> #ifdef CONFIG_MULTICORE_RAID456
> init_waitqueue_head(&nsh->ops.wait_for_ops);
> #endif
> @@ -2148,7 +2144,6 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx, in
> (unsigned long long)sh->sector);
>
>
> - spin_lock(&sh->lock);
> spin_lock_irq(&conf->device_lock);
> if (forwrite) {
> bip = &sh->dev[dd_idx].towrite;
> @@ -2184,7 +2179,6 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx, in
> set_bit(R5_OVERWRITE, &sh->dev[dd_idx].flags);
> }
> spin_unlock_irq(&conf->device_lock);
> - spin_unlock(&sh->lock);
>
> pr_debug("added bi b#%llu to stripe s#%llu, disk %d.\n",
> (unsigned long long)(*bip)->bi_sector,
> @@ -2201,7 +2195,6 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx, in
> overlap:
> set_bit(R5_Overlap, &sh->dev[dd_idx].flags);
> spin_unlock_irq(&conf->device_lock);
> - spin_unlock(&sh->lock);
> return 0;
> }
>
> @@ -3023,12 +3016,10 @@ static void handle_stripe5(struct stripe_head *sh)
> atomic_read(&sh->count), sh->pd_idx, sh->check_state,
> sh->reconstruct_state);
>
> - spin_lock(&sh->lock);
> if (test_and_clear_bit(STRIPE_SYNC_REQUESTED, &sh->state)) {
> set_bit(STRIPE_SYNCING, &sh->state);
> clear_bit(STRIPE_INSYNC, &sh->state);
> }
> - clear_bit(STRIPE_HANDLE, &sh->state);
> clear_bit(STRIPE_DELAYED, &sh->state);
>
> s.syncing = test_bit(STRIPE_SYNCING, &sh->state);
> @@ -3037,6 +3028,7 @@ static void handle_stripe5(struct stripe_head *sh)
>
> /* Now to look around and see what can be done */
> rcu_read_lock();
> + spin_lock_irq(&conf->device_lock);
Do we still need rcu_read_lock()? AFAIK rcu_read_lock() only prevents
task from preemption but spin_lock does same thing as well.
I know it's been already there under sh->lock before this patch, and
it doesn't hurt anything, but I'm not sure it is really needed.
> for (i=disks; i--; ) {
> mdk_rdev_t *rdev;
>
> @@ -3099,6 +3091,7 @@ static void handle_stripe5(struct stripe_head *sh)
> s.failed_num = i;
> }
> }
> + spin_unlock_irq(&conf->device_lock);
> rcu_read_unlock();
>
> if (unlikely(blocked_rdev)) {
> @@ -3275,7 +3268,6 @@ static void handle_stripe5(struct stripe_head *sh)
> handle_stripe_expansion(conf, sh, NULL);
>
> unlock:
> - spin_unlock(&sh->lock);
>
> /* wait for this device to become unblocked */
> if (unlikely(blocked_rdev))
> @@ -3318,12 +3310,10 @@ static void handle_stripe6(struct stripe_head *sh)
> sh->check_state, sh->reconstruct_state);
> memset(&s, 0, sizeof(s));
>
> - spin_lock(&sh->lock);
> if (test_and_clear_bit(STRIPE_SYNC_REQUESTED, &sh->state)) {
> set_bit(STRIPE_SYNCING, &sh->state);
> clear_bit(STRIPE_INSYNC, &sh->state);
> }
> - clear_bit(STRIPE_HANDLE, &sh->state);
> clear_bit(STRIPE_DELAYED, &sh->state);
>
> s.syncing = test_bit(STRIPE_SYNCING, &sh->state);
> @@ -3332,6 +3322,7 @@ static void handle_stripe6(struct stripe_head *sh)
> /* Now to look around and see what can be done */
>
> rcu_read_lock();
> + spin_lock_irq(&conf->device_lock);
Same here.
> for (i=disks; i--; ) {
> mdk_rdev_t *rdev;
> dev = &sh->dev[i];
> @@ -3395,6 +3386,7 @@ static void handle_stripe6(struct stripe_head *sh)
> s.failed++;
> }
> }
> + spin_unlock_irq(&conf->device_lock);
> rcu_read_unlock();
>
> if (unlikely(blocked_rdev)) {
> @@ -3580,7 +3572,6 @@ static void handle_stripe6(struct stripe_head *sh)
> handle_stripe_expansion(conf, sh, &r6s);
>
> unlock:
> - spin_unlock(&sh->lock);
>
> /* wait for this device to become unblocked */
> if (unlikely(blocked_rdev))
> @@ -3608,10 +3599,19 @@ static void handle_stripe6(struct stripe_head *sh)
>
> static void handle_stripe(struct stripe_head *sh)
> {
> + clear_bit(STRIPE_HANDLE, &sh->state);
> + if (test_and_set_bit(STRIPE_ACTIVE, &sh->state)) {
> + /* already being handled, ensure it gets handled
> + * again when current action finishes */
> + set_bit(STRIPE_HANDLE, &sh->state);
> + return;
> + }
> +
> if (sh->raid_conf->level == 6)
> handle_stripe6(sh);
> else
> handle_stripe5(sh);
> + clear_bit(STRIPE_ACTIVE, &sh->state);
> }
>
> static void raid5_activate_delayed(raid5_conf_t *conf)
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index a330011..217a9d4 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -209,7 +209,6 @@ struct stripe_head {
> short ddf_layout;/* use DDF ordering to calculate Q */
> unsigned long state; /* state flags */
> atomic_t count; /* nr of active thread/requests */
> - spinlock_t lock;
> int bm_seq; /* sequence number for bitmap flushes */
> int disks; /* disks in stripe */
> enum check_states check_state;
> @@ -290,6 +289,7 @@ struct r6_state {
> * Stripe state
> */
> enum {
> + STRIPE_ACTIVE,
> STRIPE_HANDLE,
> STRIPE_SYNC_REQUESTED,
> STRIPE_SYNCING,
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2011-07-22 4:27 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-21 2:32 [md PATCH 00/34] md patches for 3.1 - part 1 NeilBrown
2011-07-21 2:32 ` [md PATCH 02/34] md/raid10: factor out common bio handling code NeilBrown
2011-07-21 2:32 ` [md PATCH 01/34] md/raid10: get rid of duplicated conditional expression NeilBrown
2011-07-21 2:32 ` [md PATCH 03/34] md/raid10: share pages between read and write bio's during recovery NeilBrown
2011-07-21 2:32 ` [md PATCH 09/34] md/raid5: move common code into handle_stripe NeilBrown
2011-07-22 4:30 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 11/34] md/raid5: add some more fields to stripe_head_state NeilBrown
2011-07-22 5:31 ` Namhyung Kim
2011-07-26 1:35 ` NeilBrown
2011-07-21 2:32 ` [md PATCH 04/34] md/raid5: use kmem_cache_zalloc() NeilBrown
2011-07-21 2:32 ` [md PATCH 07/34] md/raid5: Protect some more code with ->device_lock NeilBrown
2011-07-22 3:54 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 06/34] md/raid5: Remove use of sh->lock in sync_request NeilBrown
2011-07-22 3:39 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 05/34] md/raid5: get rid of duplicated call to bio_data_dir() NeilBrown
2011-07-21 2:32 ` [md PATCH 10/34] md/raid5: unify stripe_head_state and r6_state NeilBrown
2011-07-22 4:49 ` Namhyung Kim
2011-07-22 5:15 ` NeilBrown
2011-07-22 5:37 ` NeilBrown
2011-07-22 5:53 ` Namhyung Kim
2011-07-26 6:44 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 08/34] md/raid5: replace sh->lock with an 'active' flag NeilBrown
2011-07-22 4:27 ` Namhyung Kim [this message]
2011-07-22 4:49 ` NeilBrown
2011-07-22 5:03 ` Namhyung Kim
2011-08-03 22:47 ` Dan Williams
2011-08-03 23:35 ` NeilBrown
2011-08-03 23:45 ` Williams, Dan J
2011-08-04 0:18 ` NeilBrown
2011-07-21 2:32 ` [md PATCH 14/34] md/raid5: move more code into common handle_stripe NeilBrown
2011-07-22 7:32 ` Namhyung Kim
2011-07-26 1:48 ` NeilBrown
2011-07-21 2:32 ` [md PATCH 13/34] md/raid5: Move code for finishing a reconstruction into handle_stripe NeilBrown
2011-07-22 7:09 ` Namhyung Kim
2011-07-26 1:44 ` NeilBrown
2011-07-21 2:32 ` [md PATCH 17/34] md/raid5: unite handle_stripe_dirtying5 and handle_stripe_dirtying6 NeilBrown
2011-07-22 9:10 ` Namhyung Kim
2011-07-26 1:52 ` NeilBrown
2011-07-26 2:41 ` H. Peter Anvin
2011-07-26 9:40 ` David Brown
2011-07-26 13:23 ` Namhyung Kim
2011-07-26 15:01 ` David Brown
2011-07-21 2:32 ` [md PATCH 12/34] md/raid5: move stripe_head_state and more code into handle_stripe NeilBrown
2011-07-22 5:41 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 15/34] md/raid5: rearrange a test in fetch_block6 NeilBrown
2011-07-22 7:37 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 18/34] md/raid5: move more common code into handle_stripe NeilBrown
2011-07-22 9:20 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 19/34] md/raid5: move some " NeilBrown
2011-07-22 9:29 ` Namhyung Kim
2011-07-26 1:59 ` NeilBrown
2011-07-21 2:32 ` [md PATCH 16/34] md/raid5: unite fetch_block5 and fetch_block6 NeilBrown
2011-07-22 8:24 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 23/34] md: introduce link/unlink_rdev() helpers NeilBrown
2011-07-21 2:32 ` [md PATCH 26/34] md/raid10: Make use of new recovery_disabled handling NeilBrown
2011-07-21 2:32 ` [md PATCH 24/34] md: remove ro check in md_check_recovery() NeilBrown
2011-07-21 2:32 ` [md PATCH 21/34] md: use proper little-endian bitops NeilBrown
2011-07-21 2:32 ` [md PATCH 20/34] md/raid5: finalise new merged handle_stripe NeilBrown
2011-07-22 9:36 ` Namhyung Kim
2011-07-26 2:02 ` NeilBrown
2011-07-26 4:50 ` Namhyung Kim
2011-07-21 2:32 ` [md PATCH 27/34] md/raid10: Improve decision on whether to fail a device with a read error NeilBrown
2011-07-21 2:32 ` [md PATCH 22/34] md/raid: use printk_ratelimited instead of printk_ratelimit NeilBrown
2011-07-21 2:32 ` [md PATCH 25/34] md: change managed of recovery_disabled NeilBrown
2011-07-21 2:32 ` [md PATCH 33/34] MD: raid1 s/sysfs_notify_dirent/sysfs_notify_dirent_safe NeilBrown
2011-07-21 2:32 ` [md PATCH 28/34] md: get rid of unnecessary casts on page_address() NeilBrown
2011-07-21 2:32 ` [md PATCH 34/34] MD bitmap: Revert DM dirty log hooks NeilBrown
2011-07-21 2:32 ` [md PATCH 30/34] md/raid5: move rdev->corrected_errors counting NeilBrown
2011-07-21 2:32 ` [md PATCH 29/34] md/raid1: " NeilBrown
2011-07-21 2:32 ` [md PATCH 31/34] md/raid10: " NeilBrown
2011-07-21 2:32 ` [md PATCH 32/34] md/raid5: Avoid BUG caused by multiple failures NeilBrown
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=87hb6fdjnr.fsf@gmail.com \
--to=namhyung@gmail.com \
--cc=linux-raid@vger.kernel.org \
--cc=neilb@suse.de \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.