From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 04/19] quota: protect dqget() from parallels quotaoff via SRCU Date: Mon, 22 Nov 2010 22:21:21 +0100 Message-ID: <20101122212121.GE6141@quack.suse.cz> References: <1289477678-5669-1-git-send-email-dmonakhov@openvz.org> <1289477678-5669-5-git-send-email-dmonakhov@openvz.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, jack@suse.cz, hch@infradead.org To: Dmitry Monakhov Return-path: Received: from cantor2.suse.de ([195.135.220.15]:53146 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756341Ab0KVVVX (ORCPT ); Mon, 22 Nov 2010 16:21:23 -0500 Content-Disposition: inline In-Reply-To: <1289477678-5669-5-git-send-email-dmonakhov@openvz.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 11-11-10 15:14:23, Dmitry Monakhov wrote: > In order to hide quota internals inside didicated structure pointer. > We have to serialize that object lifetime with dqget(), and change/uncharge > functions. > Quota_info construction/destruction will be protected via ->dq_srcu. > SRCU counter temproraly placed inside sb, but will be moved inside > quota object pointer in next patch. The changelog seems rather insufficient to me. Could you please write here the new locking rules more in detail? What structures are exactly protected by RCU? Which lock are you able to relax after these changes (is it only dq_state_lock?)? The rules should be also placed in dquot.c where locking is described. > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 748d744..7e937b0 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -805,7 +805,7 @@ static struct dquot *get_empty_dquot(struct super_block *sb, int type) > /* > * Get reference to dquot > * > - * Locking is slightly tricky here. We are guarded from parallel quotaoff() > + * We are guarded from parallel quotaoff() by holding srcu_read_lock The comment does not make sense after your change anymore. > * destroying our dquot by: > * a) checking for quota flags under dq_list_lock and > * b) getting a reference to dquot before we release dq_list_lock > @@ -814,9 +814,15 @@ struct dquot *dqget(struct super_block *sb, unsigned int id, int type) > { > unsigned int hashent = hashfn(sb, id, type); > struct dquot *dquot = NULL, *empty = NULL; > + int idx; > > - if (!sb_has_quota_active(sb, type)) > + rcu_read_lock(); > + if (!sb_has_quota_active(sb, type)) { > + rcu_read_unlock(); > return NULL; > + } > + idx = srcu_read_lock(&dqopts(sb)->dq_srcu); > + rcu_read_unlock(); Ugh, I'm kind of puzzled by your combination of RCU and SRCU. What's the point? Honza -- Jan Kara SUSE Labs, CR