From: Dan Williams <dan.j.williams@intel.com>
To: Shaohua Li <shli@kernel.org>
Cc: NeilBrown <neilb@suse.de>,
linux-raid@vger.kernel.org, axboe@kernel.dk, shli@fusionio.com
Subject: Re: [patch 1/8] raid5: add a per-stripe lock
Date: Tue, 12 Jun 2012 14:02:30 -0700 [thread overview]
Message-ID: <CAA9_cmet-h8NUNGHff6EV_JRo=jqGFb_Q+RQ2Ep4ZDH9eymVtQ@mail.gmail.com> (raw)
In-Reply-To: <20120607065224.GE779@kernel.org>
On Wed, Jun 6, 2012 at 11:52 PM, Shaohua Li <shli@kernel.org> wrote:
> On Thu, Jun 07, 2012 at 04:35:22PM +1000, NeilBrown wrote:
>> On Thu, 7 Jun 2012 14:29:39 +0800 Shaohua Li <shli@kernel.org> wrote:
>>
>> > On Thu, Jun 07, 2012 at 10:54:10AM +1000, NeilBrown wrote:
>> > > On Mon, 04 Jun 2012 16:01:53 +0800 Shaohua Li <shli@kernel.org> wrote:
>> > >
>> > > > Add a per-stripe lock to protect stripe specific data, like dev->read,
>> > > > written, ... The purpose is to reduce lock contention of conf->device_lock.
>> > >
>> > > I'm not convinced that you need to add a lock.
>> > > I am convinced that if you do add one you need to explain exactly what it is
>> > > protecting.
>> > >
>> > > The STRIPE_ACTIVE bit serves as a lock and ensures that only one process can
>> > > be in handle_stripe at a time.
>> > > So I don't think dev->read actually needs any protection (though I haven't
>> > > checked thoroughly).
>> > >
>> > > I think the only things that device_lock protects are things shared by
>> > > multiple stripes, so adding a per-stripe spinlock isn't going to help remove
>> > > device_lock.
>> >
>> > This sounds not true to me. both the async callbacks and request completion
>> > access stripe data, like dev->read. Such things are not protected by
>> > STRIPE_ACTIVE bit. Thought we can delete STRIPE_ACTIVE bit with stripe lock
>> > introduced.
>>
>> Please give specifics. What race do you see with access to dev->read that is
>> not protected by STRIPE_ACTIVE ?
>
> For example, ops_complete_biofill() will change dev->read which isn't protected
> by STRIPE_ACTIVE. add_stripe_bio() checks ->toread ->towrite, which isn't
> protected by the bit too. Am I missing anything?
STRIPE_ACTIVE is the replacement for the old per-stripe lock. That
lock never was meant/able to synchronize add_stripe_bio() vs ops_run_*
(producer vs consumer). That's always been device_lock's job because
an individual bio may be added to several stripes. If device_lock is
gone we need a different scheme. That's what tripped me up last time
I looked at this.
--
Dan
--
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:[~2012-06-12 21:02 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-04 8:01 [patch 0/8] raid5: improve write performance for fast storage Shaohua Li
2012-06-04 8:01 ` [patch 1/8] raid5: add a per-stripe lock Shaohua Li
2012-06-07 0:54 ` NeilBrown
2012-06-07 6:29 ` Shaohua Li
2012-06-07 6:35 ` NeilBrown
2012-06-07 6:52 ` Shaohua Li
2012-06-12 21:02 ` Dan Williams [this message]
2012-06-13 4:08 ` Dan Williams
2012-06-13 4:23 ` Shaohua Li
2012-06-12 21:10 ` Dan Williams
2012-06-04 8:01 ` [patch 2/8] raid5: lockless access raid5 overrided bi_phys_segments Shaohua Li
2012-06-07 1:06 ` NeilBrown
2012-06-12 20:41 ` Dan Williams
2012-06-04 8:01 ` [patch 3/8] raid5: remove some device_lock locking places Shaohua Li
2012-06-04 8:01 ` [patch 4/8] raid5: reduce chance release_stripe() taking device_lock Shaohua Li
2012-06-07 0:50 ` NeilBrown
2012-06-04 8:01 ` [patch 5/8] raid5: add batch stripe release Shaohua Li
2012-06-04 8:01 ` [patch 6/8] raid5: make_request use " Shaohua Li
2012-06-07 1:23 ` NeilBrown
2012-06-07 6:33 ` Shaohua Li
2012-06-07 7:33 ` NeilBrown
2012-06-07 7:58 ` Shaohua Li
2012-06-08 6:16 ` Shaohua Li
2012-06-08 6:42 ` NeilBrown
2012-06-04 8:01 ` [patch 7/8] raid5: raid5d handle stripe in batch way Shaohua Li
2012-06-07 1:32 ` NeilBrown
2012-06-07 6:35 ` Shaohua Li
2012-06-07 7:38 ` NeilBrown
2012-06-04 8:02 ` [patch 8/8] raid5: create multiple threads to handle stripes Shaohua Li
2012-06-07 1:39 ` NeilBrown
2012-06-07 6:45 ` Shaohua Li
2012-06-13 4:08 ` Dan Williams
2012-06-21 10:09 ` Shaohua Li
2012-07-02 20:43 ` Dan Williams
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='CAA9_cmet-h8NUNGHff6EV_JRo=jqGFb_Q+RQ2Ep4ZDH9eymVtQ@mail.gmail.com' \
--to=dan.j.williams@intel.com \
--cc=axboe@kernel.dk \
--cc=linux-raid@vger.kernel.org \
--cc=neilb@suse.de \
--cc=shli@fusionio.com \
--cc=shli@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 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.