Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes

[Jaegeuk Kim]

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 <jaegeuk@xxxxxxxxxx>
> > >>>>> ---
> > >>>>>
> > >>>>> 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