Re: [f2fs-dev] [PATCH] f2fs: fix deadlock between quota writes and checkpoint

2020-08-04 Thread Chao Yu 

On 2020/8/4 11:54, Jaegeuk Kim wrote:

On 08/04, Chao Yu wrote:

On 2020/7/29 15:02, Jaegeuk Kim wrote:

f2fs_write_data_pages(quota_mapping)
   __f2fs_write_data_pages f2fs_write_checkpoint
* blk_start_plug();
* add bio in write_io[DATA]
- block_operations
- skip syncing quota by
  
>DEFAULT_RETRY_QUOTA_FLUSH_COUNT
- down_write(>node_write);


f2fs_flush_merged_writes() will be called after block_operations(), why this 
doesn't
help?


Well, I think bio can be added after f2fs_flush_merged_writes() as well.


- down_read(node_write)
- f2fs_do_write_data_page
 - f2fs_trylock_op
  - f2fs_outplace_write_data
 - f2fs_submit_page_write

bio only be added under node_write lock? during f2fs_flush_merged_writes(), 
node_write
should have been held by checkpoint() avoiding quota data writeback & bio 
submission.






- f2fs_write_single_data_page
  - f2fs_do_write_data_page
- f2fs_outplace_write_data
  - do_write_page
 - f2fs_allocate_data_block
  - down_write(node_write)
- 
f2fs_wait_on_all_pages(F2FS_WB_CP_DATA);

Signed-off-by: Daeho Jeong 
Signed-off-by: Jaegeuk Kim 
---
   fs/f2fs/checkpoint.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 8c782d3f324f0..99c8061da55b9 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1269,6 +1269,8 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int 
type)
if (type == F2FS_DIRTY_META)
f2fs_sync_meta_pages(sbi, META, LONG_MAX,
FS_CP_META_IO);
+   else if (type == F2FS_WB_CP_DATA)
+   f2fs_submit_merged_write(sbi, DATA);
io_schedule_timeout(DEFAULT_IO_TIMEOUT);
}
finish_wait(>cp_wait, );


.

Re: [f2fs-dev] [PATCH] f2fs: fix deadlock between quota writes and checkpoint

2020-08-03 Thread Jaegeuk Kim 
On 08/04, Chao Yu wrote:
> On 2020/7/29 15:02, Jaegeuk Kim wrote:
> > f2fs_write_data_pages(quota_mapping)
> >   __f2fs_write_data_pages f2fs_write_checkpoint
> >* blk_start_plug();
> >* add bio in write_io[DATA]
> >- block_operations
> >- skip syncing quota by
> >  
> > >DEFAULT_RETRY_QUOTA_FLUSH_COUNT
> >- down_write(>node_write);
> 
> f2fs_flush_merged_writes() will be called after block_operations(), why this 
> doesn't
> help?

Well, I think bio can be added after f2fs_flush_merged_writes() as well.

> 
> >- f2fs_write_single_data_page
> >  - f2fs_do_write_data_page
> >- f2fs_outplace_write_data
> >  - do_write_page
> > - f2fs_allocate_data_block
> >  - down_write(node_write)
> >- 
> > f2fs_wait_on_all_pages(F2FS_WB_CP_DATA);
> > 
> > Signed-off-by: Daeho Jeong 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >   fs/f2fs/checkpoint.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 8c782d3f324f0..99c8061da55b9 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1269,6 +1269,8 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, 
> > int type)
> > if (type == F2FS_DIRTY_META)
> > f2fs_sync_meta_pages(sbi, META, LONG_MAX,
> > FS_CP_META_IO);
> > +   else if (type == F2FS_WB_CP_DATA)
> > +   f2fs_submit_merged_write(sbi, DATA);
> > io_schedule_timeout(DEFAULT_IO_TIMEOUT);
> > }
> > finish_wait(>cp_wait, );
> >

Re: [f2fs-dev] [PATCH] f2fs: fix deadlock between quota writes and checkpoint

2020-08-03 Thread Chao Yu 

On 2020/7/29 15:02, Jaegeuk Kim wrote:

f2fs_write_data_pages(quota_mapping)
  __f2fs_write_data_pages f2fs_write_checkpoint
   * blk_start_plug();
   * add bio in write_io[DATA]
   - block_operations
   - skip syncing quota by
 
>DEFAULT_RETRY_QUOTA_FLUSH_COUNT
   - down_write(>node_write);


f2fs_flush_merged_writes() will be called after block_operations(), why this 
doesn't
help?


   - f2fs_write_single_data_page
 - f2fs_do_write_data_page
   - f2fs_outplace_write_data
 - do_write_page
