Re: [PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Jaegeuk Kim]

On 05/12, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2017/5/12 2:36, Jaegeuk Kim wrote:
> > Hi Chao,
> > 
> > On 05/09, Chao Yu wrote:
> >> From: Chao Yu 
> >>
> >> Serialize data/node IOs by using fifo list instead of mutex lock,
> >> it will help to enhance concurrency of f2fs, meanwhile keeping LFS
> >> IO semantics.
> > 
> > I'm not against to give it a try, but not sure how much we can get a benefit
> > from this approach frankly. Have you got a trouble on any lock contention 
> > from
> > the below io_rwsem or mutex?
> 
> Yes, because submitting IOs can be blocked in block layer since there may be:
> - limitation of some resources, e.g. request number.
> - IO throttle
> Holding a global mutex lock in the path is not good idea, as it may cause
> potential hungtask or concurrency performance regression.
> 
> So I add this patch to relief impacting of global mutex.

Okay, could you send a modified patch? Then, let me evaluate it.

Thanks,

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> >>
> >> Signed-off-by: Chao Yu 
> >> ---
> >>  fs/f2fs/checkpoint.c |  1 +
> >>  fs/f2fs/data.c   | 28 
> >>  fs/f2fs/f2fs.h   |  5 -
> >>  fs/f2fs/gc.c |  3 ++-
> >>  fs/f2fs/segment.c| 20 ++--
> >>  fs/f2fs/segment.h|  3 ++-
> >>  6 files changed, 47 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >> index 2a475e83a092..7b3393474f6b 100644
> >> --- a/fs/f2fs/checkpoint.c
> >> +++ b/fs/f2fs/checkpoint.c
> >> @@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t 
> >> start, int nrpages,
> >>.op = REQ_OP_READ,
> >>.op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
> >>.encrypted_page = NULL,
> >> +  .in_list = false,
> >>};
> >>struct blk_plug plug;
> >>  
> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >> index 5f001b471252..89eaa8aaa97b 100644
> >> --- a/fs/f2fs/data.c
> >> +++ b/fs/f2fs/data.c
> >> @@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
> >>struct f2fs_bio_info *io;
> >>bool is_read = is_read_io(fio->op);
> >>struct page *bio_page;
> >> +  struct curseg_info *curseg;
> >>int err = 0;
> >>  
> >> +  if (fio->in_list)
> >> +  curseg = CURSEG_I(sbi, fio->seg_type);
> >> +
> >>io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
> >>  
> >> +  down_write(>io_rwsem);
> >> +next:
> >> +  if (fio->in_list) {
> >> +  spin_lock(>io_lock);
> >> +  if (list_empty(>io_list)) {
> >> +  spin_unlock(>io_lock);
> >> +  goto out_fail;
> >> +  }
> >> +  fio = list_first_entry(>io_list,
> >> +  struct f2fs_io_info, list);
> >> +  list_del(>list);
> >> +  spin_unlock(>io_lock);
> >> +  }
> >> +
> >>if (fio->old_blkaddr != NEW_ADDR)
> >>verify_block_addr(sbi, fio->old_blkaddr);
> >>verify_block_addr(sbi, fio->new_blkaddr);
> >> @@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
> >>if (!is_read)
> >>inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> >>  
> >> -  down_write(>io_rwsem);
> >> -
> >>if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
> >>(io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
> >>!__same_bdev(sbi, fio->new_blkaddr, io->bio)))
> >> @@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
> >>  
> >>io->last_block_in_bio = fio->new_blkaddr;
> >>f2fs_trace_ios(fio, 0);
> >> +
> >> +  trace_f2fs_submit_page_mbio(fio->page, fio);
> >> +
> >> +  if (fio->in_list)
> >> +  goto next;
> >>  out_fail:
> >>up_write(>io_rwsem);
> >> -  trace_f2fs_submit_page_mbio(fio->page, fio);
> >>return err;
> >>  }
> >>  
> >> @@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data 
> >> *dn)
> >>set_summary(, dn->nid, dn->ofs_in_node, ni.version);
> >>  
> >>allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
> >> -  , CURSEG_WARM_DATA);
> >> +  , CURSEG_WARM_DATA, NULL, false);
> >>set_data_blkaddr(dn);
> >>  
> >>/* update i_size */
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index 9129a6229bc8..6b8e9f051aa2 100644
> >> --- a/fs/f2fs/f2fs.h
> >> +++ b/fs/f2fs/f2fs.h
> >> @@ -802,8 +802,10 @@ struct f2fs_io_info {
> >>block_t old_blkaddr;/* old block address before Cow */
> >>struct page *page;  /* page to be written */
> >>struct page *encrypted_page;/* encrypted page */
> >> +  struct list_head list;  /* serialize IOs */
> >>bool submitted; /* indicate IO submission */
> >>bool need_lock; /* indicate we need to lock cp_rwsem */
> >> +  bool in_list;   /* indicate fio is in io_list */

Re: [PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Jaegeuk Kim]

On 05/12, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2017/5/12 2:36, Jaegeuk Kim wrote:
> > Hi Chao,
> > 
> > On 05/09, Chao Yu wrote:
> >> From: Chao Yu 
> >>
> >> Serialize data/node IOs by using fifo list instead of mutex lock,
> >> it will help to enhance concurrency of f2fs, meanwhile keeping LFS
> >> IO semantics.
> > 
> > I'm not against to give it a try, but not sure how much we can get a benefit
> > from this approach frankly. Have you got a trouble on any lock contention 
> > from
> > the below io_rwsem or mutex?
> 
> Yes, because submitting IOs can be blocked in block layer since there may be:
> - limitation of some resources, e.g. request number.
> - IO throttle
> Holding a global mutex lock in the path is not good idea, as it may cause
> potential hungtask or concurrency performance regression.
> 
> So I add this patch to relief impacting of global mutex.

Okay, could you send a modified patch? Then, let me evaluate it.

Thanks,

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> >>
> >> Signed-off-by: Chao Yu 
> >> ---
> >>  fs/f2fs/checkpoint.c |  1 +
> >>  fs/f2fs/data.c   | 28 
> >>  fs/f2fs/f2fs.h   |  5 -
> >>  fs/f2fs/gc.c |  3 ++-
> >>  fs/f2fs/segment.c| 20 ++--
> >>  fs/f2fs/segment.h|  3 ++-
> >>  6 files changed, 47 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >> index 2a475e83a092..7b3393474f6b 100644
> >> --- a/fs/f2fs/checkpoint.c
> >> +++ b/fs/f2fs/checkpoint.c
> >> @@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t 
> >> start, int nrpages,
> >>.op = REQ_OP_READ,
> >>.op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
> >>.encrypted_page = NULL,
> >> +  .in_list = false,
> >>};
> >>struct blk_plug plug;
> >>  
> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >> index 5f001b471252..89eaa8aaa97b 100644
> >> --- a/fs/f2fs/data.c
> >> +++ b/fs/f2fs/data.c
> >> @@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
> >>struct f2fs_bio_info *io;
> >>bool is_read = is_read_io(fio->op);
> >>struct page *bio_page;
> >> +  struct curseg_info *curseg;
> >>int err = 0;
> >>  
> >> +  if (fio->in_list)
> >> +  curseg = CURSEG_I(sbi, fio->seg_type);
> >> +
> >>io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
> >>  
> >> +  down_write(>io_rwsem);
> >> +next:
> >> +  if (fio->in_list) {
> >> +  spin_lock(>io_lock);
> >> +  if (list_empty(>io_list)) {
> >> +  spin_unlock(>io_lock);
> >> +  goto out_fail;
> >> +  }
> >> +  fio = list_first_entry(>io_list,
> >> +  struct f2fs_io_info, list);
> >> +  list_del(>list);
> >> +  spin_unlock(>io_lock);
> >> +  }
> >> +
> >>if (fio->old_blkaddr != NEW_ADDR)
> >>verify_block_addr(sbi, fio->old_blkaddr);
> >>verify_block_addr(sbi, fio->new_blkaddr);
> >> @@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
> >>if (!is_read)
> >>inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> >>  
> >> -  down_write(>io_rwsem);
> >> -
> >>if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
> >>(io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
> >>!__same_bdev(sbi, fio->new_blkaddr, io->bio)))
> >> @@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
> >>  
> >>io->last_block_in_bio = fio->new_blkaddr;
> >>f2fs_trace_ios(fio, 0);
> >> +
> >> +  trace_f2fs_submit_page_mbio(fio->page, fio);
> >> +
> >> +  if (fio->in_list)
> >> +  goto next;
> >>  out_fail:
> >>up_write(>io_rwsem);
> >> -  trace_f2fs_submit_page_mbio(fio->page, fio);
> >>return err;
> >>  }
> >>  
> >> @@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data 
> >> *dn)
> >>set_summary(, dn->nid, dn->ofs_in_node, ni.version);
> >>  
> >>allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
> >> -  , CURSEG_WARM_DATA);
> >> +  , CURSEG_WARM_DATA, NULL, false);
> >>set_data_blkaddr(dn);
> >>  
> >>/* update i_size */
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index 9129a6229bc8..6b8e9f051aa2 100644
> >> --- a/fs/f2fs/f2fs.h
> >> +++ b/fs/f2fs/f2fs.h
> >> @@ -802,8 +802,10 @@ struct f2fs_io_info {
> >>block_t old_blkaddr;/* old block address before Cow */
> >>struct page *page;  /* page to be written */
> >>struct page *encrypted_page;/* encrypted page */
> >> +  struct list_head list;  /* serialize IOs */
> >>bool submitted; /* indicate IO submission */
> >>bool need_lock; /* indicate we need to lock cp_rwsem */
> >> +  bool in_list;   /* indicate fio is in io_list */
> >>  };
> >>  
> >>  #define 

