* [patch]raid5: get_active_stripe avoids device_lock
@ 2014-04-08 4:05 Shaohua Li
2014-04-09 2:17 ` NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: Shaohua Li @ 2014-04-08 4:05 UTC (permalink / raw)
To: linux-raid, neilb
For sequential workload (or request size big workload), get_active_stripe can
find cached stripe. In this case, we always hold device_lock, which exposes a
lot of lock contention for such workload. If stripe count isn't 0, we don't
need hold the lock actually, since we just increase its count. And this is the
hot code path for such workload. Unfortunately we must delete the BUG_ON.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-04-08 09:16:39.377536607 +0800
+++ linux/drivers/md/raid5.c 2014-04-08 09:16:39.369536607 +0800
@@ -679,14 +679,9 @@ get_active_stripe(struct r5conf *conf, s
init_stripe(sh, sector, previous);
atomic_inc(&sh->count);
}
- } else {
+ } else if (!atomic_add_unless(&sh->count, 1, 0)) {
spin_lock(&conf->device_lock);
- if (atomic_read(&sh->count)) {
- BUG_ON(!list_empty(&sh->lru)
- && !test_bit(STRIPE_EXPANDING, &sh->state)
- && !test_bit(STRIPE_ON_UNPLUG_LIST, &sh->state)
- );
- } else {
+ if (!atomic_read(&sh->count)) {
if (!test_bit(STRIPE_HANDLE, &sh->state))
atomic_inc(&conf->active_stripes);
BUG_ON(list_empty(&sh->lru) &&
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch]raid5: get_active_stripe avoids device_lock
2014-04-08 4:05 [patch]raid5: get_active_stripe avoids device_lock Shaohua Li
@ 2014-04-09 2:17 ` NeilBrown
2014-04-09 3:27 ` Shaohua Li
0 siblings, 1 reply; 4+ messages in thread
From: NeilBrown @ 2014-04-09 2:17 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]
On Tue, 8 Apr 2014 12:05:53 +0800 Shaohua Li <shli@kernel.org> wrote:
>
> For sequential workload (or request size big workload), get_active_stripe can
> find cached stripe. In this case, we always hold device_lock, which exposes a
> lot of lock contention for such workload. If stripe count isn't 0, we don't
> need hold the lock actually, since we just increase its count. And this is the
> hot code path for such workload. Unfortunately we must delete the BUG_ON.
>
> Signed-off-by: Shaohua Li <shli@fusionio.com>
> ---
> drivers/md/raid5.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> Index: linux/drivers/md/raid5.c
> ===================================================================
> --- linux.orig/drivers/md/raid5.c 2014-04-08 09:16:39.377536607 +0800
> +++ linux/drivers/md/raid5.c 2014-04-08 09:16:39.369536607 +0800
> @@ -679,14 +679,9 @@ get_active_stripe(struct r5conf *conf, s
> init_stripe(sh, sector, previous);
> atomic_inc(&sh->count);
> }
> - } else {
> + } else if (!atomic_add_unless(&sh->count, 1, 0)) {
Can I be really fussy and ask you to use "atomic_inc_not_zero" rather than
"atomic_add_unless" ??
I feel it makes the code a bit clearer.
Otherwise it's good - thanks.
NeilBrown
> spin_lock(&conf->device_lock);
> - if (atomic_read(&sh->count)) {
> - BUG_ON(!list_empty(&sh->lru)
> - && !test_bit(STRIPE_EXPANDING, &sh->state)
> - && !test_bit(STRIPE_ON_UNPLUG_LIST, &sh->state)
> - );
> - } else {
> + if (!atomic_read(&sh->count)) {
> if (!test_bit(STRIPE_HANDLE, &sh->state))
> atomic_inc(&conf->active_stripes);
> BUG_ON(list_empty(&sh->lru) &&
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch]raid5: get_active_stripe avoids device_lock
2014-04-09 2:17 ` NeilBrown
@ 2014-04-09 3:27 ` Shaohua Li
2014-04-09 5:24 ` NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: Shaohua Li @ 2014-04-09 3:27 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
On Wed, Apr 09, 2014 at 12:17:29PM +1000, NeilBrown wrote:
> On Tue, 8 Apr 2014 12:05:53 +0800 Shaohua Li <shli@kernel.org> wrote:
>
> >
> > For sequential workload (or request size big workload), get_active_stripe can
> > find cached stripe. In this case, we always hold device_lock, which exposes a
> > lot of lock contention for such workload. If stripe count isn't 0, we don't
> > need hold the lock actually, since we just increase its count. And this is the
> > hot code path for such workload. Unfortunately we must delete the BUG_ON.
> >
> > Signed-off-by: Shaohua Li <shli@fusionio.com>
> > ---
> > drivers/md/raid5.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > Index: linux/drivers/md/raid5.c
> > ===================================================================
> > --- linux.orig/drivers/md/raid5.c 2014-04-08 09:16:39.377536607 +0800
> > +++ linux/drivers/md/raid5.c 2014-04-08 09:16:39.369536607 +0800
> > @@ -679,14 +679,9 @@ get_active_stripe(struct r5conf *conf, s
> > init_stripe(sh, sector, previous);
> > atomic_inc(&sh->count);
> > }
> > - } else {
> > + } else if (!atomic_add_unless(&sh->count, 1, 0)) {
>
> Can I be really fussy and ask you to use "atomic_inc_not_zero" rather than
> "atomic_add_unless" ??
> I feel it makes the code a bit clearer.
Missed this API. Yes, it's better.
Subject: raid5: get_active_stripe avoids device_lock
For sequential workload (or request size big workload), get_active_stripe can
find cached stripe. In this case, we always hold device_lock, which exposes a
lot of lock contention for such workload. If stripe count isn't 0, we don't
need hold the lock actually, since we just increase its count. And this is the
hot code path for such workload. Unfortunately we must delete the BUG_ON.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-04-09 11:07:01.165231305 +0800
+++ linux/drivers/md/raid5.c 2014-04-09 11:07:26.492913067 +0800
@@ -679,14 +679,9 @@ get_active_stripe(struct r5conf *conf, s
init_stripe(sh, sector, previous);
atomic_inc(&sh->count);
}
- } else {
+ } else if (!atomic_inc_not_zero(&sh->count)) {
spin_lock(&conf->device_lock);
- if (atomic_read(&sh->count)) {
- BUG_ON(!list_empty(&sh->lru)
- && !test_bit(STRIPE_EXPANDING, &sh->state)
- && !test_bit(STRIPE_ON_UNPLUG_LIST, &sh->state)
- );
- } else {
+ if (!atomic_read(&sh->count)) {
if (!test_bit(STRIPE_HANDLE, &sh->state))
atomic_inc(&conf->active_stripes);
BUG_ON(list_empty(&sh->lru) &&
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch]raid5: get_active_stripe avoids device_lock
2014-04-09 3:27 ` Shaohua Li
@ 2014-04-09 5:24 ` NeilBrown
0 siblings, 0 replies; 4+ messages in thread
From: NeilBrown @ 2014-04-09 5:24 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
[-- Attachment #1: Type: text/plain, Size: 3086 bytes --]
On Wed, 9 Apr 2014 11:27:42 +0800 Shaohua Li <shli@kernel.org> wrote:
> On Wed, Apr 09, 2014 at 12:17:29PM +1000, NeilBrown wrote:
> > On Tue, 8 Apr 2014 12:05:53 +0800 Shaohua Li <shli@kernel.org> wrote:
> >
> > >
> > > For sequential workload (or request size big workload), get_active_stripe can
> > > find cached stripe. In this case, we always hold device_lock, which exposes a
> > > lot of lock contention for such workload. If stripe count isn't 0, we don't
> > > need hold the lock actually, since we just increase its count. And this is the
> > > hot code path for such workload. Unfortunately we must delete the BUG_ON.
> > >
> > > Signed-off-by: Shaohua Li <shli@fusionio.com>
> > > ---
> > > drivers/md/raid5.c | 9 ++-------
> > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > >
> > > Index: linux/drivers/md/raid5.c
> > > ===================================================================
> > > --- linux.orig/drivers/md/raid5.c 2014-04-08 09:16:39.377536607 +0800
> > > +++ linux/drivers/md/raid5.c 2014-04-08 09:16:39.369536607 +0800
> > > @@ -679,14 +679,9 @@ get_active_stripe(struct r5conf *conf, s
> > > init_stripe(sh, sector, previous);
> > > atomic_inc(&sh->count);
> > > }
> > > - } else {
> > > + } else if (!atomic_add_unless(&sh->count, 1, 0)) {
> >
> > Can I be really fussy and ask you to use "atomic_inc_not_zero" rather than
> > "atomic_add_unless" ??
> > I feel it makes the code a bit clearer.
>
> Missed this API. Yes, it's better.
>
>
> Subject: raid5: get_active_stripe avoids device_lock
>
> For sequential workload (or request size big workload), get_active_stripe can
> find cached stripe. In this case, we always hold device_lock, which exposes a
> lot of lock contention for such workload. If stripe count isn't 0, we don't
> need hold the lock actually, since we just increase its count. And this is the
> hot code path for such workload. Unfortunately we must delete the BUG_ON.
>
> Signed-off-by: Shaohua Li <shli@fusionio.com>
> ---
> drivers/md/raid5.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> Index: linux/drivers/md/raid5.c
> ===================================================================
> --- linux.orig/drivers/md/raid5.c 2014-04-09 11:07:01.165231305 +0800
> +++ linux/drivers/md/raid5.c 2014-04-09 11:07:26.492913067 +0800
> @@ -679,14 +679,9 @@ get_active_stripe(struct r5conf *conf, s
> init_stripe(sh, sector, previous);
> atomic_inc(&sh->count);
> }
> - } else {
> + } else if (!atomic_inc_not_zero(&sh->count)) {
> spin_lock(&conf->device_lock);
> - if (atomic_read(&sh->count)) {
> - BUG_ON(!list_empty(&sh->lru)
> - && !test_bit(STRIPE_EXPANDING, &sh->state)
> - && !test_bit(STRIPE_ON_UNPLUG_LIST, &sh->state)
> - );
> - } else {
> + if (!atomic_read(&sh->count)) {
> if (!test_bit(STRIPE_HANDLE, &sh->state))
> atomic_inc(&conf->active_stripes);
> BUG_ON(list_empty(&sh->lru) &&
Applied, thanks.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-04-09 5:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-08 4:05 [patch]raid5: get_active_stripe avoids device_lock Shaohua Li
2014-04-09 2:17 ` NeilBrown
2014-04-09 3:27 ` Shaohua Li
2014-04-09 5:24 ` NeilBrown
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).