Re: [PATCH 04/19] quota: protect dqget() from parallels quotaoff via SRCU

[Dmitry <dmonakhov@openvz.org>]

On Mon, 22 Nov 2010 22:21:21 +0100, Jan Kara <jack@suse.cz> 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@suse.cz>
> 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