2019-05-30 03:32:44

by Jaegeuk Kim

Subject: [PATCH] f2fs: add a rw_sem to cover quota flag changes

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 <[email protected]>
---
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

2019-05-30 14:05:11

by Chao Yu

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

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 <[email protected]>
> ---
> 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);
>
>

2019-05-30 17:58:42

by Jaegeuk Kim

Subject: Re: [PATCH v2] f2fs: add a rw_sem to cover quota flag changes

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 <[email protected]>
---

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

2019-06-04 09:58:40

by Chao Yu

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

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 <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

Thanks,

2019-06-04 18:39:08

by Jaegeuk Kim

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

Two paths to update quota and f2fs_lock_op:

1.
- lock_op
| - quota_update
`- unlock_op

2.
- quota_update
- lock_op
`- unlock_op

But, we need to make a transaction on quota_update + lock_op in #2 case.
So, this patch introduces:
1. lock_op
2. down_write
3. check __need_flush
4. up_write
5. if there is dirty quota entries, flush them
6. otherwise, good to go

Signed-off-by: Jaegeuk Kim <[email protected]>
---

v3 from v2:
- refactor to fix quota corruption issue
: it seems that the previous scenario is not real and no deadlock case was
encountered.

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

2019-06-11 08:09:24

by Chao Yu

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

On 2019/6/5 2:36, Jaegeuk Kim wrote:
> Two paths to update quota and f2fs_lock_op:
>
> 1.
> - lock_op
> | - quota_update
> `- unlock_op
>
> 2.
> - quota_update
> - lock_op
> `- unlock_op
>
> But, we need to make a transaction on quota_update + lock_op in #2 case.
> So, this patch introduces:
> 1. lock_op
> 2. down_write
> 3. check __need_flush
> 4. up_write
> 5. if there is dirty quota entries, flush them
> 6. otherwise, good to go
>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
>
> v3 from v2:
> - refactor to fix quota corruption issue
> : it seems that the previous scenario is not real and no deadlock case was
> encountered.

- f2fs_dquot_commit
- down_read(&sbi->quota_sem)
- block_operation
- f2fs_lock_all
- need_flush_quota
- down_write(&sbi->quota_sem)
- f2fs_quota_write
- f2fs_lock_op

Why can't this happen?

Once more question, should we hold quota_sem during checkpoint to avoid further
quota update? f2fs_lock_op can do this job as well?

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);
>
>

2019-06-14 02:47:17

by Jaegeuk Kim

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

On 06/11, Chao Yu wrote:
> On 2019/6/5 2:36, Jaegeuk Kim wrote:
> > Two paths to update quota and f2fs_lock_op:
> >
> > 1.
> > - lock_op
> > | - quota_update
> > `- unlock_op
> >
> > 2.
> > - quota_update
> > - lock_op
> > `- unlock_op
> >
> > But, we need to make a transaction on quota_update + lock_op in #2 case.
> > So, this patch introduces:
> > 1. lock_op
> > 2. down_write
> > 3. check __need_flush
> > 4. up_write
> > 5. if there is dirty quota entries, flush them
> > 6. otherwise, good to go
> >
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> >
> > v3 from v2:
> > - refactor to fix quota corruption issue
> > : it seems that the previous scenario is not real and no deadlock case was
> > encountered.
>
> - f2fs_dquot_commit
> - down_read(&sbi->quota_sem)
> - block_operation
> - f2fs_lock_all
> - need_flush_quota
> - down_write(&sbi->quota_sem)
> - f2fs_quota_write
> - f2fs_lock_op
>
> Why can't this happen?
>
> Once more question, should we hold quota_sem during checkpoint to avoid further
> quota update? f2fs_lock_op can do this job as well?

I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not
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);
> >
> >

2019-06-18 06:54:00

by Chao Yu

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

