On Aug 16, 2017, at 9:41 AM, Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Message signed with OpenPGP