Re: [PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Chao Yu]

Hi Jaegeuk,

On 2017/5/12 2:36, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On 05/09, Chao Yu wrote:
>> From: Chao Yu 
>>
>> Serialize data/node IOs by using fifo list instead of mutex lock,
>> it will help to enhance concurrency of f2fs, meanwhile keeping LFS
>> IO semantics.
> 
> I'm not against to give it a try, but not sure how much we can get a benefit
> from this approach frankly. Have you got a trouble on any lock contention from
> the below io_rwsem or mutex?

Yes, because submitting IOs can be blocked in block layer since there may be:
- limitation of some resources, e.g. request number.
- IO throttle
Holding a global mutex lock in the path is not good idea, as it may cause
potential hungtask or concurrency performance regression.

So I add this patch to relief impacting of global mutex.

Thanks,

> 
> Thanks,
> 
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  fs/f2fs/checkpoint.c |  1 +
>>  fs/f2fs/data.c   | 28 
>>  fs/f2fs/f2fs.h   |  5 -
>>  fs/f2fs/gc.c |  3 ++-
>>  fs/f2fs/segment.c| 20 ++--
>>  fs/f2fs/segment.h|  3 ++-
>>  6 files changed, 47 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 2a475e83a092..7b3393474f6b 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t 
>> start, int nrpages,
>>  .op = REQ_OP_READ,
>>  .op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
>>  .encrypted_page = NULL,
>> +.in_list = false,
>>  };
>>  struct blk_plug plug;
>>  
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 5f001b471252..89eaa8aaa97b 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>>  struct f2fs_bio_info *io;
>>  bool is_read = is_read_io(fio->op);
>>  struct page *bio_page;
>> +struct curseg_info *curseg;
>>  int err = 0;
>>  
>> +if (fio->in_list)
>> +curseg = CURSEG_I(sbi, fio->seg_type);
>> +
>>  io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
>>  
>> +down_write(>io_rwsem);
>> +next:
>> +if (fio->in_list) {
>> +spin_lock(>io_lock);
>> +if (list_empty(>io_list)) {
>> +spin_unlock(>io_lock);
>> +goto out_fail;
>> +}
>> +fio = list_first_entry(>io_list,
>> +struct f2fs_io_info, list);
>> +list_del(>list);
>> +spin_unlock(>io_lock);
>> +}
>> +
>>  if (fio->old_blkaddr != NEW_ADDR)
>>  verify_block_addr(sbi, fio->old_blkaddr);
>>  verify_block_addr(sbi, fio->new_blkaddr);
>> @@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>>  if (!is_read)
>>  inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>>  
>> -down_write(>io_rwsem);
>> -
>>  if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
>>  (io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
>>  !__same_bdev(sbi, fio->new_blkaddr, io->bio)))
>> @@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>>  
>>  io->last_block_in_bio = fio->new_blkaddr;
>>  f2fs_trace_ios(fio, 0);
>> +
>> +trace_f2fs_submit_page_mbio(fio->page, fio);
>> +
>> +if (fio->in_list)
>> +goto next;
>>  out_fail:
>>  up_write(>io_rwsem);
>> -trace_f2fs_submit_page_mbio(fio->page, fio);
>>  return err;
>>  }
>>  
>> @@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data 
>> *dn)
>>  set_summary(, dn->nid, dn->ofs_in_node, ni.version);
>>  
>>  allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
>> -, CURSEG_WARM_DATA);
>> +, CURSEG_WARM_DATA, NULL, false);
>>  set_data_blkaddr(dn);
>>  
>>  /* update i_size */
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 9129a6229bc8..6b8e9f051aa2 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -802,8 +802,10 @@ struct f2fs_io_info {
>>  block_t old_blkaddr;/* old block address before Cow */
>>  struct page *page;  /* page to be written */
>>  struct page *encrypted_page;/* encrypted page */
>> +struct list_head list;  /* serialize IOs */
>>  bool submitted; /* indicate IO submission */
>>  bool need_lock; /* indicate we need to lock cp_rwsem */
>> +bool in_list;   /* indicate fio is in io_list */
>>  };
>>  
>>  #define is_read_io(rw) ((rw) == READ)
>> @@ -2274,7 +2276,8 @@ void f2fs_replace_block(struct f2fs_sb_info *sbi, 
>> struct dnode_of_data *dn,
>>  bool recover_newaddr);
>>  void allocate_data_block(struct f2fs_sb_info 

Re: [PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Chao Yu]

Hi Jaegeuk,

On 2017/5/12 2:36, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On 05/09, Chao Yu wrote:
>> From: Chao Yu 
>>
>> Serialize data/node IOs by using fifo list instead of mutex lock,
>> it will help to enhance concurrency of f2fs, meanwhile keeping LFS
>> IO semantics.
> 
> I'm not against to give it a try, but not sure how much we can get a benefit
> from this approach frankly. Have you got a trouble on any lock contention from
> the below io_rwsem or mutex?

Yes, because submitting IOs can be blocked in block layer since there may be:
- limitation of some resources, e.g. request number.
- IO throttle
Holding a global mutex lock in the path is not good idea, as it may cause
potential hungtask or concurrency performance regression.

So I add this patch to relief impacting of global mutex.

Thanks,

> 
> Thanks,
> 
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  fs/f2fs/checkpoint.c |  1 +
>>  fs/f2fs/data.c   | 28 
>>  fs/f2fs/f2fs.h   |  5 -
>>  fs/f2fs/gc.c |  3 ++-
>>  fs/f2fs/segment.c| 20 ++--
>>  fs/f2fs/segment.h|  3 ++-
>>  6 files changed, 47 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 2a475e83a092..7b3393474f6b 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t 
>> start, int nrpages,
>>  .op = REQ_OP_READ,
>>  .op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
>>  .encrypted_page = NULL,
>> +.in_list = false,
>>  };
>>  struct blk_plug plug;
>>  
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 5f001b471252..89eaa8aaa97b 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>>  struct f2fs_bio_info *io;
>>  bool is_read = is_read_io(fio->op);
>>  struct page *bio_page;
>> +struct curseg_info *curseg;
>>  int err = 0;
>>  
>> +if (fio->in_list)
>> +curseg = CURSEG_I(sbi, fio->seg_type);
>> +
>>  io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
>>  
>> +down_write(>io_rwsem);
>> +next:
>> +if (fio->in_list) {
>> +spin_lock(>io_lock);
>> +if (list_empty(>io_list)) {
>> +spin_unlock(>io_lock);
>> +goto out_fail;
>> +}
>> +fio = list_first_entry(>io_list,
>> +struct f2fs_io_info, list);
>> +list_del(>list);
>> +spin_unlock(>io_lock);
>> +}
>> +
>>  if (fio->old_blkaddr != NEW_ADDR)
>>  verify_block_addr(sbi, fio->old_blkaddr);
>>  verify_block_addr(sbi, fio->new_blkaddr);
>> @@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>>  if (!is_read)
>>  inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>>  
>> -down_write(>io_rwsem);
>> -
>>  if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
>>  (io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
>>  !__same_bdev(sbi, fio->new_blkaddr, io->bio)))
>> @@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>>  
>>  io->last_block_in_bio = fio->new_blkaddr;
>>  f2fs_trace_ios(fio, 0);
>> +
>> +trace_f2fs_submit_page_mbio(fio->page, fio);
>> +
>> +if (fio->in_list)
>> +goto next;
>>  out_fail:
>>  up_write(>io_rwsem);
>> -trace_f2fs_submit_page_mbio(fio->page, fio);
>>  return err;
>>  }
>>  
>> @@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data 
>> *dn)
>>  set_summary(, dn->nid, dn->ofs_in_node, ni.version);
>>  
>>  allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
>> -, CURSEG_WARM_DATA);
>> +, CURSEG_WARM_DATA, NULL, false);
>>  set_data_blkaddr(dn);
>>  
>>  /* update i_size */
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 9129a6229bc8..6b8e9f051aa2 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -802,8 +802,10 @@ struct f2fs_io_info {
>>  block_t old_blkaddr;/* old block address before Cow */
>>  struct page *page;  /* page to be written */
>>  struct page *encrypted_page;/* encrypted page */
>> +struct list_head list;  /* serialize IOs */
>>  bool submitted; /* indicate IO submission */
>>  bool need_lock; /* indicate we need to lock cp_rwsem */
>> +bool in_list;   /* indicate fio is in io_list */
>>  };
>>  
>>  #define is_read_io(rw) ((rw) == READ)
>> @@ -2274,7 +2276,8 @@ void f2fs_replace_block(struct f2fs_sb_info *sbi, 
>> struct dnode_of_data *dn,
>>  bool recover_newaddr);
>>  void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
>> 

