From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753381AbbJaMZt (ORCPT ); Sat, 31 Oct 2015 08:25:49 -0400 Received: from forward-corp1m.cmail.yandex.net ([5.255.216.100]:34042 "EHLO forward-corp1m.cmail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752988AbbJaMZr (ORCPT ); Sat, 31 Oct 2015 08:25:47 -0400 From: Roman Gushchin To: Neil Brown , Shaohua Li Cc: "linux-kernel@vger.kernel.org" , "linux-raid@vger.kernel.org" In-Reply-To: <877fm4vw6j.fsf@notabene.neil.brown.name> References: <1446022340-1453-1-git-send-email-klamm@yandex-team.ru> <87r3kebjgx.fsf@notabene.neil.brown.name> <30651446128148@webcorp02d.yandex-team.ru> <87ziz1w33r.fsf@notabene.neil.brown.name> <47541446213767@webcorp02e.yandex-team.ru> <20151030162443.GA31413@kernel.org> <877fm4vw6j.fsf@notabene.neil.brown.name> Subject: Re: [PATCH] md/raid5: fix locking in handle_stripe_clean_event() MIME-Version: 1.0 Message-Id: <102381446294342@webcorp02e.yandex-team.ru> X-Mailer: Yamail [ http://yandex.ru ] 5.0 Date: Sat, 31 Oct 2015 15:25:42 +0300 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=koi8-r Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ok, thank you for clarifications! -- Roman 31.10.2015, 01:17, "Neil Brown" : > 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