* [PATCH] f2fs: add a rw_sem to cover quota flag changes @ 2019-05-30 3:31 Jaegeuk Kim 2019-05-30 14:01 ` [f2fs-dev] " Chao Yu 2019-05-30 17:57 ` [PATCH v2] " Jaegeuk Kim 0 siblings, 2 replies; 15+ messages in thread From: Jaegeuk Kim @ 2019-05-30 3:31 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel; +Cc: Jaegeuk Kim thread 1: thread 2: writeback checkpoint set QUOTA_NEED_FLUSH clear QUOTA_NEED_FLUSH f2fs_dquot_commit dquot_commit clear_dquot_dirty f2fs_quota_sync dquot_writeback_dquots nothing to commit commit_dqblk quota_write f2fs_quota_write waiting for f2fs_lock_op() pass __need_flush_quota (no F2FS_DIRTY_QDATA) -> up-to-date quota is not written Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/checkpoint.c | 26 ++++++++++++++++---------- fs/f2fs/f2fs.h | 1 + fs/f2fs/super.c | 27 ++++++++++++++++++++++----- 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 89825261d474..cf3b15c963d2 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -1131,17 +1131,23 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) static bool __need_flush_quota(struct f2fs_sb_info *sbi) { + bool ret = false; + + down_write(&sbi->quota_sem); if (!is_journalled_quota(sbi)) - return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) - return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) - return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) - return true; - if (get_pages(sbi, F2FS_DIRTY_QDATA)) - return true; - return false; + ret = false; + else if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) + ret = false; + else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) + ret = false; + else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) + ret = true; + else if (get_pages(sbi, F2FS_DIRTY_QDATA)) + ret = true; + else + ret = false; + up_write(&sbi->quota_sem); + return ret; } /* diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 9b3d9977cd1e..692c0922f5b2 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1250,6 +1250,7 @@ struct f2fs_sb_info { block_t unusable_block_count; /* # of blocks saved by last cp */ unsigned int nquota_files; /* # of quota sysfile */ + struct rw_semaphore quota_sem; /* blocking cp for flags */ /* # of pages, see count_type */ atomic_t nr_pages[NR_COUNT_TYPE]; diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 912e2619d581..5ddf5e97ee60 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1944,7 +1944,10 @@ int f2fs_quota_sync(struct super_block *sb, int type) int cnt; int ret; + down_read(&sbi->quota_sem); ret = dquot_writeback_dquots(sb, type); + up_read(&sbi->quota_sem); + if (ret) goto out; @@ -2074,32 +2077,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) static int f2fs_dquot_commit(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_commit(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_acquire(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_acquire(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); - + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_release(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_release(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } @@ -2109,22 +2120,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) struct f2fs_sb_info *sbi = F2FS_SB(sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_mark_dquot_dirty(dquot); /* if we are using journalled quota */ if (is_journalled_quota(sbi)) set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_commit_info(struct super_block *sb, int type) { + struct f2fs_sb_info *sbi = F2FS_SB(sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_commit_info(sb, type); if (ret < 0) - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } @@ -3233,6 +3249,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) } init_rwsem(&sbi->cp_rwsem); + init_rwsem(&sbi->quota_sem); init_waitqueue_head(&sbi->cp_wait); init_sb_info(sbi); -- 2.19.0.605.g01d371f741-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: add a rw_sem to cover quota flag changes 2019-05-30 3:31 [PATCH] f2fs: add a rw_sem to cover quota flag changes Jaegeuk Kim @ 2019-05-30 14:01 ` Chao Yu 2019-05-30 17:57 ` [PATCH v2] " Jaegeuk Kim 1 sibling, 0 replies; 15+ messages in thread From: Chao Yu @ 2019-05-30 14:01 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel On 2019-5-30 11:31, Jaegeuk Kim wrote: > thread 1: thread 2: > writeback checkpoint > set QUOTA_NEED_FLUSH > clear QUOTA_NEED_FLUSH > f2fs_dquot_commit > dquot_commit > clear_dquot_dirty > f2fs_quota_sync > dquot_writeback_dquots > nothing to commit > commit_dqblk > quota_write > f2fs_quota_write > waiting for f2fs_lock_op() > pass __need_flush_quota > (no F2FS_DIRTY_QDATA) At a glance, will it cause deadlock: - 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 Thanks, > > -> up-to-date quota is not written > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/checkpoint.c | 26 ++++++++++++++++---------- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/super.c | 27 ++++++++++++++++++++++----- > 3 files changed, 39 insertions(+), 15 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 89825261d474..cf3b15c963d2 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -1131,17 +1131,23 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) > > static bool __need_flush_quota(struct f2fs_sb_info *sbi) > { > + bool ret = false; > + > + down_write(&sbi->quota_sem); > if (!is_journalled_quota(sbi)) > - return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > - return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > - return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > - return true; > - if (get_pages(sbi, F2FS_DIRTY_QDATA)) > - return true; > - return false; > + ret = false; > + else if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > + ret = false; > + else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > + ret = false; > + else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > + ret = true; > + else if (get_pages(sbi, F2FS_DIRTY_QDATA)) > + ret = true; > + else > + ret = false; > + up_write(&sbi->quota_sem); > + return ret; > } > > /* > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 9b3d9977cd1e..692c0922f5b2 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1250,6 +1250,7 @@ struct f2fs_sb_info { > block_t unusable_block_count; /* # of blocks saved by last cp */ > > unsigned int nquota_files; /* # of quota sysfile */ > + struct rw_semaphore quota_sem; /* blocking cp for flags */ > > /* # of pages, see count_type */ > atomic_t nr_pages[NR_COUNT_TYPE]; > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 912e2619d581..5ddf5e97ee60 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -1944,7 +1944,10 @@ int f2fs_quota_sync(struct super_block *sb, int type) > int cnt; > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_writeback_dquots(sb, type); > + up_read(&sbi->quota_sem); > + > if (ret) > goto out; > > @@ -2074,32 +2077,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) > > static int f2fs_dquot_commit(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_commit(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_acquire(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_acquire(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > - > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_release(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_release(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -2109,22 +2120,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) > struct f2fs_sb_info *sbi = F2FS_SB(sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_mark_dquot_dirty(dquot); > > /* if we are using journalled quota */ > if (is_journalled_quota(sbi)) > set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_commit_info(struct super_block *sb, int type) > { > + struct f2fs_sb_info *sbi = F2FS_SB(sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_commit_info(sb, type); > if (ret < 0) > - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -3233,6 +3249,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > } > > init_rwsem(&sbi->cp_rwsem); > + init_rwsem(&sbi->quota_sem); > init_waitqueue_head(&sbi->cp_wait); > init_sb_info(sbi); > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] f2fs: add a rw_sem to cover quota flag changes 2019-05-30 3:31 [PATCH] f2fs: add a rw_sem to cover quota flag changes Jaegeuk Kim 2019-05-30 14:01 ` [f2fs-dev] " Chao Yu @ 2019-05-30 17:57 ` Jaegeuk Kim 2019-06-04 9:48 ` Chao Yu 2019-06-04 18:36 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 1 sibling, 2 replies; 15+ messages in thread From: Jaegeuk Kim @ 2019-05-30 17:57 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel thread 1: thread 2: writeback checkpoint set QUOTA_NEED_FLUSH clear QUOTA_NEED_FLUSH f2fs_dquot_commit dquot_commit clear_dquot_dirty f2fs_quota_sync dquot_writeback_dquots nothing to commit commit_dqblk quota_write f2fs_quota_write waiting for f2fs_lock_op() pass __need_flush_quota (no F2FS_DIRTY_QDATA) -> up-to-date quota is not written Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- v2 from v1: - avoid deadlock. - shorten the loop path fs/f2fs/checkpoint.c | 40 ++++++++++++++++++++-------------------- fs/f2fs/f2fs.h | 1 + fs/f2fs/super.c | 27 ++++++++++++++++++++++----- 3 files changed, 43 insertions(+), 25 deletions(-) diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 89825261d474..66f29907fc0e 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -1131,17 +1131,26 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) static bool __need_flush_quota(struct f2fs_sb_info *sbi) { + bool ret; + if (!is_journalled_quota(sbi)) return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) - return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) - return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) - return true; - if (get_pages(sbi, F2FS_DIRTY_QDATA)) + + if (!down_write_trylock(&sbi->quota_sem)) return true; - return false; + + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) + ret = false; + else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) + ret = false; + else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) + ret = true; + else if (get_pages(sbi, F2FS_DIRTY_QDATA)) + ret = true; + else + ret = false; + up_write(&sbi->quota_sem); + return ret; } /* @@ -1160,14 +1169,16 @@ static int block_operations(struct f2fs_sb_info *sbi) blk_start_plug(&plug); retry_flush_quotas: + f2fs_lock_all(sbi); if (__need_flush_quota(sbi)) { int locked; if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); - f2fs_lock_all(sbi); goto retry_flush_dents; } + + f2fs_unlock_all(sbi); clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); /* only failed during mount/umount/freeze/quotactl */ @@ -1175,11 +1186,6 @@ static int block_operations(struct f2fs_sb_info *sbi) f2fs_quota_sync(sbi->sb, -1); if (locked) up_read(&sbi->sb->s_umount); - } - - f2fs_lock_all(sbi); - if (__need_flush_quota(sbi)) { - f2fs_unlock_all(sbi); cond_resched(); goto retry_flush_quotas; } @@ -1201,12 +1207,6 @@ static int block_operations(struct f2fs_sb_info *sbi) */ down_write(&sbi->node_change); - if (__need_flush_quota(sbi)) { - up_write(&sbi->node_change); - f2fs_unlock_all(sbi); - goto retry_flush_quotas; - } - if (get_pages(sbi, F2FS_DIRTY_IMETA)) { up_write(&sbi->node_change); f2fs_unlock_all(sbi); diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 9b3d9977cd1e..692c0922f5b2 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1250,6 +1250,7 @@ struct f2fs_sb_info { block_t unusable_block_count; /* # of blocks saved by last cp */ unsigned int nquota_files; /* # of quota sysfile */ + struct rw_semaphore quota_sem; /* blocking cp for flags */ /* # of pages, see count_type */ atomic_t nr_pages[NR_COUNT_TYPE]; diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 1dc02101e5e5..7a6d70d8e851 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1946,7 +1946,10 @@ int f2fs_quota_sync(struct super_block *sb, int type) int cnt; int ret; + down_read(&sbi->quota_sem); ret = dquot_writeback_dquots(sb, type); + up_read(&sbi->quota_sem); + if (ret) goto out; @@ -2076,32 +2079,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) static int f2fs_dquot_commit(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_commit(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_acquire(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_acquire(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); - + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_release(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_release(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } @@ -2111,22 +2122,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) struct f2fs_sb_info *sbi = F2FS_SB(sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_mark_dquot_dirty(dquot); /* if we are using journalled quota */ if (is_journalled_quota(sbi)) set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_commit_info(struct super_block *sb, int type) { + struct f2fs_sb_info *sbi = F2FS_SB(sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_commit_info(sb, type); if (ret < 0) - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } @@ -3235,6 +3251,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) } init_rwsem(&sbi->cp_rwsem); + init_rwsem(&sbi->quota_sem); init_waitqueue_head(&sbi->cp_wait); init_sb_info(sbi); -- 2.19.0.605.g01d371f741-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] f2fs: add a rw_sem to cover quota flag changes 2019-05-30 17:57 ` [PATCH v2] " Jaegeuk Kim @ 2019-06-04 9:48 ` Chao Yu 2019-06-04 18:36 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 1 sibling, 0 replies; 15+ messages in thread From: Chao Yu @ 2019-06-04 9:48 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel On 2019/5/31 1:57, Jaegeuk Kim wrote: > thread 1: thread 2: > writeback checkpoint > set QUOTA_NEED_FLUSH > clear QUOTA_NEED_FLUSH > f2fs_dquot_commit > dquot_commit > clear_dquot_dirty > f2fs_quota_sync > dquot_writeback_dquots > nothing to commit > commit_dqblk > quota_write > f2fs_quota_write > waiting for f2fs_lock_op() > pass __need_flush_quota > (no F2FS_DIRTY_QDATA) > > -> up-to-date quota is not written > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> Reviewed-by: Chao Yu <yuchao0@huawei.com> Thanks, ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-05-30 17:57 ` [PATCH v2] " Jaegeuk Kim 2019-06-04 9:48 ` Chao Yu @ 2019-06-04 18:36 ` Jaegeuk Kim 2019-06-11 7:46 ` Chao Yu 1 sibling, 1 reply; 15+ messages in thread From: Jaegeuk Kim @ 2019-06-04 18:36 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel 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@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. fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- fs/f2fs/f2fs.h | 1 + fs/f2fs/super.c | 26 +++++++++++++++++++++----- 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 89825261d474..43f65f0962e5 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) static bool __need_flush_quota(struct f2fs_sb_info *sbi) { + bool ret = false; + if (!is_journalled_quota(sbi)) return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) - return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) - return false; - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) - return true; - if (get_pages(sbi, F2FS_DIRTY_QDATA)) - return true; - return false; + + down_write(&sbi->quota_sem); + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { + ret = false; + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { + ret = false; + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); + ret = true; + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { + ret = true; + } + up_write(&sbi->quota_sem); + return ret; } /* @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) blk_start_plug(&plug); retry_flush_quotas: + f2fs_lock_all(sbi); if (__need_flush_quota(sbi)) { int locked; if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); - f2fs_lock_all(sbi); + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); goto retry_flush_dents; } - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); + f2fs_unlock_all(sbi); /* only failed during mount/umount/freeze/quotactl */ locked = down_read_trylock(&sbi->sb->s_umount); f2fs_quota_sync(sbi->sb, -1); if (locked) up_read(&sbi->sb->s_umount); - } - - f2fs_lock_all(sbi); - if (__need_flush_quota(sbi)) { - f2fs_unlock_all(sbi); cond_resched(); goto retry_flush_quotas; } @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) */ down_write(&sbi->node_change); - if (__need_flush_quota(sbi)) { - up_write(&sbi->node_change); - f2fs_unlock_all(sbi); - goto retry_flush_quotas; - } - if (get_pages(sbi, F2FS_DIRTY_IMETA)) { up_write(&sbi->node_change); f2fs_unlock_all(sbi); diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 9674a85154b2..9bd2bf0f559b 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { block_t unusable_block_count; /* # of blocks saved by last cp */ unsigned int nquota_files; /* # of quota sysfile */ + struct rw_semaphore quota_sem; /* blocking cp for flags */ /* # of pages, see count_type */ atomic_t nr_pages[NR_COUNT_TYPE]; diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 15d7e30bfc72..5a318399a2fa 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) int cnt; int ret; + down_read(&sbi->quota_sem); ret = dquot_writeback_dquots(sb, type); if (ret) goto out; @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) out: if (ret) set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) static int f2fs_dquot_commit(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_commit(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_acquire(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_acquire(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); - + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_release(struct dquot *dquot) { + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_release(dquot); if (ret < 0) - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) struct f2fs_sb_info *sbi = F2FS_SB(sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_mark_dquot_dirty(dquot); /* if we are using journalled quota */ if (is_journalled_quota(sbi)) set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); + up_read(&sbi->quota_sem); return ret; } static int f2fs_dquot_commit_info(struct super_block *sb, int type) { + struct f2fs_sb_info *sbi = F2FS_SB(sb); int ret; + down_read(&sbi->quota_sem); ret = dquot_commit_info(sb, type); if (ret < 0) - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); + up_read(&sbi->quota_sem); return ret; } @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) } init_rwsem(&sbi->cp_rwsem); + init_rwsem(&sbi->quota_sem); init_waitqueue_head(&sbi->cp_wait); init_sb_info(sbi); -- 2.19.0.605.g01d371f741-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-04 18:36 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim @ 2019-06-11 7:46 ` Chao Yu 2019-06-11 7:46 ` Chao Yu 2019-06-14 2:46 ` Jaegeuk Kim 0 siblings, 2 replies; 15+ messages in thread From: Chao Yu @ 2019-06-11 7:46 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel 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@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? Thanks, > > fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/super.c | 26 +++++++++++++++++++++----- > 3 files changed, 41 insertions(+), 27 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 89825261d474..43f65f0962e5 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) > > static bool __need_flush_quota(struct f2fs_sb_info *sbi) > { > + bool ret = false; > + > if (!is_journalled_quota(sbi)) > return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > - return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > - return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > - return true; > - if (get_pages(sbi, F2FS_DIRTY_QDATA)) > - return true; > - return false; > + > + down_write(&sbi->quota_sem); > + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { > + ret = false; > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { > + ret = false; > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { > + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > + ret = true; > + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { > + ret = true; > + } > + up_write(&sbi->quota_sem); > + return ret; > } > > /* > @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) > blk_start_plug(&plug); > > retry_flush_quotas: > + f2fs_lock_all(sbi); > if (__need_flush_quota(sbi)) { > int locked; > > if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { > set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); > - f2fs_lock_all(sbi); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > goto retry_flush_dents; > } > - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > + f2fs_unlock_all(sbi); > > /* only failed during mount/umount/freeze/quotactl */ > locked = down_read_trylock(&sbi->sb->s_umount); > f2fs_quota_sync(sbi->sb, -1); > if (locked) > up_read(&sbi->sb->s_umount); > - } > - > - f2fs_lock_all(sbi); > - if (__need_flush_quota(sbi)) { > - f2fs_unlock_all(sbi); > cond_resched(); > goto retry_flush_quotas; > } > @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) > */ > down_write(&sbi->node_change); > > - if (__need_flush_quota(sbi)) { > - up_write(&sbi->node_change); > - f2fs_unlock_all(sbi); > - goto retry_flush_quotas; > - } > - > if (get_pages(sbi, F2FS_DIRTY_IMETA)) { > up_write(&sbi->node_change); > f2fs_unlock_all(sbi); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 9674a85154b2..9bd2bf0f559b 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { > block_t unusable_block_count; /* # of blocks saved by last cp */ > > unsigned int nquota_files; /* # of quota sysfile */ > + struct rw_semaphore quota_sem; /* blocking cp for flags */ > > /* # of pages, see count_type */ > atomic_t nr_pages[NR_COUNT_TYPE]; > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 15d7e30bfc72..5a318399a2fa 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > int cnt; > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_writeback_dquots(sb, type); > if (ret) > goto out; > @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > out: > if (ret) > set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) > > static int f2fs_dquot_commit(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_commit(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_acquire(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_acquire(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > - > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_release(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_release(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) > struct f2fs_sb_info *sbi = F2FS_SB(sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_mark_dquot_dirty(dquot); > > /* if we are using journalled quota */ > if (is_journalled_quota(sbi)) > set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_commit_info(struct super_block *sb, int type) > { > + struct f2fs_sb_info *sbi = F2FS_SB(sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_commit_info(sb, type); > if (ret < 0) > - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > } > > init_rwsem(&sbi->cp_rwsem); > + init_rwsem(&sbi->quota_sem); > init_waitqueue_head(&sbi->cp_wait); > init_sb_info(sbi); > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-11 7:46 ` Chao Yu @ 2019-06-11 7:46 ` Chao Yu 2019-06-14 2:46 ` Jaegeuk Kim 1 sibling, 0 replies; 15+ messages in thread From: Chao Yu @ 2019-06-11 7:46 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel 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@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? Thanks, > > fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/super.c | 26 +++++++++++++++++++++----- > 3 files changed, 41 insertions(+), 27 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 89825261d474..43f65f0962e5 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) > > static bool __need_flush_quota(struct f2fs_sb_info *sbi) > { > + bool ret = false; > + > if (!is_journalled_quota(sbi)) > return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > - return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > - return false; > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > - return true; > - if (get_pages(sbi, F2FS_DIRTY_QDATA)) > - return true; > - return false; > + > + down_write(&sbi->quota_sem); > + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { > + ret = false; > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { > + ret = false; > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { > + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > + ret = true; > + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { > + ret = true; > + } > + up_write(&sbi->quota_sem); > + return ret; > } > > /* > @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) > blk_start_plug(&plug); > > retry_flush_quotas: > + f2fs_lock_all(sbi); > if (__need_flush_quota(sbi)) { > int locked; > > if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { > set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); > - f2fs_lock_all(sbi); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > goto retry_flush_dents; > } > - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > + f2fs_unlock_all(sbi); > > /* only failed during mount/umount/freeze/quotactl */ > locked = down_read_trylock(&sbi->sb->s_umount); > f2fs_quota_sync(sbi->sb, -1); > if (locked) > up_read(&sbi->sb->s_umount); > - } > - > - f2fs_lock_all(sbi); > - if (__need_flush_quota(sbi)) { > - f2fs_unlock_all(sbi); > cond_resched(); > goto retry_flush_quotas; > } > @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) > */ > down_write(&sbi->node_change); > > - if (__need_flush_quota(sbi)) { > - up_write(&sbi->node_change); > - f2fs_unlock_all(sbi); > - goto retry_flush_quotas; > - } > - > if (get_pages(sbi, F2FS_DIRTY_IMETA)) { > up_write(&sbi->node_change); > f2fs_unlock_all(sbi); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 9674a85154b2..9bd2bf0f559b 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { > block_t unusable_block_count; /* # of blocks saved by last cp */ > > unsigned int nquota_files; /* # of quota sysfile */ > + struct rw_semaphore quota_sem; /* blocking cp for flags */ > > /* # of pages, see count_type */ > atomic_t nr_pages[NR_COUNT_TYPE]; > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 15d7e30bfc72..5a318399a2fa 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > int cnt; > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_writeback_dquots(sb, type); > if (ret) > goto out; > @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > out: > if (ret) > set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) > > static int f2fs_dquot_commit(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_commit(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_acquire(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_acquire(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > - > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_release(struct dquot *dquot) > { > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_release(dquot); > if (ret < 0) > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) > struct f2fs_sb_info *sbi = F2FS_SB(sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_mark_dquot_dirty(dquot); > > /* if we are using journalled quota */ > if (is_journalled_quota(sbi)) > set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > + up_read(&sbi->quota_sem); > return ret; > } > > static int f2fs_dquot_commit_info(struct super_block *sb, int type) > { > + struct f2fs_sb_info *sbi = F2FS_SB(sb); > int ret; > > + down_read(&sbi->quota_sem); > ret = dquot_commit_info(sb, type); > if (ret < 0) > - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > + up_read(&sbi->quota_sem); > return ret; > } > > @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > } > > init_rwsem(&sbi->cp_rwsem); > + init_rwsem(&sbi->quota_sem); > init_waitqueue_head(&sbi->cp_wait); > init_sb_info(sbi); > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-11 7:46 ` Chao Yu 2019-06-11 7:46 ` Chao Yu @ 2019-06-14 2:46 ` Jaegeuk Kim 2019-06-14 2:46 ` [f2fs-dev] " Jaegeuk Kim 2019-06-18 6:52 ` Chao Yu 1 sibling, 2 replies; 15+ messages in thread From: Jaegeuk Kim @ 2019-06-14 2:46 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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@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 enough to cover quota updates. Current stress & power-cut tests are running for several days without problem with this patch. > > Thanks, > > > > > fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- > > fs/f2fs/f2fs.h | 1 + > > fs/f2fs/super.c | 26 +++++++++++++++++++++----- > > 3 files changed, 41 insertions(+), 27 deletions(-) > > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > > index 89825261d474..43f65f0962e5 100644 > > --- a/fs/f2fs/checkpoint.c > > +++ b/fs/f2fs/checkpoint.c > > @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) > > > > static bool __need_flush_quota(struct f2fs_sb_info *sbi) > > { > > + bool ret = false; > > + > > if (!is_journalled_quota(sbi)) > > return false; > > - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > > - return false; > > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > > - return false; > > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > > - return true; > > - if (get_pages(sbi, F2FS_DIRTY_QDATA)) > > - return true; > > - return false; > > + > > + down_write(&sbi->quota_sem); > > + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { > > + ret = false; > > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { > > + ret = false; > > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { > > + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > + ret = true; > > + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { > > + ret = true; > > + } > > + up_write(&sbi->quota_sem); > > + return ret; > > } > > > > /* > > @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) > > blk_start_plug(&plug); > > > > retry_flush_quotas: > > + f2fs_lock_all(sbi); > > if (__need_flush_quota(sbi)) { > > int locked; > > > > if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { > > set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); > > - f2fs_lock_all(sbi); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > goto retry_flush_dents; > > } > > - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > + f2fs_unlock_all(sbi); > > > > /* only failed during mount/umount/freeze/quotactl */ > > locked = down_read_trylock(&sbi->sb->s_umount); > > f2fs_quota_sync(sbi->sb, -1); > > if (locked) > > up_read(&sbi->sb->s_umount); > > - } > > - > > - f2fs_lock_all(sbi); > > - if (__need_flush_quota(sbi)) { > > - f2fs_unlock_all(sbi); > > cond_resched(); > > goto retry_flush_quotas; > > } > > @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) > > */ > > down_write(&sbi->node_change); > > > > - if (__need_flush_quota(sbi)) { > > - up_write(&sbi->node_change); > > - f2fs_unlock_all(sbi); > > - goto retry_flush_quotas; > > - } > > - > > if (get_pages(sbi, F2FS_DIRTY_IMETA)) { > > up_write(&sbi->node_change); > > f2fs_unlock_all(sbi); > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 9674a85154b2..9bd2bf0f559b 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { > > block_t unusable_block_count; /* # of blocks saved by last cp */ > > > > unsigned int nquota_files; /* # of quota sysfile */ > > + struct rw_semaphore quota_sem; /* blocking cp for flags */ > > > > /* # of pages, see count_type */ > > atomic_t nr_pages[NR_COUNT_TYPE]; > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index 15d7e30bfc72..5a318399a2fa 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > > int cnt; > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_writeback_dquots(sb, type); > > if (ret) > > goto out; > > @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > > out: > > if (ret) > > set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) > > > > static int f2fs_dquot_commit(struct dquot *dquot) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_commit(dquot); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > static int f2fs_dquot_acquire(struct dquot *dquot) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_acquire(dquot); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > > - > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > static int f2fs_dquot_release(struct dquot *dquot) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_release(dquot); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) > > struct f2fs_sb_info *sbi = F2FS_SB(sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_mark_dquot_dirty(dquot); > > > > /* if we are using journalled quota */ > > if (is_journalled_quota(sbi)) > > set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > static int f2fs_dquot_commit_info(struct super_block *sb, int type) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_commit_info(sb, type); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > > } > > > > init_rwsem(&sbi->cp_rwsem); > > + init_rwsem(&sbi->quota_sem); > > init_waitqueue_head(&sbi->cp_wait); > > init_sb_info(sbi); > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-14 2:46 ` Jaegeuk Kim @ 2019-06-14 2:46 ` Jaegeuk Kim 2019-06-18 6:52 ` Chao Yu 1 sibling, 0 replies; 15+ messages in thread From: Jaegeuk Kim @ 2019-06-14 2:46 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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@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 enough to cover quota updates. Current stress & power-cut tests are running for several days without problem with this patch. > > Thanks, > > > > > fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- > > fs/f2fs/f2fs.h | 1 + > > fs/f2fs/super.c | 26 +++++++++++++++++++++----- > > 3 files changed, 41 insertions(+), 27 deletions(-) > > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > > index 89825261d474..43f65f0962e5 100644 > > --- a/fs/f2fs/checkpoint.c > > +++ b/fs/f2fs/checkpoint.c > > @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) > > > > static bool __need_flush_quota(struct f2fs_sb_info *sbi) > > { > > + bool ret = false; > > + > > if (!is_journalled_quota(sbi)) > > return false; > > - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > > - return false; > > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > > - return false; > > - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > > - return true; > > - if (get_pages(sbi, F2FS_DIRTY_QDATA)) > > - return true; > > - return false; > > + > > + down_write(&sbi->quota_sem); > > + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { > > + ret = false; > > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { > > + ret = false; > > + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { > > + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > + ret = true; > > + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { > > + ret = true; > > + } > > + up_write(&sbi->quota_sem); > > + return ret; > > } > > > > /* > > @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) > > blk_start_plug(&plug); > > > > retry_flush_quotas: > > + f2fs_lock_all(sbi); > > if (__need_flush_quota(sbi)) { > > int locked; > > > > if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { > > set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); > > - f2fs_lock_all(sbi); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > goto retry_flush_dents; > > } > > - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > + f2fs_unlock_all(sbi); > > > > /* only failed during mount/umount/freeze/quotactl */ > > locked = down_read_trylock(&sbi->sb->s_umount); > > f2fs_quota_sync(sbi->sb, -1); > > if (locked) > > up_read(&sbi->sb->s_umount); > > - } > > - > > - f2fs_lock_all(sbi); > > - if (__need_flush_quota(sbi)) { > > - f2fs_unlock_all(sbi); > > cond_resched(); > > goto retry_flush_quotas; > > } > > @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) > > */ > > down_write(&sbi->node_change); > > > > - if (__need_flush_quota(sbi)) { > > - up_write(&sbi->node_change); > > - f2fs_unlock_all(sbi); > > - goto retry_flush_quotas; > > - } > > - > > if (get_pages(sbi, F2FS_DIRTY_IMETA)) { > > up_write(&sbi->node_change); > > f2fs_unlock_all(sbi); > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 9674a85154b2..9bd2bf0f559b 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { > > block_t unusable_block_count; /* # of blocks saved by last cp */ > > > > unsigned int nquota_files; /* # of quota sysfile */ > > + struct rw_semaphore quota_sem; /* blocking cp for flags */ > > > > /* # of pages, see count_type */ > > atomic_t nr_pages[NR_COUNT_TYPE]; > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index 15d7e30bfc72..5a318399a2fa 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > > int cnt; > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_writeback_dquots(sb, type); > > if (ret) > > goto out; > > @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > > out: > > if (ret) > > set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) > > > > static int f2fs_dquot_commit(struct dquot *dquot) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_commit(dquot); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > static int f2fs_dquot_acquire(struct dquot *dquot) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_acquire(dquot); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > > - > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > static int f2fs_dquot_release(struct dquot *dquot) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_release(dquot); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) > > struct f2fs_sb_info *sbi = F2FS_SB(sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_mark_dquot_dirty(dquot); > > > > /* if we are using journalled quota */ > > if (is_journalled_quota(sbi)) > > set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > > > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > static int f2fs_dquot_commit_info(struct super_block *sb, int type) > > { > > + struct f2fs_sb_info *sbi = F2FS_SB(sb); > > int ret; > > > > + down_read(&sbi->quota_sem); > > ret = dquot_commit_info(sb, type); > > if (ret < 0) > > - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > > + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > > + up_read(&sbi->quota_sem); > > return ret; > > } > > > > @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > > } > > > > init_rwsem(&sbi->cp_rwsem); > > + init_rwsem(&sbi->quota_sem); > > init_waitqueue_head(&sbi->cp_wait); > > init_sb_info(sbi); > > > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-14 2:46 ` Jaegeuk Kim 2019-06-14 2:46 ` [f2fs-dev] " Jaegeuk Kim @ 2019-06-18 6:52 ` Chao Yu 2019-06-19 17:26 ` Jaegeuk Kim 1 sibling, 1 reply; 15+ messages in thread From: Chao Yu @ 2019-06-18 6:52 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel 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@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? Thanks, > enough to cover quota updates. Current stress & power-cut tests are running for > several days without problem with this patch. > >> >> Thanks, >> >>> >>> fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- >>> fs/f2fs/f2fs.h | 1 + >>> fs/f2fs/super.c | 26 +++++++++++++++++++++----- >>> 3 files changed, 41 insertions(+), 27 deletions(-) >>> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >>> index 89825261d474..43f65f0962e5 100644 >>> --- a/fs/f2fs/checkpoint.c >>> +++ b/fs/f2fs/checkpoint.c >>> @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) >>> >>> static bool __need_flush_quota(struct f2fs_sb_info *sbi) >>> { >>> + bool ret = false; >>> + >>> if (!is_journalled_quota(sbi)) >>> return false; >>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) >>> - return false; >>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) >>> - return false; >>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) >>> - return true; >>> - if (get_pages(sbi, F2FS_DIRTY_QDATA)) >>> - return true; >>> - return false; >>> + >>> + down_write(&sbi->quota_sem); >>> + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { >>> + ret = false; >>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { >>> + ret = false; >>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { >>> + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>> + ret = true; >>> + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { >>> + ret = true; >>> + } >>> + up_write(&sbi->quota_sem); >>> + return ret; >>> } >>> >>> /* >>> @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) >>> blk_start_plug(&plug); >>> >>> retry_flush_quotas: >>> + f2fs_lock_all(sbi); >>> if (__need_flush_quota(sbi)) { >>> int locked; >>> >>> if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { >>> set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); >>> - f2fs_lock_all(sbi); >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>> goto retry_flush_dents; >>> } >>> - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>> + f2fs_unlock_all(sbi); >>> >>> /* only failed during mount/umount/freeze/quotactl */ >>> locked = down_read_trylock(&sbi->sb->s_umount); >>> f2fs_quota_sync(sbi->sb, -1); >>> if (locked) >>> up_read(&sbi->sb->s_umount); >>> - } >>> - >>> - f2fs_lock_all(sbi); >>> - if (__need_flush_quota(sbi)) { >>> - f2fs_unlock_all(sbi); >>> cond_resched(); >>> goto retry_flush_quotas; >>> } >>> @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) >>> */ >>> down_write(&sbi->node_change); >>> >>> - if (__need_flush_quota(sbi)) { >>> - up_write(&sbi->node_change); >>> - f2fs_unlock_all(sbi); >>> - goto retry_flush_quotas; >>> - } >>> - >>> if (get_pages(sbi, F2FS_DIRTY_IMETA)) { >>> up_write(&sbi->node_change); >>> f2fs_unlock_all(sbi); >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 9674a85154b2..9bd2bf0f559b 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { >>> block_t unusable_block_count; /* # of blocks saved by last cp */ >>> >>> unsigned int nquota_files; /* # of quota sysfile */ >>> + struct rw_semaphore quota_sem; /* blocking cp for flags */ >>> >>> /* # of pages, see count_type */ >>> atomic_t nr_pages[NR_COUNT_TYPE]; >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 15d7e30bfc72..5a318399a2fa 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) >>> int cnt; >>> int ret; >>> >>> + down_read(&sbi->quota_sem); >>> ret = dquot_writeback_dquots(sb, type); >>> if (ret) >>> goto out; >>> @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) >>> out: >>> if (ret) >>> set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); >>> + up_read(&sbi->quota_sem); >>> return ret; >>> } >>> >>> @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) >>> >>> static int f2fs_dquot_commit(struct dquot *dquot) >>> { >>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); >>> int ret; >>> >>> + down_read(&sbi->quota_sem); >>> ret = dquot_commit(dquot); >>> if (ret < 0) >>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>> + up_read(&sbi->quota_sem); >>> return ret; >>> } >>> >>> static int f2fs_dquot_acquire(struct dquot *dquot) >>> { >>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); >>> int ret; >>> >>> + down_read(&sbi->quota_sem); >>> ret = dquot_acquire(dquot); >>> if (ret < 0) >>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); >>> - >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>> + up_read(&sbi->quota_sem); >>> return ret; >>> } >>> >>> static int f2fs_dquot_release(struct dquot *dquot) >>> { >>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); >>> int ret; >>> >>> + down_read(&sbi->quota_sem); >>> ret = dquot_release(dquot); >>> if (ret < 0) >>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>> + up_read(&sbi->quota_sem); >>> return ret; >>> } >>> >>> @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) >>> struct f2fs_sb_info *sbi = F2FS_SB(sb); >>> int ret; >>> >>> + down_read(&sbi->quota_sem); >>> ret = dquot_mark_dquot_dirty(dquot); >>> >>> /* if we are using journalled quota */ >>> if (is_journalled_quota(sbi)) >>> set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>> >>> + up_read(&sbi->quota_sem); >>> return ret; >>> } >>> >>> static int f2fs_dquot_commit_info(struct super_block *sb, int type) >>> { >>> + struct f2fs_sb_info *sbi = F2FS_SB(sb); >>> int ret; >>> >>> + down_read(&sbi->quota_sem); >>> ret = dquot_commit_info(sb, type); >>> if (ret < 0) >>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>> + up_read(&sbi->quota_sem); >>> return ret; >>> } >>> >>> @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) >>> } >>> >>> init_rwsem(&sbi->cp_rwsem); >>> + init_rwsem(&sbi->quota_sem); >>> init_waitqueue_head(&sbi->cp_wait); >>> init_sb_info(sbi); >>> >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-18 6:52 ` Chao Yu @ 2019-06-19 17:26 ` Jaegeuk Kim 2019-06-20 2:03 ` Chao Yu 0 siblings, 1 reply; 15+ messages in thread From: Jaegeuk Kim @ 2019-06-19 17:26 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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@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. > > Thanks, > > > enough to cover quota updates. Current stress & power-cut tests are running for > > several days without problem with this patch. > > > >> > >> Thanks, > >> > >>> > >>> fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- > >>> fs/f2fs/f2fs.h | 1 + > >>> fs/f2fs/super.c | 26 +++++++++++++++++++++----- > >>> 3 files changed, 41 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > >>> index 89825261d474..43f65f0962e5 100644 > >>> --- a/fs/f2fs/checkpoint.c > >>> +++ b/fs/f2fs/checkpoint.c > >>> @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) > >>> > >>> static bool __need_flush_quota(struct f2fs_sb_info *sbi) > >>> { > >>> + bool ret = false; > >>> + > >>> if (!is_journalled_quota(sbi)) > >>> return false; > >>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > >>> - return false; > >>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > >>> - return false; > >>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > >>> - return true; > >>> - if (get_pages(sbi, F2FS_DIRTY_QDATA)) > >>> - return true; > >>> - return false; > >>> + > >>> + down_write(&sbi->quota_sem); > >>> + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { > >>> + ret = false; > >>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { > >>> + ret = false; > >>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { > >>> + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>> + ret = true; > >>> + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { > >>> + ret = true; > >>> + } > >>> + up_write(&sbi->quota_sem); > >>> + return ret; > >>> } > >>> > >>> /* > >>> @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) > >>> blk_start_plug(&plug); > >>> > >>> retry_flush_quotas: > >>> + f2fs_lock_all(sbi); > >>> if (__need_flush_quota(sbi)) { > >>> int locked; > >>> > >>> if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { > >>> set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); > >>> - f2fs_lock_all(sbi); > >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>> goto retry_flush_dents; > >>> } > >>> - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>> + f2fs_unlock_all(sbi); > >>> > >>> /* only failed during mount/umount/freeze/quotactl */ > >>> locked = down_read_trylock(&sbi->sb->s_umount); > >>> f2fs_quota_sync(sbi->sb, -1); > >>> if (locked) > >>> up_read(&sbi->sb->s_umount); > >>> - } > >>> - > >>> - f2fs_lock_all(sbi); > >>> - if (__need_flush_quota(sbi)) { > >>> - f2fs_unlock_all(sbi); > >>> cond_resched(); > >>> goto retry_flush_quotas; > >>> } > >>> @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) > >>> */ > >>> down_write(&sbi->node_change); > >>> > >>> - if (__need_flush_quota(sbi)) { > >>> - up_write(&sbi->node_change); > >>> - f2fs_unlock_all(sbi); > >>> - goto retry_flush_quotas; > >>> - } > >>> - > >>> if (get_pages(sbi, F2FS_DIRTY_IMETA)) { > >>> up_write(&sbi->node_change); > >>> f2fs_unlock_all(sbi); > >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >>> index 9674a85154b2..9bd2bf0f559b 100644 > >>> --- a/fs/f2fs/f2fs.h > >>> +++ b/fs/f2fs/f2fs.h > >>> @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { > >>> block_t unusable_block_count; /* # of blocks saved by last cp */ > >>> > >>> unsigned int nquota_files; /* # of quota sysfile */ > >>> + struct rw_semaphore quota_sem; /* blocking cp for flags */ > >>> > >>> /* # of pages, see count_type */ > >>> atomic_t nr_pages[NR_COUNT_TYPE]; > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>> index 15d7e30bfc72..5a318399a2fa 100644 > >>> --- a/fs/f2fs/super.c > >>> +++ b/fs/f2fs/super.c > >>> @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > >>> int cnt; > >>> int ret; > >>> > >>> + down_read(&sbi->quota_sem); > >>> ret = dquot_writeback_dquots(sb, type); > >>> if (ret) > >>> goto out; > >>> @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > >>> out: > >>> if (ret) > >>> set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > >>> + up_read(&sbi->quota_sem); > >>> return ret; > >>> } > >>> > >>> @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) > >>> > >>> static int f2fs_dquot_commit(struct dquot *dquot) > >>> { > >>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > >>> int ret; > >>> > >>> + down_read(&sbi->quota_sem); > >>> ret = dquot_commit(dquot); > >>> if (ret < 0) > >>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>> + up_read(&sbi->quota_sem); > >>> return ret; > >>> } > >>> > >>> static int f2fs_dquot_acquire(struct dquot *dquot) > >>> { > >>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > >>> int ret; > >>> > >>> + down_read(&sbi->quota_sem); > >>> ret = dquot_acquire(dquot); > >>> if (ret < 0) > >>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > >>> - > >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>> + up_read(&sbi->quota_sem); > >>> return ret; > >>> } > >>> > >>> static int f2fs_dquot_release(struct dquot *dquot) > >>> { > >>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > >>> int ret; > >>> > >>> + down_read(&sbi->quota_sem); > >>> ret = dquot_release(dquot); > >>> if (ret < 0) > >>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>> + up_read(&sbi->quota_sem); > >>> return ret; > >>> } > >>> > >>> @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) > >>> struct f2fs_sb_info *sbi = F2FS_SB(sb); > >>> int ret; > >>> > >>> + down_read(&sbi->quota_sem); > >>> ret = dquot_mark_dquot_dirty(dquot); > >>> > >>> /* if we are using journalled quota */ > >>> if (is_journalled_quota(sbi)) > >>> set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>> > >>> + up_read(&sbi->quota_sem); > >>> return ret; > >>> } > >>> > >>> static int f2fs_dquot_commit_info(struct super_block *sb, int type) > >>> { > >>> + struct f2fs_sb_info *sbi = F2FS_SB(sb); > >>> int ret; > >>> > >>> + down_read(&sbi->quota_sem); > >>> ret = dquot_commit_info(sb, type); > >>> if (ret < 0) > >>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > >>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>> + up_read(&sbi->quota_sem); > >>> return ret; > >>> } > >>> > >>> @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > >>> } > >>> > >>> init_rwsem(&sbi->cp_rwsem); > >>> + init_rwsem(&sbi->quota_sem); > >>> init_waitqueue_head(&sbi->cp_wait); > >>> init_sb_info(sbi); > >>> > >>> > > . > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-19 17:26 ` Jaegeuk Kim @ 2019-06-20 2:03 ` Chao Yu 2019-06-21 17:38 ` Jaegeuk Kim 0 siblings, 1 reply; 15+ messages in thread From: Chao Yu @ 2019-06-20 2:03 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel 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@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 Thanks, > >> >> Thanks, >> >>> enough to cover quota updates. Current stress & power-cut tests are running for >>> several days without problem with this patch. >>> >>>> >>>> Thanks, >>>> >>>>> >>>>> fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- >>>>> fs/f2fs/f2fs.h | 1 + >>>>> fs/f2fs/super.c | 26 +++++++++++++++++++++----- >>>>> 3 files changed, 41 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >>>>> index 89825261d474..43f65f0962e5 100644 >>>>> --- a/fs/f2fs/checkpoint.c >>>>> +++ b/fs/f2fs/checkpoint.c >>>>> @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) >>>>> >>>>> static bool __need_flush_quota(struct f2fs_sb_info *sbi) >>>>> { >>>>> + bool ret = false; >>>>> + >>>>> if (!is_journalled_quota(sbi)) >>>>> return false; >>>>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) >>>>> - return false; >>>>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) >>>>> - return false; >>>>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) >>>>> - return true; >>>>> - if (get_pages(sbi, F2FS_DIRTY_QDATA)) >>>>> - return true; >>>>> - return false; >>>>> + >>>>> + down_write(&sbi->quota_sem); >>>>> + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { >>>>> + ret = false; >>>>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { >>>>> + ret = false; >>>>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { >>>>> + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>>>> + ret = true; >>>>> + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { >>>>> + ret = true; >>>>> + } >>>>> + up_write(&sbi->quota_sem); >>>>> + return ret; >>>>> } >>>>> >>>>> /* >>>>> @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) >>>>> blk_start_plug(&plug); >>>>> >>>>> retry_flush_quotas: >>>>> + f2fs_lock_all(sbi); >>>>> if (__need_flush_quota(sbi)) { >>>>> int locked; >>>>> >>>>> if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { >>>>> set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); >>>>> - f2fs_lock_all(sbi); >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>>>> goto retry_flush_dents; >>>>> } >>>>> - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>>>> + f2fs_unlock_all(sbi); >>>>> >>>>> /* only failed during mount/umount/freeze/quotactl */ >>>>> locked = down_read_trylock(&sbi->sb->s_umount); >>>>> f2fs_quota_sync(sbi->sb, -1); >>>>> if (locked) >>>>> up_read(&sbi->sb->s_umount); >>>>> - } >>>>> - >>>>> - f2fs_lock_all(sbi); >>>>> - if (__need_flush_quota(sbi)) { >>>>> - f2fs_unlock_all(sbi); >>>>> cond_resched(); >>>>> goto retry_flush_quotas; >>>>> } >>>>> @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) >>>>> */ >>>>> down_write(&sbi->node_change); >>>>> >>>>> - if (__need_flush_quota(sbi)) { >>>>> - up_write(&sbi->node_change); >>>>> - f2fs_unlock_all(sbi); >>>>> - goto retry_flush_quotas; >>>>> - } >>>>> - >>>>> if (get_pages(sbi, F2FS_DIRTY_IMETA)) { >>>>> up_write(&sbi->node_change); >>>>> f2fs_unlock_all(sbi); >>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>>> index 9674a85154b2..9bd2bf0f559b 100644 >>>>> --- a/fs/f2fs/f2fs.h >>>>> +++ b/fs/f2fs/f2fs.h >>>>> @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { >>>>> block_t unusable_block_count; /* # of blocks saved by last cp */ >>>>> >>>>> unsigned int nquota_files; /* # of quota sysfile */ >>>>> + struct rw_semaphore quota_sem; /* blocking cp for flags */ >>>>> >>>>> /* # of pages, see count_type */ >>>>> atomic_t nr_pages[NR_COUNT_TYPE]; >>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>>> index 15d7e30bfc72..5a318399a2fa 100644 >>>>> --- a/fs/f2fs/super.c >>>>> +++ b/fs/f2fs/super.c >>>>> @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) >>>>> int cnt; >>>>> int ret; >>>>> >>>>> + down_read(&sbi->quota_sem); >>>>> ret = dquot_writeback_dquots(sb, type); >>>>> if (ret) >>>>> goto out; >>>>> @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) >>>>> out: >>>>> if (ret) >>>>> set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); >>>>> + up_read(&sbi->quota_sem); >>>>> return ret; >>>>> } >>>>> >>>>> @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) >>>>> >>>>> static int f2fs_dquot_commit(struct dquot *dquot) >>>>> { >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); >>>>> int ret; >>>>> >>>>> + down_read(&sbi->quota_sem); >>>>> ret = dquot_commit(dquot); >>>>> if (ret < 0) >>>>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>>>> + up_read(&sbi->quota_sem); >>>>> return ret; >>>>> } >>>>> >>>>> static int f2fs_dquot_acquire(struct dquot *dquot) >>>>> { >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); >>>>> int ret; >>>>> >>>>> + down_read(&sbi->quota_sem); >>>>> ret = dquot_acquire(dquot); >>>>> if (ret < 0) >>>>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); >>>>> - >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>>>> + up_read(&sbi->quota_sem); >>>>> return ret; >>>>> } >>>>> >>>>> static int f2fs_dquot_release(struct dquot *dquot) >>>>> { >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); >>>>> int ret; >>>>> >>>>> + down_read(&sbi->quota_sem); >>>>> ret = dquot_release(dquot); >>>>> if (ret < 0) >>>>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>>>> + up_read(&sbi->quota_sem); >>>>> return ret; >>>>> } >>>>> >>>>> @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) >>>>> struct f2fs_sb_info *sbi = F2FS_SB(sb); >>>>> int ret; >>>>> >>>>> + down_read(&sbi->quota_sem); >>>>> ret = dquot_mark_dquot_dirty(dquot); >>>>> >>>>> /* if we are using journalled quota */ >>>>> if (is_journalled_quota(sbi)) >>>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); >>>>> >>>>> + up_read(&sbi->quota_sem); >>>>> return ret; >>>>> } >>>>> >>>>> static int f2fs_dquot_commit_info(struct super_block *sb, int type) >>>>> { >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(sb); >>>>> int ret; >>>>> >>>>> + down_read(&sbi->quota_sem); >>>>> ret = dquot_commit_info(sb, type); >>>>> if (ret < 0) >>>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); >>>>> + up_read(&sbi->quota_sem); >>>>> return ret; >>>>> } >>>>> >>>>> @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) >>>>> } >>>>> >>>>> init_rwsem(&sbi->cp_rwsem); >>>>> + init_rwsem(&sbi->quota_sem); >>>>> init_waitqueue_head(&sbi->cp_wait); >>>>> init_sb_info(sbi); >>>>> >>>>> >>> . >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-20 2:03 ` Chao Yu @ 2019-06-21 17:38 ` Jaegeuk Kim 2019-06-21 17:51 ` Jaegeuk Kim 0 siblings, 1 reply; 15+ messages in thread From: Jaegeuk Kim @ 2019-06-21 17:38 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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@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? > > Thanks, > > > > >> > >> Thanks, > >> > >>> enough to cover quota updates. Current stress & power-cut tests are running for > >>> several days without problem with this patch. > >>> > >>>> > >>>> Thanks, > >>>> > >>>>> > >>>>> fs/f2fs/checkpoint.c | 41 +++++++++++++++++++---------------------- > >>>>> fs/f2fs/f2fs.h | 1 + > >>>>> fs/f2fs/super.c | 26 +++++++++++++++++++++----- > >>>>> 3 files changed, 41 insertions(+), 27 deletions(-) > >>>>> > >>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > >>>>> index 89825261d474..43f65f0962e5 100644 > >>>>> --- a/fs/f2fs/checkpoint.c > >>>>> +++ b/fs/f2fs/checkpoint.c > >>>>> @@ -1131,17 +1131,24 @@ static void __prepare_cp_block(struct f2fs_sb_info *sbi) > >>>>> > >>>>> static bool __need_flush_quota(struct f2fs_sb_info *sbi) > >>>>> { > >>>>> + bool ret = false; > >>>>> + > >>>>> if (!is_journalled_quota(sbi)) > >>>>> return false; > >>>>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) > >>>>> - return false; > >>>>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) > >>>>> - return false; > >>>>> - if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) > >>>>> - return true; > >>>>> - if (get_pages(sbi, F2FS_DIRTY_QDATA)) > >>>>> - return true; > >>>>> - return false; > >>>>> + > >>>>> + down_write(&sbi->quota_sem); > >>>>> + if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH)) { > >>>>> + ret = false; > >>>>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR)) { > >>>>> + ret = false; > >>>>> + } else if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_FLUSH)) { > >>>>> + clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>>>> + ret = true; > >>>>> + } else if (get_pages(sbi, F2FS_DIRTY_QDATA)) { > >>>>> + ret = true; > >>>>> + } > >>>>> + up_write(&sbi->quota_sem); > >>>>> + return ret; > >>>>> } > >>>>> > >>>>> /* > >>>>> @@ -1160,26 +1167,22 @@ static int block_operations(struct f2fs_sb_info *sbi) > >>>>> blk_start_plug(&plug); > >>>>> > >>>>> retry_flush_quotas: > >>>>> + f2fs_lock_all(sbi); > >>>>> if (__need_flush_quota(sbi)) { > >>>>> int locked; > >>>>> > >>>>> if (++cnt > DEFAULT_RETRY_QUOTA_FLUSH_COUNT) { > >>>>> set_sbi_flag(sbi, SBI_QUOTA_SKIP_FLUSH); > >>>>> - f2fs_lock_all(sbi); > >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>>>> goto retry_flush_dents; > >>>>> } > >>>>> - clear_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>>>> + f2fs_unlock_all(sbi); > >>>>> > >>>>> /* only failed during mount/umount/freeze/quotactl */ > >>>>> locked = down_read_trylock(&sbi->sb->s_umount); > >>>>> f2fs_quota_sync(sbi->sb, -1); > >>>>> if (locked) > >>>>> up_read(&sbi->sb->s_umount); > >>>>> - } > >>>>> - > >>>>> - f2fs_lock_all(sbi); > >>>>> - if (__need_flush_quota(sbi)) { > >>>>> - f2fs_unlock_all(sbi); > >>>>> cond_resched(); > >>>>> goto retry_flush_quotas; > >>>>> } > >>>>> @@ -1201,12 +1204,6 @@ static int block_operations(struct f2fs_sb_info *sbi) > >>>>> */ > >>>>> down_write(&sbi->node_change); > >>>>> > >>>>> - if (__need_flush_quota(sbi)) { > >>>>> - up_write(&sbi->node_change); > >>>>> - f2fs_unlock_all(sbi); > >>>>> - goto retry_flush_quotas; > >>>>> - } > >>>>> - > >>>>> if (get_pages(sbi, F2FS_DIRTY_IMETA)) { > >>>>> up_write(&sbi->node_change); > >>>>> f2fs_unlock_all(sbi); > >>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >>>>> index 9674a85154b2..9bd2bf0f559b 100644 > >>>>> --- a/fs/f2fs/f2fs.h > >>>>> +++ b/fs/f2fs/f2fs.h > >>>>> @@ -1253,6 +1253,7 @@ struct f2fs_sb_info { > >>>>> block_t unusable_block_count; /* # of blocks saved by last cp */ > >>>>> > >>>>> unsigned int nquota_files; /* # of quota sysfile */ > >>>>> + struct rw_semaphore quota_sem; /* blocking cp for flags */ > >>>>> > >>>>> /* # of pages, see count_type */ > >>>>> atomic_t nr_pages[NR_COUNT_TYPE]; > >>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>>>> index 15d7e30bfc72..5a318399a2fa 100644 > >>>>> --- a/fs/f2fs/super.c > >>>>> +++ b/fs/f2fs/super.c > >>>>> @@ -1964,6 +1964,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > >>>>> int cnt; > >>>>> int ret; > >>>>> > >>>>> + down_read(&sbi->quota_sem); > >>>>> ret = dquot_writeback_dquots(sb, type); > >>>>> if (ret) > >>>>> goto out; > >>>>> @@ -2001,6 +2002,7 @@ int f2fs_quota_sync(struct super_block *sb, int type) > >>>>> out: > >>>>> if (ret) > >>>>> set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > >>>>> + up_read(&sbi->quota_sem); > >>>>> return ret; > >>>>> } > >>>>> > >>>>> @@ -2094,32 +2096,40 @@ static void f2fs_truncate_quota_inode_pages(struct super_block *sb) > >>>>> > >>>>> static int f2fs_dquot_commit(struct dquot *dquot) > >>>>> { > >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > >>>>> int ret; > >>>>> > >>>>> + down_read(&sbi->quota_sem); > >>>>> ret = dquot_commit(dquot); > >>>>> if (ret < 0) > >>>>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>>>> + up_read(&sbi->quota_sem); > >>>>> return ret; > >>>>> } > >>>>> > >>>>> static int f2fs_dquot_acquire(struct dquot *dquot) > >>>>> { > >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > >>>>> int ret; > >>>>> > >>>>> + down_read(&sbi->quota_sem); > >>>>> ret = dquot_acquire(dquot); > >>>>> if (ret < 0) > >>>>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > >>>>> - > >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>>>> + up_read(&sbi->quota_sem); > >>>>> return ret; > >>>>> } > >>>>> > >>>>> static int f2fs_dquot_release(struct dquot *dquot) > >>>>> { > >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(dquot->dq_sb); > >>>>> int ret; > >>>>> > >>>>> + down_read(&sbi->quota_sem); > >>>>> ret = dquot_release(dquot); > >>>>> if (ret < 0) > >>>>> - set_sbi_flag(F2FS_SB(dquot->dq_sb), SBI_QUOTA_NEED_REPAIR); > >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>>>> + up_read(&sbi->quota_sem); > >>>>> return ret; > >>>>> } > >>>>> > >>>>> @@ -2129,22 +2139,27 @@ static int f2fs_dquot_mark_dquot_dirty(struct dquot *dquot) > >>>>> struct f2fs_sb_info *sbi = F2FS_SB(sb); > >>>>> int ret; > >>>>> > >>>>> + down_read(&sbi->quota_sem); > >>>>> ret = dquot_mark_dquot_dirty(dquot); > >>>>> > >>>>> /* if we are using journalled quota */ > >>>>> if (is_journalled_quota(sbi)) > >>>>> set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH); > >>>>> > >>>>> + up_read(&sbi->quota_sem); > >>>>> return ret; > >>>>> } > >>>>> > >>>>> static int f2fs_dquot_commit_info(struct super_block *sb, int type) > >>>>> { > >>>>> + struct f2fs_sb_info *sbi = F2FS_SB(sb); > >>>>> int ret; > >>>>> > >>>>> + down_read(&sbi->quota_sem); > >>>>> ret = dquot_commit_info(sb, type); > >>>>> if (ret < 0) > >>>>> - set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR); > >>>>> + set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR); > >>>>> + up_read(&sbi->quota_sem); > >>>>> return ret; > >>>>> } > >>>>> > >>>>> @@ -3253,6 +3268,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > >>>>> } > >>>>> > >>>>> init_rwsem(&sbi->cp_rwsem); > >>>>> + init_rwsem(&sbi->quota_sem); > >>>>> init_waitqueue_head(&sbi->cp_wait); > >>>>> init_sb_info(sbi); > >>>>> > >>>>> > >>> . > >>> > > . > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-21 17:38 ` Jaegeuk Kim @ 2019-06-21 17:51 ` Jaegeuk Kim 2019-06-24 1:06 ` Chao Yu 0 siblings, 1 reply; 15+ messages in thread From: Jaegeuk Kim @ 2019-06-21 17:51 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel 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@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 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes 2019-06-21 17:51 ` Jaegeuk Kim @ 2019-06-24 1:06 ` Chao Yu 0 siblings, 0 replies; 15+ messages in thread From: Chao Yu @ 2019-06-24 1:06 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel 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 <jaegeuk@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? 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 Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-06-24 1:06 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-30 3:31 [PATCH] f2fs: add a rw_sem to cover quota flag changes Jaegeuk Kim 2019-05-30 14:01 ` [f2fs-dev] " Chao Yu 2019-05-30 17:57 ` [PATCH v2] " Jaegeuk Kim 2019-06-04 9:48 ` Chao Yu 2019-06-04 18:36 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 2019-06-11 7:46 ` Chao Yu 2019-06-11 7:46 ` Chao Yu 2019-06-14 2:46 ` Jaegeuk Kim 2019-06-14 2:46 ` [f2fs-dev] " Jaegeuk Kim 2019-06-18 6:52 ` Chao Yu 2019-06-19 17:26 ` Jaegeuk Kim 2019-06-20 2:03 ` Chao Yu 2019-06-21 17:38 ` Jaegeuk Kim 2019-06-21 17:51 ` Jaegeuk Kim 2019-06-24 1:06 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).