Re: [f2fs-dev] [PATCH] f2fs: use BIO_MAX_PAGES for bio allocation

2016-10-27 Thread Jaegeuk Kim 
On Thu, Oct 27, 2016 at 06:28:04PM +0800, Chao Yu wrote:
> On 2016/10/19 2:48, Jaegeuk Kim wrote:
> > We don't need to allocate bio partially in order to maximize sequential 
> > writes.
> 
> We are going to allocate bio with max size supposing that there will be more
> opportunity to merge small IOs into one bio. Otherwise, if MAX_BIO_BLOCKS is
> smaller than BIO_MAX_PAGES, less IOs can be merged. Is my understanding 
> correct?

Yes. When I test /dev/md0 with ramdisks, this happens and results in lower
performance than other filesystems.

Thanks,

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >  fs/f2fs/checkpoint.c |  2 +-
> >  fs/f2fs/data.c   |  4 +---
> >  fs/f2fs/node.c   |  3 +--
> >  fs/f2fs/segment.c|  4 ++--
> >  fs/f2fs/segment.h| 17 +++--
> >  5 files changed, 8 insertions(+), 22 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 654f5d7..157b7fd 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -228,7 +228,7 @@ void ra_meta_pages_cond(struct f2fs_sb_info *sbi, 
> > pgoff_t index)
> > f2fs_put_page(page, 0);
> >  
> > if (readahead)
> > -   ra_meta_pages(sbi, index, MAX_BIO_BLOCKS(sbi), META_POR, true);
> > +   ra_meta_pages(sbi, index, BIO_MAX_PAGES, META_POR, true);
> >  }
> >  
> >  static int f2fs_write_meta_page(struct page *page,
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 77cd9ac..fe8ce5a 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -307,10 +307,8 @@ void f2fs_submit_page_mbio(struct f2fs_io_info *fio)
> > __submit_merged_bio(io);
> >  alloc_new:
> > if (io->bio == NULL) {
> > -   int bio_blocks = MAX_BIO_BLOCKS(sbi);
> > -
> > io->bio = __bio_alloc(sbi, fio->new_blkaddr,
> > -   bio_blocks, is_read);
> > +   BIO_MAX_PAGES, is_read);
> > io->fio = *fio;
> > }
> >  
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 69c42e2..285e2a5 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -2101,7 +2101,6 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
> > struct f2fs_node *rn;
> > struct f2fs_summary *sum_entry;
> > block_t addr;
> > -   int bio_blocks = MAX_BIO_BLOCKS(sbi);
> > int i, idx, last_offset, nrpages;
> >  
> > /* scan the node segment */
> > @@ -2110,7 +2109,7 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
> > sum_entry = >entries[0];
> >  
> > for (i = 0; i < last_offset; i += nrpages, addr += nrpages) {
> > -   nrpages = min(last_offset - i, bio_blocks);
> > +   nrpages = min(last_offset - i, BIO_MAX_PAGES);
> >  
> > /* readahead node pages */
> > ra_meta_pages(sbi, addr, nrpages, META_POR, true);
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 7a7e3f1..a21a1a3 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2355,10 +2355,10 @@ static void build_sit_entries(struct f2fs_sb_info 
> > *sbi)
> > int sit_blk_cnt = SIT_BLK_CNT(sbi);
> > unsigned int i, start, end;
> > unsigned int readed, start_blk = 0;
> > -   int nrpages = MAX_BIO_BLOCKS(sbi) * 8;
> >  
> > do {
> > -   readed = ra_meta_pages(sbi, start_blk, nrpages, META_SIT, true);
> > +   readed = ra_meta_pages(sbi, start_blk, BIO_MAX_PAGES,
> > +   META_SIT, true);
> >  
> > start = start_blk * sit_i->sents_per_block;
> > end = (start_blk + readed) * sit_i->sents_per_block;
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index a6efb5c..d8ff069 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -102,8 +102,6 @@
> > (((sector_t)blk_addr) << F2FS_LOG_SECTORS_PER_BLOCK)
> >  #define SECTOR_TO_BLOCK(sectors)   \
> > (sectors >> F2FS_LOG_SECTORS_PER_BLOCK)
> > -#define MAX_BIO_BLOCKS(sbi)
> > \
> > -   ((int)min((int)max_hw_blocks(sbi), BIO_MAX_PAGES))
> >  
> >  /*
> >   * indicate a block allocation direction: RIGHT and LEFT.
> > @@ -698,13 +696,6 @@ static inline bool sec_usage_check(struct f2fs_sb_info 
> > *sbi, unsigned int secno)
> > return false;
> >  }
> >  
> > -static inline unsigned int max_hw_blocks(struct f2fs_sb_info *sbi)
> > -{
> > -   struct block_device *bdev = sbi->sb->s_bdev;
> > -   struct request_queue *q = bdev_get_queue(bdev);
> > -   return SECTOR_TO_BLOCK(queue_max_sectors(q));
> > -}
> > -
> >  /*
> >   * It is very important to gather dirty pages and write at once, so that 
> > we can
> >   * submit a big bio without interfering other data writes.
> > @@ -722,7 +713,7 @@ static inline int nr_pages_to_skip(struct f2fs_sb_info 
> > *sbi, int type)
> > else if (type == NODE)
> > return 8 * sbi->blocks_per_seg;
> >

Re: [f2fs-dev] [PATCH] f2fs: return directly if block has been removed from the victim

2016-10-27 Thread Jaegeuk Kim 
On Wed, Oct 26, 2016 at 10:14:04AM +0800, heyunlei wrote:
> 
> 
> On 2016/10/26 9:09, Jaegeuk Kim wrote:
> Hi Kim,
> > Hi Yunlei,
> >
> > On Mon, Oct 24, 2016 at 11:37:28AM +0800, Yunlei He wrote:
> >> If one block has been to written to a new place, just return
> >> in move data process.
> >>
> >> Signed-off-by: Yunlei He 
> >> ---
> >>  fs/f2fs/gc.c | 17 -
> >>  1 file changed, 12 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >> index f35fca5..fc78f3e 100644
> >> --- a/fs/f2fs/gc.c
> >> +++ b/fs/f2fs/gc.c
> >> @@ -545,7 +545,8 @@ static bool is_alive(struct f2fs_sb_info *sbi, struct 
> >> f2fs_summary *sum,
> >>return true;
> >>  }
> >>
> >> -static void move_encrypted_block(struct inode *inode, block_t bidx)
> >> +static void move_encrypted_block(struct inode *inode, block_t bidx,
> >> +  unsigned int segno, int off)
> >>  {
> >>struct f2fs_io_info fio = {
> >>.sbi = F2FS_I_SB(inode),
> >> @@ -580,6 +581,9 @@ static void move_encrypted_block(struct inode *inode, 
> >> block_t bidx)
> >> * don't cache encrypted data into meta inode until previous dirty
> >> * data were writebacked to avoid racing between GC and flush.
> >> */
> >> +  if (!check_valid_map(F2FS_I_SB(inode), segno, off))
> >> +  goto out;
> >
> > This is done by gc_data_segment() before calling move function.
> > Why do we need this again?
> 
> gc_data_segment() check this without lock the data page. If this block
> is written to a new place between check in gc_data_segment() and locked
> the page, it will need to wait this page write back, and write it again,
> it's unnecessary.
> 
> >
> >> +
> >>f2fs_wait_on_page_writeback(page, DATA, true);
> >>
> >>get_node_info(fio.sbi, dn.nid, );
> >> @@ -646,7 +650,8 @@ static void move_encrypted_block(struct inode *inode, 
> >> block_t bidx)
> >>f2fs_put_page(page, 1);
> >>  }
> >>
> >> -static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
> >> +static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
> >> +  unsigned int segno, int 
> >> off)
> >>  {
> >>struct page *page;
> >>
> >> @@ -655,8 +660,10 @@ static void move_data_page(struct inode *inode, 
> >> block_t bidx, int gc_type)
> >>return;
> >>
> >>if (gc_type == BG_GC) {
> >> -  if (PageWriteback(page))
> >
> > Seems there is no reason to remove the existing writeback case.
> > This is a BG_GC case and we don't need to proceed GC in this case.
>Here, PageWriteback(page) means two cases:
> 1. write to this victim segment
> 2. write to a new place
> 
> In the first case, if we return directly, this victim bg_gc will fail
> for some blocks are still valid.

It should be fine, and its main reason would be to avoid wait_on_page_writeback.
Next bg_gc will do better.

Thanks,

--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v2] f2fs: replace a build-time warning with runtime WARN_ON

2016-10-27 Thread Arnd Bergmann 
gcc is unsure about the use of last_ofs_in_node, which might happen
without a prior initialization:

fs/f2fs//git/arm-soc/fs/f2fs/data.c: In function ‘f2fs_map_blocks’:
fs/f2fs/data.c:799:54: warning: ‘last_ofs_in_node’ may be used uninitialized in 
this function [-Wmaybe-uninitialized]
   if (prealloc && dn.ofs_in_node != last_ofs_in_node + 1) {

As pointed out by Chao Yu, the code is actually correct as 'prealloc'
is only set if the last_ofs_in_node has been set, the two always
get updated together.

This initializes last_ofs_in_node to dn.ofs_in_node for each
new dnode at the start of the 'next_block' loop, which at that
point is a correct initialization as well. I assume that compilers
that correctly track the contents of the variables and do not
warn about the condition also figure out that they can eliminate
the extra assignment here.

Fixes: 46008c6d4232 ("f2fs: support in batch multi blocks preallocation")
Signed-off-by: Arnd Bergmann 
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

v2: use simpler workaround, as discussed with Chao Yu.

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 68edb47f5f71..cf5ec39f907d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -713,7 +713,7 @@ int f2fs_map_blocks(struct inode *inode, struct 
f2fs_map_blocks *map,
}
 
prealloc = 0;
-   ofs_in_node = dn.ofs_in_node;
+   last_ofs_in_node = ofs_in_node = dn.ofs_in_node;
end_offset = ADDRS_PER_PAGE(dn.node_page, inode);
 
 next_block:
-- 
2.9.0


--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 04/28] f2fs: replace a build-time warning with runtime WARN_ON

2016-10-27 Thread Chao Yu 
On 2016/10/26 22:57, Arnd Bergmann wrote:
> On Wednesday, October 26, 2016 10:05:00 PM CEST Chao Yu wrote:
>> On 2016/10/18 6:05, Arnd Bergmann wrote:
>>> gcc is unsure about the use of last_ofs_in_node, which might happen
>>> without a prior initialization:
>>>
>>> fs/f2fs//git/arm-soc/fs/f2fs/data.c: In function ‘f2fs_map_blocks’:
>>> fs/f2fs/data.c:799:54: warning: ‘last_ofs_in_node’ may be used 
>>> uninitialized in this function [-Wmaybe-uninitialized]
>>>if (prealloc && dn.ofs_in_node != last_ofs_in_node + 1) {
>>
>> In each round of dnode block traverse, we will init 'prealloc' and then 
>> update
>> 'prealloc' and 'last_ofs_in_node' together in below lines of f2fs_map_blocks:
>> if (flag == F2FS_GET_BLOCK_PRE_AIO) {
>> if (blkaddr == NULL_ADDR) {
>> prealloc++;
>> last_ofs_in_node = dn.ofs_in_node;
>> }
>> }
>>
>> Then in below codes, it is safe to use 'last_ofs_in_node' since we will check
>> 'prealloc' firstly, so if 'prealloc' is non-zero, 'last_ofs_in_node' must be 
>> valid.
>> if (prealloc && dn.ofs_in_node != last_ofs_in_node + 1) {
>>
>> So I think we should not add WARN_ON there.
> 
> Ok, that make sense. Thanks for taking a closer look!
> 
> Should we just set last_ofs_in_node to the same value as ofs_in_node
> before the loop?

I think it's OK as it can remove warning compiler reports. :)

Thanks,

> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 9ae194f..14db4b7 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -716,7 +716,7 @@ int f2fs_map_blocks(struct inode *inode, struct 
> f2fs_map_blocks *map,
>   }
>  
>   prealloc = 0;
> - ofs_in_node = dn.ofs_in_node;
> + last_ofs_in_node = ofs_in_node = dn.ofs_in_node;
>   end_offset = ADDRS_PER_PAGE(dn.node_page, inode);
>  
>  next_block:
> 
>   Arnd
> 
> .
> 


--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: use BIO_MAX_PAGES for bio allocation

2016-10-27 Thread Chao Yu 
On 2016/10/19 2:48, Jaegeuk Kim wrote:
> We don't need to allocate bio partially in order to maximize sequential 
> writes.

We are going to allocate bio with max size supposing that there will be more
opportunity to merge small IOs into one bio. Otherwise, if MAX_BIO_BLOCKS is
smaller than BIO_MAX_PAGES, less IOs can be merged. Is my understanding correct?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim 
> ---
>  fs/f2fs/checkpoint.c |  2 +-
>  fs/f2fs/data.c   |  4 +---
>  fs/f2fs/node.c   |  3 +--
>  fs/f2fs/segment.c|  4 ++--
>  fs/f2fs/segment.h| 17 +++--
>  5 files changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 654f5d7..157b7fd 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -228,7 +228,7 @@ void ra_meta_pages_cond(struct f2fs_sb_info *sbi, pgoff_t 
> index)
>   f2fs_put_page(page, 0);
>  
>   if (readahead)
> - ra_meta_pages(sbi, index, MAX_BIO_BLOCKS(sbi), META_POR, true);
> + ra_meta_pages(sbi, index, BIO_MAX_PAGES, META_POR, true);
>  }
>  
>  static int f2fs_write_meta_page(struct page *page,
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 77cd9ac..fe8ce5a 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -307,10 +307,8 @@ void f2fs_submit_page_mbio(struct f2fs_io_info *fio)
>   __submit_merged_bio(io);
>  alloc_new:
>   if (io->bio == NULL) {
> - int bio_blocks = MAX_BIO_BLOCKS(sbi);
> -
>   io->bio = __bio_alloc(sbi, fio->new_blkaddr,
> - bio_blocks, is_read);
> + BIO_MAX_PAGES, is_read);
>   io->fio = *fio;
>   }
>  
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 69c42e2..285e2a5 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -2101,7 +2101,6 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
>   struct f2fs_node *rn;
>   struct f2fs_summary *sum_entry;
>   block_t addr;
> - int bio_blocks = MAX_BIO_BLOCKS(sbi);
>   int i, idx, last_offset, nrpages;
>  
>   /* scan the node segment */
> @@ -2110,7 +2109,7 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
>   sum_entry = >entries[0];
>  
>   for (i = 0; i < last_offset; i += nrpages, addr += nrpages) {
> - nrpages = min(last_offset - i, bio_blocks);
> + nrpages = min(last_offset - i, BIO_MAX_PAGES);
>  
>   /* readahead node pages */
>   ra_meta_pages(sbi, addr, nrpages, META_POR, true);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 7a7e3f1..a21a1a3 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2355,10 +2355,10 @@ static void build_sit_entries(struct f2fs_sb_info 
> *sbi)
>   int sit_blk_cnt = SIT_BLK_CNT(sbi);
>   unsigned int i, start, end;
>   unsigned int readed, start_blk = 0;
> - int nrpages = MAX_BIO_BLOCKS(sbi) * 8;
>  
>   do {
> - readed = ra_meta_pages(sbi, start_blk, nrpages, META_SIT, true);
> + readed = ra_meta_pages(sbi, start_blk, BIO_MAX_PAGES,
> + META_SIT, true);
>  
>   start = start_blk * sit_i->sents_per_block;
>   end = (start_blk + readed) * sit_i->sents_per_block;
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index a6efb5c..d8ff069 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -102,8 +102,6 @@
>   (((sector_t)blk_addr) << F2FS_LOG_SECTORS_PER_BLOCK)
>  #define SECTOR_TO_BLOCK(sectors) \
>   (sectors >> F2FS_LOG_SECTORS_PER_BLOCK)
> -#define MAX_BIO_BLOCKS(sbi)  \
> - ((int)min((int)max_hw_blocks(sbi), BIO_MAX_PAGES))
>  
>  /*
>   * indicate a block allocation direction: RIGHT and LEFT.
> @@ -698,13 +696,6 @@ static inline bool sec_usage_check(struct f2fs_sb_info 
> *sbi, unsigned int secno)
>   return false;
>  }
>  
> -static inline unsigned int max_hw_blocks(struct f2fs_sb_info *sbi)
> -{
> - struct block_device *bdev = sbi->sb->s_bdev;
> - struct request_queue *q = bdev_get_queue(bdev);
> - return SECTOR_TO_BLOCK(queue_max_sectors(q));
> -}
> -
>  /*
>   * It is very important to gather dirty pages and write at once, so that we 
> can
>   * submit a big bio without interfering other data writes.
> @@ -722,7 +713,7 @@ static inline int nr_pages_to_skip(struct f2fs_sb_info 
> *sbi, int type)
>   else if (type == NODE)
>   return 8 * sbi->blocks_per_seg;
>   else if (type == META)
> - return 8 * MAX_BIO_BLOCKS(sbi);
> + return 8 * BIO_MAX_PAGES;
>   else
>   return 0;
>  }
> @@ -739,11 +730,9 @@ static inline long nr_pages_to_write(struct f2fs_sb_info 
> *sbi, int type,
>   return 0;
>  
>   nr_to_write = wbc->nr_to_write;
> -
> + desired =