On 2019/6/14 10:46, Jaegeuk Kim wrote:
> On 06/11, Chao Yu wrote:
>> On 2019/6/5 2:36, Jaegeuk Kim wrote:
>>> Two paths to update quota and f2fs_lock_op:
>>>
>>> 1.
>>> - lock_op
>>> | - quota_update
>>> `- unlock_op
>>>
>>> 2.
>>> - quota_update
>>> - lock_op
>>> `- unlock_op
>>>
>>> But, we need to make a transaction on quota_update + lock_op in #2 case.
>>> So, this patch introduces:
>>> 1. lock_op
>>> 2. down_write
>>> 3. check __need_flush
>>> 4. up_write
>>> 5. if there is dirty quota entries, flush them
>>> 6. otherwise, good to go
>>>
>>> Signed-off-by: Jaegeuk Kim <[email protected]>
>>> ---
>>>
>>> v3 from v2:
>>> - refactor to fix quota corruption issue
>>> : it seems that the previous scenario is not real and no deadlock case was
>>> encountered.
>>
>> - f2fs_dquot_commit
>> - down_read(&sbi->quota_sem)
>> - block_operation
>> - f2fs_lock_all
>> - need_flush_quota
>> - down_write(&sbi->quota_sem)
>> - f2fs_quota_write
>> - f2fs_lock_op
>>
>> Why can't this happen?
>>
>> Once more question, should we hold quota_sem during checkpoint to avoid further
>> quota update? f2fs_lock_op can do this job as well?
>
> I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not

- f2fs_dquot_commit
- dquot_commit
->commit_dqblk (v2_write_dquot)
- qtree_write_dquot
->quota_write (f2fs_quota_write)
- f2fs_lock_op

Do you mean there is no such way that calling f2fs_lock_op() from
f2fs_quota_write()? So that deadlock condition is not existing?

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);
>>>
>>>
> .
>

2019-06-19 17:27:42

by Jaegeuk Kim

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

On 06/18, Chao Yu wrote:
> On 2019/6/14 10:46, Jaegeuk Kim wrote:
> > On 06/11, Chao Yu wrote:
> >> On 2019/6/5 2:36, Jaegeuk Kim wrote:
> >>> Two paths to update quota and f2fs_lock_op:
> >>>
> >>> 1.
> >>> - lock_op
> >>> | - quota_update
> >>> `- unlock_op
> >>>
> >>> 2.
> >>> - quota_update
> >>> - lock_op
> >>> `- unlock_op
> >>>
> >>> But, we need to make a transaction on quota_update + lock_op in #2 case.
> >>> So, this patch introduces:
> >>> 1. lock_op
> >>> 2. down_write
> >>> 3. check __need_flush
> >>> 4. up_write
> >>> 5. if there is dirty quota entries, flush them
> >>> 6. otherwise, good to go
> >>>
> >>> Signed-off-by: Jaegeuk Kim <[email protected]>
> >>> ---
> >>>
> >>> v3 from v2:
> >>> - refactor to fix quota corruption issue
> >>> : it seems that the previous scenario is not real and no deadlock case was
> >>> encountered.
> >>
> >> - f2fs_dquot_commit
> >> - down_read(&sbi->quota_sem)
> >> - block_operation
> >> - f2fs_lock_all
> >> - need_flush_quota
> >> - down_write(&sbi->quota_sem)
> >> - f2fs_quota_write
> >> - f2fs_lock_op
> >>
> >> Why can't this happen?
> >>
> >> Once more question, should we hold quota_sem during checkpoint to avoid further
> >> quota update? f2fs_lock_op can do this job as well?
> >
> > I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not
>
> - f2fs_dquot_commit
> - dquot_commit
> ->commit_dqblk (v2_write_dquot)
> - qtree_write_dquot
> ->quota_write (f2fs_quota_write)
> - f2fs_lock_op
>
> Do you mean there is no such way that calling f2fs_lock_op() from
> f2fs_quota_write()? So that deadlock condition is not existing?

I mean write_dquot->f2fs_dquot_commit and block_operation seems not racing
together.

>
> 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);
> >>>
> >>>
> > .
> >

2019-06-20 02:03:41

by Chao Yu

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

