From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Subject: Re: [PATCH 04/19] quota: protect dqget() from parallels quotaoff via SRCU Date: Tue, 23 Nov 2010 00:53:59 +0300 Message-ID: <87y68lja88.fsf@dmon-lap.sw.ru> References: <1289477678-5669-1-git-send-email-dmonakhov@openvz.org> <1289477678-5669-5-git-send-email-dmonakhov@openvz.org> <20101122212121.GE6141@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, jack@suse.cz, hch@infradead.org To: Jan Kara Return-path: Received: from mail-ew0-f46.google.com ([209.85.215.46]:54967 "EHLO mail-ew0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932156Ab0KVVyE (ORCPT ); Mon, 22 Nov 2010 16:54:04 -0500 Received: by ewy5 with SMTP id 5so2050196ewy.19 for ; Mon, 22 Nov 2010 13:54:03 -0800 (PST) In-Reply-To: <20101122212121.GE6141@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, 22 Nov 2010 22:21:21 +0100, Jan Kara wrote: > 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. Unfortunately you right, comments are not very descriptive, will redo. > > > 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? This is just a trick for non static srcu variables, to prevent races between use/free, see http://lwn.net/Articles/202847/ In my case i implement it like follows /* All readers do */ rcu_read_lock(); /* Stage1: at this moment we prevent dq_srcu from being fried */ if (!sb_has_quota_active(sb, type)) { rcu_read_unlock(); return NULL; } /* Stage2: grab reference to quota_info. */ idx = srcu_read_lock(&dqopts(sb)->dq_srcu); rcu_read_unlock(); /* quota_disable: Cleanup path looks like this */ quota_clear_active(sb, type); /* Wait for all callers in stage1 */ synchronize_rcu(); /* Wait for all caller in stage2 */ synchronize_srcu(&dqopts(sb)->dq_srcu); /* Now we can finely destroy srcu pointer cleanup_srcu(&dqopts(sb)->dq_srcu); > > Honza > -- > Jan Kara > SUSE Labs, CR > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html