Path: csiph.com!eternal-september.org!feeder.eternal-september.org!aioe.org!bofh.it!news.nic.it!robomod
From: Roman Gushchin <klamm@yandex-team.ru>
Newsgroups: linux.kernel
Subject: Re: [PATCH] md/raid5: fix locking in handle_stripe_clean_event()
Date: Sat, 31 Oct 2015 13:30:02 +0100
Message-ID: <qpDse-2Li-5@gated-at.bofh.it>
References: <qouKm-lA-5@gated-at.bofh.it> <qoJq1-1ui-1@gated-at.bofh.it> <qoWng-1hg-21@gated-at.bofh.it> <qp6PD-7Qh-1@gated-at.bofh.it> <qpixs-6LW-19@gated-at.bofh.it> <qpkIW-84i-9@gated-at.bofh.it> <qpqbE-30S-13@gated-at.bofh.it>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=koi8-r
Sender: robomod@news.nic.it
Approved: robomod@news.nic.it
Lines: 72
Organization: linux.* mail to news gateway
Xref: csiph.com linux.kernel:1259977

Ok, thank you for clarifications!

--
Roman


31.10.2015, 01:17, "Neil Brown" <neilb@suse.de>:
> On Sat, Oct 31 2015, Shaohua Li wrote:
>
>> šOn Fri, Oct 30, 2015 at 05:02:47PM +0300, Roman Gushchin wrote:
>>> š> Isn't the 4.1 fix just:
>>> š>
>>> š> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>>> š> index e5befa356dbe..6e4350a78257 100644
>>> š> --- a/drivers/md/raid5.c
>>> š> +++ b/drivers/md/raid5.c
>>> š> @@ -3522,16 +3522,16 @@ returnbi:
>>> š> šššššššššššššššššš* no updated data, so remove it from hash list and the stripe
>>> š> šššššššššššššššššš* will be reinitialized
>>> š> šššššššššššššššššš*/
>>> š> - spin_lock_irq(&conf->device_lock);
>>> š> šunhash:
>>> š> + spin_lock_irq(conf->hash_locks + sh->hash_lock_index);
>>> š> šššššššššššššššššremove_hash(sh);
>>> š> + spin_unlock_irq(conf->hash_locks + sh->hash_lock_index);
>>> š> šššššššššššššššššif (head_sh->batch_head) {
>>> š> šššššššššššššššššššššššššsh = list_first_entry(&sh->batch_list,
>>> š> šššššššššššššššššššššššššššššššššššššššššššššššstruct stripe_head, batch_list);
>>> š> šššššššššššššššššššššššššif (sh != head_sh)
>>> š> šššššššššššššššššššššššššššššššššššššššššgoto unhash;
>>> š> ššššššššššššššššš}
>>> š> - spin_unlock_irq(&conf->device_lock);
>>> š> šššššššššššššššššsh = head_sh;
>>> š>
>>> š> šššššššššššššššššif (test_bit(STRIPE_SYNC_REQUESTED, &sh->state))
>>> š>
>>> š> ??
>>>
>>> šIn my opion, this patch looks correct, although it seems to me, that there is an another issue here.
>>>
>>> š> šššššššššššššššššif (head_sh->batch_head) {
>>> š> šššššššššššššššššššššššššsh = list_first_entry(&sh->batch_list,
>>> š> šššššššššššššššššššššššššššššššššššššššššššššššstruct stripe_head, batch_list);
>>> š> šššššššššššššššššššššššššif (sh != head_sh)
>>> š> šššššššššššššššššššššššššššššššššššššššššgoto unhash;
>>> š> ššššššššššššššššš}
>>>
>>> šWith a patch above this code will be executed without taking any locks. It it correct?
>>> šIn my opinion, we need to take at least sh->stripe_lock, which protects sh->batch_head.
>>> šOr do I miss something?
>>>
>>> šIf you want, we can handle this issue separately.
>>
>> šThe batch_list list doesn't need the protection. Only the remove_hash() need it.
>
> Yes, that's my understanding too. The key to understanding is that
> comment you (helpfully!) put in clear_batch_ready():
>
> šššššššš/*
> ššššššššš* BATCH_READY is cleared, no new stripes can be added.
> ššššššššš* batch_list can be accessed without lock
> ššššššššš*/
>
> I'll wrangle some patches...
>
> Thanks,
> NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/