- f2fs_allocate_data_block
 - down_write(node_write)
   - 
f2fs_wait_on_all_pages(F2FS_WB_CP_DATA);

Signed-off-by: Daeho Jeong 
Signed-off-by: Jaegeuk Kim 
---
  fs/f2fs/checkpoint.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 8c782d3f324f0..99c8061da55b9 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1269,6 +1269,8 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int 
type)
if (type == F2FS_DIRTY_META)
f2fs_sync_meta_pages(sbi, META, LONG_MAX,
FS_CP_META_IO);
+   else if (type == F2FS_WB_CP_DATA)
+   f2fs_submit_merged_write(sbi, DATA);
io_schedule_timeout(DEFAULT_IO_TIMEOUT);
}
finish_wait(>cp_wait, );

Re: [f2fs-dev] [PATCH] f2fs: fix deadlock between quota writes and checkpoint

2020-07-29 Thread Jaegeuk Kim 
On 07/29, Chao Yu wrote:
> On 2020-7-29 15:02, Jaegeuk Kim wrote:
> > f2fs_write_data_pages(quota_mapping)
> >  __f2fs_write_data_pages f2fs_write_checkpoint
> >   * blk_start_plug();
> >   * add bio in write_io[DATA]
> >   - block_operations
> >   - skip syncing quota by
> > 
> > >DEFAULT_RETRY_QUOTA_FLUSH_COUNT
> >   - down_write(>node_write);
> >   - f2fs_write_single_data_page
> 
> After commit 79963d967b49 ("f2fs: shrink node_write lock coverage"),
> node_write lock was moved to f2fs_write_single_data_page() and
> f2fs_write_compressed_pages().
> 
> So it needs to update the callstack.
> 
> - down_write(node_write)

Yeah, applied. :)

> 
> Otherwise it looks good to me.
> 
> Reviewed-by: Chao Yu 
> 
> Thanks,
> 
> > - f2fs_do_write_data_page
> >   - f2fs_outplace_write_data
> > - do_write_page
> >- f2fs_allocate_data_block
> > - down_write(node_write)
> >   - 
> > f2fs_wait_on_all_pages(F2FS_WB_CP_DATA);
> > 
> > Signed-off-by: Daeho Jeong 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >  fs/f2fs/checkpoint.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 8c782d3f324f0..99c8061da55b9 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1269,6 +1269,8 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, 
> > int type)
> > if (type == F2FS_DIRTY_META)
> > f2fs_sync_meta_pages(sbi, META, LONG_MAX,
> > FS_CP_META_IO);
> > +   else if (type == F2FS_WB_CP_DATA)
> > +   f2fs_submit_merged_write(sbi, DATA);
> > io_schedule_timeout(DEFAULT_IO_TIMEOUT);
> > }
> > finish_wait(>cp_wait, );
> >

Re: [f2fs-dev] [PATCH] f2fs: fix deadlock between quota writes and checkpoint

2020-07-29 Thread Chao Yu 

On 2020-7-29 15:02, Jaegeuk Kim wrote:

f2fs_write_data_pages(quota_mapping)
 __f2fs_write_data_pages f2fs_write_checkpoint
  * blk_start_plug();
  * add bio in write_io[DATA]
  - block_operations
  - skip syncing quota by
>DEFAULT_RETRY_QUOTA_FLUSH_COUNT
  - down_write(>node_write);
  - f2fs_write_single_data_page


After commit 79963d967b49 ("f2fs: shrink node_write lock coverage"),
node_write lock was moved to f2fs_write_single_data_page() and
f2fs_write_compressed_pages().

So it needs to update the callstack.

- down_write(node_write)

Otherwise it looks good to me.

Reviewed-by: Chao Yu 

Thanks,


- f2fs_do_write_data_page
  - f2fs_outplace_write_data
- do_write_page
   - f2fs_allocate_data_block
- down_write(node_write)
  - f2fs_wait_on_all_pages(F2FS_WB_CP_DATA);

Signed-off-by: Daeho Jeong 
Signed-off-by: Jaegeuk Kim 
---
 fs/f2fs/checkpoint.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 8c782d3f324f0..99c8061da55b9 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1269,6 +1269,8 @@ void f2fs_wait_on_all_pages(struct f2fs_sb_info *sbi, int 
type)
if (type == F2FS_DIRTY_META)
f2fs_sync_meta_pages(sbi, META, LONG_MAX,
FS_CP_META_IO);
+   else if (type == F2FS_WB_CP_DATA)
+   f2fs_submit_merged_write(sbi, DATA);
io_schedule_timeout(DEFAULT_IO_TIMEOUT);
}
finish_wait(>cp_wait, );