[Virtio-fs] [PATCH 0/2] virtiofs: remove dmap when truncating inode

[Virtio-fs] [PATCH 0/2] virtiofs: remove dmap when truncating inode

[Peng Tao]

Hi,

The first patch is a simple cleanup. The second patch removes
dmaps when truncating a file since they are no longer valid
and we can return them to free pool directly. Otherwise they are
cleaned up by the reclaim code that might try to reclaim valid
dmaps first.


Peng Tao (2):
  fuse: remove unnecessary allocated_ranges
  fuse: remove dmap when truncating inode

 fs/fuse/dir.c    |  5 ++++
 fs/fuse/file.c   | 69 +++++++++++++++++++++++++++++++++++++++---------
 fs/fuse/fuse_i.h |  2 ++
 fs/fuse/inode.c  |  7 +++--
 4 files changed, 66 insertions(+), 17 deletions(-)

-- 
2.17.1

[Virtio-fs] [PATCH 1/2] fuse: remove unnecessary allocated_ranges

[Peng Tao]

It is always nr_ranges.

Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
---
 fs/fuse/inode.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index dd16c7f6a561..cb56572c4b26 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -640,7 +640,7 @@ static int fuse_dax_mem_range_init(struct fuse_conn *fc,
 	phys_addr_t phys_addr;
 	int ret = 0, id;
 	size_t dax_size = -1;
-	unsigned long allocated_ranges = 0, i;
+	unsigned long i;
 
 	id = dax_read_lock();
 	nr_pages = dax_direct_access(dax_dev, 0, PHYS_PFN(dax_size), &kaddr,
@@ -670,12 +670,11 @@ static int fuse_dax_mem_range_init(struct fuse_conn *fc,
 		range->length = FUSE_DAX_MEM_RANGE_SZ;
 		list_add_tail(&range->list, &mem_ranges);
 		INIT_LIST_HEAD(&range->busy_list);
-		allocated_ranges++;
 	}
 
 	list_replace_init(&mem_ranges, &fc->free_ranges);
-	fc->nr_free_ranges = allocated_ranges;
-	fc->nr_ranges = allocated_ranges;
+	fc->nr_free_ranges = nr_ranges;
+	fc->nr_ranges = nr_ranges;
 	return 0;
 out_err:
 	/* Free All allocated elements */
-- 
2.17.1

[Virtio-fs] [PATCH 2/2] fuse: remove dmap when truncating inode

[Peng Tao]

The obseleted dmap entries can be put back to global free list
immediately and we don't have to rely on reclaim code to free them.

Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
---
 fs/fuse/dir.c    |  5 ++++
 fs/fuse/file.c   | 69 +++++++++++++++++++++++++++++++++++++++---------
 fs/fuse/fuse_i.h |  2 ++
 3 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 3f923fe7841a..233c3ed391f1 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1751,6 +1751,11 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 		down_write(&fi->i_mmap_sem);
 		truncate_pagecache(inode, outarg.attr.size);
 		invalidate_inode_pages2(inode->i_mapping);
+		// Free dmap beyond i_size
+		if (IS_DAX(inode) && oldsize > outarg.attr.size)
+			fuse_dax_free_mappings_range(fc, inode,
+						     outarg.attr.size, -1);
+
 		up_write(&fi->i_mmap_sem);
 	}
 
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 51faed351c7c..6d130f2f3a23 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -30,6 +30,8 @@ static long __fuse_file_fallocate(struct file *file, int mode,
 					loff_t offset, loff_t length);
 static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
 					struct inode *inode);
+static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi,
+					struct fuse_dax_mapping *dmap);
 
 static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
 			  int opcode, struct fuse_open_out *outargp)
@@ -3752,9 +3754,7 @@ int fuse_dax_reclaim_dmap_locked(struct fuse_conn *fc, struct inode *inode,
 		return ret;
 	}
 
-	/* Remove dax mapping from inode interval tree now */
-	fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
-	fi->nr_dmaps--;
+	fuse_dax_do_remove_mapping_locked(fi, dmap);
 	return 0;
 }
 
@@ -3831,6 +3831,58 @@ static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
 	}
 }
 