On 2019/6/20 1:26, Jaegeuk Kim wrote:
> On 06/18, Chao Yu wrote:
>> On 2019/6/14 10:46, Jaegeuk Kim wrote:
>>> On 06/11, Chao Yu wrote:
>>>> On 2019/6/5 2:36, Jaegeuk Kim wrote:
>>>>> Two paths to update quota and f2fs_lock_op:
>>>>>
>>>>> 1.
>>>>> - lock_op
>>>>> | - quota_update
>>>>> `- unlock_op
>>>>>
>>>>> 2.
>>>>> - quota_update
>>>>> - lock_op
>>>>> `- unlock_op
>>>>>
>>>>> But, we need to make a transaction on quota_update + lock_op in #2 case.
>>>>> So, this patch introduces:
>>>>> 1. lock_op
>>>>> 2. down_write
>>>>> 3. check __need_flush
>>>>> 4. up_write
>>>>> 5. if there is dirty quota entries, flush them
>>>>> 6. otherwise, good to go
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <[email protected]>
>>>>> ---
>>>>>
>>>>> v3 from v2:
>>>>> - refactor to fix quota corruption issue
>>>>> : it seems that the previous scenario is not real and no deadlock case was
>>>>> encountered.
>>>>
>>>> - f2fs_dquot_commit
>>>> - down_read(&sbi->quota_sem)
>>>> - block_operation
>>>> - f2fs_lock_all
>>>> - need_flush_quota
>>>> - down_write(&sbi->quota_sem)
>>>> - f2fs_quota_write
>>>> - f2fs_lock_op
>>>>
>>>> Why can't this happen?
>>>>
>>>> Once more question, should we hold quota_sem during checkpoint to avoid further
>>>> quota update? f2fs_lock_op can do this job as well?
>>>
>>> I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not
>>
>> - f2fs_dquot_commit
>> - dquot_commit
>> ->commit_dqblk (v2_write_dquot)
>> - qtree_write_dquot
>> ->quota_write (f2fs_quota_write)
>> - f2fs_lock_op
>>
>> Do you mean there is no such way that calling f2fs_lock_op() from
>> f2fs_quota_write()? So that deadlock condition is not existing?
>
> I mean write_dquot->f2fs_dquot_commit and block_operation seems not racing
> together.

quota ioctl has the path calling write_dquot->f2fs_dquot_commit as below, which
can race with checkpoint().

- do_quotactl
- sb->s_qcop->quota_sync (f2fs_quota_sync)
- down_read(&sbi->quota_sem); ---- First
- dquot_writeback_dquots
- sb->dq_op->write_dquot (f2fs_dquot_commit)
- block_operation can race here
- down_read(&sbi->quota_sem); ---- Second

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);
>>>>>
>>>>>
>>> .
>>>
> .
>

2019-06-21 17:38:29

by Jaegeuk Kim

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

On 06/20, Chao Yu wrote:
> On 2019/6/20 1:26, Jaegeuk Kim wrote:
> > On 06/18, Chao Yu wrote:
> >> On 2019/6/14 10:46, Jaegeuk Kim wrote:
> >>> On 06/11, Chao Yu wrote:
> >>>> On 2019/6/5 2:36, Jaegeuk Kim wrote:
> >>>>> Two paths to update quota and f2fs_lock_op:
> >>>>>
> >>>>> 1.
> >>>>> - lock_op
> >>>>> | - quota_update
> >>>>> `- unlock_op
> >>>>>
> >>>>> 2.
> >>>>> - quota_update
> >>>>> - lock_op
> >>>>> `- unlock_op
> >>>>>
> >>>>> But, we need to make a transaction on quota_update + lock_op in #2 case.
> >>>>> So, this patch introduces:
> >>>>> 1. lock_op
> >>>>> 2. down_write
> >>>>> 3. check __need_flush
> >>>>> 4. up_write
> >>>>> 5. if there is dirty quota entries, flush them
> >>>>> 6. otherwise, good to go
> >>>>>
> >>>>> Signed-off-by: Jaegeuk Kim <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> v3 from v2:
> >>>>> - refactor to fix quota corruption issue
> >>>>> : it seems that the previous scenario is not real and no deadlock case was
> >>>>> encountered.
> >>>>
> >>>> - f2fs_dquot_commit
> >>>> - down_read(&sbi->quota_sem)
> >>>> - block_operation
> >>>> - f2fs_lock_all
> >>>> - need_flush_quota
> >>>> - down_write(&sbi->quota_sem)
> >>>> - f2fs_quota_write
> >>>> - f2fs_lock_op
> >>>>
> >>>> Why can't this happen?
> >>>>
> >>>> Once more question, should we hold quota_sem during checkpoint to avoid further
> >>>> quota update? f2fs_lock_op can do this job as well?
> >>>
> >>> I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not
> >>
> >> - f2fs_dquot_commit
> >> - dquot_commit
> >> ->commit_dqblk (v2_write_dquot)
> >> - qtree_write_dquot
> >> ->quota_write (f2fs_quota_write)
> >> - f2fs_lock_op
> >>
> >> Do you mean there is no such way that calling f2fs_lock_op() from
> >> f2fs_quota_write()? So that deadlock condition is not existing?
> >
> > I mean write_dquot->f2fs_dquot_commit and block_operation seems not racing
> > together.
>
> quota ioctl has the path calling write_dquot->f2fs_dquot_commit as below, which
> can race with checkpoint().
>
> - do_quotactl
> - sb->s_qcop->quota_sync (f2fs_quota_sync)
> - down_read(&sbi->quota_sem); ---- First
> - dquot_writeback_dquots
> - sb->dq_op->write_dquot (f2fs_dquot_commit)
> - block_operation can race here
> - down_read(&sbi->quota_sem); ---- Second

