[Virtio-fs] [PATCH] fuse: implement FUSE_INIT map_alignment field

[Virtio-fs] [PATCH] fuse: implement FUSE_INIT map_alignment field

[Stefan Hajnoczi]

The device communicates FUSE_SETUPMAPPING/FUSE_REMOVMAPPING alignment
constraints via the FUST_INIT map_alignment field.  Parse this field and
ensure our DAX mappings meet the alignment constraints.

We don't actually align anything differently since our mappings are
already 2MB aligned.  Just check the value when the connection is
established.  If it becomes necessary to honor arbitrary alignments in
the future we'll have to adjust how mappings are sized.

The upshot of this commit is that we can be confident that mappings will
work even when emulating x86 on Power and similar combinations where the
host page sizes are different.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Please see separate virtiofsd patch for the server-side implementation.

 fs/fuse/fuse_i.h          |  5 ++++-
 include/uapi/linux/fuse.h |  7 ++++++-
 fs/fuse/inode.c           | 18 ++++++++++++++++--
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index edcc4a3e119b..988ea5c39d41 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -46,7 +46,10 @@
 /** Number of page pointers embedded in fuse_req */
 #define FUSE_REQ_INLINE_PAGES 1
 
-/* Default memory range size, 2MB */
+/*
+ * Default memory range size.  A power of 2 so it agrees with common FUSE_INIT
+ * map_alignment values 4KB and 64KB.
+ */
 #define FUSE_DAX_MEM_RANGE_SZ	(2*1024*1024)
 #define FUSE_DAX_MEM_RANGE_PAGES	(FUSE_DAX_MEM_RANGE_SZ/PAGE_SIZE)
 
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 0f4b69b9c751..41d384b189b6 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -263,6 +263,9 @@ struct fuse_file_lock {
  * FUSE_MAX_PAGES: init_out.max_pages contains the max number of req pages
  * FUSE_CACHE_SYMLINKS: cache READLINK responses
  * FUSE_NO_OPENDIR_SUPPORT: kernel supports zero-message opendir
+ * FUSE_MAP_ALIGNMENT: init_out.map_alignment contains byte alignment for
+ *                     foffset and moffset fields in struct
+ *                     fuse_setupmapping_out and fuse_removemapping_one.
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -289,6 +292,7 @@ struct fuse_file_lock {
 #define FUSE_MAX_PAGES		(1 << 22)
 #define FUSE_CACHE_SYMLINKS	(1 << 23)
 #define FUSE_NO_OPENDIR_SUPPORT (1 << 24)
+#define FUSE_MAP_ALIGNMENT      (1 << 25)
 
 /**
  * CUSE INIT request/reply flags
@@ -632,7 +636,8 @@ struct fuse_init_out {
 	uint32_t	time_gran;
 	uint16_t	max_pages;
 	uint16_t	padding;
-	uint32_t	unused[8];
+	uint32_t	map_alignment;
+	uint32_t	unused[7];
 };
 
 #define CUSE_INIT_INFO_MAX 4096
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index ffa00caeea01..ecb47c65f514 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -969,9 +969,10 @@ static void process_init_limits(struct fuse_conn *fc, struct fuse_init_out *arg)
 static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 {
 	struct fuse_init_out *arg = &req->misc.init_out;
+	bool ok = true;
 
 	if (req->out.h.error || arg->major != FUSE_KERNEL_VERSION)
-		fc->conn_error = 1;
+		ok = false;
 	else {
 		unsigned long ra_pages;
 
@@ -1032,6 +1033,13 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 					min_t(unsigned int, FUSE_MAX_MAX_PAGES,
 					max_t(unsigned int, arg->max_pages, 1));
 			}
+			if ((arg->flags & FUSE_MAP_ALIGNMENT) &&
+			    (FUSE_DAX_MEM_RANGE_SZ % arg->map_alignment)) {
+				printk(KERN_ERR "FUSE: map_alignment %u incompatible with dax mem range size %u\n",
+				       arg->map_alignment,
+				       FUSE_DAX_MEM_RANGE_SZ);
+				ok = false;
+			}
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1045,6 +1053,12 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 		fc->max_write = max_t(unsigned, 4096, fc->max_write);
 		fc->conn_init = 1;
 	}
+
+	if (!ok) {
+		fc->conn_init = 0;
+		fc->conn_error = 1;
+	}
+
 	fuse_set_initialized(fc);
 	wake_up_all(&fc->blocked_waitq);
 }
@@ -1064,7 +1078,7 @@ void fuse_send_init(struct fuse_conn *fc, struct fuse_req *req)
 		FUSE_WRITEBACK_CACHE | FUSE_NO_OPEN_SUPPORT |
 		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
 		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
-		FUSE_NO_OPENDIR_SUPPORT;
+		FUSE_NO_OPENDIR_SUPPORT | FUSE_MAP_ALIGNMENT;
 	req->in.h.opcode = FUSE_INIT;
 	req->in.numargs = 1;
 	req->in.args[0].size = sizeof(*arg);
-- 
2.21.0

Re: [Virtio-fs] [PATCH] fuse: implement FUSE_INIT map_alignment field

[Vivek Goyal]

On Fri, Jul 26, 2019 at 09:29:33AM +0100, Stefan Hajnoczi wrote:
> The device communicates FUSE_SETUPMAPPING/FUSE_REMOVMAPPING alignment
> constraints via the FUST_INIT map_alignment field.  Parse this field and
> ensure our DAX mappings meet the alignment constraints.

Hi Stefan,

To meet alignment constraints, we also need to make sure dax window
start/end address is 2MB aligned as well?

Thanks
Vivek

> 
> We don't actually align anything differently since our mappings are
> already 2MB aligned.  Just check the value when the connection is
> established.  If it becomes necessary to honor arbitrary alignments in
> the future we'll have to adjust how mappings are sized.
> 
> The upshot of this commit is that we can be confident that mappings will
> work even when emulating x86 on Power and similar combinations where the
> host page sizes are different.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Please see separate virtiofsd patch for the server-side implementation.
> 
>  fs/fuse/fuse_i.h          |  5 ++++-
>  include/uapi/linux/fuse.h |  7 ++++++-
>  fs/fuse/inode.c           | 18 ++++++++++++++++--
>  3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index edcc4a3e119b..988ea5c39d41 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -46,7 +46,10 @@
>  /** Number of page pointers embedded in fuse_req */
>  #define FUSE_REQ_INLINE_PAGES 1
>  
> -/* Default memory range size, 2MB */
> +/*
> + * Default memory range size.  A power of 2 so it agrees with common FUSE_INIT
> + * map_alignment values 4KB and 64KB.
> + */
>  #define FUSE_DAX_MEM_RANGE_SZ	(2*1024*1024)
>  #define FUSE_DAX_MEM_RANGE_PAGES	(FUSE_DAX_MEM_RANGE_SZ/PAGE_SIZE)
>  
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 0f4b69b9c751..41d384b189b6 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -263,6 +263,9 @@ struct fuse_file_lock {
>   * FUSE_MAX_PAGES: init_out.max_pages contains the max number of req pages
>   * FUSE_CACHE_SYMLINKS: cache READLINK responses
>   * FUSE_NO_OPENDIR_SUPPORT: kernel supports zero-message opendir
> + * FUSE_MAP_ALIGNMENT: init_out.map_alignment contains byte alignment for
> + *                     foffset and moffset fields in struct
> + *                     fuse_setupmapping_out and fuse_removemapping_one.
>   */
>  #define FUSE_ASYNC_READ		(1 << 0)
>  #define FUSE_POSIX_LOCKS	(1 << 1)
> @@ -289,6 +292,7 @@ struct fuse_file_lock {
>  #define FUSE_MAX_PAGES		(1 << 22)
>  #define FUSE_CACHE_SYMLINKS	(1 << 23)
>  #define FUSE_NO_OPENDIR_SUPPORT (1 << 24)
> +#define FUSE_MAP_ALIGNMENT      (1 << 25)
>  
>  /**
>   * CUSE INIT request/reply flags
> @@ -632,7 +636,8 @@ struct fuse_init_out {
>  	uint32_t	time_gran;
>  	uint16_t	max_pages;
>  	uint16_t	padding;
> -	uint32_t	unused[8];
> +	uint32_t	map_alignment;
> +	uint32_t	unused[7];
>  };
>  
>  #define CUSE_INIT_INFO_MAX 4096
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index ffa00caeea01..ecb47c65f514 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -969,9 +969,10 @@ static void process_init_limits(struct fuse_conn *fc, struct fuse_init_out *arg)
>  static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
>  {
>  	struct fuse_init_out *arg = &req->misc.init_out;
> +	bool ok = true;
>  
>  	if (req->out.h.error || arg->major != FUSE_KERNEL_VERSION)
> -		fc->conn_error = 1;
> +		ok = false;
>  	else {
>  		unsigned long ra_pages;
>  
> @@ -1032,6 +1033,13 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
>  					min_t(unsigned int, FUSE_MAX_MAX_PAGES,
>  					max_t(unsigned int, arg->max_pages, 1));
>  			}
> +			if ((arg->flags & FUSE_MAP_ALIGNMENT) &&
> +			    (FUSE_DAX_MEM_RANGE_SZ % arg->map_alignment)) {
> +				printk(KERN_ERR "FUSE: map_alignment %u incompatible with dax mem range size %u\n",
> +				       arg->map_alignment,
> +				       FUSE_DAX_MEM_RANGE_SZ);
> +				ok = false;
> +			}
>  		} else {
>  			ra_pages = fc->max_read / PAGE_SIZE;
>  			fc->no_lock = 1;
> @@ -1045,6 +1053,12 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
>  		fc->max_write = max_t(unsigned, 4096, fc->max_write);
>  		fc->conn_init = 1;
>  	}
> +
> +	if (!ok) {
> +		fc->conn_init = 0;
> +		fc->conn_error = 1;
> +	}
> +
>  	fuse_set_initialized(fc);
>  	wake_up_all(&fc->blocked_waitq);
>  }
> @@ -1064,7 +1078,7 @@ void fuse_send_init(struct fuse_conn *fc, struct fuse_req *req)
>  		FUSE_WRITEBACK_CACHE | FUSE_NO_OPEN_SUPPORT |
>  		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
>  		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> -		FUSE_NO_OPENDIR_SUPPORT;
> +		FUSE_NO_OPENDIR_SUPPORT | FUSE_MAP_ALIGNMENT;
>  	req->in.h.opcode = FUSE_INIT;
>  	req->in.numargs = 1;
>  	req->in.args[0].size = sizeof(*arg);
> -- 
> 2.21.0
> 

Re: [Virtio-fs] [PATCH] fuse: implement FUSE_INIT map_alignment field

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On Fri, Jul 26, 2019 at 04:39:41PM -0400, Vivek Goyal wrote:
> On Fri, Jul 26, 2019 at 09:29:33AM +0100, Stefan Hajnoczi wrote:
> > The device communicates FUSE_SETUPMAPPING/FUSE_REMOVMAPPING alignment
> > constraints via the FUST_INIT map_alignment field.  Parse this field and
> > ensure our DAX mappings meet the alignment constraints.
> 
> Hi Stefan,
> 
> To meet alignment constraints, we also need to make sure dax window
> start/end address is 2MB aligned as well?

No, because the mmap on the host is independent of the guest virtual
address at which the BAR is mapped inside the guest.

QEMU hw/virtio/vhost-user-fs.c:vuf_device_realize() mmaps the DAX window
on the host side and that's where the mmap alignment requirement is in
effect.

The guest can map the DAX window at any address compatible with its own
virtual memory constraints (i.e. page size).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH] fuse: implement FUSE_INIT map_alignment field

[Vivek Goyal]

On Mon, Jul 29, 2019 at 09:21:49AM +0100, Stefan Hajnoczi wrote:
> On Fri, Jul 26, 2019 at 04:39:41PM -0400, Vivek Goyal wrote:
> > On Fri, Jul 26, 2019 at 09:29:33AM +0100, Stefan Hajnoczi wrote:
> > > The device communicates FUSE_SETUPMAPPING/FUSE_REMOVMAPPING alignment
> > > constraints via the FUST_INIT map_alignment field.  Parse this field and
> > > ensure our DAX mappings meet the alignment constraints.
> > 
> > Hi Stefan,
> > 
> > To meet alignment constraints, we also need to make sure dax window
> > start/end address is 2MB aligned as well?
> 
> No, because the mmap on the host is independent of the guest virtual
> address at which the BAR is mapped inside the guest.
> 
> QEMU hw/virtio/vhost-user-fs.c:vuf_device_realize() mmaps the DAX window
> on the host side and that's where the mmap alignment requirement is in
> effect.

Ok.

What about checking alignment constratins on inarg.moffset and
inarg.foffset in fuse_setup_one_mapping().

Thanks
Vivek

> 
> The guest can map the DAX window at any address compatible with its own
> virtual memory constraints (i.e. page size).
> 
> Stefan

Re: [Virtio-fs] [PATCH] fuse: implement FUSE_INIT map_alignment field

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Mon, Jul 29, 2019 at 08:48:16AM -0400, Vivek Goyal wrote:
> On Mon, Jul 29, 2019 at 09:21:49AM +0100, Stefan Hajnoczi wrote:
> > On Fri, Jul 26, 2019 at 04:39:41PM -0400, Vivek Goyal wrote:
> > > On Fri, Jul 26, 2019 at 09:29:33AM +0100, Stefan Hajnoczi wrote:
> > > > The device communicates FUSE_SETUPMAPPING/FUSE_REMOVMAPPING alignment
> > > > constraints via the FUST_INIT map_alignment field.  Parse this field and
> > > > ensure our DAX mappings meet the alignment constraints.
> > > 
> > > Hi Stefan,
> > > 
> > > To meet alignment constraints, we also need to make sure dax window
> > > start/end address is 2MB aligned as well?
> > 
> > No, because the mmap on the host is independent of the guest virtual
> > address at which the BAR is mapped inside the guest.
> > 
> > QEMU hw/virtio/vhost-user-fs.c:vuf_device_realize() mmaps the DAX window
> > on the host side and that's where the mmap alignment requirement is in
> > effect.
> 
> Ok.
> 
> What about checking alignment constratins on inarg.moffset and
> inarg.foffset in fuse_setup_one_mapping().

foffset is already implicitly checked:

WARN_ON(offset % FUSE_DAX_MEM_RANGE_SZ);

And moffset is a multiple of FUSE_DAX_MEM_RANGE_SZ.

FUSE_DAX_MEM_RANGE_SZ was already checked against map_alignment when the
FUSE_INIT response was processed, so we're good here.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]