+/* Cleanup dmap entry and add back to free list */
+static void fuse_dax_do_free_mapping_locked(struct fuse_conn *fc, struct fuse_dax_mapping *dmap)
+{
+	pr_debug("fuse: freed memory range start=0x%llx end=0x%llx "
+		"window_offset=0x%llx length=0x%llx\n", dmap->start,
+		dmap->end, dmap->window_offset, dmap->length);
+	spin_lock(&fc->lock);
+	__dmap_remove_busy_list(fc, dmap);
+	dmap->inode = NULL;
+	dmap->start = dmap->end = 0;
+	__free_dax_mapping(fc, dmap);
+	spin_unlock(&fc->lock);
+}
+
+/* Remove dax mapping from inode interval tree */
+static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi, struct fuse_dax_mapping *dmap)
+{
+	fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
+	fi->nr_dmaps--;
+}
+
+/*
+ * Free inode dmap entries whose range falls entirely inside [start, end].
+ * Called with inode->i_rwsem and fuse_inode->i_mmap_sem held.
+ *
+ * No need to send FUSE_REMOVEMAPPING to userspace because the userspace mappings
+ * will be overridden next time dmap is used -- same logic is applied in
+ * fuse_dax_reclaim_first_mapping().
+ */
+void fuse_dax_free_mappings_range(struct fuse_conn *fc, struct inode *inode, loff_t start, loff_t end)
+{
+	struct fuse_inode *fi = get_fuse_inode(inode);
+	struct fuse_dax_mapping *dmap;
+
+	WARN_ON(!inode_is_locked(inode));
+	WARN_ON(!rwsem_is_locked(&fi->i_mmap_sem));
+
+	/* interval tree search matches intersecting entries.
+	 * Adjust the range to avoid dropping partial valid entries. */
+	start = ALIGN(start, FUSE_DAX_MEM_RANGE_SZ);
+	end = ALIGN_DOWN(end, FUSE_DAX_MEM_RANGE_SZ);
+
+	pr_debug("fuse: fuse_dax_free_mappings_range start=0x%llx, end=0x%llx\n", start, end);
+	/* Lock ordering follows fuse_dax_free_one_mapping() */
+	down_write(&fi->i_dmap_sem);
+	while ((dmap = fuse_dax_interval_tree_iter_first(&fi->dmap_tree, start, end))) {
+		fuse_dax_do_remove_mapping_locked(fi, dmap);
+		fuse_dax_do_free_mapping_locked(fc, dmap);
+	}
+	up_write(&fi->i_dmap_sem);
+}
+
 int fuse_dax_free_one_mapping_locked(struct fuse_conn *fc, struct inode *inode,
 				u64 dmap_start)
 {
@@ -3852,17 +3904,8 @@ int fuse_dax_free_one_mapping_locked(struct fuse_conn *fc, struct inode *inode,
 	if (ret < 0)
 		return ret;
 
-	/* Cleanup dmap entry and add back to free list */
-	spin_lock(&fc->lock);
-	__dmap_remove_busy_list(fc, dmap);
-	dmap->inode = NULL;
-	dmap->start = dmap->end = 0;
-	__free_dax_mapping(fc, dmap);
-	spin_unlock(&fc->lock);
+	fuse_dax_do_free_mapping_locked(fc, dmap);
 
-	pr_debug("fuse: freed memory range window_offset=0x%llx,"
-				" length=0x%llx\n", dmap->window_offset,
-				dmap->length);
 	return ret;
 }
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 1149281ab1e8..e273f1209f5b 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1189,5 +1189,7 @@ unsigned fuse_len_args(unsigned numargs, struct fuse_arg *args);
 u64 fuse_get_unique(struct fuse_iqueue *fiq);
 void fuse_dax_free_mem_worker(struct work_struct *work);
 void fuse_removemapping(struct inode *inode);
+void fuse_dax_free_mappings_range(struct fuse_conn *fc, struct inode *inode,
+			loff_t start, loff_t end);
 
 #endif /* _FS_FUSE_I_H */
-- 
2.17.1

Re: [Virtio-fs] [PATCH 2/2] fuse: remove dmap when truncating inode

[Vivek Goyal]

On Fri, May 10, 2019 at 06:29:57PM +0800, Peng Tao wrote:
> The obseleted dmap entries can be put back to global free list
> immediately and we don't have to rely on reclaim code to free them.
> 
> Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> ---
>  fs/fuse/dir.c    |  5 ++++
>  fs/fuse/file.c   | 69 +++++++++++++++++++++++++++++++++++++++---------
>  fs/fuse/fuse_i.h |  2 ++
>  3 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 3f923fe7841a..233c3ed391f1 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1751,6 +1751,11 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
>  		down_write(&fi->i_mmap_sem);
>  		truncate_pagecache(inode, outarg.attr.size);
>  		invalidate_inode_pages2(inode->i_mapping);
> +		// Free dmap beyond i_size
> +		if (IS_DAX(inode) && oldsize > outarg.attr.size)
> +			fuse_dax_free_mappings_range(fc, inode,
> +						     outarg.attr.size, -1);
> +
>  		up_write(&fi->i_mmap_sem);
>  	}
>  
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 51faed351c7c..6d130f2f3a23 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -30,6 +30,8 @@ static long __fuse_file_fallocate(struct file *file, int mode,
>  					loff_t offset, loff_t length);
>  static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
>  					struct inode *inode);
> +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi,
> +					struct fuse_dax_mapping *dmap);
>  
>  static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
>  			  int opcode, struct fuse_open_out *outargp)
> @@ -3752,9 +3754,7 @@ int fuse_dax_reclaim_dmap_locked(struct fuse_conn *fc, struct inode *inode,
>  		return ret;
>  	}
>  
> -	/* Remove dax mapping from inode interval tree now */
> -	fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> -	fi->nr_dmaps--;
> +	fuse_dax_do_remove_mapping_locked(fi, dmap);
>  	return 0;
>  }
>  
> @@ -3831,6 +3831,58 @@ static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
>  	}
>  }
>  
> +/* Cleanup dmap entry and add back to free list */
> +static void fuse_dax_do_free_mapping_locked(struct fuse_conn *fc, struct fuse_dax_mapping *dmap)
> +{
> +	pr_debug("fuse: freed memory range start=0x%llx end=0x%llx "
> +		"window_offset=0x%llx length=0x%llx\n", dmap->start,
> +		dmap->end, dmap->window_offset, dmap->length);
> +	spin_lock(&fc->lock);
> +	__dmap_remove_busy_list(fc, dmap);
> +	dmap->inode = NULL;
> +	dmap->start = dmap->end = 0;
> +	__free_dax_mapping(fc, dmap);
> +	spin_unlock(&fc->lock);
> +}
> +
> +/* Remove dax mapping from inode interval tree */
> +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi, struct fuse_dax_mapping *dmap)
> +{
> +	fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> +	fi->nr_dmaps--;
> +}