Re: [PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Jaegeuk Kim]

Hi Chao,

On 05/09, Chao Yu wrote:
> From: Chao Yu 
> 
> Serialize data/node IOs by using fifo list instead of mutex lock,
> it will help to enhance concurrency of f2fs, meanwhile keeping LFS
> IO semantics.

I'm not against to give it a try, but not sure how much we can get a benefit
from this approach frankly. Have you got a trouble on any lock contention from
the below io_rwsem or mutex?

Thanks,

> 
> Signed-off-by: Chao Yu 
> ---
>  fs/f2fs/checkpoint.c |  1 +
>  fs/f2fs/data.c   | 28 
>  fs/f2fs/f2fs.h   |  5 -
>  fs/f2fs/gc.c |  3 ++-
>  fs/f2fs/segment.c| 20 ++--
>  fs/f2fs/segment.h|  3 ++-
>  6 files changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 2a475e83a092..7b3393474f6b 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t 
> start, int nrpages,
>   .op = REQ_OP_READ,
>   .op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
>   .encrypted_page = NULL,
> + .in_list = false,
>   };
>   struct blk_plug plug;
>  
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 5f001b471252..89eaa8aaa97b 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>   struct f2fs_bio_info *io;
>   bool is_read = is_read_io(fio->op);
>   struct page *bio_page;
> + struct curseg_info *curseg;
>   int err = 0;
>  
> + if (fio->in_list)
> + curseg = CURSEG_I(sbi, fio->seg_type);
> +
>   io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
>  
> + down_write(>io_rwsem);
> +next:
> + if (fio->in_list) {
> + spin_lock(>io_lock);
> + if (list_empty(>io_list)) {
> + spin_unlock(>io_lock);
> + goto out_fail;
> + }
> + fio = list_first_entry(>io_list,
> + struct f2fs_io_info, list);
> + list_del(>list);
> + spin_unlock(>io_lock);
> + }
> +
>   if (fio->old_blkaddr != NEW_ADDR)
>   verify_block_addr(sbi, fio->old_blkaddr);
>   verify_block_addr(sbi, fio->new_blkaddr);
> @@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>   if (!is_read)
>   inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>  
> - down_write(>io_rwsem);
> -
>   if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
>   (io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
>   !__same_bdev(sbi, fio->new_blkaddr, io->bio)))
> @@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>  
>   io->last_block_in_bio = fio->new_blkaddr;
>   f2fs_trace_ios(fio, 0);
> +
> + trace_f2fs_submit_page_mbio(fio->page, fio);
> +
> + if (fio->in_list)
> + goto next;
>  out_fail:
>   up_write(>io_rwsem);
> - trace_f2fs_submit_page_mbio(fio->page, fio);
>   return err;
>  }
>  
> @@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data *dn)
>   set_summary(, dn->nid, dn->ofs_in_node, ni.version);
>  
>   allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
> - , CURSEG_WARM_DATA);
> + , CURSEG_WARM_DATA, NULL, false);
>   set_data_blkaddr(dn);
>  
>   /* update i_size */
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9129a6229bc8..6b8e9f051aa2 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -802,8 +802,10 @@ struct f2fs_io_info {
>   block_t old_blkaddr;/* old block address before Cow */
>   struct page *page;  /* page to be written */
>   struct page *encrypted_page;/* encrypted page */
> + struct list_head list;  /* serialize IOs */
>   bool submitted; /* indicate IO submission */
>   bool need_lock; /* indicate we need to lock cp_rwsem */
> + bool in_list;   /* indicate fio is in io_list */
>  };
>  
>  #define is_read_io(rw) ((rw) == READ)
> @@ -2274,7 +2276,8 @@ void f2fs_replace_block(struct f2fs_sb_info *sbi, 
> struct dnode_of_data *dn,
>   bool recover_newaddr);
>  void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
>   block_t old_blkaddr, block_t *new_blkaddr,
> - struct f2fs_summary *sum, int type);
> + struct f2fs_summary *sum, int type,
> + struct f2fs_io_info *fio, bool add_list);
>  void f2fs_wait_on_page_writeback(struct page *page,
>   enum page_type type, bool ordered);
>  void f2fs_wait_on_encrypted_page_writeback(struct f2fs_sb_info *sbi,
> diff --git 

Re: [PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Jaegeuk Kim]

Hi Chao,

On 05/09, Chao Yu wrote:
> From: Chao Yu 
> 
> Serialize data/node IOs by using fifo list instead of mutex lock,
> it will help to enhance concurrency of f2fs, meanwhile keeping LFS
> IO semantics.

I'm not against to give it a try, but not sure how much we can get a benefit
from this approach frankly. Have you got a trouble on any lock contention from
the below io_rwsem or mutex?

Thanks,

> 
> Signed-off-by: Chao Yu 
> ---
>  fs/f2fs/checkpoint.c |  1 +
>  fs/f2fs/data.c   | 28 
>  fs/f2fs/f2fs.h   |  5 -
>  fs/f2fs/gc.c |  3 ++-
>  fs/f2fs/segment.c| 20 ++--
>  fs/f2fs/segment.h|  3 ++-
>  6 files changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 2a475e83a092..7b3393474f6b 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t 
> start, int nrpages,
>   .op = REQ_OP_READ,
>   .op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
>   .encrypted_page = NULL,
> + .in_list = false,
>   };
>   struct blk_plug plug;
>  
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 5f001b471252..89eaa8aaa97b 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>   struct f2fs_bio_info *io;
>   bool is_read = is_read_io(fio->op);
>   struct page *bio_page;
> + struct curseg_info *curseg;
>   int err = 0;
>  
> + if (fio->in_list)
> + curseg = CURSEG_I(sbi, fio->seg_type);
> +
>   io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
>  
> + down_write(>io_rwsem);
> +next:
> + if (fio->in_list) {
> + spin_lock(>io_lock);
> + if (list_empty(>io_list)) {
> + spin_unlock(>io_lock);
> + goto out_fail;
> + }
> + fio = list_first_entry(>io_list,
> + struct f2fs_io_info, list);
> + list_del(>list);
> + spin_unlock(>io_lock);
> + }
> +
>   if (fio->old_blkaddr != NEW_ADDR)
>   verify_block_addr(sbi, fio->old_blkaddr);
>   verify_block_addr(sbi, fio->new_blkaddr);
> @@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>   if (!is_read)
>   inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>  
> - down_write(>io_rwsem);
> -
>   if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
>   (io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
>   !__same_bdev(sbi, fio->new_blkaddr, io->bio)))
> @@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>  
>   io->last_block_in_bio = fio->new_blkaddr;
>   f2fs_trace_ios(fio, 0);
> +
> + trace_f2fs_submit_page_mbio(fio->page, fio);
> +
> + if (fio->in_list)
> + goto next;
>  out_fail:
>   up_write(>io_rwsem);
> - trace_f2fs_submit_page_mbio(fio->page, fio);
>   return err;
>  }
>  
> @@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data *dn)
>   set_summary(, dn->nid, dn->ofs_in_node, ni.version);
>  
>   allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
> - , CURSEG_WARM_DATA);
> + , CURSEG_WARM_DATA, NULL, false);
>   set_data_blkaddr(dn);
>  
>   /* update i_size */
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9129a6229bc8..6b8e9f051aa2 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -802,8 +802,10 @@ struct f2fs_io_info {
>   block_t old_blkaddr;/* old block address before Cow */
>   struct page *page;  /* page to be written */
>   struct page *encrypted_page;/* encrypted page */
> + struct list_head list;  /* serialize IOs */
>   bool submitted; /* indicate IO submission */
>   bool need_lock; /* indicate we need to lock cp_rwsem */
> + bool in_list;   /* indicate fio is in io_list */
>  };
>  
>  #define is_read_io(rw) ((rw) == READ)
> @@ -2274,7 +2276,8 @@ void f2fs_replace_block(struct f2fs_sb_info *sbi, 
> struct dnode_of_data *dn,
>   bool recover_newaddr);
>  void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
>   block_t old_blkaddr, block_t *new_blkaddr,
> - struct f2fs_summary *sum, int type);
> + struct f2fs_summary *sum, int type,
> + struct f2fs_io_info *fio, bool add_list);
>  void f2fs_wait_on_page_writeback(struct page *page,
>   enum page_type type, bool ordered);
>  void f2fs_wait_on_encrypted_page_writeback(struct f2fs_sb_info *sbi,
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 

[PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Chao Yu]

From: Chao Yu 

Serialize data/node IOs by using fifo list instead of mutex lock,
it will help to enhance concurrency of f2fs, meanwhile keeping LFS
IO semantics.

Signed-off-by: Chao Yu 
---
 fs/f2fs/checkpoint.c |  1 +
 fs/f2fs/data.c   | 28 
 fs/f2fs/f2fs.h   |  5 -
 fs/f2fs/gc.c |  3 ++-
 fs/f2fs/segment.c| 20 ++--
 fs/f2fs/segment.h|  3 ++-
 6 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 2a475e83a092..7b3393474f6b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, 
int nrpages,
.op = REQ_OP_READ,
.op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
.encrypted_page = NULL,
+   .in_list = false,
};
struct blk_plug plug;
 
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 5f001b471252..89eaa8aaa97b 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
struct f2fs_bio_info *io;
bool is_read = is_read_io(fio->op);
struct page *bio_page;
+   struct curseg_info *curseg;
int err = 0;
 
