* [PATCH] f2fs: avoid race between zero_range and background GC
@ 2018-07-26 10:45 ` Chao Yu
0 siblings, 0 replies; 15+ messages in thread
From: Chao Yu @ 2018-07-26 10:45 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
Thread A Background GC
- f2fs_zero_range
- truncate_pagecache_range
- gc_data_segment
- get_read_data_page
- move_data_page
- set_page_dirty
- set_cold_data
- f2fs_do_zero_range
- dn->data_blkaddr = NEW_ADDR;
- f2fs_set_data_blkaddr
Actually, we don't need to set dirty & checked flag on the page, since
all valid data in the page should be zeroed by zero_range().
Use i_gc_rwsem[WRITE] to avoid such race condition.
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
fs/f2fs/file.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 267ec3794e1e..7bd2412a8c37 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
if (ret)
return ret;
+ down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
down_write(&F2FS_I(inode)->i_mmap_sem);
ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
if (ret)
@@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
}
out_sem:
up_write(&F2FS_I(inode)->i_mmap_sem);
+ up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
return ret;
}
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] f2fs: avoid race between zero_range and background GC
@ 2018-07-26 10:45 ` Chao Yu
0 siblings, 0 replies; 15+ messages in thread
From: Chao Yu @ 2018-07-26 10:45 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
Thread A Background GC
- f2fs_zero_range
- truncate_pagecache_range
- gc_data_segment
- get_read_data_page
- move_data_page
- set_page_dirty
- set_cold_data
- f2fs_do_zero_range
- dn->data_blkaddr = NEW_ADDR;
- f2fs_set_data_blkaddr
Actually, we don't need to set dirty & checked flag on the page, since
all valid data in the page should be zeroed by zero_range().
Use i_gc_rwsem[WRITE] to avoid such race condition.
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
fs/f2fs/file.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 267ec3794e1e..7bd2412a8c37 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
if (ret)
return ret;
+ down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
down_write(&F2FS_I(inode)->i_mmap_sem);
ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
if (ret)
@@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
}
out_sem:
up_write(&F2FS_I(inode)->i_mmap_sem);
+ up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
return ret;
}
--
2.18.0.rc1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-26 10:45 ` Chao Yu
(?)
@ 2018-07-27 10:29 ` Jaegeuk Kim
2018-07-27 12:16 ` Chao Yu
-1 siblings, 1 reply; 15+ messages in thread
From: Jaegeuk Kim @ 2018-07-27 10:29 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao
On 07/26, Chao Yu wrote:
> Thread A Background GC
> - f2fs_zero_range
> - truncate_pagecache_range
> - gc_data_segment
> - get_read_data_page
> - move_data_page
> - set_page_dirty
> - set_cold_data
> - f2fs_do_zero_range
> - dn->data_blkaddr = NEW_ADDR;
> - f2fs_set_data_blkaddr
>
> Actually, we don't need to set dirty & checked flag on the page, since
> all valid data in the page should be zeroed by zero_range().
But, it doesn't matter too much, right?
> Use i_gc_rwsem[WRITE] to avoid such race condition.
Hope to avoid abusing i_gc_rwsem[] tho.
>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> fs/f2fs/file.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 267ec3794e1e..7bd2412a8c37 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> if (ret)
> return ret;
>
> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> down_write(&F2FS_I(inode)->i_mmap_sem);
> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
> if (ret)
> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> }
> out_sem:
> up_write(&F2FS_I(inode)->i_mmap_sem);
> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>
> return ret;
> }
> --
> 2.18.0.rc1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-27 10:29 ` Jaegeuk Kim
@ 2018-07-27 12:16 ` Chao Yu
2018-07-29 2:02 ` Jaegeuk Kim
0 siblings, 1 reply; 15+ messages in thread
From: Chao Yu @ 2018-07-27 12:16 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel
On 2018/7/27 18:29, Jaegeuk Kim wrote:
> On 07/26, Chao Yu wrote:
>> Thread A Background GC
>> - f2fs_zero_range
>> - truncate_pagecache_range
>> - gc_data_segment
>> - get_read_data_page
>> - move_data_page
>> - set_page_dirty
>> - set_cold_data
>> - f2fs_do_zero_range
>> - dn->data_blkaddr = NEW_ADDR;
>> - f2fs_set_data_blkaddr
>>
>> Actually, we don't need to set dirty & checked flag on the page, since
>> all valid data in the page should be zeroed by zero_range().
>
> But, it doesn't matter too much, right?
No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
zero_range() should be wrong, as zeroed page contains valid user data.
>
>> Use i_gc_rwsem[WRITE] to avoid such race condition.
>
> Hope to avoid abusing i_gc_rwsem[] tho.
Agreed, let's try avoiding until we have to use it.
Thanks,
>
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fs/f2fs/file.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 267ec3794e1e..7bd2412a8c37 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>> if (ret)
>> return ret;
>>
>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>> down_write(&F2FS_I(inode)->i_mmap_sem);
>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
>> if (ret)
>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>> }
>> out_sem:
>> up_write(&F2FS_I(inode)->i_mmap_sem);
>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>
>> return ret;
>> }
>> --
>> 2.18.0.rc1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-27 12:16 ` Chao Yu
@ 2018-07-29 2:02 ` Jaegeuk Kim
0 siblings, 0 replies; 15+ messages in thread
From: Jaegeuk Kim @ 2018-07-29 2:02 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 07/27, Chao Yu wrote:
> On 2018/7/27 18:29, Jaegeuk Kim wrote:
> > On 07/26, Chao Yu wrote:
> >> Thread A Background GC
> >> - f2fs_zero_range
> >> - truncate_pagecache_range
> >> - gc_data_segment
> >> - get_read_data_page
> >> - move_data_page
> >> - set_page_dirty
> >> - set_cold_data
> >> - f2fs_do_zero_range
> >> - dn->data_blkaddr = NEW_ADDR;
> >> - f2fs_set_data_blkaddr
> >>
> >> Actually, we don't need to set dirty & checked flag on the page, since
> >> all valid data in the page should be zeroed by zero_range().
> >
> > But, it doesn't matter too much, right?
>
> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
> zero_range() should be wrong, as zeroed page contains valid user data.
How about truncating page caches after block address change or doing it twice
before and after?
>
> >
> >> Use i_gc_rwsem[WRITE] to avoid such race condition.
> >
> > Hope to avoid abusing i_gc_rwsem[] tho.
>
> Agreed, let's try avoiding until we have to use it.
>
> Thanks,
>
> >
> >>
> >> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >> ---
> >> fs/f2fs/file.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index 267ec3794e1e..7bd2412a8c37 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >> if (ret)
> >> return ret;
> >>
> >> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >> down_write(&F2FS_I(inode)->i_mmap_sem);
> >> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
> >> if (ret)
> >> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >> }
> >> out_sem:
> >> up_write(&F2FS_I(inode)->i_mmap_sem);
> >> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>
> >> return ret;
> >> }
> >> --
> >> 2.18.0.rc1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
@ 2018-07-29 2:02 ` Jaegeuk Kim
0 siblings, 0 replies; 15+ messages in thread
From: Jaegeuk Kim @ 2018-07-29 2:02 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 07/27, Chao Yu wrote:
> On 2018/7/27 18:29, Jaegeuk Kim wrote:
> > On 07/26, Chao Yu wrote:
> >> Thread A Background GC
> >> - f2fs_zero_range
> >> - truncate_pagecache_range
> >> - gc_data_segment
> >> - get_read_data_page
> >> - move_data_page
> >> - set_page_dirty
> >> - set_cold_data
> >> - f2fs_do_zero_range
> >> - dn->data_blkaddr = NEW_ADDR;
> >> - f2fs_set_data_blkaddr
> >>
> >> Actually, we don't need to set dirty & checked flag on the page, since
> >> all valid data in the page should be zeroed by zero_range().
> >
> > But, it doesn't matter too much, right?
>
> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
> zero_range() should be wrong, as zeroed page contains valid user data.
How about truncating page caches after block address change or doing it twice
before and after?
>
> >
> >> Use i_gc_rwsem[WRITE] to avoid such race condition.
> >
> > Hope to avoid abusing i_gc_rwsem[] tho.
>
> Agreed, let's try avoiding until we have to use it.
>
> Thanks,
>
> >
> >>
> >> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >> ---
> >> fs/f2fs/file.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index 267ec3794e1e..7bd2412a8c37 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >> if (ret)
> >> return ret;
> >>
> >> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >> down_write(&F2FS_I(inode)->i_mmap_sem);
> >> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
> >> if (ret)
> >> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >> }
> >> out_sem:
> >> up_write(&F2FS_I(inode)->i_mmap_sem);
> >> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>
> >> return ret;
> >> }
> >> --
> >> 2.18.0.rc1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-29 2:02 ` Jaegeuk Kim
(?)
@ 2018-07-29 2:53 ` Chao Yu
2018-07-29 2:59 ` Jaegeuk Kim
-1 siblings, 1 reply; 15+ messages in thread
From: Chao Yu @ 2018-07-29 2:53 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 2018/7/29 10:02, Jaegeuk Kim wrote:
> On 07/27, Chao Yu wrote:
>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
>>> On 07/26, Chao Yu wrote:
>>>> Thread A Background GC
>>>> - f2fs_zero_range
>>>> - truncate_pagecache_range
>>>> - gc_data_segment
>>>> - get_read_data_page
>>>> - move_data_page
>>>> - set_page_dirty
>>>> - set_cold_data
>>>> - f2fs_do_zero_range
>>>> - dn->data_blkaddr = NEW_ADDR;
>>>> - f2fs_set_data_blkaddr
>>>>
>>>> Actually, we don't need to set dirty & checked flag on the page, since
>>>> all valid data in the page should be zeroed by zero_range().
>>>
>>> But, it doesn't matter too much, right?
>>
>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
>> zero_range() should be wrong, as zeroed page contains valid user data.
>
> How about truncating page caches after block address change or doing it twice
> before and after?
Thread A Background GC
- f2fs_zero_range
- truncate_pagecache_range
- gc_data_segment
- get_read_data_page
- move_data_page
- set_page_dirty
- set_cold_data
- f2fs_do_zero_range
- dn->data_blkaddr = NEW_ADDR;
- f2fs_set_data_blkaddr
bdi-flusher
- __write_data_page
- f2fs_update_data_blkaddr
: data_blkaddr has been updated here.
- truncate_pagecache_range
: data & dnode has been writebacked before page cache truncation?
How about this case?
Thanks,
>
>>
>>>
>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
>>>
>>> Hope to avoid abusing i_gc_rwsem[] tho.
>>
>> Agreed, let's try avoiding until we have to use it.
>>
>> Thanks,
>>
>>>
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>> fs/f2fs/file.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index 267ec3794e1e..7bd2412a8c37 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>> if (ret)
>>>> return ret;
>>>>
>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
>>>> if (ret)
>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>> }
>>>> out_sem:
>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>
>>>> return ret;
>>>> }
>>>> --
>>>> 2.18.0.rc1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-29 2:53 ` Chao Yu
@ 2018-07-29 2:59 ` Jaegeuk Kim
2018-07-29 3:03 ` Chao Yu
0 siblings, 1 reply; 15+ messages in thread
From: Jaegeuk Kim @ 2018-07-29 2:59 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 07/29, Chao Yu wrote:
> On 2018/7/29 10:02, Jaegeuk Kim wrote:
> > On 07/27, Chao Yu wrote:
> >> On 2018/7/27 18:29, Jaegeuk Kim wrote:
> >>> On 07/26, Chao Yu wrote:
> >>>> Thread A Background GC
> >>>> - f2fs_zero_range
> >>>> - truncate_pagecache_range
> >>>> - gc_data_segment
> >>>> - get_read_data_page
> >>>> - move_data_page
> >>>> - set_page_dirty
> >>>> - set_cold_data
> >>>> - f2fs_do_zero_range
> >>>> - dn->data_blkaddr = NEW_ADDR;
> >>>> - f2fs_set_data_blkaddr
> >>>>
> >>>> Actually, we don't need to set dirty & checked flag on the page, since
> >>>> all valid data in the page should be zeroed by zero_range().
> >>>
> >>> But, it doesn't matter too much, right?
> >>
> >> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
> >> zero_range() should be wrong, as zeroed page contains valid user data.
> >
> > How about truncating page caches after block address change or doing it twice
> > before and after?
>
> Thread A Background GC
> - f2fs_zero_range
> - truncate_pagecache_range
> - gc_data_segment
> - get_read_data_page
> - move_data_page
> - set_page_dirty
> - set_cold_data
> - f2fs_do_zero_range
> - dn->data_blkaddr = NEW_ADDR;
> - f2fs_set_data_blkaddr
> bdi-flusher
> - __write_data_page
> - f2fs_update_data_blkaddr
> : data_blkaddr has been updated here.
> - truncate_pagecache_range
> : data & dnode has been writebacked before page cache truncation?
>
> How about this case?
So, truncating pages under dnode lock can address it?
>
> Thanks,
>
> >
> >>
> >>>
> >>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
> >>>
> >>> Hope to avoid abusing i_gc_rwsem[] tho.
> >>
> >> Agreed, let's try avoiding until we have to use it.
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >>>> ---
> >>>> fs/f2fs/file.c | 2 ++
> >>>> 1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>>> index 267ec3794e1e..7bd2412a8c37 100644
> >>>> --- a/fs/f2fs/file.c
> >>>> +++ b/fs/f2fs/file.c
> >>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >>>> if (ret)
> >>>> return ret;
> >>>>
> >>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>>> down_write(&F2FS_I(inode)->i_mmap_sem);
> >>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
> >>>> if (ret)
> >>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >>>> }
> >>>> out_sem:
> >>>> up_write(&F2FS_I(inode)->i_mmap_sem);
> >>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>>>
> >>>> return ret;
> >>>> }
> >>>> --
> >>>> 2.18.0.rc1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-29 2:59 ` Jaegeuk Kim
@ 2018-07-29 3:03 ` Chao Yu
0 siblings, 0 replies; 15+ messages in thread
From: Chao Yu @ 2018-07-29 3:03 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 2018/7/29 10:59, Jaegeuk Kim wrote:
> On 07/29, Chao Yu wrote:
>> On 2018/7/29 10:02, Jaegeuk Kim wrote:
>>> On 07/27, Chao Yu wrote:
>>>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
>>>>> On 07/26, Chao Yu wrote:
>>>>>> Thread A Background GC
>>>>>> - f2fs_zero_range
>>>>>> - truncate_pagecache_range
>>>>>> - gc_data_segment
>>>>>> - get_read_data_page
>>>>>> - move_data_page
>>>>>> - set_page_dirty
>>>>>> - set_cold_data
>>>>>> - f2fs_do_zero_range
>>>>>> - dn->data_blkaddr = NEW_ADDR;
>>>>>> - f2fs_set_data_blkaddr
>>>>>>
>>>>>> Actually, we don't need to set dirty & checked flag on the page, since
>>>>>> all valid data in the page should be zeroed by zero_range().
>>>>>
>>>>> But, it doesn't matter too much, right?
>>>>
>>>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
>>>> zero_range() should be wrong, as zeroed page contains valid user data.
>>>
>>> How about truncating page caches after block address change or doing it twice
>>> before and after?
>>
>> Thread A Background GC
>> - f2fs_zero_range
>> - truncate_pagecache_range
>> - gc_data_segment
>> - get_read_data_page
>> - move_data_page
>> - set_page_dirty
>> - set_cold_data
>> - f2fs_do_zero_range
>> - dn->data_blkaddr = NEW_ADDR;
>> - f2fs_set_data_blkaddr
>> bdi-flusher
>> - __write_data_page
>> - f2fs_update_data_blkaddr
>> : data_blkaddr has been updated here.
>> - truncate_pagecache_range
>> : data & dnode has been writebacked before page cache truncation?
>>
>> How about this case?
>
> So, truncating pages under dnode lock can address it?
Normally, our lock dependency is
->writepage()
lock data page -> lock dnode page
here
lock dnode page -> truncate_pagecache_range::lock data page
Will easily cause deadlock?
Thanks,
>
>>
>> Thanks,
>>
>>>
>>>>
>>>>>
>>>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
>>>>>
>>>>> Hope to avoid abusing i_gc_rwsem[] tho.
>>>>
>>>> Agreed, let's try avoiding until we have to use it.
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>> ---
>>>>>> fs/f2fs/file.c | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 267ec3794e1e..7bd2412a8c37 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
>>>>>> if (ret)
>>>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>> }
>>>>>> out_sem:
>>>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>>
>>>>>> return ret;
>>>>>> }
>>>>>> --
>>>>>> 2.18.0.rc1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
@ 2018-07-29 3:03 ` Chao Yu
0 siblings, 0 replies; 15+ messages in thread
From: Chao Yu @ 2018-07-29 3:03 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
On 2018/7/29 10:59, Jaegeuk Kim wrote:
> On 07/29, Chao Yu wrote:
>> On 2018/7/29 10:02, Jaegeuk Kim wrote:
>>> On 07/27, Chao Yu wrote:
>>>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
>>>>> On 07/26, Chao Yu wrote:
>>>>>> Thread A Background GC
>>>>>> - f2fs_zero_range
>>>>>> - truncate_pagecache_range
>>>>>> - gc_data_segment
>>>>>> - get_read_data_page
>>>>>> - move_data_page
>>>>>> - set_page_dirty
>>>>>> - set_cold_data
>>>>>> - f2fs_do_zero_range
>>>>>> - dn->data_blkaddr = NEW_ADDR;
>>>>>> - f2fs_set_data_blkaddr
>>>>>>
>>>>>> Actually, we don't need to set dirty & checked flag on the page, since
>>>>>> all valid data in the page should be zeroed by zero_range().
>>>>>
>>>>> But, it doesn't matter too much, right?
>>>>
>>>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
>>>> zero_range() should be wrong, as zeroed page contains valid user data.
>>>
>>> How about truncating page caches after block address change or doing it twice
>>> before and after?
>>
>> Thread A Background GC
>> - f2fs_zero_range
>> - truncate_pagecache_range
>> - gc_data_segment
>> - get_read_data_page
>> - move_data_page
>> - set_page_dirty
>> - set_cold_data
>> - f2fs_do_zero_range
>> - dn->data_blkaddr = NEW_ADDR;
>> - f2fs_set_data_blkaddr
>> bdi-flusher
>> - __write_data_page
>> - f2fs_update_data_blkaddr
>> : data_blkaddr has been updated here.
>> - truncate_pagecache_range
>> : data & dnode has been writebacked before page cache truncation?
>>
>> How about this case?
>
> So, truncating pages under dnode lock can address it?
Normally, our lock dependency is
->writepage()
lock data page -> lock dnode page
here
lock dnode page -> truncate_pagecache_range::lock data page
Will easily cause deadlock?
Thanks,
>
>>
>> Thanks,
>>
>>>
>>>>
>>>>>
>>>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
>>>>>
>>>>> Hope to avoid abusing i_gc_rwsem[] tho.
>>>>
>>>> Agreed, let's try avoiding until we have to use it.
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>> ---
>>>>>> fs/f2fs/file.c | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 267ec3794e1e..7bd2412a8c37 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
>>>>>> if (ret)
>>>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>> }
>>>>>> out_sem:
>>>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>>
>>>>>> return ret;
>>>>>> }
>>>>>> --
>>>>>> 2.18.0.rc1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-29 3:03 ` Chao Yu
@ 2018-07-29 5:14 ` Jaegeuk Kim
-1 siblings, 0 replies; 15+ messages in thread
From: Jaegeuk Kim @ 2018-07-29 5:14 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 07/29, Chao Yu wrote:
> On 2018/7/29 10:59, Jaegeuk Kim wrote:
> > On 07/29, Chao Yu wrote:
> >> On 2018/7/29 10:02, Jaegeuk Kim wrote:
> >>> On 07/27, Chao Yu wrote:
> >>>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
> >>>>> On 07/26, Chao Yu wrote:
> >>>>>> Thread A Background GC
> >>>>>> - f2fs_zero_range
> >>>>>> - truncate_pagecache_range
> >>>>>> - gc_data_segment
> >>>>>> - get_read_data_page
> >>>>>> - move_data_page
> >>>>>> - set_page_dirty
> >>>>>> - set_cold_data
> >>>>>> - f2fs_do_zero_range
> >>>>>> - dn->data_blkaddr = NEW_ADDR;
> >>>>>> - f2fs_set_data_blkaddr
> >>>>>>
> >>>>>> Actually, we don't need to set dirty & checked flag on the page, since
> >>>>>> all valid data in the page should be zeroed by zero_range().
> >>>>>
> >>>>> But, it doesn't matter too much, right?
> >>>>
> >>>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
> >>>> zero_range() should be wrong, as zeroed page contains valid user data.
> >>>
> >>> How about truncating page caches after block address change or doing it twice
> >>> before and after?
> >>
> >> Thread A Background GC
> >> - f2fs_zero_range
> >> - truncate_pagecache_range
> >> - gc_data_segment
> >> - get_read_data_page
> >> - move_data_page
> >> - set_page_dirty
> >> - set_cold_data
> >> - f2fs_do_zero_range
> >> - dn->data_blkaddr = NEW_ADDR;
> >> - f2fs_set_data_blkaddr
> >> bdi-flusher
> >> - __write_data_page
> >> - f2fs_update_data_blkaddr
> >> : data_blkaddr has been updated here.
> >> - truncate_pagecache_range
> >> : data & dnode has been writebacked before page cache truncation?
> >>
> >> How about this case?
> >
> > So, truncating pages under dnode lock can address it?
>
> Normally, our lock dependency is
>
> ->writepage()
> lock data page -> lock dnode page
>
> here
> lock dnode page -> truncate_pagecache_range::lock data page
>
> Will easily cause deadlock?
Yeah. Can we add an inode flag to bypass GC in this case, then?
>
> Thanks,
>
> >
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
> >>>>>
> >>>>> Hope to avoid abusing i_gc_rwsem[] tho.
> >>>>
> >>>> Agreed, let's try avoiding until we have to use it.
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >>>>>> ---
> >>>>>> fs/f2fs/file.c | 2 ++
> >>>>>> 1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>>>>> index 267ec3794e1e..7bd2412a8c37 100644
> >>>>>> --- a/fs/f2fs/file.c
> >>>>>> +++ b/fs/f2fs/file.c
> >>>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >>>>>> if (ret)
> >>>>>> return ret;
> >>>>>>
> >>>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
> >>>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
> >>>>>> if (ret)
> >>>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >>>>>> }
> >>>>>> out_sem:
> >>>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
> >>>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>>>>>
> >>>>>> return ret;
> >>>>>> }
> >>>>>> --
> >>>>>> 2.18.0.rc1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] f2fs: avoid race between zero_range and background GC
@ 2018-07-29 5:14 ` Jaegeuk Kim
0 siblings, 0 replies; 15+ messages in thread
From: Jaegeuk Kim @ 2018-07-29 5:14 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 07/29, Chao Yu wrote:
> On 2018/7/29 10:59, Jaegeuk Kim wrote:
> > On 07/29, Chao Yu wrote:
> >> On 2018/7/29 10:02, Jaegeuk Kim wrote:
> >>> On 07/27, Chao Yu wrote:
> >>>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
> >>>>> On 07/26, Chao Yu wrote:
> >>>>>> Thread A Background GC
> >>>>>> - f2fs_zero_range
> >>>>>> - truncate_pagecache_range
> >>>>>> - gc_data_segment
> >>>>>> - get_read_data_page
> >>>>>> - move_data_page
> >>>>>> - set_page_dirty
> >>>>>> - set_cold_data
> >>>>>> - f2fs_do_zero_range
> >>>>>> - dn->data_blkaddr = NEW_ADDR;
> >>>>>> - f2fs_set_data_blkaddr
> >>>>>>
> >>>>>> Actually, we don't need to set dirty & checked flag on the page, since
> >>>>>> all valid data in the page should be zeroed by zero_range().
> >>>>>
> >>>>> But, it doesn't matter too much, right?
> >>>>
> >>>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
> >>>> zero_range() should be wrong, as zeroed page contains valid user data.
> >>>
> >>> How about truncating page caches after block address change or doing it twice
> >>> before and after?
> >>
> >> Thread A Background GC
> >> - f2fs_zero_range
> >> - truncate_pagecache_range
> >> - gc_data_segment
> >> - get_read_data_page
> >> - move_data_page
> >> - set_page_dirty
> >> - set_cold_data
> >> - f2fs_do_zero_range
> >> - dn->data_blkaddr = NEW_ADDR;
> >> - f2fs_set_data_blkaddr
> >> bdi-flusher
> >> - __write_data_page
> >> - f2fs_update_data_blkaddr
> >> : data_blkaddr has been updated here.
> >> - truncate_pagecache_range
> >> : data & dnode has been writebacked before page cache truncation?
> >>
> >> How about this case?
> >
> > So, truncating pages under dnode lock can address it?
>
> Normally, our lock dependency is
>
> ->writepage()
> lock data page -> lock dnode page
>
> here
> lock dnode page -> truncate_pagecache_range::lock data page
>
> Will easily cause deadlock?
Yeah. Can we add an inode flag to bypass GC in this case, then?
>
> Thanks,
>
> >
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
> >>>>>
> >>>>> Hope to avoid abusing i_gc_rwsem[] tho.
> >>>>
> >>>> Agreed, let's try avoiding until we have to use it.
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >>>>>> ---
> >>>>>> fs/f2fs/file.c | 2 ++
> >>>>>> 1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>>>>> index 267ec3794e1e..7bd2412a8c37 100644
> >>>>>> --- a/fs/f2fs/file.c
> >>>>>> +++ b/fs/f2fs/file.c
> >>>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >>>>>> if (ret)
> >>>>>> return ret;
> >>>>>>
> >>>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
> >>>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
> >>>>>> if (ret)
> >>>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> >>>>>> }
> >>>>>> out_sem:
> >>>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
> >>>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>>>>>
> >>>>>> return ret;
> >>>>>> }
> >>>>>> --
> >>>>>> 2.18.0.rc1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: avoid race between zero_range and background GC
2018-07-29 5:14 ` Jaegeuk Kim
(?)
@ 2018-07-30 1:38 ` Jaegeuk Kim
2018-07-30 1:55 ` Chao Yu
-1 siblings, 1 reply; 15+ messages in thread
From: Jaegeuk Kim @ 2018-07-30 1:38 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 07/28, Jaegeuk Kim wrote:
> On 07/29, Chao Yu wrote:
> > On 2018/7/29 10:59, Jaegeuk Kim wrote:
> > > On 07/29, Chao Yu wrote:
> > >> On 2018/7/29 10:02, Jaegeuk Kim wrote:
> > >>> On 07/27, Chao Yu wrote:
> > >>>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
> > >>>>> On 07/26, Chao Yu wrote:
> > >>>>>> Thread A Background GC
> > >>>>>> - f2fs_zero_range
> > >>>>>> - truncate_pagecache_range
> > >>>>>> - gc_data_segment
> > >>>>>> - get_read_data_page
> > >>>>>> - move_data_page
> > >>>>>> - set_page_dirty
> > >>>>>> - set_cold_data
> > >>>>>> - f2fs_do_zero_range
> > >>>>>> - dn->data_blkaddr = NEW_ADDR;
> > >>>>>> - f2fs_set_data_blkaddr
> > >>>>>>
> > >>>>>> Actually, we don't need to set dirty & checked flag on the page, since
> > >>>>>> all valid data in the page should be zeroed by zero_range().
> > >>>>>
> > >>>>> But, it doesn't matter too much, right?
> > >>>>
> > >>>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
> > >>>> zero_range() should be wrong, as zeroed page contains valid user data.
> > >>>
> > >>> How about truncating page caches after block address change or doing it twice
> > >>> before and after?
> > >>
> > >> Thread A Background GC
> > >> - f2fs_zero_range
> > >> - truncate_pagecache_range
> > >> - gc_data_segment
> > >> - get_read_data_page
> > >> - move_data_page
> > >> - set_page_dirty
> > >> - set_cold_data
> > >> - f2fs_do_zero_range
> > >> - dn->data_blkaddr = NEW_ADDR;
> > >> - f2fs_set_data_blkaddr
> > >> bdi-flusher
> > >> - __write_data_page
> > >> - f2fs_update_data_blkaddr
> > >> : data_blkaddr has been updated here.
> > >> - truncate_pagecache_range
> > >> : data & dnode has been writebacked before page cache truncation?
> > >>
> > >> How about this case?
> > >
> > > So, truncating pages under dnode lock can address it?
> >
> > Normally, our lock dependency is
> >
> > ->writepage()
> > lock data page -> lock dnode page
> >
> > here
> > lock dnode page -> truncate_pagecache_range::lock data page
> >
> > Will easily cause deadlock?
>
> Yeah. Can we add an inode flag to bypass GC in this case, then?
Hmm, BTW, how about using i_gc_rwsem[WRITE] in a very narrow scope?
for (index = pg_start; index < pg_end;) {
f2fs_lock_op();
down_write(i_gc_rwsem[WRITE]);
truncate_page_cache_range(index, index + 4k);
f2fs_do_zero_range(&dn, index, end);
up_write(i_gc_rwsem[WRITE]);
f2fs_unlock_op();
f2fs_balance_fs();
}
>
> >
> > Thanks,
> >
> > >
> > >>
> > >> Thanks,
> > >>
> > >>>
> > >>>>
> > >>>>>
> > >>>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
> > >>>>>
> > >>>>> Hope to avoid abusing i_gc_rwsem[] tho.
> > >>>>
> > >>>> Agreed, let's try avoiding until we have to use it.
> > >>>>
> > >>>> Thanks,
> > >>>>
> > >>>>>
> > >>>>>>
> > >>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > >>>>>> ---
> > >>>>>> fs/f2fs/file.c | 2 ++
> > >>>>>> 1 file changed, 2 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > >>>>>> index 267ec3794e1e..7bd2412a8c37 100644
> > >>>>>> --- a/fs/f2fs/file.c
> > >>>>>> +++ b/fs/f2fs/file.c
> > >>>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> > >>>>>> if (ret)
> > >>>>>> return ret;
> > >>>>>>
> > >>>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> > >>>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
> > >>>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
> > >>>>>> if (ret)
> > >>>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
> > >>>>>> }
> > >>>>>> out_sem:
> > >>>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
> > >>>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> > >>>>>>
> > >>>>>> return ret;
> > >>>>>> }
> > >>>>>> --
> > >>>>>> 2.18.0.rc1
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> 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] f2fs: avoid race between zero_range and background GC
2018-07-30 1:38 ` [f2fs-dev] " Jaegeuk Kim
@ 2018-07-30 1:55 ` Chao Yu
0 siblings, 0 replies; 15+ messages in thread
From: Chao Yu @ 2018-07-30 1:55 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 2018/7/30 9:38, Jaegeuk Kim wrote:
> On 07/28, Jaegeuk Kim wrote:
>> On 07/29, Chao Yu wrote:
>>> On 2018/7/29 10:59, Jaegeuk Kim wrote:
>>>> On 07/29, Chao Yu wrote:
>>>>> On 2018/7/29 10:02, Jaegeuk Kim wrote:
>>>>>> On 07/27, Chao Yu wrote:
>>>>>>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
>>>>>>>> On 07/26, Chao Yu wrote:
>>>>>>>>> Thread A Background GC
>>>>>>>>> - f2fs_zero_range
>>>>>>>>> - truncate_pagecache_range
>>>>>>>>> - gc_data_segment
>>>>>>>>> - get_read_data_page
>>>>>>>>> - move_data_page
>>>>>>>>> - set_page_dirty
>>>>>>>>> - set_cold_data
>>>>>>>>> - f2fs_do_zero_range
>>>>>>>>> - dn->data_blkaddr = NEW_ADDR;
>>>>>>>>> - f2fs_set_data_blkaddr
>>>>>>>>>
>>>>>>>>> Actually, we don't need to set dirty & checked flag on the page, since
>>>>>>>>> all valid data in the page should be zeroed by zero_range().
>>>>>>>>
>>>>>>>> But, it doesn't matter too much, right?
>>>>>>>
>>>>>>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
>>>>>>> zero_range() should be wrong, as zeroed page contains valid user data.
>>>>>>
>>>>>> How about truncating page caches after block address change or doing it twice
>>>>>> before and after?
>>>>>
>>>>> Thread A Background GC
>>>>> - f2fs_zero_range
>>>>> - truncate_pagecache_range
>>>>> - gc_data_segment
>>>>> - get_read_data_page
>>>>> - move_data_page
>>>>> - set_page_dirty
>>>>> - set_cold_data
>>>>> - f2fs_do_zero_range
>>>>> - dn->data_blkaddr = NEW_ADDR;
>>>>> - f2fs_set_data_blkaddr
>>>>> bdi-flusher
>>>>> - __write_data_page
>>>>> - f2fs_update_data_blkaddr
>>>>> : data_blkaddr has been updated here.
>>>>> - truncate_pagecache_range
>>>>> : data & dnode has been writebacked before page cache truncation?
>>>>>
>>>>> How about this case?
>>>>
>>>> So, truncating pages under dnode lock can address it?
>>>
>>> Normally, our lock dependency is
>>>
>>> ->writepage()
>>> lock data page -> lock dnode page
>>>
>>> here
>>> lock dnode page -> truncate_pagecache_range::lock data page
>>>
>>> Will easily cause deadlock?
>>
>> Yeah. Can we add an inode flag to bypass GC in this case, then?
>
> Hmm, BTW, how about using i_gc_rwsem[WRITE] in a very narrow scope?
Oh, I can see that you submitted a patch to change lock dependency to:
f2fs_lock_op() -> down_write(i_gc_rwsem[WRITE])
At a glance, I haven't see any place can cause deadlock now.
>
> for (index = pg_start; index < pg_end;) {
> f2fs_lock_op();
> down_write(i_gc_rwsem[WRITE]);
> truncate_page_cache_range(index, index + 4k);
> f2fs_do_zero_range(&dn, index, end);
> up_write(i_gc_rwsem[WRITE]);
> f2fs_unlock_op();
> f2fs_balance_fs();
> }
Let me update the patch as you suggested.
Thanks,
>
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
>>>>>>>>
>>>>>>>> Hope to avoid abusing i_gc_rwsem[] tho.
>>>>>>>
>>>>>>> Agreed, let's try avoiding until we have to use it.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>>>> ---
>>>>>>>>> fs/f2fs/file.c | 2 ++
>>>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>>>> index 267ec3794e1e..7bd2412a8c37 100644
>>>>>>>>> --- a/fs/f2fs/file.c
>>>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>>>>> if (ret)
>>>>>>>>> return ret;
>>>>>>>>>
>>>>>>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
>>>>>>>>> if (ret)
>>>>>>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>>>>> }
>>>>>>>>> out_sem:
>>>>>>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>>>>>
>>>>>>>>> return ret;
>>>>>>>>> }
>>>>>>>>> --
>>>>>>>>> 2.18.0.rc1
>>
>> ------------------------------------------------------------------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> _______________________________________________
>> 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] f2fs: avoid race between zero_range and background GC
@ 2018-07-30 1:55 ` Chao Yu
0 siblings, 0 replies; 15+ messages in thread
From: Chao Yu @ 2018-07-30 1:55 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 2018/7/30 9:38, Jaegeuk Kim wrote:
> On 07/28, Jaegeuk Kim wrote:
>> On 07/29, Chao Yu wrote:
>>> On 2018/7/29 10:59, Jaegeuk Kim wrote:
>>>> On 07/29, Chao Yu wrote:
>>>>> On 2018/7/29 10:02, Jaegeuk Kim wrote:
>>>>>> On 07/27, Chao Yu wrote:
>>>>>>> On 2018/7/27 18:29, Jaegeuk Kim wrote:
>>>>>>>> On 07/26, Chao Yu wrote:
>>>>>>>>> Thread A Background GC
>>>>>>>>> - f2fs_zero_range
>>>>>>>>> - truncate_pagecache_range
>>>>>>>>> - gc_data_segment
>>>>>>>>> - get_read_data_page
>>>>>>>>> - move_data_page
>>>>>>>>> - set_page_dirty
>>>>>>>>> - set_cold_data
>>>>>>>>> - f2fs_do_zero_range
>>>>>>>>> - dn->data_blkaddr = NEW_ADDR;
>>>>>>>>> - f2fs_set_data_blkaddr
>>>>>>>>>
>>>>>>>>> Actually, we don't need to set dirty & checked flag on the page, since
>>>>>>>>> all valid data in the page should be zeroed by zero_range().
>>>>>>>>
>>>>>>>> But, it doesn't matter too much, right?
>>>>>>>
>>>>>>> No, if the dirtied page is writebacked after f2fs_do_zero_range(), result of
>>>>>>> zero_range() should be wrong, as zeroed page contains valid user data.
>>>>>>
>>>>>> How about truncating page caches after block address change or doing it twice
>>>>>> before and after?
>>>>>
>>>>> Thread A Background GC
>>>>> - f2fs_zero_range
>>>>> - truncate_pagecache_range
>>>>> - gc_data_segment
>>>>> - get_read_data_page
>>>>> - move_data_page
>>>>> - set_page_dirty
>>>>> - set_cold_data
>>>>> - f2fs_do_zero_range
>>>>> - dn->data_blkaddr = NEW_ADDR;
>>>>> - f2fs_set_data_blkaddr
>>>>> bdi-flusher
>>>>> - __write_data_page
>>>>> - f2fs_update_data_blkaddr
>>>>> : data_blkaddr has been updated here.
>>>>> - truncate_pagecache_range
>>>>> : data & dnode has been writebacked before page cache truncation?
>>>>>
>>>>> How about this case?
>>>>
>>>> So, truncating pages under dnode lock can address it?
>>>
>>> Normally, our lock dependency is
>>>
>>> ->writepage()
>>> lock data page -> lock dnode page
>>>
>>> here
>>> lock dnode page -> truncate_pagecache_range::lock data page
>>>
>>> Will easily cause deadlock?
>>
>> Yeah. Can we add an inode flag to bypass GC in this case, then?
>
> Hmm, BTW, how about using i_gc_rwsem[WRITE] in a very narrow scope?
Oh, I can see that you submitted a patch to change lock dependency to:
f2fs_lock_op() -> down_write(i_gc_rwsem[WRITE])
At a glance, I haven't see any place can cause deadlock now.
>
> for (index = pg_start; index < pg_end;) {
> f2fs_lock_op();
> down_write(i_gc_rwsem[WRITE]);
> truncate_page_cache_range(index, index + 4k);
> f2fs_do_zero_range(&dn, index, end);
> up_write(i_gc_rwsem[WRITE]);
> f2fs_unlock_op();
> f2fs_balance_fs();
> }
Let me update the patch as you suggested.
Thanks,
>
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> Use i_gc_rwsem[WRITE] to avoid such race condition.
>>>>>>>>
>>>>>>>> Hope to avoid abusing i_gc_rwsem[] tho.
>>>>>>>
>>>>>>> Agreed, let's try avoiding until we have to use it.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>>>>> ---
>>>>>>>>> fs/f2fs/file.c | 2 ++
>>>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>>>> index 267ec3794e1e..7bd2412a8c37 100644
>>>>>>>>> --- a/fs/f2fs/file.c
>>>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>>>> @@ -1309,6 +1309,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>>>>> if (ret)
>>>>>>>>> return ret;
>>>>>>>>>
>>>>>>>>> + down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>>>>> down_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>>>>> ret = filemap_write_and_wait_range(mapping, offset, offset + len - 1);
>>>>>>>>> if (ret)
>>>>>>>>> @@ -1389,6 +1390,7 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
>>>>>>>>> }
>>>>>>>>> out_sem:
>>>>>>>>> up_write(&F2FS_I(inode)->i_mmap_sem);
>>>>>>>>> + up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>>>>>>>
>>>>>>>>> return ret;
>>>>>>>>> }
>>>>>>>>> --
>>>>>>>>> 2.18.0.rc1
>>
>> ------------------------------------------------------------------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> _______________________________________________
>> 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:[~2018-07-30 1:55 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-26 10:45 [PATCH] f2fs: avoid race between zero_range and background GC Chao Yu
2018-07-26 10:45 ` Chao Yu
2018-07-27 10:29 ` Jaegeuk Kim
2018-07-27 12:16 ` Chao Yu
2018-07-29 2:02 ` Jaegeuk Kim
2018-07-29 2:02 ` Jaegeuk Kim
2018-07-29 2:53 ` Chao Yu
2018-07-29 2:59 ` Jaegeuk Kim
2018-07-29 3:03 ` Chao Yu
2018-07-29 3:03 ` Chao Yu
2018-07-29 5:14 ` Jaegeuk Kim
2018-07-29 5:14 ` Jaegeuk Kim
2018-07-30 1:38 ` [f2fs-dev] " Jaegeuk Kim
2018-07-30 1:55 ` Chao Yu
2018-07-30 1:55 ` Chao Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.