* [PATCH v2] f2fs: fix race between write_checkpoint and write_begin
@ 2018-11-14 11:34 Sheng Yong
2018-11-15 1:59 ` Chao Yu
0 siblings, 1 reply; 5+ messages in thread
From: Sheng Yong @ 2018-11-14 11:34 UTC (permalink / raw)
To: yuchao0, jaegeuk; +Cc: linux-f2fs-devel
The following race could lead to inconsistent SIT bitmap:
Task A Task B
====== ======
f2fs_write_checkpoint
block_operations
f2fs_lock_all
down_write(node_change)
down_write(node_write)
... sync ...
up_write(node_change)
f2fs_file_write_iter
set_inode_flag(FI_NO_PREALLOC)
......
f2fs_write_begin(index=0, has inline data)
prepare_write_begin
__do_map_lock(AIO) => down_read(node_change)
f2fs_convert_inline_page => update SIT
__do_map_lock(AIO) => up_read(node_change)
f2fs_flush_sit_entries <= inconsistent SIT
finish write checkpoint
sudden-power-off
If SPO occurs after checkpoint is finished, SIT bitmap will be set
incorrectly.
Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
v2->v1:
Sorry for late. We can use f2fs_lock_op directly, but it makes it a bit
complicate to unlock. So v2 still uses __do_map_lock with different flags
to avoid race.
Use F2FS_GET_BLOCK_PRE_AIO and F2FS_GET_BLOCK_DEFAULT to tell different
condition.
Thanks
---
fs/f2fs/data.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 6e0ffb1749ca..4abd47d82cdb 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2376,6 +2376,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
struct page *ipage;
bool locked = false;
struct extent_info ei = {0,0,0};
+ int flag = F2FS_GET_BLOCK_PRE_AIO;
int err = 0;
/*
@@ -2386,9 +2387,12 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
!is_inode_flag_set(inode, FI_NO_PREALLOC))
return 0;
+ if (f2fs_has_inline_data(inode) && pos + len > MAX_INLINE_DATA(inode))
+ /* avoid race between write CP and convert_inline_page */
+ flag = F2FS_GET_BLOCK_DEFAULT;
if (f2fs_has_inline_data(inode) ||
(pos & PAGE_MASK) >= i_size_read(inode)) {
- __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, true);
+ __do_map_lock(sbi, flag, true);
locked = true;
}
restart:
@@ -2424,8 +2428,8 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
err = f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE);
if (err || dn.data_blkaddr == NULL_ADDR) {
f2fs_put_dnode(&dn);
- __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO,
- true);
+ flag = F2FS_GET_BLOCK_PRE_AIO;
+ __do_map_lock(sbi, flag, true);
locked = true;
goto restart;
}
@@ -2439,7 +2443,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
f2fs_put_dnode(&dn);
unlock_out:
if (locked)
- __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, false);
+ __do_map_lock(sbi, flag, false);
return err;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] f2fs: fix race between write_checkpoint and write_begin
2018-11-14 11:34 [PATCH v2] f2fs: fix race between write_checkpoint and write_begin Sheng Yong
@ 2018-11-15 1:59 ` Chao Yu
2018-11-15 2:39 ` Sheng Yong
0 siblings, 1 reply; 5+ messages in thread
From: Chao Yu @ 2018-11-15 1:59 UTC (permalink / raw)
To: Sheng Yong, jaegeuk; +Cc: linux-f2fs-devel
On 2018/11/14 19:34, Sheng Yong wrote:
> The following race could lead to inconsistent SIT bitmap:
>
> Task A Task B
> ====== ======
> f2fs_write_checkpoint
> block_operations
> f2fs_lock_all
> down_write(node_change)
> down_write(node_write)
> ... sync ...
> up_write(node_change)
> f2fs_file_write_iter
> set_inode_flag(FI_NO_PREALLOC)
> ......
> f2fs_write_begin(index=0, has inline data)
> prepare_write_begin
> __do_map_lock(AIO) => down_read(node_change)
> f2fs_convert_inline_page => update SIT
> __do_map_lock(AIO) => up_read(node_change)
> f2fs_flush_sit_entries <= inconsistent SIT
> finish write checkpoint
> sudden-power-off
>
> If SPO occurs after checkpoint is finished, SIT bitmap will be set
> incorrectly.
>
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
> v2->v1:
> Sorry for late. We can use f2fs_lock_op directly, but it makes it a bit
> complicate to unlock. So v2 still uses __do_map_lock with different flags
> to avoid race.
>
> Use F2FS_GET_BLOCK_PRE_AIO and F2FS_GET_BLOCK_DEFAULT to tell different
> condition.
>
> Thanks
> ---
> fs/f2fs/data.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 6e0ffb1749ca..4abd47d82cdb 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2376,6 +2376,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> struct page *ipage;
> bool locked = false;
> struct extent_info ei = {0,0,0};
> + int flag = F2FS_GET_BLOCK_PRE_AIO;
> int err = 0;
>
> /*
> @@ -2386,9 +2387,12 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> !is_inode_flag_set(inode, FI_NO_PREALLOC))
> return 0;
>
> + if (f2fs_has_inline_data(inode) && pos + len > MAX_INLINE_DATA(inode))
> + /* avoid race between write CP and convert_inline_page */
> + flag = F2FS_GET_BLOCK_DEFAULT;
> if (f2fs_has_inline_data(inode) ||
> (pos & PAGE_MASK) >= i_size_read(inode)) {
> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, true);
> + __do_map_lock(sbi, flag, true);
> locked = true;
> }
> restart:
> @@ -2424,8 +2428,8 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> err = f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE);
> if (err || dn.data_blkaddr == NULL_ADDR) {
> f2fs_put_dnode(&dn);
> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO,
> - true);
> + flag = F2FS_GET_BLOCK_PRE_AIO;
> + __do_map_lock(sbi, flag, true);
It looks there is no different after you changed above two lines? I guess
you just want to keep parameter @flag be consistent in __do_{un,}map_lock
pair, right?
Anyway, it's okay to me.
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Thanks,
> locked = true;
> goto restart;
> }
> @@ -2439,7 +2443,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> f2fs_put_dnode(&dn);
> unlock_out:
> if (locked)
> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, false);
> + __do_map_lock(sbi, flag, false);
> return err;
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] f2fs: fix race between write_checkpoint and write_begin
2018-11-15 1:59 ` Chao Yu
@ 2018-11-15 2:39 ` Sheng Yong
2018-11-15 8:24 ` Jaegeuk Kim
0 siblings, 1 reply; 5+ messages in thread
From: Sheng Yong @ 2018-11-15 2:39 UTC (permalink / raw)
To: Chao Yu, jaegeuk; +Cc: linux-f2fs-devel
On 2018/11/15 9:59, Chao Yu wrote:
> On 2018/11/14 19:34, Sheng Yong wrote:
>> The following race could lead to inconsistent SIT bitmap:
>>
>> Task A Task B
>> ====== ======
>> f2fs_write_checkpoint
>> block_operations
>> f2fs_lock_all
>> down_write(node_change)
>> down_write(node_write)
>> ... sync ...
>> up_write(node_change)
>> f2fs_file_write_iter
>> set_inode_flag(FI_NO_PREALLOC)
>> ......
>> f2fs_write_begin(index=0, has inline data)
>> prepare_write_begin
>> __do_map_lock(AIO) => down_read(node_change)
>> f2fs_convert_inline_page => update SIT
>> __do_map_lock(AIO) => up_read(node_change)
>> f2fs_flush_sit_entries <= inconsistent SIT
>> finish write checkpoint
>> sudden-power-off
>>
>> If SPO occurs after checkpoint is finished, SIT bitmap will be set
>> incorrectly.
>>
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>> ---
>> v2->v1:
>> Sorry for late. We can use f2fs_lock_op directly, but it makes it a bit
>> complicate to unlock. So v2 still uses __do_map_lock with different flags
>> to avoid race.
>>
>> Use F2FS_GET_BLOCK_PRE_AIO and F2FS_GET_BLOCK_DEFAULT to tell different
>> condition.
>>
>> Thanks
>> ---
>> fs/f2fs/data.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 6e0ffb1749ca..4abd47d82cdb 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -2376,6 +2376,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>> struct page *ipage;
>> bool locked = false;
>> struct extent_info ei = {0,0,0};
>> + int flag = F2FS_GET_BLOCK_PRE_AIO;
>> int err = 0;
>>
>> /*
>> @@ -2386,9 +2387,12 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>> !is_inode_flag_set(inode, FI_NO_PREALLOC))
>> return 0;
>>
>> + if (f2fs_has_inline_data(inode) && pos + len > MAX_INLINE_DATA(inode))
>> + /* avoid race between write CP and convert_inline_page */
>> + flag = F2FS_GET_BLOCK_DEFAULT;
>> if (f2fs_has_inline_data(inode) ||
>> (pos & PAGE_MASK) >= i_size_read(inode)) {
>> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, true);
>> + __do_map_lock(sbi, flag, true);
>> locked = true;
>> }
>> restart:
>> @@ -2424,8 +2428,8 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>> err = f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE);
>> if (err || dn.data_blkaddr == NULL_ADDR) {
>> f2fs_put_dnode(&dn);
>> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO,
>> - true);
>> + flag = F2FS_GET_BLOCK_PRE_AIO;
>> + __do_map_lock(sbi, flag, true);
>
> It looks there is no different after you changed above two lines? I guess
> you just want to keep parameter @flag be consistent in __do_{un,}map_lock
> pair, right?
Yes. The flag can only be F2FS_GET_BLOCK_PRE_AIO here (the same as initialized
value), this is only used to ensure the flag is not changed (it is not for now)
before unlock. :)
thanks
>
> Anyway, it's okay to me.
>
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>
> Thanks,
>
>> locked = true;
>> goto restart;
>> }
>> @@ -2439,7 +2443,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>> f2fs_put_dnode(&dn);
>> unlock_out:
>> if (locked)
>> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, false);
>> + __do_map_lock(sbi, flag, false);
>> return err;
>> }
>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] f2fs: fix race between write_checkpoint and write_begin
2018-11-15 2:39 ` Sheng Yong
@ 2018-11-15 8:24 ` Jaegeuk Kim
2018-11-15 8:37 ` Chao Yu
0 siblings, 1 reply; 5+ messages in thread
From: Jaegeuk Kim @ 2018-11-15 8:24 UTC (permalink / raw)
To: Sheng Yong; +Cc: linux-f2fs-devel
On 11/15, Sheng Yong wrote:
>
>
> On 2018/11/15 9:59, Chao Yu wrote:
> > On 2018/11/14 19:34, Sheng Yong wrote:
> > > The following race could lead to inconsistent SIT bitmap:
> > >
> > > Task A Task B
> > > ====== ======
> > > f2fs_write_checkpoint
> > > block_operations
> > > f2fs_lock_all
> > > down_write(node_change)
> > > down_write(node_write)
> > > ... sync ...
> > > up_write(node_change)
> > > f2fs_file_write_iter
> > > set_inode_flag(FI_NO_PREALLOC)
> > > ......
> > > f2fs_write_begin(index=0, has inline data)
> > > prepare_write_begin
> > > __do_map_lock(AIO) => down_read(node_change)
> > > f2fs_convert_inline_page => update SIT
> > > __do_map_lock(AIO) => up_read(node_change)
> > > f2fs_flush_sit_entries <= inconsistent SIT
> > > finish write checkpoint
> > > sudden-power-off
> > >
> > > If SPO occurs after checkpoint is finished, SIT bitmap will be set
> > > incorrectly.
> > >
> > > Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> > > ---
> > > v2->v1:
> > > Sorry for late. We can use f2fs_lock_op directly, but it makes it a bit
> > > complicate to unlock. So v2 still uses __do_map_lock with different flags
> > > to avoid race.
> > >
> > > Use F2FS_GET_BLOCK_PRE_AIO and F2FS_GET_BLOCK_DEFAULT to tell different
> > > condition.
> > >
> > > Thanks
> > > ---
> > > fs/f2fs/data.c | 12 ++++++++----
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 6e0ffb1749ca..4abd47d82cdb 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -2376,6 +2376,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > struct page *ipage;
> > > bool locked = false;
> > > struct extent_info ei = {0,0,0};
> > > + int flag = F2FS_GET_BLOCK_PRE_AIO;
> > > int err = 0;
> > > /*
> > > @@ -2386,9 +2387,12 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > !is_inode_flag_set(inode, FI_NO_PREALLOC))
> > > return 0;
> > > + if (f2fs_has_inline_data(inode) && pos + len > MAX_INLINE_DATA(inode))
> > > + /* avoid race between write CP and convert_inline_page */
> > > + flag = F2FS_GET_BLOCK_DEFAULT;
> > > if (f2fs_has_inline_data(inode) ||
> > > (pos & PAGE_MASK) >= i_size_read(inode)) {
> > > - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, true);
> > > + __do_map_lock(sbi, flag, true);
> > > locked = true;
> > > }
> > > restart:
> > > @@ -2424,8 +2428,8 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > err = f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > > if (err || dn.data_blkaddr == NULL_ADDR) {
> > > f2fs_put_dnode(&dn);
> > > - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO,
> > > - true);
> > > + flag = F2FS_GET_BLOCK_PRE_AIO;
> > > + __do_map_lock(sbi, flag, true);
> >
> > It looks there is no different after you changed above two lines? I guess
> > you just want to keep parameter @flag be consistent in __do_{un,}map_lock
> > pair, right?
>
> Yes. The flag can only be F2FS_GET_BLOCK_PRE_AIO here (the same as initialized
> value), this is only used to ensure the flag is not changed (it is not for now)
> before unlock. :)
Could you please check this?
https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test
Thanks,
>
> thanks
> >
> > Anyway, it's okay to me.
> >
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> >
> > Thanks,
> >
> > > locked = true;
> > > goto restart;
> > > }
> > > @@ -2439,7 +2443,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > f2fs_put_dnode(&dn);
> > > unlock_out:
> > > if (locked)
> > > - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, false);
> > > + __do_map_lock(sbi, flag, false);
> > > return err;
> > > }
> > >
> >
> >
> > .
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] f2fs: fix race between write_checkpoint and write_begin
2018-11-15 8:24 ` Jaegeuk Kim
@ 2018-11-15 8:37 ` Chao Yu
0 siblings, 0 replies; 5+ messages in thread
From: Chao Yu @ 2018-11-15 8:37 UTC (permalink / raw)
To: Jaegeuk Kim, Sheng Yong; +Cc: linux-f2fs-devel
On 2018/11/15 16:24, Jaegeuk Kim wrote:
> Could you please check this?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev-test
Reviewed +1
Thanks,
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-15 8:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-14 11:34 [PATCH v2] f2fs: fix race between write_checkpoint and write_begin Sheng Yong
2018-11-15 1:59 ` Chao Yu
2018-11-15 2:39 ` Sheng Yong
2018-11-15 8:24 ` Jaegeuk Kim
2018-11-15 8:37 ` 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.