+   if (fio->in_list)
+   curseg = CURSEG_I(sbi, fio->seg_type);
+
io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
 
+   down_write(>io_rwsem);
+next:
+   if (fio->in_list) {
+   spin_lock(>io_lock);
+   if (list_empty(>io_list)) {
+   spin_unlock(>io_lock);
+   goto out_fail;
+   }
+   fio = list_first_entry(>io_list,
+   struct f2fs_io_info, list);
+   list_del(>list);
+   spin_unlock(>io_lock);
+   }
+
if (fio->old_blkaddr != NEW_ADDR)
verify_block_addr(sbi, fio->old_blkaddr);
verify_block_addr(sbi, fio->new_blkaddr);
@@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
if (!is_read)
inc_page_count(sbi, WB_DATA_TYPE(bio_page));
 
-   down_write(>io_rwsem);
-
if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
(io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
!__same_bdev(sbi, fio->new_blkaddr, io->bio)))
@@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 
io->last_block_in_bio = fio->new_blkaddr;
f2fs_trace_ios(fio, 0);
+
+   trace_f2fs_submit_page_mbio(fio->page, fio);
+
+   if (fio->in_list)
+   goto next;
 out_fail:
up_write(>io_rwsem);
-   trace_f2fs_submit_page_mbio(fio->page, fio);
return err;
 }
 
