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

[Chao Yu]

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@xxxxxxxxxx>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> v3 from v2:
>>>>>>>>  - refactor to fix quota corruption issue
>>>>>>>>   : it seems that the previous scenario is not real and no deadlock case was
>>>>>>>>     encountered.
>>>>>>>
>>>>>>> - f2fs_dquot_commit
>>>>>>>  - down_read(&sbi->quota_sem)
>>>>>>> 					- block_operation
>>>>>>> 					 - f2fs_lock_all
>>>>>>> 					  - need_flush_quota
>>>>>>> 					   - down_write(&sbi->quota_sem)
>>>>>>>   - f2fs_quota_write
>>>>>>>    - f2fs_lock_op
>>>>>>>
>>>>>>> Why can't this happen?
>>>>>>>
>>>>>>> Once more question, should we hold quota_sem during checkpoint to avoid further
>>>>>>> quota update? f2fs_lock_op can do this job as well?
>>>>>>
>>>>>> I couldn't find write_dquot() call to make this happen, and f2fs_lock_op was not
>>>>>
>>>>> - f2fs_dquot_commit
>>>>>  - dquot_commit
>>>>>   ->commit_dqblk (v2_write_dquot)
>>>>>    - qtree_write_dquot
>>>>>     ->quota_write (f2fs_quota_write)
>>>>>      - f2fs_lock_op
>>>>>
>>>>> Do you mean there is no such way that calling f2fs_lock_op() from
>>>>> f2fs_quota_write()? So that deadlock condition is not existing?
>>>>
>>>> I mean write_dquot->f2fs_dquot_commit and block_operation seems not racing
>>>> together.
>>>
>>> quota ioctl has the path calling write_dquot->f2fs_dquot_commit as below, which
>>> can race with checkpoint().
>>>
>>> - do_quotactl
>>>  - sb->s_qcop->quota_sync (f2fs_quota_sync)
>>>   - down_read(&sbi->quota_sem);      ----  First
>>>    - dquot_writeback_dquots
>>>     - sb->dq_op->write_dquot (f2fs_dquot_commit)
>>> 							- block_operation can race here
>>>      - down_read(&sbi->quota_sem);   ----  Second
>>
>> Adding f2fs_lock_op() in f2fs_quota_sync() should be fine?
> 
> Something like this?

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