| Message ID | 20170816154127.7048-3-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Aug 16, 2017, at 9:41 AM, Jan Kara <jack@suse.cz> wrote: > > We need dqio_sem held just for reading when calling ->read_dqblk() in > dquot_acquire(). Also dqio_sem is not needed when manipulating dquot > flags (those are protected by dq_lock) Nothing against the patch itself, but this comment is a bit confusing. It would be good to list this dq_lock dependency at the dq_flags declaration. Looking through the code that accesses dq_flags, I see dquot_mark_dquot_dirty(), clear_dquot_dirty(), dquot_commit(), dqput() depend on dq_list_lock, but I don't see many of the users besides dquot_acquire() and dquot_release() getting dq_lock, only using the atomic bit operations on dq_flags. Cheers, Andreas > so acquire and release it closer to the place where it is needed. This > reduces lock hold time and will make locking changes easier. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/quota/dquot.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 29d447598642..21358f31923d 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -406,9 +406,11 @@ int dquot_acquire(struct dquot *dquot) > struct quota_info *dqopt = sb_dqopt(dquot->dq_sb); > > mutex_lock(&dquot->dq_lock); > - down_write(&dqopt->dqio_sem); > - if (!test_bit(DQ_READ_B, &dquot->dq_flags)) > + if (!test_bit(DQ_READ_B, &dquot->dq_flags)) { > + down_read(&dqopt->dqio_sem); > ret = dqopt->ops[dquot->dq_id.type]->read_dqblk(dquot); > + up_read(&dqopt->dqio_sem); > + } > if (ret < 0) > goto out_iolock; > /* Make sure flags update is visible after dquot has been filled */ > @@ -416,12 +418,14 @@ int dquot_acquire(struct dquot *dquot) > set_bit(DQ_READ_B, &dquot->dq_flags); > /* Instantiate dquot if needed */ > if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags) && !dquot->dq_off) { > + down_write(&dqopt->dqio_sem); > ret = dqopt->ops[dquot->dq_id.type]->commit_dqblk(dquot); > /* Write the info if needed */ > if (info_dirty(&dqopt->info[dquot->dq_id.type])) { > ret2 = dqopt->ops[dquot->dq_id.type]->write_file_info( > dquot->dq_sb, dquot->dq_id.type); > } > + up_write(&dqopt->dqio_sem); > if (ret < 0) > goto out_iolock; > if (ret2 < 0) { > @@ -436,7 +440,6 @@ int dquot_acquire(struct dquot *dquot) > smp_mb__before_atomic(); > set_bit(DQ_ACTIVE_B, &dquot->dq_flags); > out_iolock: > - up_write(&dqopt->dqio_sem); > mutex_unlock(&dquot->dq_lock); > return ret; > } > -- > 2.12.3 > Cheers, Andreas
On Wed 16-08-17 10:35:48, Andreas Dilger wrote: > On Aug 16, 2017, at 9:41 AM, Jan Kara <jack@suse.cz> wrote: > > > > We need dqio_sem held just for reading when calling ->read_dqblk() in > > dquot_acquire(). Also dqio_sem is not needed when manipulating dquot > > flags (those are protected by dq_lock) > > Nothing against the patch itself, but this comment is a bit confusing. > > It would be good to list this dq_lock dependency at the dq_flags declaration. > Looking through the code that accesses dq_flags, I see dquot_mark_dquot_dirty(), > clear_dquot_dirty(), dquot_commit(), dqput() depend on dq_list_lock, but > I don't see many of the users besides dquot_acquire() and dquot_release() > getting dq_lock, only using the atomic bit operations on dq_flags. Updated changelog to: We need dqio_sem held just for reading when calling ->read_dqblk() in dquot_acquire(). Also dqio_sem is not needed when setting DQ_READ_B and DQ_ACTIVE_B as concurrent reads and dquot activation are serialized by dq_lock. So acquire and release dqio_sem closer to the place where it is needed. This reduces lock hold time and will make locking changes easier. Honza
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 29d447598642..21358f31923d 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -406,9 +406,11 @@ int dquot_acquire(struct dquot *dquot) struct quota_info *dqopt = sb_dqopt(dquot->dq_sb); mutex_lock(&dquot->dq_lock); - down_write(&dqopt->dqio_sem); - if (!test_bit(DQ_READ_B, &dquot->dq_flags)) + if (!test_bit(DQ_READ_B, &dquot->dq_flags)) { + down_read(&dqopt->dqio_sem); ret = dqopt->ops[dquot->dq_id.type]->read_dqblk(dquot); + up_read(&dqopt->dqio_sem); + } if (ret < 0) goto out_iolock; /* Make sure flags update is visible after dquot has been filled */ @@ -416,12 +418,14 @@ int dquot_acquire(struct dquot *dquot) set_bit(DQ_READ_B, &dquot->dq_flags); /* Instantiate dquot if needed */ if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags) && !dquot->dq_off) { + down_write(&dqopt->dqio_sem); ret = dqopt->ops[dquot->dq_id.type]->commit_dqblk(dquot); /* Write the info if needed */ if (info_dirty(&dqopt->info[dquot->dq_id.type])) { ret2 = dqopt->ops[dquot->dq_id.type]->write_file_info( dquot->dq_sb, dquot->dq_id.type); } + up_write(&dqopt->dqio_sem); if (ret < 0) goto out_iolock; if (ret2 < 0) { @@ -436,7 +440,6 @@ int dquot_acquire(struct dquot *dquot) smp_mb__before_atomic(); set_bit(DQ_ACTIVE_B, &dquot->dq_flags); out_iolock: - up_write(&dqopt->dqio_sem); mutex_unlock(&dquot->dq_lock); return ret; }
We need dqio_sem held just for reading when calling ->read_dqblk() in dquot_acquire(). Also dqio_sem is not needed when manipulating dquot flags (those are protected by dq_lock) so acquire and release it closer to the place where it is needed. This reduces lock hold time and will make locking changes easier. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/quota/dquot.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)