@@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data *dn)
set_summary(, dn->nid, dn->ofs_in_node, ni.version);
 
allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
-   , CURSEG_WARM_DATA);
+   , CURSEG_WARM_DATA, NULL, false);
set_data_blkaddr(dn);
 
/* update i_size */
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9129a6229bc8..6b8e9f051aa2 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -802,8 +802,10 @@ struct f2fs_io_info {
block_t old_blkaddr;/* old block address before Cow */
struct page *page;  /* page to be written */
struct page *encrypted_page;/* encrypted page */
+   struct list_head list;  /* serialize IOs */
bool submitted; /* indicate IO submission */
bool need_lock; /* indicate we need to lock cp_rwsem */
+   bool in_list;   /* indicate fio is in io_list */
 };
 
 #define is_read_io(rw) ((rw) == READ)
@@ -2274,7 +2276,8 @@ void f2fs_replace_block(struct f2fs_sb_info *sbi, struct 
dnode_of_data *dn,
bool recover_newaddr);
 void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
block_t old_blkaddr, block_t *new_blkaddr,
-   struct f2fs_summary *sum, int type);
+   struct f2fs_summary *sum, int type,
+   struct f2fs_io_info *fio, bool add_list);
 void f2fs_wait_on_page_writeback(struct page *page,
enum page_type type, bool ordered);
 void f2fs_wait_on_encrypted_page_writeback(struct f2fs_sb_info *sbi,
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 8b267ca30926..ac2f74e40eea 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -590,6 +590,7 @@ static void move_encrypted_block(struct inode *inode, 
block_t bidx,
.op = REQ_OP_READ,
.op_flags = 0,
.encrypted_page = NULL,
+   .in_list = false,
};
   

[PATCH 3/3] f2fs: introduce io_list for serialize data/node IOs

[Chao Yu]

From: Chao Yu 

Serialize data/node IOs by using fifo list instead of mutex lock,
it will help to enhance concurrency of f2fs, meanwhile keeping LFS
IO semantics.

Signed-off-by: Chao Yu 
---
 fs/f2fs/checkpoint.c |  1 +
 fs/f2fs/data.c   | 28 
 fs/f2fs/f2fs.h   |  5 -
 fs/f2fs/gc.c |  3 ++-
 fs/f2fs/segment.c| 20 ++--
 fs/f2fs/segment.h|  3 ++-
 6 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 2a475e83a092..7b3393474f6b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -162,6 +162,7 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, 
int nrpages,
.op = REQ_OP_READ,
.op_flags = sync ? (REQ_META | REQ_PRIO) : REQ_RAHEAD,
.encrypted_page = NULL,
+   .in_list = false,
};
struct blk_plug plug;
 
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 5f001b471252..89eaa8aaa97b 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -393,10 +393,28 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
struct f2fs_bio_info *io;
bool is_read = is_read_io(fio->op);
struct page *bio_page;
+   struct curseg_info *curseg;
int err = 0;
 
