[PATCH v7 0/7] fuse,virtiofs: support per-file DAX

[PATCH v7 0/7] fuse,virtiofs: support per-file DAX

[Jeffle Xu]

changes since v6:
- rename the feature bit of FUSE_INIT to 'FUSE_HAS_INODE_DAX'
- rename 'per-file DAX' to 'per inode DAX'

Some user visible behavior elaboration:
1. The default behavior equals to 'dax=inode' when neither '-o dax' nor
'-o dax=always|never|inode' is specified.
2. It will silently fallback to 'dax=never' if server doesn't support
per inode DAX when it's in 'dax=inode' mode, no matter whether it's
mounted in 'dax=inode' mode explicitly or by default. Thus virtiofs can
be consistnet with ext4/xfs, since 'dax=inode' is advisory only.
Server may not support per inode DAX due to the following reasons:
  - the virtiofs device doesn't support DAX at all
    (VIRTIO_FS_SHMCAP_ID_CACHE not defined at all)
  - server's map alignment is non-compliant (fail
    fuse_dax_check_alignment())
  - server doesn't advertise support for per inode DAX
    (FUSE_HAS_INODE_DAX) during FUSE_INIT
3. As said before, the default behavior equals to 'dax=inode' when no
DAX related mount option specified, however it won't be printed among
the mount option. (consistnet with ext4/xfs).
4. As said before, it may fallback to 'dax=never' when it's in
'dax=inode' mode, however the mount option will always be printed as
'dax=inode'. (same as ext4, though xfs indeed print 'dax=never' if
it fallback to 'dax=never')
5. By default per inode DAX won't be enabled on the virtiofsd side, and
thus guest virtiofs will see that all files are DAX disabled though it
works in 'dax=inode' mode by default. That is, the default behavior won't
surprise users. If users indeed want to use per inode DAX feature, then
they need to enable this feature *explicitly* on the virtiofsd side.

v6: https://lore.kernel.org/all/20211011030052.98923-1-jefflexu@linux.alibaba.com/
v5: https://lore.kernel.org/all/20210923092526.72341-1-jefflexu@linux.alibaba.com/
v4: https://lore.kernel.org/linux-fsdevel/20210817022220.17574-1-jefflexu@linux.alibaba.com/
v3: https://www.spinics.net/lists/linux-fsdevel/msg200852.html
v2: https://www.spinics.net/lists/linux-fsdevel/msg199584.html
v1: https://www.spinics.net/lists/linux-virtualization/msg51008.html


Original Rationale for this Patchset
====================================

This patchset adds support of per-file DAX for virtiofs, which is
inspired by Ira Weiny's work on ext4[1] and xfs[2].

Any comment is welcome.

[1] commit 9cb20f94afcd ("fs/ext4: Make DAX mount option a tri-state")
[2] commit 02beb2686ff9 ("fs/xfs: Make DAX mount option a tri-state")

[Purpose]
DAX may be limited in some specific situation. When the number of usable
DAX windows is under watermark, the recalim routine will be triggered to
reclaim some DAX windows. It may have a negative impact on the
performance, since some processes may need to wait for DAX windows to be
recalimed and reused then. To mitigate the performance degradation, the
overall DAX window need to be expanded larger.

However, simply expanding the DAX window may not be a good deal in some
scenario. To maintain one DAX window chunk (i.e., 2MB in size), 32KB
(512 * 64 bytes) memory footprint will be consumed for page descriptors
inside guest, which is greater than the memory footprint if it uses
guest page cache when DAX disabled. Thus it'd better disable DAX for
those files smaller than 32KB, to reduce the demand for DAX window and
thus avoid the unworthy memory overhead.

Per-file DAX feature is introduced to address this issue, by offering a
finer grained control for dax to users, trying to achieve a balance
between performance and memory overhead.


[Note]
When the per-file DAX hint changes while the file is still *opened*, it
is quite complicated and maybe fragile to dynamically change the DAX
state, since dynamic switching needs to switch a_ops atomiclly. Ira
Weiny had ever implemented a so called i_aops_sem lock [3] but
eventually gave up since the complexity of the implementation
[4][5][6][7].

Hence mark the inode and corresponding dentries as DONE_CACHE once the
per-file DAX hint changes, so that the inode instance will be evicted
and freed as soon as possible once the file is closed and the last
reference to the inode is put. And then when the file gets reopened next
time, the new instantiated inode will reflect the new DAX state.

In summary, when the per-file DAX hint changes for an *opened* file, the
DAX state of the file won't be updated until this file is closed and
reopened later. This is also how ext4/xfs per-file DAX works.

[3] https://lore.kernel.org/lkml/20200227052442.22524-7-ira.weiny@intel.com/
[4] https://patchwork.kernel.org/project/xfs/cover/20200407182958.568475-1-ira.weiny@intel.com/
[5] https://lore.kernel.org/lkml/20200305155144.GA5598@lst.de/
[6] https://lore.kernel.org/lkml/20200401040021.GC56958@magnolia/
[7] https://lore.kernel.org/lkml/20200403182904.GP80283@magnolia/

Jeffle Xu (7):
  fuse: add fuse_should_enable_dax() helper
  fuse: make DAX mount option a tri-state
  fuse: support per inode DAX in fuse protocol
  fuse: enable per inode DAX
  fuse: negotiate per inode DAX in FUSE_INIT
  fuse: mark inode DONT_CACHE when per inode DAX hint changes
  Documentation/filesystem/dax: record DAX on virtiofs

 Documentation/filesystems/dax.rst | 20 +++++++++++++++++--
 fs/fuse/dax.c                     | 32 +++++++++++++++++++++++++++++--
 fs/fuse/file.c                    |  4 ++--
 fs/fuse/fuse_i.h                  | 28 +++++++++++++++++++++++----
 fs/fuse/inode.c                   | 31 +++++++++++++++++++++++-------
 fs/fuse/virtio_fs.c               | 18 ++++++++++++++---
 include/uapi/linux/fuse.h         |  9 ++++++++-
 7 files changed, 121 insertions(+), 21 deletions(-)

-- 
2.27.0

[PATCH v7 1/7] fuse: add fuse_should_enable_dax() helper

[Jeffle Xu]

This is in prep for following per inode DAX checking.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/dax.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 79df61ed7481..8c187b04874e 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1332,11 +1332,19 @@ static const struct address_space_operations fuse_dax_file_aops  = {
 	.invalidatepage	= noop_invalidatepage,
 };
 
-void fuse_dax_inode_init(struct inode *inode)
+static bool fuse_should_enable_dax(struct inode *inode)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
 
 	if (!fc->dax)
+		return false;
+
+	return true;
+}
+
+void fuse_dax_inode_init(struct inode *inode)
+{
+	if (!fuse_should_enable_dax(inode))
 		return;
 
 	inode->i_flags |= S_DAX;
-- 
2.27.0

[PATCH v7 2/7] fuse: make DAX mount option a tri-state

[Jeffle Xu]

We add 'always', 'never', and 'inode' (default). '-o dax' continues to
operate the same which is equivalent to 'always'.

The following behavior is consistent with that on ext4/xfs:
- The default behavior (when neither '-o dax' nor
  '-o dax=always|never|inode' option is specified) is equal to 'inode'
  mode, while 'dax=inode' won't be printed among the mount option list.
- The 'inode' mode is only advisory. It will silently fallback to
  'never' mode if fuse server doesn't support that.

Also noted that by the time of this commit, 'inode' mode is actually
equal to 'always' mode, before the per inode DAX flag is introduced in
the following patch.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/dax.c       |  9 ++++++++-
 fs/fuse/fuse_i.h    | 20 ++++++++++++++++++--
 fs/fuse/inode.c     | 10 +++++++---
 fs/fuse/virtio_fs.c | 18 +++++++++++++++---
 4 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 8c187b04874e..91c8d146dbc4 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1284,11 +1284,14 @@ static int fuse_dax_mem_range_init(struct fuse_conn_dax *fcd)
 	return ret;
 }
 