Adding f2fs_lock_op() in f2fs_quota_sync() should be fine?

>
> 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);
> >>>>>
> >>>>>
> >>> .
> >>>
> > .
> >

2019-06-21 17:52:02

by Jaegeuk Kim

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

On 06/21, Jaegeuk Kim wrote:
> On 06/20, Chao Yu wrote:
> > On 2019/6/20 1:26, Jaegeuk Kim wrote:
> > > On 06/18, Chao Yu wrote:
> > >> On 2019/6/14 10:46, Jaegeuk Kim wrote:
> > >>> On 06/11, Chao Yu wrote:
> > >>>> On 2019/6/5 2:36, Jaegeuk Kim wrote:
> > >>>>> Two paths to update quota and f2fs_lock_op:
> > >>>>>
> > >>>>> 1.
> > >>>>> - lock_op
> > >>>>> | - quota_update
> > >>>>> `- unlock_op
> > >>>>>
> > >>>>> 2.
> > >>>>> - quota_update
> > >>>>> - lock_op
> > >>>>> `- unlock_op
> > >>>>>
> > >>>>> But, we need to make a transaction on quota_update + lock_op in #2 case.
> > >>>>> So, this patch introduces:
> > >>>>> 1. lock_op
> > >>>>> 2. down_write
> > >>>>> 3. check __need_flush
> > >>>>> 4. up_write
> > >>>>> 5. if there is dirty quota entries, flush them
> > >>>>> 6. otherwise, good to go
> > >>>>>
> > >>>>> Signed-off-by: Jaegeuk Kim <[email protected]>
> > >>>>> ---
> > >>>>>
> > >>>>> v3 from v2:
> > >>>>> - refactor to fix quota corruption issue
> > >>>>> : it seems that the previous scenario is not real and no deadlock case was
> > >>>>> encountered.
> > >>>>
> > >>>> - f2fs_dquot_commit
> > >>>> - down_read(&sbi->quota_sem)
> > >>>> - block_operation
> > >>>> - f2fs_lock_all
> > >>>> - need_flush_quota
> > >>>> - down_write(&sbi->quota_sem)
> > >>>> - f2fs_quota_write
> > >>>> - f2fs_lock_op
> > >>>>
> > >>>> Why can't this happen?
> > >>>>
> > >>>> Once more question, should we hold quota_sem during checkpoint to avoid further
> > >>>> quota update? f2fs_lock_op can do this job as well?
> > >>>
> > >>> I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not
> > >>
> > >> - f2fs_dquot_commit
> > >> - dquot_commit
> > >> ->commit_dqblk (v2_write_dquot)
> > >> - qtree_write_dquot
> > >> ->quota_write (f2fs_quota_write)
> > >> - f2fs_lock_op
> > >>
> > >> Do you mean there is no such way that calling f2fs_lock_op() from
> > >> f2fs_quota_write()? So that deadlock condition is not existing?
> > >
> > > I mean write_dquot->f2fs_dquot_commit and block_operation seems not racing
> > > together.
> >
> > quota ioctl has the path calling write_dquot->f2fs_dquot_commit as below, which
> > can race with checkpoint().
> >
> > - do_quotactl
> > - sb->s_qcop->quota_sync (f2fs_quota_sync)
> > - down_read(&sbi->quota_sem); ---- First
> > - dquot_writeback_dquots
> > - sb->dq_op->write_dquot (f2fs_dquot_commit)
> > - block_operation can race here
> > - down_read(&sbi->quota_sem); ---- Second
>
> Adding f2fs_lock_op() in f2fs_quota_sync() should be fine?

Something like this?

---
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

2019-06-24 01:52:45

by Chao Yu

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