this is just a two line funciton. I think lets open code it and we don't
need a separate function for this.


> +
> +/*
> + * Free inode dmap entries whose range falls entirely inside [start, end].
> + * Called with inode->i_rwsem and fuse_inode->i_mmap_sem held.
> + *
> + * No need to send FUSE_REMOVEMAPPING to userspace because the userspace mappings
> + * will be overridden next time dmap is used -- same logic is applied in
> + * fuse_dax_reclaim_first_mapping().

This is true that sending FUSE_REMOVEMAPPING is not strictly needed. But,
thinking more about it, what if range does not get used soon. Then it
will keep resources unnecessarily busy on host? (fd, file, inode, dentry,
vma etc).

I think it probably is a good idea to send FUSE_REMOVEMAPPING to daemon
when range is being freed. Only case where we are reclaiming a range
and reusing immediately, we can skip sending FUSE_REMOVEMAPPING.

This will especially be more meaniningful if dax window is large and
has potential to create lots of mappings.


Thanks
Vivek

> + */
> +void fuse_dax_free_mappings_range(struct fuse_conn *fc, struct inode *inode, loff_t start, loff_t end)
> +{
> +	struct fuse_inode *fi = get_fuse_inode(inode);
> +	struct fuse_dax_mapping *dmap;
> +
> +	WARN_ON(!inode_is_locked(inode));
> +	WARN_ON(!rwsem_is_locked(&fi->i_mmap_sem));
> +
> +	/* interval tree search matches intersecting entries.
> +	 * Adjust the range to avoid dropping partial valid entries. */
> +	start = ALIGN(start, FUSE_DAX_MEM_RANGE_SZ);
> +	end = ALIGN_DOWN(end, FUSE_DAX_MEM_RANGE_SZ);
> +
> +	pr_debug("fuse: fuse_dax_free_mappings_range start=0x%llx, end=0x%llx\n", start, end);
> +	/* Lock ordering follows fuse_dax_free_one_mapping() */
> +	down_write(&fi->i_dmap_sem);
> +	while ((dmap = fuse_dax_interval_tree_iter_first(&fi->dmap_tree, start, end))) {
> +		fuse_dax_do_remove_mapping_locked(fi, dmap);
> +		fuse_dax_do_free_mapping_locked(fc, dmap);
> +	}
> +	up_write(&fi->i_dmap_sem);
> +}
> +
>  int fuse_dax_free_one_mapping_locked(struct fuse_conn *fc, struct inode *inode,
>  				u64 dmap_start)
>  {
> @@ -3852,17 +3904,8 @@ int fuse_dax_free_one_mapping_locked(struct fuse_conn *fc, struct inode *inode,
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Cleanup dmap entry and add back to free list */
> -	spin_lock(&fc->lock);
> -	__dmap_remove_busy_list(fc, dmap);
> -	dmap->inode = NULL;
> -	dmap->start = dmap->end = 0;
> -	__free_dax_mapping(fc, dmap);
> -	spin_unlock(&fc->lock);
> +	fuse_dax_do_free_mapping_locked(fc, dmap);
>  
> -	pr_debug("fuse: freed memory range window_offset=0x%llx,"
> -				" length=0x%llx\n", dmap->window_offset,
> -				dmap->length);
>  	return ret;
>  }
>  
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 1149281ab1e8..e273f1209f5b 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1189,5 +1189,7 @@ unsigned fuse_len_args(unsigned numargs, struct fuse_arg *args);
>  u64 fuse_get_unique(struct fuse_iqueue *fiq);
>  void fuse_dax_free_mem_worker(struct work_struct *work);
>  void fuse_removemapping(struct inode *inode);
> +void fuse_dax_free_mappings_range(struct fuse_conn *fc, struct inode *inode,
> +			loff_t start, loff_t end);
>  
>  #endif /* _FS_FUSE_I_H */
> -- 
> 2.17.1
> 

Re: [Virtio-fs] [PATCH 1/2] fuse: remove unnecessary allocated_ranges

[Vivek Goyal]

On Fri, May 10, 2019 at 06:29:56PM +0800, Peng Tao wrote:
> It is always nr_ranges.

This looks like a good cleanup. Will squash it with existing patches.

Vivek

> 
> Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> ---
>  fs/fuse/inode.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index dd16c7f6a561..cb56572c4b26 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -640,7 +640,7 @@ static int fuse_dax_mem_range_init(struct fuse_conn *fc,
>  	phys_addr_t phys_addr;
>  	int ret = 0, id;
>  	size_t dax_size = -1;
> -	unsigned long allocated_ranges = 0, i;
> +	unsigned long i;
>  
>  	id = dax_read_lock();
>  	nr_pages = dax_direct_access(dax_dev, 0, PHYS_PFN(dax_size), &kaddr,
> @@ -670,12 +670,11 @@ static int fuse_dax_mem_range_init(struct fuse_conn *fc,
>  		range->length = FUSE_DAX_MEM_RANGE_SZ;
>  		list_add_tail(&range->list, &mem_ranges);
>  		INIT_LIST_HEAD(&range->busy_list);
> -		allocated_ranges++;
>  	}
>  
>  	list_replace_init(&mem_ranges, &fc->free_ranges);
> -	fc->nr_free_ranges = allocated_ranges;
> -	fc->nr_ranges = allocated_ranges;
> +	fc->nr_free_ranges = nr_ranges;
> +	fc->nr_ranges = nr_ranges;
>  	return 0;
>  out_err:
>  	/* Free All allocated elements */
> -- 
> 2.17.1
> 

Re: [Virtio-fs] [PATCH 2/2] fuse: remove dmap when truncating inode

[Tao Peng]

On Fri, May 10, 2019 at 11:23 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, May 10, 2019 at 06:29:57PM +0800, Peng Tao wrote:
> > The obseleted dmap entries can be put back to global free list
> > immediately and we don't have to rely on reclaim code to free them.
> >
> > Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> > ---
> >  fs/fuse/dir.c    |  5 ++++
> >  fs/fuse/file.c   | 69 +++++++++++++++++++++++++++++++++++++++---------
> >  fs/fuse/fuse_i.h |  2 ++
> >  3 files changed, 63 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 3f923fe7841a..233c3ed391f1 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1751,6 +1751,11 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
> >               down_write(&fi->i_mmap_sem);
> >               truncate_pagecache(inode, outarg.attr.size);
> >               invalidate_inode_pages2(inode->i_mapping);
> > +             // Free dmap beyond i_size
> > +             if (IS_DAX(inode) && oldsize > outarg.attr.size)
> > +                     fuse_dax_free_mappings_range(fc, inode,
> > +                                                  outarg.attr.size, -1);
> > +
> >               up_write(&fi->i_mmap_sem);
> >       }
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 51faed351c7c..6d130f2f3a23 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -30,6 +30,8 @@ static long __fuse_file_fallocate(struct file *file, int mode,
> >                                       loff_t offset, loff_t length);
> >  static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
> >                                       struct inode *inode);
> > +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi,
> > +                                     struct fuse_dax_mapping *dmap);
> >
> >  static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
> >                         int opcode, struct fuse_open_out *outargp)
> > @@ -3752,9 +3754,7 @@ int fuse_dax_reclaim_dmap_locked(struct fuse_conn *fc, struct inode *inode,
> >               return ret;
> >       }
> >
> > -     /* Remove dax mapping from inode interval tree now */
> > -     fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> > -     fi->nr_dmaps--;
> > +     fuse_dax_do_remove_mapping_locked(fi, dmap);
> >       return 0;
> >  }
> >
> > @@ -3831,6 +3831,58 @@ static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
> >       }
> >  }
> >
> > +/* Cleanup dmap entry and add back to free list */
> > +static void fuse_dax_do_free_mapping_locked(struct fuse_conn *fc, struct fuse_dax_mapping *dmap)
> > +{
> > +     pr_debug("fuse: freed memory range start=0x%llx end=0x%llx "
> > +             "window_offset=0x%llx length=0x%llx\n", dmap->start,
> > +             dmap->end, dmap->window_offset, dmap->length);
> > +     spin_lock(&fc->lock);
> > +     __dmap_remove_busy_list(fc, dmap);
> > +     dmap->inode = NULL;
> > +     dmap->start = dmap->end = 0;
> > +     __free_dax_mapping(fc, dmap);
> > +     spin_unlock(&fc->lock);
> > +}
> > +
> > +/* Remove dax mapping from inode interval tree */
> > +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi, struct fuse_dax_mapping *dmap)
> > +{
> > +     fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> > +     fi->nr_dmaps--;
> > +}
>
> this is just a two line funciton. I think lets open code it and we don't
> need a separate function for this.
ok.

