lkml.org 
[lkml]   [2019]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [f2fs-dev] [PATCH v3] f2fs: add a rw_sem to cover quota flag changes
From
Date
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;
> }
>
>

\
 
 \ /
  Last update: 2019-06-24 03:52    [W:0.065 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site