On 06/21, Jaegeuk Kim wrote:
> On 06/20, Chao Yu wrote:
> > On 2019/6/20 1:26, Jaegeuk Kim wrote:
> > > On 06/18, Chao Yu wrote:
> > >> On 2019/6/14 10:46, Jaegeuk Kim wrote:
> > >>> On 06/11, Chao Yu wrote:
> > >>>> On 2019/6/5 2:36, Jaegeuk Kim wrote:
> > >>>>> Two paths to update quota and f2fs_lock_op:
> > >>>>>
> > >>>>> 1.
> > >>>>> - lock_op
> > >>>>> | - quota_update
> > >>>>> `- unlock_op
> > >>>>>
> > >>>>> 2.
> > >>>>> - quota_update
> > >>>>> - lock_op
> > >>>>> `- unlock_op
> > >>>>>
> > >>>>> But, we need to make a transaction on quota_update + lock_op in #2
> > >>>>> case.
> > >>>>> So, this patch introduces:
> > >>>>> 1. lock_op
> > >>>>> 2. down_write
> > >>>>> 3. check __need_flush
> > >>>>> 4. up_write
> > >>>>> 5. if there is dirty quota entries, flush them
> > >>>>> 6. otherwise, good to go
> > >>>>>
> > >>>>> Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> > >>>>> ---
> > >>>>>
> > >>>>> v3 from v2:
> > >>>>> - refactor to fix quota corruption issue
> > >>>>> : it seems that the previous scenario is not real and no deadlock
> > >>>>> case was
> > >>>>> encountered.
> > >>>>
> > >>>> - f2fs_dquot_commit
> > >>>> - down_read(&sbi->quota_sem)
> > >>>> - block_operation
> > >>>> - f2fs_lock_all
> > >>>> - need_flush_quota
> > >>>> - down_write(&sbi->quota_sem)
> > >>>> - f2fs_quota_write
> > >>>> - f2fs_lock_op
> > >>>>
> > >>>> Why can't this happen?
> > >>>>
> > >>>> Once more question, should we hold quota_sem during checkpoint to
> > >>>> avoid further
> > >>>> quota update? f2fs_lock_op can do this job as well?
> > >>>
> > >>> I couldn't find write_dquot() call to make this happen, and
> > >>> f2fs_lock_op was not
> > >>
> > >> - f2fs_dquot_commit
> > >> - dquot_commit
> > >> ->commit_dqblk (v2_write_dquot)
> > >> - qtree_write_dquot
> > >> ->quota_write (f2fs_quota_write)
> > >> - f2fs_lock_op
> > >>
> > >> Do you mean there is no such way that calling f2fs_lock_op() from
> > >> f2fs_quota_write()? So that deadlock condition is not existing?
> > >
> > > I mean write_dquot->f2fs_dquot_commit and block_operation seems not racing
> > > together.
> >
> > quota ioctl has the path calling write_dquot->f2fs_dquot_commit as below,
> > which
> > can race with checkpoint().
> >
> > - do_quotactl
> > - sb->s_qcop->quota_sync (f2fs_quota_sync)
> > - down_read(&sbi->quota_sem); ---- First
> > - dquot_writeback_dquots
> > - sb->dq_op->write_dquot (f2fs_dquot_commit)
> > - block_operation can
> > race here
> > - down_read(&sbi->quota_sem); ---- Second
>
> Adding f2fs_lock_op() in f2fs_quota_sync() should be fine?
Something like this?
---
fs/f2fs/super.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7f2829b1192e..1d33ca1a8c09 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1919,6 +1919,17 @@ int f2fs_quota_sync(struct super_block *sb, int type)
int cnt;
int ret;
+ /*
+ * do_quotactl
+ * f2fs_quota_sync
+ * down_read(quota_sem)
+ * dquot_writeback_dquots()
+ * f2fs_dquot_commit
+ * block_operation
+ * down_read(quota_sem)
+ */
+ f2fs_lock_op(sbi);
+
down_read(&sbi->quota_sem);
ret = dquot_writeback_dquots(sb, type);
if (ret)
@@ -1958,6 +1969,7 @@ int f2fs_quota_sync(struct super_block *sb, int type)
if (ret)
set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
up_read(&sbi->quota_sem);
+ f2fs_unlock_op(sbi);
return ret;
}
--
2.19.0.605.g01d371f741-goog