+   if (fio->in_list)
+   curseg = CURSEG_I(sbi, fio->seg_type);
+
io = __get_bio_info(sbi, fio->op, fio->type, fio->seg_type);
 
+   down_write(>io_rwsem);
+next:
+   if (fio->in_list) {
+   spin_lock(>io_lock);
+   if (list_empty(>io_list)) {
+   spin_unlock(>io_lock);
+   goto out_fail;
+   }
+   fio = list_first_entry(>io_list,
+   struct f2fs_io_info, list);
+   list_del(>list);
+   spin_unlock(>io_lock);
+   }
+
if (fio->old_blkaddr != NEW_ADDR)
verify_block_addr(sbi, fio->old_blkaddr);
verify_block_addr(sbi, fio->new_blkaddr);
@@ -409,8 +427,6 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
if (!is_read)
inc_page_count(sbi, WB_DATA_TYPE(bio_page));
 
-   down_write(>io_rwsem);
-
if (io->bio && (io->last_block_in_bio != fio->new_blkaddr - 1 ||
(io->fio.op != fio->op || io->fio.op_flags != fio->op_flags) ||
!__same_bdev(sbi, fio->new_blkaddr, io->bio)))
@@ -437,9 +453,13 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 
io->last_block_in_bio = fio->new_blkaddr;
f2fs_trace_ios(fio, 0);
+
+   trace_f2fs_submit_page_mbio(fio->page, fio);
+
+   if (fio->in_list)
+   goto next;
 out_fail:
