On Mon, 22 Nov 2010 22:21:21 +0100, Jan Kara <jack@xxxxxxx> 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 <jack@xxxxxxx>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
