Hi Jaegeuk,
On 2016/5/10 7:00, Jaegeuk Kim wrote:
> Hi Chao,
>
> On Mon, May 09, 2016 at 07:56:30PM +0800, Chao Yu wrote:
>> This patch introduces reserve_new_blocks to make preallocation of multi
>> blocks as in batch operation, so it can avoid lots of redundant
>> operation, result in better performance.
>>
>> In virtual machine, with rotational device:
>>
>> time fallocate -l 32G /mnt/f2fs/file
>>
>> Before:
>> real 0m4.584s
>> user 0m0.000s
>> sys 0m4.580s
>>
>> After:
>> real 0m0.292s
>> user 0m0.000s
>> sys 0m0.272s
>
> It's cool.
> Let me add my test results as well.
>
> In x86, with SSD:
>
> time fallocate -l 500G $MNT/testfile
>
> Before : 24.758 s
> After : 1.604 s
>
> By the way, there is one thing we should consider, which is the ENOSPC case.
> Could you check this out on top of your patch?
>
> If you don't mind, let me integrate this into your patch.
No problem. :)
And see below comments please.
> Let me know.
>
> Thanks,
>
> ---
> fs/f2fs/data.c | 9 +++++----
> fs/f2fs/f2fs.h | 20 +++++++++++++-------
> include/trace/events/f2fs.h | 8 ++++----
> 3 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index ea0abdc..da640e1 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -278,7 +278,7 @@ alloc_new:
> trace_f2fs_submit_page_mbio(fio->page, fio);
> }
>
> -void __set_data_blkaddr(struct dnode_of_data *dn)
> +static void __set_data_blkaddr(struct dnode_of_data *dn)
> {
> struct f2fs_node *rn = F2FS_NODE(dn->node_page);
> __le32 *addr_array;
> @@ -310,7 +310,7 @@ void f2fs_update_data_blkaddr(struct dnode_of_data *dn,
> block_t blkaddr)
> }
>
> int reserve_new_blocks(struct dnode_of_data *dn, unsigned int start,
> - unsigned int count)
> + blkcnt_t count)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(dn->inode);
> unsigned int ofs_in_node;
> @@ -320,7 +320,7 @@ int reserve_new_blocks(struct dnode_of_data *dn, unsigned
> int start,
>
> if (unlikely(is_inode_flag_set(F2FS_I(dn->inode), FI_NO_ALLOC)))
> return -EPERM;
> - if (unlikely(!inc_valid_block_count(sbi, dn->inode, count)))
> + if (unlikely(!inc_valid_block_count(sbi, dn->inode, &count)))
> return -ENOSPC;
>
> trace_f2fs_reserve_new_blocks(dn->inode, dn->nid,
> @@ -574,6 +574,7 @@ static int __allocate_data_block(struct dnode_of_data *dn)
> struct node_info ni;
> int seg = CURSEG_WARM_DATA;
> pgoff_t fofs;
> + blkcnt_t count = 1;
>
> if (unlikely(is_inode_flag_set(F2FS_I(dn->inode), FI_NO_ALLOC)))
> return -EPERM;
> @@ -582,7 +583,7 @@ static int __allocate_data_block(struct dnode_of_data *dn)
> if (dn->data_blkaddr == NEW_ADDR)
> goto alloc;
>
> - if (unlikely(!inc_valid_block_count(sbi, dn->inode, 1)))
> + if (unlikely(!inc_valid_block_count(sbi, dn->inode, &count)))
> return -ENOSPC;
>
> alloc:
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 75b0084..00fe63c 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1114,7 +1114,7 @@ static inline bool f2fs_has_xattr_block(unsigned int
> ofs)
> }
>
> static inline bool inc_valid_block_count(struct f2fs_sb_info *sbi,
> - struct inode *inode, blkcnt_t count)
> + struct inode *inode, blkcnt_t *count)
> {
> block_t valid_block_count;
>
> @@ -1126,14 +1126,19 @@ static inline bool inc_valid_block_count(struct
> f2fs_sb_info *sbi,
> }
> #endif
> valid_block_count =
> - sbi->total_valid_block_count + (block_t)count;
> + sbi->total_valid_block_count + (block_t)(*count);
> if (unlikely(valid_block_count > sbi->user_block_count)) {
> - spin_unlock(&sbi->stat_lock);
> - return false;
> + *count = sbi->user_block_count - sbi->total_valid_block_count;
> + if (!*count) {
> + spin_unlock(&sbi->stat_lock);
> + return false;
> + }
If we can only allocate partial blocks, we should let f2fs_map_blocks being
ware of that, otherwise, map->m_len will be updated incorrectly.
Thanks,
> }
> - inode->i_blocks += count;
> - sbi->total_valid_block_count = valid_block_count;
> - sbi->alloc_valid_block_count += (block_t)count;
> + /* *count can be recalculated */
> + inode->i_blocks += *count;
> + sbi->total_valid_block_count =
> + sbi->total_valid_block_count + (block_t)(*count);
> + sbi->alloc_valid_block_count += (block_t)(*count);
> spin_unlock(&sbi->stat_lock);
> return true;
> }
> @@ -1965,6 +1970,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *);
> void f2fs_submit_page_mbio(struct f2fs_io_info *);
> void set_data_blkaddr(struct dnode_of_data *);
> void f2fs_update_data_blkaddr(struct dnode_of_data *, block_t);
> +int reserve_new_blocks(struct dnode_of_data *, unsigned int, blkcnt_t);
> int reserve_new_block(struct dnode_of_data *);
> int f2fs_get_block(struct dnode_of_data *, pgoff_t);
> ssize_t f2fs_preallocate_blocks(struct kiocb *, struct iov_iter *);
> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> index 5f927ff..497e6e8 100644
> --- a/include/trace/events/f2fs.h
> +++ b/include/trace/events/f2fs.h
> @@ -697,7 +697,7 @@ TRACE_EVENT(f2fs_direct_IO_exit,
> TRACE_EVENT(f2fs_reserve_new_blocks,
>
> TP_PROTO(struct inode *inode, nid_t nid, unsigned int ofs_in_node,
> - unsigned int count),
> + blkcnt_t count),
>
> TP_ARGS(inode, nid, ofs_in_node, count),
>
> @@ -705,7 +705,7 @@ TRACE_EVENT(f2fs_reserve_new_blocks,
> __field(dev_t, dev)
> __field(nid_t, nid)
> __field(unsigned int, ofs_in_node)
> - __field(unsigned int, count)
> + __field(blkcnt_t, count)
> ),
>
> TP_fast_assign(
> @@ -715,11 +715,11 @@ TRACE_EVENT(f2fs_reserve_new_blocks,
> __entry->count = count;
> ),
>
> - TP_printk("dev = (%d,%d), nid = %u, ofs_in_node = %u, count = %u",
> + TP_printk("dev = (%d,%d), nid = %u, ofs_in_node = %u, count = %llu",
> show_dev(__entry),
> (unsigned int)__entry->nid,
> __entry->ofs_in_node,
> - __entry->count)
> + (unsigned long long)__entry->count)
> );
>
> DECLARE_EVENT_CLASS(f2fs__submit_page_bio,
>
