On Mon, 22 Nov 2010 22:12:09 +0100, Jan Kara <jack@xxxxxxx> wrote:
> On Thu 11-11-10 15:14:25, Dmitry Monakhov wrote:
> > The only reader which use state_lock is dqget(), and is it serialized
> > with quota_disable via SRCU. And state_lock doesn't guaranties anything
> > for that case. All methods which modify quota flags already protected by
> > dqonoff_mutex. Get rid of useless state_lock.
> dq_state_lock has two properties you miss here:
> a) It guarantees we read a consistent value from a quota state variable.
So what, it is not guaranteed anything. Quota make becomes disabled
one nanosecond after check.
And with srcu_read_lock we can be guarantee what quota will be available
for the caller. Even in case of stale value.
> b) We read an uptodate value from quota state variable.
All callers which modify the state already holds onoff_mutex so
implicit mem barriers always guaranteed.
>
> b) is kind of achieved by rcu_read_lock() which implies a memory barrier
> but still stores can be reordered and it's not completely clear it does not
> matter. a) is simply not achieved by your code.
>
> So I think you have to change quota code to manipulate state flags in an
> atomic manner and at least add comments why reordering the state flags
> store e.g. during quotaon does not matter...
Ok, agree comment would be reasonable here.
>
> Honza
>
> > Signed-off-by: Dmitry Monakhov <dmonakhov@xxxxxxxxxx>
> > ---
> > fs/quota/dquot.c | 24 ++----------------------
> > 1 files changed, 2 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> > index 78e48f3..a1efacd 100644
> > --- a/fs/quota/dquot.c
> > +++ b/fs/quota/dquot.c
> > @@ -86,12 +86,9 @@
> > * dq_data_lock protects data from dq_dqb and also mem_dqinfo structures and
> > * also guards consistency of dquot->dq_dqb with inode->i_blocks, i_bytes.
> > * i_blocks and i_bytes updates itself are guarded by i_lock acquired directly
> > - * in inode_add_bytes() and inode_sub_bytes(). dq_state_lock protects
> > - * modifications of quota state (on quotaon and quotaoff) and readers who care
> > - * about latest values take it as well.
> > + * in inode_add_bytes() and inode_sub_bytes().
> > *
> > - * The spinlock ordering is hence: dq_data_lock > dq_list_lock > i_lock,
> > - * dq_list_lock > dq_state_lock
> > + * The spinlock ordering is hence: dq_data_lock > dq_list_lock > i_lock.
> > *
> > * Note that some things (eg. sb pointer, type, id) doesn't change during
> > * the life of the dquot structure and so needn't to be protected by a lock
> > @@ -128,7 +125,6 @@
> > */
> >
> > static __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_list_lock);
> > -static __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_state_lock);
> > __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_data_lock);
> > EXPORT_SYMBOL(dq_data_lock);
> >
> > @@ -827,14 +823,10 @@ struct dquot *dqget(struct super_block *sb, unsigned int id, int type)
> > rcu_read_unlock();
> > we_slept:
> > spin_lock(&dq_list_lock);
> > - spin_lock(&dq_state_lock);
> > if (!sb_has_quota_active(sb, type)) {
> > - spin_unlock(&dq_state_lock);
> > spin_unlock(&dq_list_lock);
> > goto out;
> > }
> > - spin_unlock(&dq_state_lock);
> > -
> > dquot = find_dquot(hashent, sb, id, type);
> > if (!dquot) {
> > if (!empty) {
> > @@ -2022,24 +2014,19 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags)
> > continue;
> >
> > if (flags & DQUOT_SUSPENDED) {
> > - spin_lock(&dq_state_lock);
> > qctl->flags |=
> > dquot_state_flag(DQUOT_SUSPENDED, cnt);
> > - spin_unlock(&dq_state_lock);
> > } else {
> > - spin_lock(&dq_state_lock);
> > qctl->flags &= ~dquot_state_flag(flags, cnt);
> > /* Turning off suspended quotas? */
> > if (!sb_has_quota_loaded(sb, cnt) &&
> > sb_has_quota_suspended(sb, cnt)) {
> > qctl->flags &= ~dquot_state_flag(
> > DQUOT_SUSPENDED, cnt);
> > - spin_unlock(&dq_state_lock);
> > iput(dqopt->files[cnt]);
> > dqopt->files[cnt] = NULL;
> > continue;
> > }
> > - spin_unlock(&dq_state_lock);
> > }
> >
> > /* We still have to keep quota loaded? */
> > @@ -2235,10 +2222,7 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
> > goto out_file_init;
> > }
> > mutex_unlock(&dqopt->dqio_mutex);
> > - spin_lock(&dq_state_lock);
> > dqctl(sb)->flags |= dquot_state_flag(flags, type);
> > - spin_unlock(&dq_state_lock);
> > -
> > add_dquot_ref(sb, type);
> > mutex_unlock(&dqctl(sb)->dqonoff_mutex);
> >
> > @@ -2290,12 +2274,10 @@ int dquot_resume(struct super_block *sb, int type)
> > }
> > inode = qctl->dq_opt->files[cnt];
> > qctl->dq_opt->files[cnt] = NULL;
> > - spin_lock(&dq_state_lock);
> > flags = qctl->flags & dquot_state_flag(DQUOT_USAGE_ENABLED |
> > DQUOT_LIMITS_ENABLED,
> > cnt);
> > qctl->flags &= ~dquot_state_flag(DQUOT_STATE_FLAGS, cnt);
> > - spin_unlock(&dq_state_lock);
> > mutex_unlock(&qctl->dqonoff_mutex);
> >
> > flags = dquot_generic_flag(flags, cnt);
> > @@ -2369,9 +2351,7 @@ int dquot_enable(struct inode *inode, int type, int format_id,
> > ret = -EBUSY;
> > goto out_lock;
> > }
> > - spin_lock(&dq_state_lock);
> > qctl->flags |= dquot_state_flag(flags, type);
> > - spin_unlock(&dq_state_lock);
> > out_lock:
> > mutex_unlock(&qctl->dqonoff_mutex);
> > return ret;
> > --
> > 1.6.5.2
> >
> --
> 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
