On 2019/6/22 1:51, Jaegeuk Kim wrote: > 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 <[email protected]> >>>>>>>> --- >>>>>>>> >>>>>>>> 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?
I'm okay with this diff. :) Thanks, > > --- > 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; > } > > _______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