>
>
> > +
> > +/*
> > + * Free inode dmap entries whose range falls entirely inside [start, end].
> > + * Called with inode->i_rwsem and fuse_inode->i_mmap_sem held.
> > + *
> > + * No need to send FUSE_REMOVEMAPPING to userspace because the userspace mappings
> > + * will be overridden next time dmap is used -- same logic is applied in
> > + * fuse_dax_reclaim_first_mapping().
>
> This is true that sending FUSE_REMOVEMAPPING is not strictly needed. But,
> thinking more about it, what if range does not get used soon. Then it
> will keep resources unnecessarily busy on host? (fd, file, inode, dentry,
> vma etc).
>
> I think it probably is a good idea to send FUSE_REMOVEMAPPING to daemon
> when range is being freed.
We are under inode->i_rwsem and i_mmap_sem here. It can be very
expensive if we wait for userspace holding locks. How about pushing it
to a work queue? Still we at least need to grab the dmap lock but it
seems less expensive and doesn't block truncate.

Cheers,
Tao

> Only case where we are reclaiming a range
> and reusing immediately, we can skip sending FUSE_REMOVEMAPPING.
>
> This will especially be more meaniningful if dax window is large and
> has potential to create lots of mappings.
>
>
> Thanks
> Vivek
>
> > + */
> > +void fuse_dax_free_mappings_range(struct fuse_conn *fc, struct inode *inode, loff_t start, loff_t end)
> > +{
> > +     struct fuse_inode *fi = get_fuse_inode(inode);
> > +     struct fuse_dax_mapping *dmap;
> > +
> > +     WARN_ON(!inode_is_locked(inode));
> > +     WARN_ON(!rwsem_is_locked(&fi->i_mmap_sem));
> > +
> > +     /* interval tree search matches intersecting entries.
> > +      * Adjust the range to avoid dropping partial valid entries. */
> > +     start = ALIGN(start, FUSE_DAX_MEM_RANGE_SZ);
> > +     end = ALIGN_DOWN(end, FUSE_DAX_MEM_RANGE_SZ);
> > +
> > +     pr_debug("fuse: fuse_dax_free_mappings_range start=0x%llx, end=0x%llx\n", start, end);
> > +     /* Lock ordering follows fuse_dax_free_one_mapping() */
> > +     down_write(&fi->i_dmap_sem);
> > +     while ((dmap = fuse_dax_interval_tree_iter_first(&fi->dmap_tree, start, end))) {
> > +             fuse_dax_do_remove_mapping_locked(fi, dmap);
> > +             fuse_dax_do_free_mapping_locked(fc, dmap);
> > +     }
> > +     up_write(&fi->i_dmap_sem);
> > +}
> > +
> >  int fuse_dax_free_one_mapping_locked(struct fuse_conn *fc, struct inode *inode,
> >                               u64 dmap_start)
> >  {
> > @@ -3852,17 +3904,8 @@ int fuse_dax_free_one_mapping_locked(struct fuse_conn *fc, struct inode *inode,
> >       if (ret < 0)
> >               return ret;
> >
> > -     /* Cleanup dmap entry and add back to free list */
> > -     spin_lock(&fc->lock);
> > -     __dmap_remove_busy_list(fc, dmap);
> > -     dmap->inode = NULL;
> > -     dmap->start = dmap->end = 0;
> > -     __free_dax_mapping(fc, dmap);
> > -     spin_unlock(&fc->lock);
> > +     fuse_dax_do_free_mapping_locked(fc, dmap);
> >
> > -     pr_debug("fuse: freed memory range window_offset=0x%llx,"
> > -                             " length=0x%llx\n", dmap->window_offset,
> > -                             dmap->length);
> >       return ret;
> >  }
> >
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 1149281ab1e8..e273f1209f5b 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -1189,5 +1189,7 @@ unsigned fuse_len_args(unsigned numargs, struct fuse_arg *args);
> >  u64 fuse_get_unique(struct fuse_iqueue *fiq);
> >  void fuse_dax_free_mem_worker(struct work_struct *work);
> >  void fuse_removemapping(struct inode *inode);
> > +void fuse_dax_free_mappings_range(struct fuse_conn *fc, struct inode *inode,
> > +                     loff_t start, loff_t end);
> >
> >  #endif /* _FS_FUSE_I_H */
> > --
> > 2.17.1
> >
>
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs



-- 
bergwolf@hyper.sh

Re: [Virtio-fs] [PATCH 2/2] fuse: remove dmap when truncating inode

[Vivek Goyal]

On Sat, May 11, 2019 at 12:16:23AM +0800, Tao Peng wrote:
> On Fri, May 10, 2019 at 11:23 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, May 10, 2019 at 06:29:57PM +0800, Peng Tao wrote:
> > > The obseleted dmap entries can be put back to global free list
> > > immediately and we don't have to rely on reclaim code to free them.
> > >
> > > Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> > > ---
> > >  fs/fuse/dir.c    |  5 ++++
> > >  fs/fuse/file.c   | 69 +++++++++++++++++++++++++++++++++++++++---------
> > >  fs/fuse/fuse_i.h |  2 ++
> > >  3 files changed, 63 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index 3f923fe7841a..233c3ed391f1 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -1751,6 +1751,11 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
> > >               down_write(&fi->i_mmap_sem);
> > >               truncate_pagecache(inode, outarg.attr.size);
> > >               invalidate_inode_pages2(inode->i_mapping);
> > > +             // Free dmap beyond i_size
> > > +             if (IS_DAX(inode) && oldsize > outarg.attr.size)
> > > +                     fuse_dax_free_mappings_range(fc, inode,
> > > +                                                  outarg.attr.size, -1);
> > > +
> > >               up_write(&fi->i_mmap_sem);
> > >       }
> > >
> > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > index 51faed351c7c..6d130f2f3a23 100644
> > > --- a/fs/fuse/file.c
> > > +++ b/fs/fuse/file.c
> > > @@ -30,6 +30,8 @@ static long __fuse_file_fallocate(struct file *file, int mode,
> > >                                       loff_t offset, loff_t length);
> > >  static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
> > >                                       struct inode *inode);
> > > +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi,
> > > +                                     struct fuse_dax_mapping *dmap);
> > >
> > >  static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
> > >                         int opcode, struct fuse_open_out *outargp)
> > > @@ -3752,9 +3754,7 @@ int fuse_dax_reclaim_dmap_locked(struct fuse_conn *fc, struct inode *inode,
> > >               return ret;
> > >       }
> > >
> > > -     /* Remove dax mapping from inode interval tree now */
> > > -     fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> > > -     fi->nr_dmaps--;
> > > +     fuse_dax_do_remove_mapping_locked(fi, dmap);
> > >       return 0;
> > >  }
> > >
> > > @@ -3831,6 +3831,58 @@ static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
> > >       }
> > >  }
> > >
> > > +/* Cleanup dmap entry and add back to free list */
> > > +static void fuse_dax_do_free_mapping_locked(struct fuse_conn *fc, struct fuse_dax_mapping *dmap)
> > > +{
> > > +     pr_debug("fuse: freed memory range start=0x%llx end=0x%llx "
> > > +             "window_offset=0x%llx length=0x%llx\n", dmap->start,
> > > +             dmap->end, dmap->window_offset, dmap->length);
> > > +     spin_lock(&fc->lock);
> > > +     __dmap_remove_busy_list(fc, dmap);
> > > +     dmap->inode = NULL;
> > > +     dmap->start = dmap->end = 0;
> > > +     __free_dax_mapping(fc, dmap);
> > > +     spin_unlock(&fc->lock);
> > > +}
> > > +
> > > +/* Remove dax mapping from inode interval tree */
> > > +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi, struct fuse_dax_mapping *dmap)
> > > +{
> > > +     fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> > > +     fi->nr_dmaps--;
> > > +}
> >
> > this is just a two line funciton. I think lets open code it and we don't
> > need a separate function for this.
> ok.
> 
> >
> >
> > > +
> > > +/*
> > > + * Free inode dmap entries whose range falls entirely inside [start, end].
> > > + * Called with inode->i_rwsem and fuse_inode->i_mmap_sem held.
> > > + *
> > > + * No need to send FUSE_REMOVEMAPPING to userspace because the userspace mappings
> > > + * will be overridden next time dmap is used -- same logic is applied in
> > > + * fuse_dax_reclaim_first_mapping().
> >
> > This is true that sending FUSE_REMOVEMAPPING is not strictly needed. But,
> > thinking more about it, what if range does not get used soon. Then it
> > will keep resources unnecessarily busy on host? (fd, file, inode, dentry,
> > vma etc).
> >
> > I think it probably is a good idea to send FUSE_REMOVEMAPPING to daemon
> > when range is being freed.
> We are under inode->i_rwsem and i_mmap_sem here. It can be very
> expensive if we wait for userspace holding locks. How about pushing it
> to a work queue? Still we at least need to grab the dmap lock but it
> seems less expensive and doesn't block truncate.

How about freeing multiple ranges in single removemapping call. I think
that's allowed. We can look into making it async also at some point of
time, but right now it probably is best to keep code as simple as
possible. Do typical workloads do truncate at high frequencey?

Vivek

Re: [Virtio-fs] [PATCH 2/2] fuse: remove dmap when truncating inode

[Tao Peng]

On Sat, May 11, 2019 at 1:31 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Sat, May 11, 2019 at 12:16:23AM +0800, Tao Peng wrote:
> > On Fri, May 10, 2019 at 11:23 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Fri, May 10, 2019 at 06:29:57PM +0800, Peng Tao wrote:
> > > > The obseleted dmap entries can be put back to global free list
> > > > immediately and we don't have to rely on reclaim code to free them.
> > > >
> > > > Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
> > > > ---
> > > >  fs/fuse/dir.c    |  5 ++++
> > > >  fs/fuse/file.c   | 69 +++++++++++++++++++++++++++++++++++++++---------
> > > >  fs/fuse/fuse_i.h |  2 ++
> > > >  3 files changed, 63 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > > index 3f923fe7841a..233c3ed391f1 100644
> > > > --- a/fs/fuse/dir.c
> > > > +++ b/fs/fuse/dir.c
> > > > @@ -1751,6 +1751,11 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
> > > >               down_write(&fi->i_mmap_sem);
> > > >               truncate_pagecache(inode, outarg.attr.size);
> > > >               invalidate_inode_pages2(inode->i_mapping);
> > > > +             // Free dmap beyond i_size
> > > > +             if (IS_DAX(inode) && oldsize > outarg.attr.size)
> > > > +                     fuse_dax_free_mappings_range(fc, inode,
> > > > +                                                  outarg.attr.size, -1);
> > > > +
> > > >               up_write(&fi->i_mmap_sem);
> > > >       }
> > > >
> > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > > index 51faed351c7c..6d130f2f3a23 100644
> > > > --- a/fs/fuse/file.c
> > > > +++ b/fs/fuse/file.c
> > > > @@ -30,6 +30,8 @@ static long __fuse_file_fallocate(struct file *file, int mode,
> > > >                                       loff_t offset, loff_t length);
> > > >  static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
> > > >                                       struct inode *inode);
> > > > +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi,
> > > > +                                     struct fuse_dax_mapping *dmap);
> > > >
> > > >  static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
> > > >                         int opcode, struct fuse_open_out *outargp)
> > > > @@ -3752,9 +3754,7 @@ int fuse_dax_reclaim_dmap_locked(struct fuse_conn *fc, struct inode *inode,
> > > >               return ret;
> > > >       }
> > > >
> > > > -     /* Remove dax mapping from inode interval tree now */
> > > > -     fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> > > > -     fi->nr_dmaps--;
> > > > +     fuse_dax_do_remove_mapping_locked(fi, dmap);
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -3831,6 +3831,58 @@ static struct fuse_dax_mapping *alloc_dax_mapping_reclaim(struct fuse_conn *fc,
> > > >       }
> > > >  }
> > > >
> > > > +/* Cleanup dmap entry and add back to free list */
> > > > +static void fuse_dax_do_free_mapping_locked(struct fuse_conn *fc, struct fuse_dax_mapping *dmap)
> > > > +{
> > > > +     pr_debug("fuse: freed memory range start=0x%llx end=0x%llx "
> > > > +             "window_offset=0x%llx length=0x%llx\n", dmap->start,
> > > > +             dmap->end, dmap->window_offset, dmap->length);
> > > > +     spin_lock(&fc->lock);
> > > > +     __dmap_remove_busy_list(fc, dmap);
> > > > +     dmap->inode = NULL;
> > > > +     dmap->start = dmap->end = 0;
> > > > +     __free_dax_mapping(fc, dmap);
> > > > +     spin_unlock(&fc->lock);
> > > > +}
> > > > +
> > > > +/* Remove dax mapping from inode interval tree */
> > > > +static void fuse_dax_do_remove_mapping_locked(struct fuse_inode *fi, struct fuse_dax_mapping *dmap)
> > > > +{
> > > > +     fuse_dax_interval_tree_remove(dmap, &fi->dmap_tree);
> > > > +     fi->nr_dmaps--;
> > > > +}
> > >
> > > this is just a two line funciton. I think lets open code it and we don't
> > > need a separate function for this.
> > ok.
> >
> > >
> > >
> > > > +
> > > > +/*
> > > > + * Free inode dmap entries whose range falls entirely inside [start, end].
> > > > + * Called with inode->i_rwsem and fuse_inode->i_mmap_sem held.
> > > > + *
> > > > + * No need to send FUSE_REMOVEMAPPING to userspace because the userspace mappings
> > > > + * will be overridden next time dmap is used -- same logic is applied in
> > > > + * fuse_dax_reclaim_first_mapping().
> > >
> > > This is true that sending FUSE_REMOVEMAPPING is not strictly needed. But,
> > > thinking more about it, what if range does not get used soon. Then it
> > > will keep resources unnecessarily busy on host? (fd, file, inode, dentry,
> > > vma etc).
> > >
> > > I think it probably is a good idea to send FUSE_REMOVEMAPPING to daemon
> > > when range is being freed.
> > We are under inode->i_rwsem and i_mmap_sem here. It can be very
> > expensive if we wait for userspace holding locks. How about pushing it
> > to a work queue? Still we at least need to grab the dmap lock but it
> > seems less expensive and doesn't block truncate.
>
> How about freeing multiple ranges in single removemapping call. I think
> that's allowed. We can look into making it async also at some point of
> time, but right now it probably is best to keep code as simple as
> possible. Do typical workloads do truncate at high frequencey?
>
Good point! I'll change to do it. Thanks!

Cheers,
Tao
-- 
bergwolf@hyper.sh