up_write(>io_rwsem);
-   trace_f2fs_submit_page_mbio(fio->page, fio);
return err;
 }
 
@@ -752,7 +772,7 @@ static int __allocate_data_block(struct dnode_of_data *dn)
set_summary(, dn->nid, dn->ofs_in_node, ni.version);
 
allocate_data_block(sbi, NULL, dn->data_blkaddr, >data_blkaddr,
-   , CURSEG_WARM_DATA);
+   , CURSEG_WARM_DATA, NULL, false);
set_data_blkaddr(dn);
 
/* update i_size */
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9129a6229bc8..6b8e9f051aa2 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -802,8 +802,10 @@ struct f2fs_io_info {
block_t old_blkaddr;/* old block address before Cow */
struct page *page;  /* page to be written */
struct page *encrypted_page;/* encrypted page */
+   struct list_head list;  /* serialize IOs */
bool submitted; /* indicate IO submission */
bool need_lock; /* indicate we need to lock cp_rwsem */
+   bool in_list;   /* indicate fio is in io_list */
 };
 
 #define is_read_io(rw) ((rw) == READ)
@@ -2274,7 +2276,8 @@ void f2fs_replace_block(struct f2fs_sb_info *sbi, struct 
dnode_of_data *dn,
bool recover_newaddr);
 void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
block_t old_blkaddr, block_t *new_blkaddr,
-   struct f2fs_summary *sum, int type);
+   struct f2fs_summary *sum, int type,
+   struct f2fs_io_info *fio, bool add_list);
 void f2fs_wait_on_page_writeback(struct page *page,
enum page_type type, bool ordered);
 void f2fs_wait_on_encrypted_page_writeback(struct f2fs_sb_info *sbi,
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 8b267ca30926..ac2f74e40eea 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -590,6 +590,7 @@ static void move_encrypted_block(struct inode *inode, 
block_t bidx,
.op = REQ_OP_READ,
.op_flags = 0,
.encrypted_page = NULL,
+   .in_list = false,
};
struct dnode_of_data dn;