[02/27] quota: Do more fine-grained locking in dquot_acquire()
diff mbox

Message ID 20170816154127.7048-3-jack@suse.cz
State New
Headers show

Commit Message

Jan Kara Aug. 16, 2017, 3:41 p.m. UTC
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(-)

Comments

Andreas Dilger Aug. 16, 2017, 4:35 p.m. UTC | #1
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
Jan Kara Aug. 17, 2017, 4:58 p.m. UTC | #2
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

Patch
diff mbox

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;
 }