On 2019/6/22 1:51, Jaegeuk Kim wrote:
> On 06/21, Jaegeuk Kim wrote:
>> On 06/20, Chao Yu wrote:
>>> On 2019/6/20 1:26, Jaegeuk Kim wrote:
>>>> On 06/18, Chao Yu wrote:
>>>>> On 2019/6/14 10:46, Jaegeuk Kim wrote:
>>>>>> On 06/11, Chao Yu wrote:
>>>>>>> On 2019/6/5 2:36, Jaegeuk Kim wrote:
>>>>>>>> Two paths to update quota and f2fs_lock_op:
>>>>>>>>
>>>>>>>> 1.
>>>>>>>> - lock_op
>>>>>>>> | - quota_update
>>>>>>>> `- unlock_op
>>>>>>>>
>>>>>>>> 2.
>>>>>>>> - quota_update
>>>>>>>> - lock_op
>>>>>>>> `- unlock_op
>>>>>>>>
>>>>>>>> But, we need to make a transaction on quota_update + lock_op in #2 case.
>>>>>>>> So, this patch introduces:
>>>>>>>> 1. lock_op
>>>>>>>> 2. down_write
>>>>>>>> 3. check __need_flush
>>>>>>>> 4. up_write
>>>>>>>> 5. if there is dirty quota entries, flush them
>>>>>>>> 6. otherwise, good to go
>>>>>>>>
>>>>>>>> Signed-off-by: Jaegeuk Kim <[email protected]>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> v3 from v2:
>>>>>>>> - refactor to fix quota corruption issue
>>>>>>>> : it seems that the previous scenario is not real and no deadlock case was
>>>>>>>> encountered.
>>>>>>>
>>>>>>> - f2fs_dquot_commit
>>>>>>> - down_read(&sbi->quota_sem)
>>>>>>> - block_operation
>>>>>>> - f2fs_lock_all
>>>>>>> - need_flush_quota
>>>>>>> - down_write(&sbi->quota_sem)
>>>>>>> - f2fs_quota_write
>>>>>>> - f2fs_lock_op
>>>>>>>
>>>>>>> Why can't this happen?
>>>>>>>
>>>>>>> Once more question, should we hold quota_sem during checkpoint to avoid further
>>>>>>> quota update? f2fs_lock_op can do this job as well?
>>>>>>
>>>>>> I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not
>>>>>
>>>>> - f2fs_dquot_commit
>>>>> - dquot_commit
>>>>> ->commit_dqblk (v2_write_dquot)
>>>>> - qtree_write_dquot
>>>>> ->quota_write (f2fs_quota_write)
>>>>> - f2fs_lock_op
>>>>>
>>>>> Do you mean there is no such way that calling f2fs_lock_op() from
>>>>> f2fs_quota_write()? So that deadlock condition is not existing?
>>>>
>>>> I mean write_dquot->f2fs_dquot_commit and block_operation seems not racing
>>>> together.
>>>
>>> quota ioctl has the path calling write_dquot->f2fs_dquot_commit as below, which
>>> can race with checkpoint().
>>>
>>> - do_quotactl
>>> - sb->s_qcop->quota_sync (f2fs_quota_sync)
>>> - down_read(&sbi->quota_sem); ---- First
>>> - dquot_writeback_dquots
>>> - sb->dq_op->write_dquot (f2fs_dquot_commit)
>>> - block_operation can race here
>>> - down_read(&sbi->quota_sem); ---- Second
>>
>> Adding f2fs_lock_op() in f2fs_quota_sync() should be fine?
>
> Something like this?

I'm okay with this diff. :)

Thanks,

>
> ---
> fs/f2fs/super.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 7f2829b1192e..1d33ca1a8c09 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1919,6 +1919,17 @@ int f2fs_quota_sync(struct super_block *sb, int type)
> int cnt;
> int ret;
>
> + /*
> + * do_quotactl
> + * f2fs_quota_sync
> + * down_read(quota_sem)
> + * dquot_writeback_dquots()
> + * f2fs_dquot_commit
> + * block_operation
> + * down_read(quota_sem)
> + */
> + f2fs_lock_op(sbi);
> +
> down_read(&sbi->quota_sem);
> ret = dquot_writeback_dquots(sb, type);
> if (ret)
> @@ -1958,6 +1969,7 @@ int f2fs_quota_sync(struct super_block *sb, int type)
> if (ret)
> set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
> up_read(&sbi->quota_sem);
> + f2fs_unlock_op(sbi);
> return ret;
> }
>
>