-int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
+int fuse_dax_conn_alloc(struct fuse_conn *fc, enum fuse_dax_mode dax_mode,
+			struct dax_device *dax_dev)
 {
 	struct fuse_conn_dax *fcd;
 	int err;
 
+	fc->dax_mode = dax_mode;
+
 	if (!dax_dev)
 		return 0;
 
@@ -1335,6 +1338,10 @@ static const struct address_space_operations fuse_dax_file_aops  = {
 static bool fuse_should_enable_dax(struct inode *inode)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
+	enum fuse_dax_mode dax_mode = fc->dax_mode;
+
+	if (dax_mode == FUSE_DAX_NEVER)
+		return false;
 
 	if (!fc->dax)
 		return false;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f55f9f94b1a4..4f9c2358f343 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -480,6 +480,18 @@ struct fuse_dev {
 	struct list_head entry;
 };
 
+enum fuse_dax_mode {
+	FUSE_DAX_NONE,	 /* default */
+	FUSE_DAX_ALWAYS, /* "-o dax=always" */
+	FUSE_DAX_NEVER,  /* "-o dax=never" */
+	FUSE_DAX_INODE,  /* "-o dax=inode" */
+};
+
+static inline bool fuse_is_inode_dax_mode(enum fuse_dax_mode mode)
+{
+	return mode == FUSE_DAX_INODE || mode == FUSE_DAX_NONE;
+}
+
 struct fuse_fs_context {
 	int fd;
 	struct file *file;
@@ -497,7 +509,7 @@ struct fuse_fs_context {
 	bool no_control:1;
 	bool no_force_umount:1;
 	bool legacy_opts_show:1;
-	bool dax:1;
+	enum fuse_dax_mode dax_mode;
 	unsigned int max_read;
 	unsigned int blksize;
 	const char *subtype;
@@ -802,6 +814,9 @@ struct fuse_conn {
 	struct list_head devices;
 
 #ifdef CONFIG_FUSE_DAX
+	/* Dax mode */
+	enum fuse_dax_mode dax_mode;
+
 	/* Dax specific conn data, non-NULL if DAX is enabled */
 	struct fuse_conn_dax *dax;
 #endif
@@ -1258,7 +1273,8 @@ ssize_t fuse_dax_read_iter(struct kiocb *iocb, struct iov_iter *to);
 ssize_t fuse_dax_write_iter(struct kiocb *iocb, struct iov_iter *from);
 int fuse_dax_mmap(struct file *file, struct vm_area_struct *vma);
 int fuse_dax_break_layouts(struct inode *inode, u64 dmap_start, u64 dmap_end);
-int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev);
+int fuse_dax_conn_alloc(struct fuse_conn *fc, enum fuse_dax_mode mode,
+			struct dax_device *dax_dev);
 void fuse_dax_conn_free(struct fuse_conn *fc);
 bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi);
 void fuse_dax_inode_init(struct inode *inode);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 12d49a1914e8..15ce56f9cf11 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -734,8 +734,12 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root)
 			seq_printf(m, ",blksize=%lu", sb->s_blocksize);
 	}
 #ifdef CONFIG_FUSE_DAX
-	if (fc->dax)
-		seq_puts(m, ",dax");
+	if (fc->dax_mode == FUSE_DAX_ALWAYS)
+		seq_puts(m, ",dax=always");
+	else if (fc->dax_mode == FUSE_DAX_NEVER)
+		seq_puts(m, ",dax=never");
+	else if (fc->dax_mode == FUSE_DAX_INODE)
+		seq_puts(m, ",dax=inode");
 #endif
 
 	return 0;
@@ -1481,7 +1485,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	sb->s_subtype = ctx->subtype;
 	ctx->subtype = NULL;
 	if (IS_ENABLED(CONFIG_FUSE_DAX)) {
-		err = fuse_dax_conn_alloc(fc, ctx->dax_dev);
+		err = fuse_dax_conn_alloc(fc, ctx->dax_mode, ctx->dax_dev);
 		if (err)
 			goto err;
 	}
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 94fc874f5de7..e8c404946c63 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -88,12 +88,21 @@ struct virtio_fs_req_work {
 static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 				 struct fuse_req *req, bool in_flight);
 
+static const struct constant_table dax_param_enums[] = {
+	{"always",	FUSE_DAX_ALWAYS },
+	{"never",	FUSE_DAX_NEVER },
+	{"inode",	FUSE_DAX_INODE },
+	{}
+};
+
 enum {
 	OPT_DAX,
+	OPT_DAX_ENUM,
 };
 
 static const struct fs_parameter_spec virtio_fs_parameters[] = {
 	fsparam_flag("dax", OPT_DAX),
+	fsparam_enum("dax", OPT_DAX_ENUM, dax_param_enums),
 	{}
 };
 
@@ -110,7 +119,10 @@ static int virtio_fs_parse_param(struct fs_context *fsc,
 
 	switch (opt) {
 	case OPT_DAX:
-		ctx->dax = 1;
+		ctx->dax_mode = FUSE_DAX_ALWAYS;
+		break;
+	case OPT_DAX_ENUM:
+		ctx->dax_mode = result.uint_32;
 		break;
 	default:
 		return -EINVAL;
@@ -1326,8 +1338,8 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
 
 	/* virtiofs allocates and installs its own fuse devices */
 	ctx->fudptr = NULL;
-	if (ctx->dax) {
-		if (!fs->dax_dev) {
+	if (ctx->dax_mode != FUSE_DAX_NEVER) {
+		if (ctx->dax_mode == FUSE_DAX_ALWAYS && !fs->dax_dev) {
 			err = -EINVAL;
 			pr_err("virtio-fs: dax can't be enabled as filesystem"
 			       " device does not support it.\n");
-- 
2.27.0

[PATCH v7 3/7] fuse: support per inode DAX in fuse protocol

[Jeffle Xu]

Expand the fuse protocol to support per inode DAX.

FUSE_HAS_INODE_DAX flag is added indicating if fuse server/client
supporting per inode DAX. It can be conveyed in both FUSE_INIT request
and reply.

FUSE_ATTR_DAX flag is added indicating if DAX shall be enabled for
corresponding file. It is conveyed in FUSE_LOOKUP reply.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 include/uapi/linux/fuse.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 36ed092227fa..4ebe06091988 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -184,6 +184,9 @@
  *
  *  7.34
  *  - add FUSE_SYNCFS
+ *
+ *  7.35
+ *  - add FUSE_HAS_INODE_DAX, FUSE_ATTR_DAX
  */
 
 #ifndef _LINUX_FUSE_H
@@ -219,7 +222,7 @@
 #define FUSE_KERNEL_VERSION 7
 
 /** Minor version number of this interface */
-#define FUSE_KERNEL_MINOR_VERSION 34
+#define FUSE_KERNEL_MINOR_VERSION 35
 
 /** The node ID of the root inode */
 #define FUSE_ROOT_ID 1
@@ -336,6 +339,7 @@ struct fuse_file_lock {
  *			write/truncate sgid is killed only if file has group
  *			execute permission. (Same as Linux VFS behavior).
  * FUSE_SETXATTR_EXT:	Server supports extended struct fuse_setxattr_in
+ * FUSE_HAS_INODE_DAX:  use per inode DAX
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -367,6 +371,7 @@ struct fuse_file_lock {
 #define FUSE_SUBMOUNTS		(1 << 27)
 #define FUSE_HANDLE_KILLPRIV_V2	(1 << 28)
 #define FUSE_SETXATTR_EXT	(1 << 29)
+#define FUSE_HAS_INODE_DAX	(1 << 30)
 
 /**
  * CUSE INIT request/reply flags
@@ -449,8 +454,10 @@ struct fuse_file_lock {
  * fuse_attr flags
  *
  * FUSE_ATTR_SUBMOUNT: Object is a submount root
+ * FUSE_ATTR_DAX: Enable DAX for this file in per inode DAX mode
  */
 #define FUSE_ATTR_SUBMOUNT      (1 << 0)
+#define FUSE_ATTR_DAX		(1 << 1)
 
 /**
  * Open flags
-- 
2.27.0

[PATCH v7 4/7] fuse: enable per inode DAX

[Jeffle Xu]

DAX may be limited in some specific situation. When the number of usable
DAX windows is under watermark, the recalim routine will be triggered to
reclaim some DAX windows. It may have a negative impact on the
performance, since some processes may need to wait for DAX windows to be
recalimed and reused then. To mitigate the performance degradation, the
overall DAX window need to be expanded larger.

However, simply expanding the DAX window may not be a good deal in some
scenario. To maintain one DAX window chunk (i.e., 2MB in size), 32KB
(512 * 64 bytes) memory footprint will be consumed for page descriptors
inside guest, which is greater than the memory footprint if it uses
guest page cache when DAX disabled. Thus it'd better disable DAX for
those files smaller than 32KB, to reduce the demand for DAX window and
thus avoid the unworthy memory overhead.

Per inode DAX feature is introduced to address this issue, by offering a
finer grained control for dax to users, trying to achieve a balance
between performance and memory overhead.

The FUSE_ATTR_DAX flag in FUSE_LOOKUP reply is used to indicate whether
DAX should be enabled or not for corresponding file. Currently the state
whether DAX is enabled or not for the file is initialized only when
inode is instantiated.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/dax.c    | 12 ++++++++----
 fs/fuse/file.c   |  4 ++--
 fs/fuse/fuse_i.h |  4 ++--
 fs/fuse/inode.c  |  2 +-
 4 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 91c8d146dbc4..8a328fb20dcb 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1335,7 +1335,7 @@ static const struct address_space_operations fuse_dax_file_aops  = {
 	.invalidatepage	= noop_invalidatepage,
 };
 
-static bool fuse_should_enable_dax(struct inode *inode)
+static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	enum fuse_dax_mode dax_mode = fc->dax_mode;
@@ -1346,12 +1346,16 @@ static bool fuse_should_enable_dax(struct inode *inode)
 	if (!fc->dax)
 		return false;
 
-	return true;
+	if (dax_mode == FUSE_DAX_ALWAYS)
+		return true;
+
+	/* dax_mode is FUSE_DAX_INODE or FUSE_DAX_NONE */
+	return flags & FUSE_ATTR_DAX;
 }
 
-void fuse_dax_inode_init(struct inode *inode)
+void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
 {
-	if (!fuse_should_enable_dax(inode))
+	if (!fuse_should_enable_dax(inode, flags))
 		return;
 
 	inode->i_flags |= S_DAX;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index e6039f22311b..8449d6fca413 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -3163,7 +3163,7 @@ static const struct address_space_operations fuse_file_aops  = {
 	.write_end	= fuse_write_end,
 };
 
-void fuse_init_file_inode(struct inode *inode)
+void fuse_init_file_inode(struct inode *inode, unsigned int flags)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
 
@@ -3177,5 +3177,5 @@ void fuse_init_file_inode(struct inode *inode)
 	fi->writepages = RB_ROOT;
 
 	if (IS_ENABLED(CONFIG_FUSE_DAX))
-		fuse_dax_inode_init(inode);
+		fuse_dax_inode_init(inode, flags);
 }
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 4f9c2358f343..055b39430540 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1022,7 +1022,7 @@ int fuse_notify_poll_wakeup(struct fuse_conn *fc,
 /**
  * Initialize file operations on a regular file
  */
-void fuse_init_file_inode(struct inode *inode);
+void fuse_init_file_inode(struct inode *inode, unsigned int flags);
 
 /**
  * Initialize inode operations on regular files and special files
@@ -1277,7 +1277,7 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, enum fuse_dax_mode mode,
 			struct dax_device *dax_dev);
 void fuse_dax_conn_free(struct fuse_conn *fc);
 bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi);
-void fuse_dax_inode_init(struct inode *inode);
+void fuse_dax_inode_init(struct inode *inode, unsigned int flags);
 void fuse_dax_inode_cleanup(struct inode *inode);
 bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment);
 void fuse_dax_cancel_work(struct fuse_conn *fc);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 15ce56f9cf11..acba14002d04 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -280,7 +280,7 @@ static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
 	inode->i_ctime.tv_nsec = attr->ctimensec;
 	if (S_ISREG(inode->i_mode)) {
 		fuse_init_common(inode);
-		fuse_init_file_inode(inode);
+		fuse_init_file_inode(inode, attr->flags);
 	} else if (S_ISDIR(inode->i_mode))
 		fuse_init_dir(inode);
 	else if (S_ISLNK(inode->i_mode))
-- 
2.27.0

[PATCH v7 5/7] fuse: negotiate per inode DAX in FUSE_INIT

[Jeffle Xu]

Among the FUSE_INIT phase, client shall advertise per inode DAX if it's
mounted with "dax=inode". Then server is aware that client is in per
inode DAX mode, and will construct per-inode DAX attribute accordingly.

Server shall also advertise support for per inode DAX. If server doesn't
support it while client is mounted with "dax=inode", client will
silently fallback to "dax=never" since "dax=inode" is advisory only.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/dax.c    |  2 +-
 fs/fuse/fuse_i.h |  3 +++
 fs/fuse/inode.c  | 16 +++++++++++++---
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 8a328fb20dcb..c8ee601b94b8 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1350,7 +1350,7 @@ static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
 		return true;
 
 	/* dax_mode is FUSE_DAX_INODE or FUSE_DAX_NONE */
-	return flags & FUSE_ATTR_DAX;
+	return fc->inode_dax && (flags & FUSE_ATTR_DAX);
 }
 
 void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 055b39430540..58e54b5a4d65 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -777,6 +777,9 @@ struct fuse_conn {
 	/* Propagate syncfs() to server */
 	unsigned int sync_fs:1;
 
+	/* Does the filesystem support per inode DAX? */
+	unsigned int inode_dax:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index acba14002d04..0512d8cb36c3 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1136,11 +1136,19 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 					min_t(unsigned int, fc->max_pages_limit,
 					max_t(unsigned int, arg->max_pages, 1));
 			}
-			if (IS_ENABLED(CONFIG_FUSE_DAX) &&
-			    arg->flags & FUSE_MAP_ALIGNMENT &&
+#ifdef CONFIG_FUSE_DAX
+			if ((arg->flags & FUSE_HAS_INODE_DAX) &&
+			    fuse_is_inode_dax_mode(fc->dax_mode)) {
+				fc->inode_dax = 1;
+			}
+			if (arg->flags & FUSE_MAP_ALIGNMENT &&
 			    !fuse_dax_check_alignment(fc, arg->map_alignment)) {
-				ok = false;
+				if (fuse_is_inode_dax_mode(fc->dax_mode))
+					fc->inode_dax = 0;
+				else
+					ok = false;
 			}
+#endif
 			if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
 				fc->handle_killpriv_v2 = 1;
 				fm->sb->s_flags |= SB_NOSEC;
@@ -1194,6 +1202,8 @@ void fuse_send_init(struct fuse_mount *fm)
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		ia->in.flags |= FUSE_MAP_ALIGNMENT;
+	if (fuse_is_inode_dax_mode(fm->fc->dax_mode))
+		ia->in.flags |= FUSE_HAS_INODE_DAX;
 #endif
 	if (fm->fc->auto_submounts)
 		ia->in.flags |= FUSE_SUBMOUNTS;
-- 
2.27.0

[PATCH v7 6/7] fuse: mark inode DONT_CACHE when per inode DAX hint changes

[Jeffle Xu]

When the per inode DAX hint changes while the file is still *opened*, it
is quite complicated and maybe fragile to dynamically change the DAX
state.

Hence mark the inode and corresponding dentries as DONE_CACHE once the
per inode DAX hint changes, so that the inode instance will be evicted
and freed as soon as possible once the file is closed and the last
reference to the inode is put. And then when the file gets reopened next
time, the new instantiated inode will reflect the new DAX state.

In summary, when the per inode DAX hint changes for an *opened* file, the
DAX state of the file won't be updated until this file is closed and
reopened later.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/dax.c    | 9 +++++++++
 fs/fuse/fuse_i.h | 1 +
 fs/fuse/inode.c  | 3 +++
 3 files changed, 13 insertions(+)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index c8ee601b94b8..e8766420c708 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1362,6 +1362,15 @@ void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
 	inode->i_data.a_ops = &fuse_dax_file_aops;
 }
 
+void fuse_dax_dontcache(struct inode *inode, unsigned int flags)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+
+	if (fuse_is_inode_dax_mode(fc->dax_mode) &&
+	    (!!IS_DAX(inode) != !!(flags & FUSE_ATTR_DAX)))
+		d_mark_dontcache(inode);
+}
+
 bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment)
 {
 	if (fc->dax && (map_alignment > FUSE_DAX_SHIFT)) {
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 58e54b5a4d65..fe8364b522ac 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1282,6 +1282,7 @@ void fuse_dax_conn_free(struct fuse_conn *fc);
 bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi);
 void fuse_dax_inode_init(struct inode *inode, unsigned int flags);
 void fuse_dax_inode_cleanup(struct inode *inode);
+void fuse_dax_dontcache(struct inode *inode, unsigned int flags);
 bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment);
 void fuse_dax_cancel_work(struct fuse_conn *fc);
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 0512d8cb36c3..4a2a58cc71af 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -268,6 +268,9 @@ void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
 		if (inval)
 			invalidate_inode_pages2(inode->i_mapping);
 	}
+
+	if (IS_ENABLED(CONFIG_FUSE_DAX))
+		fuse_dax_dontcache(inode, attr->flags);
 }
 
 static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
-- 
2.27.0

[PATCH v7 7/7] Documentation/filesystem/dax: record DAX on virtiofs

[Jeffle Xu]

Record DAX on virtiofs and the semantic difference with that on ext4
and xfs.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 Documentation/filesystems/dax.rst | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/dax.rst b/Documentation/filesystems/dax.rst
index 9a1b8fd9e82b..e3b30429d703 100644
--- a/Documentation/filesystems/dax.rst
+++ b/Documentation/filesystems/dax.rst
@@ -23,8 +23,8 @@ on it as usual.  The `DAX` code currently only supports files with a block
 size equal to your kernel's `PAGE_SIZE`, so you may need to specify a block
 size when creating the filesystem.
 
-Currently 3 filesystems support `DAX`: ext2, ext4 and xfs.  Enabling `DAX` on them
-is different.
+Currently 4 filesystems support `DAX`: ext2, ext4, xfs and virtiofs.
+Enabling `DAX` on them is different.
 
 Enabling DAX on ext2
 --------------------
@@ -168,6 +168,22 @@ if the underlying media does not support dax and/or the filesystem is
 overridden with a mount option.
 
 
+Enabling DAX on virtiofs
+----------------------------
+The semantic of DAX on virtiofs is basically equal to that on ext4 and xfs,
+except that when '-o dax=inode' is specified, virtiofs client derives the hint
+whether DAX shall be enabled or not from virtiofs server through FUSE protocol,
+rather than the persistent `FS_XFLAG_DAX` flag. That is, whether DAX shall be
+enabled or not is completely determined by virtiofs server, while virtiofs
+server itself may deploy various algorithm making this decision, e.g. depending
+on the persistent `FS_XFLAG_DAX` flag on the host.
+
+It is still supported to set or clear persistent `FS_XFLAG_DAX` flag inside
+guest, but it is not guaranteed that DAX will be enabled or disabled for
+corresponding file then. Users inside guest still need to call statx(2) and
+check the statx flag `STATX_ATTR_DAX` to see if DAX is enabled for this file.
+
+
 Implementation Tips for Block Driver Writers
 --------------------------------------------
 
-- 
2.27.0

Re: [PATCH v7 6/7] fuse: mark inode DONT_CACHE when per inode DAX hint changes

[Miklos Szeredi]

On Tue, 2 Nov 2021 at 06:26, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>
> When the per inode DAX hint changes while the file is still *opened*, it
> is quite complicated and maybe fragile to dynamically change the DAX
> state.
>
> Hence mark the inode and corresponding dentries as DONE_CACHE once the
> per inode DAX hint changes, so that the inode instance will be evicted
> and freed as soon as possible once the file is closed and the last
> reference to the inode is put. And then when the file gets reopened next
> time, the new instantiated inode will reflect the new DAX state.
>
> In summary, when the per inode DAX hint changes for an *opened* file, the
> DAX state of the file won't be updated until this file is closed and
> reopened later.

This patch does nothing, since fuse already uses .drop_inode =
generic_delete_inode, which is has the same effect as setting
I_DONTCACHE, at least in the fuse case (inode should never be dirty at
eviction).   In fact it may be cleaner to set I_DONTCACHE
unconditionally and remove the .drop_inode callback setting.

Thanks,
Miklos

Re: [PATCH v7 6/7] fuse: mark inode DONT_CACHE when per inode DAX hint changes

[JeffleXu]

On 11/10/21 11:50 PM, Miklos Szeredi wrote:
> On Tue, 2 Nov 2021 at 06:26, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>>
>> When the per inode DAX hint changes while the file is still *opened*, it
>> is quite complicated and maybe fragile to dynamically change the DAX
>> state.
>>
>> Hence mark the inode and corresponding dentries as DONE_CACHE once the
>> per inode DAX hint changes, so that the inode instance will be evicted
>> and freed as soon as possible once the file is closed and the last
>> reference to the inode is put. And then when the file gets reopened next
>> time, the new instantiated inode will reflect the new DAX state.
>>
>> In summary, when the per inode DAX hint changes for an *opened* file, the
>> DAX state of the file won't be updated until this file is closed and
>> reopened later.
> 
> This patch does nothing, since fuse already uses .drop_inode =
> generic_delete_inode, which is has the same effect as setting
> I_DONTCACHE, at least in the fuse case (inode should never be dirty at
> eviction).  

Yes, it is. .drop_inode() of FUSE will always free inode. Here we only
need to set dentry as DCACHE_DONTCACHE. Here I just call
d_mark_dontcache() directly, though I_DONTCACHE is useless but harmless
in the case of FUSE...


> In fact it may be cleaner to set I_DONTCACHE
> unconditionally and remove the .drop_inode callback setting.

It works in both cases, I mean, in current case (current code retained
untouched) and the case you described above.

-- 
Thanks,
Jeffle

Re: [PATCH v7 2/7] fuse: make DAX mount option a tri-state

[Vivek Goyal]

On Tue, Nov 02, 2021 at 01:25:59PM +0800, Jeffle Xu wrote:
> We add 'always', 'never', and 'inode' (default). '-o dax' continues to
> operate the same which is equivalent to 'always'.
> 
> The following behavior is consistent with that on ext4/xfs:
> - The default behavior (when neither '-o dax' nor
>   '-o dax=always|never|inode' option is specified) is equal to 'inode'
>   mode, while 'dax=inode' won't be printed among the mount option list.
> - The 'inode' mode is only advisory. It will silently fallback to
>   'never' mode if fuse server doesn't support that.
> 
> Also noted that by the time of this commit, 'inode' mode is actually
> equal to 'always' mode, before the per inode DAX flag is introduced in
> the following patch.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/dax.c       |  9 ++++++++-
>  fs/fuse/fuse_i.h    | 20 ++++++++++++++++++--
>  fs/fuse/inode.c     | 10 +++++++---
>  fs/fuse/virtio_fs.c | 18 +++++++++++++++---
>  4 files changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index 8c187b04874e..91c8d146dbc4 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -1284,11 +1284,14 @@ static int fuse_dax_mem_range_init(struct fuse_conn_dax *fcd)
>  	return ret;
>  }
>  
> -int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
> +int fuse_dax_conn_alloc(struct fuse_conn *fc, enum fuse_dax_mode dax_mode,
> +			struct dax_device *dax_dev)
>  {
>  	struct fuse_conn_dax *fcd;
>  	int err;
>  
> +	fc->dax_mode = dax_mode;
> +
>  	if (!dax_dev)
>  		return 0;
>  
> @@ -1335,6 +1338,10 @@ static const struct address_space_operations fuse_dax_file_aops  = {
>  static bool fuse_should_enable_dax(struct inode *inode)
>  {
>  	struct fuse_conn *fc = get_fuse_conn(inode);
> +	enum fuse_dax_mode dax_mode = fc->dax_mode;
> +
> +	if (dax_mode == FUSE_DAX_NEVER)
> +		return false;
>  
>  	if (!fc->dax)
>  		return false;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index f55f9f94b1a4..4f9c2358f343 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -480,6 +480,18 @@ struct fuse_dev {
>  	struct list_head entry;
>  };
>  
> +enum fuse_dax_mode {
> +	FUSE_DAX_NONE,	 /* default */
> +	FUSE_DAX_ALWAYS, /* "-o dax=always" */
> +	FUSE_DAX_NEVER,  /* "-o dax=never" */
> +	FUSE_DAX_INODE,  /* "-o dax=inode" */
> +};
Hi,

Not sure why do we need FUSE_DAX_NONE. Now default is FUSE_DAX_INODE
and "-o dax" will map to FUSE_DAX_ALWAYS. So after this patch series,
nobody should be using FUSE_DAX_NONE state at all? So we should be
able to get rid of entirely?

> +
> +static inline bool fuse_is_inode_dax_mode(enum fuse_dax_mode mode)
> +{
> +	return mode == FUSE_DAX_INODE || mode == FUSE_DAX_NONE;
> +}

This is confusing. Why FUSE_DAX_NONE is equivalent to inode dax mode.
Is it because you want FUSE_DAX_INODE as default. If that's the case,
lets get rid of FUSE_DAX_NONE and just set FUSE_DAX_INODE as default?

> +
>  struct fuse_fs_context {
>  	int fd;
>  	struct file *file;
> @@ -497,7 +509,7 @@ struct fuse_fs_context {
>  	bool no_control:1;
>  	bool no_force_umount:1;
>  	bool legacy_opts_show:1;
> -	bool dax:1;
> +	enum fuse_dax_mode dax_mode;
>  	unsigned int max_read;
>  	unsigned int blksize;
>  	const char *subtype;
> @@ -802,6 +814,9 @@ struct fuse_conn {
>  	struct list_head devices;
>  
>  #ifdef CONFIG_FUSE_DAX
> +	/* Dax mode */
> +	enum fuse_dax_mode dax_mode;
> +
>  	/* Dax specific conn data, non-NULL if DAX is enabled */
>  	struct fuse_conn_dax *dax;
>  #endif
> @@ -1258,7 +1273,8 @@ ssize_t fuse_dax_read_iter(struct kiocb *iocb, struct iov_iter *to);
>  ssize_t fuse_dax_write_iter(struct kiocb *iocb, struct iov_iter *from);
>  int fuse_dax_mmap(struct file *file, struct vm_area_struct *vma);
>  int fuse_dax_break_layouts(struct inode *inode, u64 dmap_start, u64 dmap_end);
> -int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev);
> +int fuse_dax_conn_alloc(struct fuse_conn *fc, enum fuse_dax_mode mode,
> +			struct dax_device *dax_dev);
>  void fuse_dax_conn_free(struct fuse_conn *fc);
>  bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi);
>  void fuse_dax_inode_init(struct inode *inode);
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 12d49a1914e8..15ce56f9cf11 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -734,8 +734,12 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root)
>  			seq_printf(m, ",blksize=%lu", sb->s_blocksize);
>  	}
>  #ifdef CONFIG_FUSE_DAX
> -	if (fc->dax)
> -		seq_puts(m, ",dax");
> +	if (fc->dax_mode == FUSE_DAX_ALWAYS)
> +		seq_puts(m, ",dax=always");
> +	else if (fc->dax_mode == FUSE_DAX_NEVER)
> +		seq_puts(m, ",dax=never");
> +	else if (fc->dax_mode == FUSE_DAX_INODE)
> +		seq_puts(m, ",dax=inode");

I guess this answers the question about FUSE_DAX_NONE. You want to
keep track if user passed in "dax=inode" or you defaulted to dax=inode.
And if you defaulted to "dax=inode" you don't want to show it in fuse
options.

Hmm..., if that's the intent, I would rather keep the names like this.

FUSE_DAX_INODE_USER and FUSE_DAX_INODE_DEFAULT. This clearly tells
me the difference between two states.

>  #endif
>  
>  	return 0;
> @@ -1481,7 +1485,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	sb->s_subtype = ctx->subtype;
>  	ctx->subtype = NULL;
>  	if (IS_ENABLED(CONFIG_FUSE_DAX)) {
> -		err = fuse_dax_conn_alloc(fc, ctx->dax_dev);
> +		err = fuse_dax_conn_alloc(fc, ctx->dax_mode, ctx->dax_dev);
>  		if (err)
>  			goto err;
>  	}
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 94fc874f5de7..e8c404946c63 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -88,12 +88,21 @@ struct virtio_fs_req_work {
>  static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>  				 struct fuse_req *req, bool in_flight);
>  
> +static const struct constant_table dax_param_enums[] = {
> +	{"always",	FUSE_DAX_ALWAYS },
> +	{"never",	FUSE_DAX_NEVER },
> +	{"inode",	FUSE_DAX_INODE },
> +	{}
> +};
> +
>  enum {
>  	OPT_DAX,
> +	OPT_DAX_ENUM,
>  };
>  
>  static const struct fs_parameter_spec virtio_fs_parameters[] = {
>  	fsparam_flag("dax", OPT_DAX),
> +	fsparam_enum("dax", OPT_DAX_ENUM, dax_param_enums),
>  	{}
>  };
>  
> @@ -110,7 +119,10 @@ static int virtio_fs_parse_param(struct fs_context *fsc,
>  
>  	switch (opt) {
>  	case OPT_DAX:
> -		ctx->dax = 1;
> +		ctx->dax_mode = FUSE_DAX_ALWAYS;
> +		break;
> +	case OPT_DAX_ENUM:
> +		ctx->dax_mode = result.uint_32;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1326,8 +1338,8 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>  
>  	/* virtiofs allocates and installs its own fuse devices */
>  	ctx->fudptr = NULL;
> -	if (ctx->dax) {
> -		if (!fs->dax_dev) {
> +	if (ctx->dax_mode != FUSE_DAX_NEVER) {
	   ^^
Why do we need this check. IOW, why following check alone is not
sufficient.

> +		if (ctx->dax_mode == FUSE_DAX_ALWAYS && !fs->dax_dev) {
		 ^^^

If user specified dax mode FUSE_DAX_ALWAYS, we need to make sure fs device
supports dax.  And second if condition seems sufficient. 

Vivek

>  			err = -EINVAL;
>  			pr_err("virtio-fs: dax can't be enabled as filesystem"
>  			       " device does not support it.\n");
> -- 
> 2.27.0
> 

Re: [PATCH v7 5/7] fuse: negotiate per inode DAX in FUSE_INIT

[Vivek Goyal]

On Tue, Nov 02, 2021 at 01:26:02PM +0800, Jeffle Xu wrote:
> Among the FUSE_INIT phase, client shall advertise per inode DAX if it's
> mounted with "dax=inode". Then server is aware that client is in per
> inode DAX mode, and will construct per-inode DAX attribute accordingly.
> 
> Server shall also advertise support for per inode DAX. If server doesn't
> support it while client is mounted with "dax=inode", client will
> silently fallback to "dax=never" since "dax=inode" is advisory only.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/dax.c    |  2 +-
>  fs/fuse/fuse_i.h |  3 +++
>  fs/fuse/inode.c  | 16 +++++++++++++---
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index 8a328fb20dcb..c8ee601b94b8 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -1350,7 +1350,7 @@ static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
>  		return true;
>  
>  	/* dax_mode is FUSE_DAX_INODE or FUSE_DAX_NONE */
> -	return flags & FUSE_ATTR_DAX;
> +	return fc->inode_dax && (flags & FUSE_ATTR_DAX);
>  }
>  
>  void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 055b39430540..58e54b5a4d65 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -777,6 +777,9 @@ struct fuse_conn {
>  	/* Propagate syncfs() to server */
>  	unsigned int sync_fs:1;
>  
> +	/* Does the filesystem support per inode DAX? */
> +	unsigned int inode_dax:1;
> +
>  	/** The number of requests waiting for completion */
>  	atomic_t num_waiting;
>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index acba14002d04..0512d8cb36c3 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1136,11 +1136,19 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>  					min_t(unsigned int, fc->max_pages_limit,
>  					max_t(unsigned int, arg->max_pages, 1));
>  			}
> -			if (IS_ENABLED(CONFIG_FUSE_DAX) &&
> -			    arg->flags & FUSE_MAP_ALIGNMENT &&
> +#ifdef CONFIG_FUSE_DAX
> +			if ((arg->flags & FUSE_HAS_INODE_DAX) &&
> +			    fuse_is_inode_dax_mode(fc->dax_mode)) {

Why do we call fuse_is_inode_dax_mode() here? While sending INIT request
we set FUSE_HAS_INODE_DAX only if fuse_is_inode_dax_mode() is true. So
we should not have to call it again when server replies.?

> +				fc->inode_dax = 1;
> +			}
> +			if (arg->flags & FUSE_MAP_ALIGNMENT &&
>  			    !fuse_dax_check_alignment(fc, arg->map_alignment)) {
> -				ok = false;
> +				if (fuse_is_inode_dax_mode(fc->dax_mode))
> +					fc->inode_dax = 0;

If mapping alignment is not right, I guess we can fail (even in case
of dax=inode). In this case client wants per dax inode, server supports
it but alignment is wrong. I think that should be an error and user should
fix it. IMHO, just leave this code path in place and we will error out.

Thanks
Vivek

> +				else
> +					ok = false;
>  			}
> +#endif
>  			if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) {
>  				fc->handle_killpriv_v2 = 1;
>  				fm->sb->s_flags |= SB_NOSEC;
> @@ -1194,6 +1202,8 @@ void fuse_send_init(struct fuse_mount *fm)
>  #ifdef CONFIG_FUSE_DAX
>  	if (fm->fc->dax)
>  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> +	if (fuse_is_inode_dax_mode(fm->fc->dax_mode))
> +		ia->in.flags |= FUSE_HAS_INODE_DAX;
>  #endif
>  	if (fm->fc->auto_submounts)
>  		ia->in.flags |= FUSE_SUBMOUNTS;
> -- 
> 2.27.0
> 

Re: [PATCH v7 6/7] fuse: mark inode DONT_CACHE when per inode DAX hint changes

[Vivek Goyal]

On Wed, Nov 10, 2021 at 04:50:25PM +0100, Miklos Szeredi wrote:
> On Tue, 2 Nov 2021 at 06:26, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
> >
> > When the per inode DAX hint changes while the file is still *opened*, it
> > is quite complicated and maybe fragile to dynamically change the DAX
> > state.
> >
> > Hence mark the inode and corresponding dentries as DONE_CACHE once the
> > per inode DAX hint changes, so that the inode instance will be evicted
> > and freed as soon as possible once the file is closed and the last
> > reference to the inode is put. And then when the file gets reopened next
> > time, the new instantiated inode will reflect the new DAX state.
> >
> > In summary, when the per inode DAX hint changes for an *opened* file, the
> > DAX state of the file won't be updated until this file is closed and
> > reopened later.
> 
> This patch does nothing, since fuse already uses .drop_inode =
> generic_delete_inode, which is has the same effect as setting
> I_DONTCACHE, at least in the fuse case (inode should never be dirty at
> eviction).   In fact it may be cleaner to set I_DONTCACHE
> unconditionally and remove the .drop_inode callback setting.

I thought idea was to drop dentry and not cache it which in turn
will drop dentry's reference on inode and lead to cleanup of inode.

Otherwise dentry might remain cached and which in-turn will keep
inode in cache. Am I missing something.

Thanks
Vivek

Re: [PATCH v7 6/7] fuse: mark inode DONT_CACHE when per inode DAX hint changes

[JeffleXu]

On 11/12/21 4:33 AM, Vivek Goyal wrote:
> On Wed, Nov 10, 2021 at 04:50:25PM +0100, Miklos Szeredi wrote:
>> On Tue, 2 Nov 2021 at 06:26, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>>>
>>> When the per inode DAX hint changes while the file is still *opened*, it
>>> is quite complicated and maybe fragile to dynamically change the DAX
>>> state.
>>>
>>> Hence mark the inode and corresponding dentries as DONE_CACHE once the
>>> per inode DAX hint changes, so that the inode instance will be evicted
>>> and freed as soon as possible once the file is closed and the last
>>> reference to the inode is put. And then when the file gets reopened next
>>> time, the new instantiated inode will reflect the new DAX state.
>>>
>>> In summary, when the per inode DAX hint changes for an *opened* file, the
>>> DAX state of the file won't be updated until this file is closed and
>>> reopened later.
>>
>> This patch does nothing, since fuse already uses .drop_inode =
>> generic_delete_inode, which is has the same effect as setting
>> I_DONTCACHE, at least in the fuse case (inode should never be dirty at
>> eviction).   In fact it may be cleaner to set I_DONTCACHE
>> unconditionally and remove the .drop_inode callback setting.
> 
> I thought idea was to drop dentry and not cache it which in turn
> will drop dentry's reference on inode and lead to cleanup of inode.
> 
> Otherwise dentry might remain cached and which in-turn will keep
> inode in cache. Am I missing something.
> 
Yes that's the case.

-- 
Thanks,
Jeffle

Re: [PATCH v7 2/7] fuse: make DAX mount option a tri-state

[JeffleXu]

On 11/12/21 2:52 AM, Vivek Goyal wrote:
> On Tue, Nov 02, 2021 at 01:25:59PM +0800, Jeffle Xu wrote:
>> We add 'always', 'never', and 'inode' (default). '-o dax' continues to
>> operate the same which is equivalent to 'always'.
>>
>> The following behavior is consistent with that on ext4/xfs:
>> - The default behavior (when neither '-o dax' nor
>>   '-o dax=always|never|inode' option is specified) is equal to 'inode'
>>   mode, while 'dax=inode' won't be printed among the mount option list.
>> - The 'inode' mode is only advisory. It will silently fallback to
>>   'never' mode if fuse server doesn't support that.
>>
>> Also noted that by the time of this commit, 'inode' mode is actually
>> equal to 'always' mode, before the per inode DAX flag is introduced in
>> the following patch.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  fs/fuse/dax.c       |  9 ++++++++-
>>  fs/fuse/fuse_i.h    | 20 ++++++++++++++++++--
>>  fs/fuse/inode.c     | 10 +++++++---
>>  fs/fuse/virtio_fs.c | 18 +++++++++++++++---
>>  4 files changed, 48 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>> index 8c187b04874e..91c8d146dbc4 100644
>> --- a/fs/fuse/dax.c
>> +++ b/fs/fuse/dax.c
>> @@ -1284,11 +1284,14 @@ static int fuse_dax_mem_range_init(struct fuse_conn_dax *fcd)
>>  	return ret;
>>  }
>>  
>> -int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
>> +int fuse_dax_conn_alloc(struct fuse_conn *fc, enum fuse_dax_mode dax_mode,
>> +			struct dax_device *dax_dev)
>>  {
>>  	struct fuse_conn_dax *fcd;
>>  	int err;
>>  
>> +	fc->dax_mode = dax_mode;
>> +
>>  	if (!dax_dev)
>>  		return 0;
>>  
>> @@ -1335,6 +1338,10 @@ static const struct address_space_operations fuse_dax_file_aops  = {
>>  static bool fuse_should_enable_dax(struct inode *inode)
>>  {
>>  	struct fuse_conn *fc = get_fuse_conn(inode);
>> +	enum fuse_dax_mode dax_mode = fc->dax_mode;
>> +
>> +	if (dax_mode == FUSE_DAX_NEVER)
>> +		return false;
>>  
>>  	if (!fc->dax)
>>  		return false;
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index f55f9f94b1a4..4f9c2358f343 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -480,6 +480,18 @@ struct fuse_dev {
>>  	struct list_head entry;
>>  };
>>  
>> +enum fuse_dax_mode {
>> +	FUSE_DAX_NONE,	 /* default */
>> +	FUSE_DAX_ALWAYS, /* "-o dax=always" */
>> +	FUSE_DAX_NEVER,  /* "-o dax=never" */
>> +	FUSE_DAX_INODE,  /* "-o dax=inode" */
>> +};
> Hi,
> 
> Not sure why do we need FUSE_DAX_NONE. Now default is FUSE_DAX_INODE
> and "-o dax" will map to FUSE_DAX_ALWAYS. So after this patch series,
> nobody should be using FUSE_DAX_NONE state at all? So we should be
> able to get rid of entirely?
> 
>> +
>> +static inline bool fuse_is_inode_dax_mode(enum fuse_dax_mode mode)
>> +{
>> +	return mode == FUSE_DAX_INODE || mode == FUSE_DAX_NONE;
>> +}
> 
> This is confusing. Why FUSE_DAX_NONE is equivalent to inode dax mode.
> Is it because you want FUSE_DAX_INODE as default. If that's the case,
> lets get rid of FUSE_DAX_NONE and just set FUSE_DAX_INODE as default?
> 
>> +
>>  struct fuse_fs_context {
>>  	int fd;
>>  	struct file *file;
>> @@ -497,7 +509,7 @@ struct fuse_fs_context {
>>  	bool no_control:1;
>>  	bool no_force_umount:1;
>>  	bool legacy_opts_show:1;
>> -	bool dax:1;
>> +	enum fuse_dax_mode dax_mode;
>>  	unsigned int max_read;
>>  	unsigned int blksize;
>>  	const char *subtype;
>> @@ -802,6 +814,9 @@ struct fuse_conn {
>>  	struct list_head devices;
>>  
>>  #ifdef CONFIG_FUSE_DAX
>> +	/* Dax mode */
>> +	enum fuse_dax_mode dax_mode;
>> +
>>  	/* Dax specific conn data, non-NULL if DAX is enabled */
>>  	struct fuse_conn_dax *dax;
>>  #endif
>> @@ -1258,7 +1273,8 @@ ssize_t fuse_dax_read_iter(struct kiocb *iocb, struct iov_iter *to);
>>  ssize_t fuse_dax_write_iter(struct kiocb *iocb, struct iov_iter *from);
>>  int fuse_dax_mmap(struct file *file, struct vm_area_struct *vma);
>>  int fuse_dax_break_layouts(struct inode *inode, u64 dmap_start, u64 dmap_end);
>> -int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev);
>> +int fuse_dax_conn_alloc(struct fuse_conn *fc, enum fuse_dax_mode mode,
>> +			struct dax_device *dax_dev);
>>  void fuse_dax_conn_free(struct fuse_conn *fc);
>>  bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi);
>>  void fuse_dax_inode_init(struct inode *inode);
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 12d49a1914e8..15ce56f9cf11 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -734,8 +734,12 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root)
>>  			seq_printf(m, ",blksize=%lu", sb->s_blocksize);
>>  	}
>>  #ifdef CONFIG_FUSE_DAX
>> -	if (fc->dax)
>> -		seq_puts(m, ",dax");
>> +	if (fc->dax_mode == FUSE_DAX_ALWAYS)
>> +		seq_puts(m, ",dax=always");
>> +	else if (fc->dax_mode == FUSE_DAX_NEVER)
>> +		seq_puts(m, ",dax=never");
>> +	else if (fc->dax_mode == FUSE_DAX_INODE)
>> +		seq_puts(m, ",dax=inode");
> 
> I guess this answers the question about FUSE_DAX_NONE. You want to
> keep track if user passed in "dax=inode" or you defaulted to dax=inode.
> And if you defaulted to "dax=inode" you don't want to show it in fuse
> options.

Yes, so that this behavior of printing fuse options is consistnet with
ext4/xfs.

> 
> Hmm..., if that's the intent, I would rather keep the names like this.
> 
> FUSE_DAX_INODE_USER and FUSE_DAX_INODE_DEFAULT. This clearly tells
> me the difference between two states.

OK this naming is obviously more readable.

> 
>>  #endif
>>  
>>  	return 0;
>> @@ -1481,7 +1485,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>>  	sb->s_subtype = ctx->subtype;
>>  	ctx->subtype = NULL;
>>  	if (IS_ENABLED(CONFIG_FUSE_DAX)) {
>> -		err = fuse_dax_conn_alloc(fc, ctx->dax_dev);
>> +		err = fuse_dax_conn_alloc(fc, ctx->dax_mode, ctx->dax_dev);
>>  		if (err)
>>  			goto err;
>>  	}
>> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
>> index 94fc874f5de7..e8c404946c63 100644
>> --- a/fs/fuse/virtio_fs.c
>> +++ b/fs/fuse/virtio_fs.c
>> @@ -88,12 +88,21 @@ struct virtio_fs_req_work {
>>  static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>>  				 struct fuse_req *req, bool in_flight);
>>  
>> +static const struct constant_table dax_param_enums[] = {
>> +	{"always",	FUSE_DAX_ALWAYS },
>> +	{"never",	FUSE_DAX_NEVER },
>> +	{"inode",	FUSE_DAX_INODE },
>> +	{}
>> +};
>> +
>>  enum {
>>  	OPT_DAX,
>> +	OPT_DAX_ENUM,
>>  };
>>  
>>  static const struct fs_parameter_spec virtio_fs_parameters[] = {
>>  	fsparam_flag("dax", OPT_DAX),
>> +	fsparam_enum("dax", OPT_DAX_ENUM, dax_param_enums),
>>  	{}
>>  };
>>  
>> @@ -110,7 +119,10 @@ static int virtio_fs_parse_param(struct fs_context *fsc,
>>  
>>  	switch (opt) {
>>  	case OPT_DAX:
>> -		ctx->dax = 1;
>> +		ctx->dax_mode = FUSE_DAX_ALWAYS;
>> +		break;
>> +	case OPT_DAX_ENUM:
>> +		ctx->dax_mode = result.uint_32;
>>  		break;
>>  	default:
>>  		return -EINVAL;
>> @@ -1326,8 +1338,8 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>>  
>>  	/* virtiofs allocates and installs its own fuse devices */
>>  	ctx->fudptr = NULL;
>> -	if (ctx->dax) {
>> -		if (!fs->dax_dev) {
>> +	if (ctx->dax_mode != FUSE_DAX_NEVER) {
> 	   ^^
> Why do we need this check. IOW, why following check alone is not
> sufficient.
> 
>> +		if (ctx->dax_mode == FUSE_DAX_ALWAYS && !fs->dax_dev) {
> 		 ^^^
> 
> If user specified dax mode FUSE_DAX_ALWAYS, we need to make sure fs device
> supports dax.  And second if condition seems sufficient. 
> 

The complete code snippet is like:

```
        if (ctx->dax_mode != FUSE_DAX_NEVER) {
                if (ctx->dax_mode == FUSE_DAX_ALWAYS && !fs->dax_dev) {
                        err = -EINVAL;
                        pr_err("virtio-fs: dax can't be enabled as
filesystem"
                               " device does not support it.\n");
                        goto err_free_fuse_devs;
                }
                ctx->dax_dev = fs->dax_dev;
        }
```

So we need the first if condition statement, so that 'ctx->dax_dev' is
assigned in both FUSE_DAX_ALWAYS and FUSE_DAX_INODE cases.


-- 
Thanks,
Jeffle

Re: [PATCH v7 5/7] fuse: negotiate per inode DAX in FUSE_INIT

[JeffleXu]

On 11/12/21 3:45 AM, Vivek Goyal wrote:
> On Tue, Nov 02, 2021 at 01:26:02PM +0800, Jeffle Xu wrote:
>> Among the FUSE_INIT phase, client shall advertise per inode DAX if it's
>> mounted with "dax=inode". Then server is aware that client is in per
>> inode DAX mode, and will construct per-inode DAX attribute accordingly.
>>
>> Server shall also advertise support for per inode DAX. If server doesn't
>> support it while client is mounted with "dax=inode", client will
>> silently fallback to "dax=never" since "dax=inode" is advisory only.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  fs/fuse/dax.c    |  2 +-
>>  fs/fuse/fuse_i.h |  3 +++
>>  fs/fuse/inode.c  | 16 +++++++++++++---
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>> index 8a328fb20dcb..c8ee601b94b8 100644
>> --- a/fs/fuse/dax.c
>> +++ b/fs/fuse/dax.c
>> @@ -1350,7 +1350,7 @@ static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
>>  		return true;
>>  
>>  	/* dax_mode is FUSE_DAX_INODE or FUSE_DAX_NONE */
>> -	return flags & FUSE_ATTR_DAX;
>> +	return fc->inode_dax && (flags & FUSE_ATTR_DAX);
>>  }
>>  
>>  void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index 055b39430540..58e54b5a4d65 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -777,6 +777,9 @@ struct fuse_conn {
>>  	/* Propagate syncfs() to server */
>>  	unsigned int sync_fs:1;
>>  
>> +	/* Does the filesystem support per inode DAX? */
>> +	unsigned int inode_dax:1;
>> +
>>  	/** The number of requests waiting for completion */
>>  	atomic_t num_waiting;
>>  
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index acba14002d04..0512d8cb36c3 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -1136,11 +1136,19 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>>  					min_t(unsigned int, fc->max_pages_limit,
>>  					max_t(unsigned int, arg->max_pages, 1));
>>  			}
>> -			if (IS_ENABLED(CONFIG_FUSE_DAX) &&
>> -			    arg->flags & FUSE_MAP_ALIGNMENT &&
>> +#ifdef CONFIG_FUSE_DAX
>> +			if ((arg->flags & FUSE_HAS_INODE_DAX) &&
>> +			    fuse_is_inode_dax_mode(fc->dax_mode)) {
> 
> Why do we call fuse_is_inode_dax_mode() here? While sending INIT request
> we set FUSE_HAS_INODE_DAX only if fuse_is_inode_dax_mode() is true. So
> we should not have to call it again when server replies.?

OK I'll remove this redundant call for fuse_is_inode_dax_mode(). If fuse
server replies with FUSE_HAS_INODE_DAX when fuse client doesn't
advertise FUSE_HAS_INODE_DAX, then obviously fuse server shall be blamed.


> 
>> +				fc->inode_dax = 1;
>> +			}
>> +			if (arg->flags & FUSE_MAP_ALIGNMENT &&
>>  			    !fuse_dax_check_alignment(fc, arg->map_alignment)) {
>> -				ok = false;
>> +				if (fuse_is_inode_dax_mode(fc->dax_mode))
>> +					fc->inode_dax = 0;
> 
> If mapping alignment is not right, I guess we can fail (even in case
> of dax=inode). In this case client wants per dax inode, server supports
> it but alignment is wrong. I think that should be an error and user should
> fix it. IMHO, just leave this code path in place and we will error out.

I'm OK with the behavior of reporting error directly, but I'm afraid the
behavior is inconsistency then. That is, the following requirements are
needed to support DAX mode:

1. the virtiofs device doesn't support DAX at all
(VIRTIO_FS_SHMCAP_ID_CACHE not defined at all)
2. server's map alignment is non-compliant (fail fuse_dax_check_alignment())
3. server doesn't advertise support for per inode DAX
(FUSE_HAS_INODE_DAX) during FUSE_INIT

When virtiofs is mounted in 'dax=inode' mode inside guest, when case 1/3
occur, we silently fallback to 'dax=never'; while case 2 occurs, we just
error out.

-- 
Thanks,
Jeffle