From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 09/11] quota: protect dquot mem info with objects's lock Date: Wed, 6 Oct 2010 15:41:56 +0200 Message-ID: <20101006134156.GN3676@quack.suse.cz> References: <1286302827-31043-1-git-send-email-dmonakhov@gmail.com> <1286302827-31043-10-git-send-email-dmonakhov@gmail.com> <20101006123726.GL3676@quack.suse.cz> <87wrpv8nqo.fsf@dmon-lap.sw.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , linux-fsdevel@vger.kernel.org, hch@infradead.org, Dmitry Monakhov To: Dmitry Return-path: Received: from cantor2.suse.de ([195.135.220.15]:54009 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932494Ab0JFNmw (ORCPT ); Wed, 6 Oct 2010 09:42:52 -0400 Content-Disposition: inline In-Reply-To: <87wrpv8nqo.fsf@dmon-lap.sw.ru> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed 06-10-10 17:17:03, Dmitry wrote: > Later i've plans to get rid of dqptr_sem by rearranging code like follows. > But may be i've introduced copy logic too soon. > > Variant (1) > 1) Protect i_dquot dereference/modification with i_lock > So charge/free function will looks like follows > do_alloc_space (struct inode* inode, qsize_t num) > { > spin_lock(&inode->i_lock); > if (!check_bdq(inode->i_dquot, num)) > return EQDUOT; > inode_add_bytes_unlocked(inode, num); > dquot_add_bytes(inode->i_dquot, num); > /* We can sleep while we call quota dirty, to protect quota from > * being deleted we have to get ref for the quota. > */ > atomic_inc(dquot->dq_count); > dquot = inode->i_dquot; > spin_unlock(&inode->i_lock); > /* i_lock was dropped, do not touch i_dquot any more */ > mark_dquot_dirty(dquot); > dqput(dquot); > } > This is beter than before, but we still have massive anomic operations > So we can try second variant. > > Variant (2) > do_alloc_space(struct inode* inode, qsize_t num) > { > idx = srcu_read_lock(&dqptr) > spin_lock(&inode->i_lock); > if (!check_bdq(inode->i_dquot, num)) > return EQDUOT; > inode_add_bytes_unlocked(inode, num); > dquot_add_bytes(inode->i_dquot, num); > /* We can sleep while we call quota dirty, > * But this is ok since quota deletion is protected by SRCU > * The only thing we have do is to save old i_dquot array > */ > dquot = inode->i_dquot; > spin_unlock(&inode->i_lock); > /* i_lock was dropped, do not touch i_dquot any more */ > mark_dquot_dirty(dquot); > srcu_read_unlock(&dqptr, idx); > } > /* Destroy method may looks like whis, it will be called from > background work. > */ > do_destroy_quotas_loop() { > /* Wait for all quotas in free list */ > synchronize_srcu(dqptr&) > /* > Ok now we can safely destroy all quotas in the list, > Some of them may be dirty, so we have to write it first. > */ > list_for_each_entry(dquot, &free_list, dq_free) > dquot_release(dquot); > > } I see. Protecting dquots with RCU seems like a reasonable thing to do and then I agree you'd need to copy the dquot pointers. But for now please refrain from doing so. Thanks. Honza -- Jan Kara SUSE Labs, CR