Re: [Virtio-fs] [PATCH 2/2] virtio-fs: do not removemapping if dmap will be used immediately

[piaojun <piaojun@huawei.com>]

On 2019/8/14 2:29, Liu Bo wrote:
> On Tue, Aug 13, 2019 at 09:40:12AM +0800, piaojun wrote:
>> Hi Bo,
>>
>> On 2019/8/13 2:32, Liu Bo wrote:
>>> According to how we implement REMOVEMAPPING in daemon, it's unnecessary to
>>> involve a REMOVEMAPPING to reuse a dmap when doing inline reclaim because
>>> dmaps got from inline reclaim get reused for another mapping without being
>>> added back to 'free' dmap pool.
>>>
>>> This skips REMOVEMAPPING for inline reclaim only and we don't do
>>> REMOVEMAPPING unless someone has raced in to add a dmap to the range.
>>>
>>> With the following two patches applied,
>>> "virtio-fs: do not removemapping if dmap will be used immediately"
>>> "virtio-fs: try hard to do inline reclaim",
>>>
>>> fio fio-tmp.job
>>> read: IOPS=8055, BW=31.5MiB/s (32.0MB/s)(3776MiB/120001msec)
>>
>> Do you mean the before result is 31.5MB/s and after is 32MB/s? I wonder
>> what is your backend storage?
>>
> 
> hmm...the 'w/o/ results are listed in the below 'w/o' section.

"w/o=without" plays a joke with me, :)

> 
> thanks,
> -liubo
> 
>> Jun
>>
>>> clat (usec): min=6, max=1842, avg=113.75, stdev=83.78
>>> clat percentiles (usec):
>>> 95.00th=[  202]
>>> 99.00th=[  221]
>>>
>>> w/o
>>> read: IOPS=4312, BW=16.8MiB/s (17.7MB/s)(2021MiB/120001msec)
>>> clat (usec): min=10, max=3415, avg=220.98, stdev=102.85
>>> clat percentiles (usec):
>>> 95.00th=[  359]
>>> 99.00th=[  392]
>>>
>>> [1]:
>>> virtiofsd -o cache="always"
>>> qemu -device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs-1,cache-size=4G
>>>
>>> [2]:
>>> cat fio-tmp.job
>>>
>>> ; fio-rand-read.job for fiotest
>>>
>>> [global]
>>> name=fio-rand-read
>>> filename=fio_file
>>> rw=randread
>>> bs=4K
>>> direct=1
>>> numjobs=1
>>> time_based=1
>>> runtime=120
>>> directory=/mnt/test/
>>>
>>> [file1]
>>> size=10G
>>> ioengine=psync
>>> iodepth=1
>>>
>>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>>> ---
>>>  fs/fuse/file.c | 18 +++++++++++++++---
>>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>> index 2ea670a..b5239b1 100644
>>> --- a/fs/fuse/file.c
>>> +++ b/fs/fuse/file.c
>>> @@ -1933,7 +1933,11 @@ static int iomap_begin_setup_new_mapping(struct inode *inode, loff_t pos,
>>>  	if (ret < 0) {
>>>  		printk("fuse_setup_one_mapping() failed. err=%d"
>>>  			" pos=0x%llx, writable=%d\n", ret, pos, writable);
>>> +
>>> +		/* liubo: ignore failure if removemapping fails. */
>>> +		dmap_removemapping_one(inode, dmap);

Why do we need remove mapping if the setup has not been done?

Jun

>>>  		dmap_add_to_free_pool(fc, alloc_dmap);
>>> +
>>>  		up_write(&fi->i_dmap_sem);
>>>  		return ret;
>>>  	}
>>> @@ -3996,7 +4000,8 @@ static int dmap_writeback_invalidate(struct inode *inode,
>>>  }
>>>  
>>>  static int reclaim_one_dmap_locked(struct fuse_conn *fc, struct inode *inode,
>>> -				   struct fuse_dax_mapping *dmap)
>>> +				   struct fuse_dax_mapping *dmap,
>>> +				   bool inline_reclaim)
>>>  {
>>>  	int ret;
>>>  	struct fuse_inode *fi = get_fuse_inode(inode);
>>> @@ -4021,6 +4026,13 @@ static int reclaim_one_dmap_locked(struct fuse_conn *fc, struct inode *inode,
>>>  	fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
>>>  	fi->nr_dmaps--;
>>>  
>>> +	/*
>>> +	 * for inline reclaim, it's unnecessary to removing mapping since it'll
>>> +	 * be used by another range immediately.
>>> +	 */
>>> +	if (inline_reclaim)
>>> +		return 0;
>>> +
>>>  	ret = dmap_removemapping_one(inode, dmap);
>>>  	if (ret) {
>>>  		pr_warn("Failed to remove mapping. offset=0x%llx len=0x%llx\n",
>>> @@ -4051,7 +4063,7 @@ static int reclaim_one_dmap_locked(struct fuse_conn *fc, struct inode *inode,
>>>  	if (!dmap)
>>>  		return NULL;
>>>  
>>> -	ret = reclaim_one_dmap_locked(fc, inode, dmap);
>>> +	ret = reclaim_one_dmap_locked(fc, inode, dmap, true);
>>>  	if (ret < 0)
>>>  		return ERR_PTR(ret);
>>>  
>>> @@ -4136,7 +4148,7 @@ int lookup_and_reclaim_dmap_locked(struct fuse_conn *fc, struct inode *inode,
>>>  	if (refcount_read(&dmap->refcnt) > 1)
>>>  		return 0;
>>>  
>>> -	ret = reclaim_one_dmap_locked(fc, inode, dmap);
>>> +	ret = reclaim_one_dmap_locked(fc, inode, dmap, false);
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>>
> .
>