[PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Jeffle Xu]

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

There are three related scenarios:
1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
2. Per-file DAX flag changes when the file has been opened. (patch 3)
In this case, the dentry and inode are all marked as DONT_CACHE, and
the DAX state won't be updated until the file is closed and reopened
later.
3. Users can change the per-file DAX flag inside the guest by chattr(1).
(patch 4)
4. Create new files under directories with DAX enabled. When creating
new files in ext4/xfs on host, the new created files will inherit the
per-file DAX flag from the directory, and thus the new created files in
virtiofs will also inherit the per-file DAX flag if the fuse server
derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
DAX flag.


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")


changes since v1:
- add support for changing per-file DAX flags inside guest (patch 4)

v1:https://www.spinics.net/lists/linux-virtualization/msg51008.html

Jeffle Xu (4):
  fuse: add fuse_should_enable_dax() helper
  fuse: Make DAX mount option a tri-state
  fuse: add per-file DAX flag
  fuse: support changing per-file DAX flag inside guest

 fs/fuse/dax.c             | 36 ++++++++++++++++++++++++++++++++++--
 fs/fuse/file.c            |  4 ++--
 fs/fuse/fuse_i.h          | 16 ++++++++++++----
 fs/fuse/inode.c           |  7 +++++--
 fs/fuse/ioctl.c           |  9 ++++++---
 fs/fuse/virtio_fs.c       | 16 ++++++++++++++--
 include/uapi/linux/fuse.h |  5 +++++
 7 files changed, 78 insertions(+), 15 deletions(-)

-- 
2.27.0

[PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Jeffle Xu]

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

There are three related scenarios:
1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
2. Per-file DAX flag changes when the file has been opened. (patch 3)
In this case, the dentry and inode are all marked as DONT_CACHE, and
the DAX state won't be updated until the file is closed and reopened
later.
3. Users can change the per-file DAX flag inside the guest by chattr(1).
(patch 4)
4. Create new files under directories with DAX enabled. When creating
new files in ext4/xfs on host, the new created files will inherit the
per-file DAX flag from the directory, and thus the new created files in
virtiofs will also inherit the per-file DAX flag if the fuse server
derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
DAX flag.


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")


changes since v1:
- add support for changing per-file DAX flags inside guest (patch 4)

v1:https://www.spinics.net/lists/linux-virtualization/msg51008.html

Jeffle Xu (4):
  fuse: add fuse_should_enable_dax() helper
  fuse: Make DAX mount option a tri-state
  fuse: add per-file DAX flag
  fuse: support changing per-file DAX flag inside guest

 fs/fuse/dax.c             | 36 ++++++++++++++++++++++++++++++++++--
 fs/fuse/file.c            |  4 ++--
 fs/fuse/fuse_i.h          | 16 ++++++++++++----
 fs/fuse/inode.c           |  7 +++++--
 fs/fuse/ioctl.c           |  9 ++++++---
 fs/fuse/virtio_fs.c       | 16 ++++++++++++++--
 include/uapi/linux/fuse.h |  5 +++++
 7 files changed, 78 insertions(+), 15 deletions(-)

-- 
2.27.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2 1/4] fuse: add fuse_should_enable_dax() helper

[Jeffle Xu]

This is in prep for following per-file 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 0e5407f48e6a..c6f4e82e65f3 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1336,11 +1336,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 v2 1/4] fuse: add fuse_should_enable_dax() helper

[Jeffle Xu]

This is in prep for following per-file 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 0e5407f48e6a..c6f4e82e65f3 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1336,11 +1336,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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2 2/4] 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'.

By the time this patch is applied, 'inode' mode is actually equal to
'always' mode, before the per-file DAX flag is introduced in the
following patch.

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

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index c6f4e82e65f3..a478e824c2d0 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -70,6 +70,9 @@ struct fuse_inode_dax {
 };
 
 struct fuse_conn_dax {
+	/** dax mode: FUSE_DAX_MOUNT_* (always, never or per-file) **/
+	unsigned int mode;
+
 	/* DAX device */
 	struct dax_device *dev;
 
@@ -1288,7 +1291,8 @@ 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, unsigned int mode,
+			struct dax_device *dax_dev)
 {
 	struct fuse_conn_dax *fcd;
 	int err;
@@ -1301,6 +1305,7 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
 		return -ENOMEM;
 
 	spin_lock_init(&fcd->lock);
+	fcd->mode = mode;
 	fcd->dev = dax_dev;
 	err = fuse_dax_mem_range_init(fcd);
 	if (err) {
@@ -1339,10 +1344,16 @@ 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);
+	unsigned int mode;
 
 	if (!fc->dax)
 		return false;
 
+	mode = fc->dax->mode;
+
+	if (mode == FUSE_DAX_MOUNT_NEVER)
+		return false;
+
 	return true;
 }
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 07829ce78695..f29018323845 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -487,6 +487,12 @@ struct fuse_dev {
 	struct list_head entry;
 };
 
+enum {
+	FUSE_DAX_MOUNT_INODE,
+	FUSE_DAX_MOUNT_ALWAYS,
+	FUSE_DAX_MOUNT_NEVER,
+};
+
 struct fuse_fs_context {
 	int fd;
 	unsigned int rootmode;
@@ -503,7 +509,7 @@ struct fuse_fs_context {
 	bool no_control:1;
 	bool no_force_umount:1;
 	bool legacy_opts_show:1;
-	bool dax:1;
+	unsigned int dax;
 	unsigned int max_read;
 	unsigned int blksize;
 	const char *subtype;
@@ -1242,7 +1248,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, unsigned int 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 b9beb39a4a18..f6b46395edb2 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1434,7 +1434,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, ctx->dax_dev);
 		if (err)
 			goto err;
 	}
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 8f52cdaa8445..561f711d1945 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[] = {
+	{"inode",	FUSE_DAX_MOUNT_INODE },
+	{"always",	FUSE_DAX_MOUNT_ALWAYS },
+	{"never",	FUSE_DAX_MOUNT_NEVER },
+	{}
+};
+
 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 *fc,
 
 	switch (opt) {
 	case OPT_DAX:
-		ctx->dax = 1;
+		ctx->dax = FUSE_DAX_MOUNT_ALWAYS;
+		break;
+	case OPT_DAX_ENUM:
+		ctx->dax = result.uint_32;
 		break;
 	default:
 		return -EINVAL;
@@ -1326,7 +1338,7 @@ 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 (ctx->dax != FUSE_DAX_MOUNT_NEVER) {
 		if (!fs->dax_dev) {
 			err = -EINVAL;
 			pr_err("virtio-fs: dax can't be enabled as filesystem"
-- 
2.27.0

[PATCH v2 2/4] 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'.

By the time this patch is applied, 'inode' mode is actually equal to
'always' mode, before the per-file DAX flag is introduced in the
following patch.

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

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index c6f4e82e65f3..a478e824c2d0 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -70,6 +70,9 @@ struct fuse_inode_dax {
 };
 
 struct fuse_conn_dax {
+	/** dax mode: FUSE_DAX_MOUNT_* (always, never or per-file) **/
+	unsigned int mode;
+
 	/* DAX device */
 	struct dax_device *dev;
 
@@ -1288,7 +1291,8 @@ 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, unsigned int mode,
+			struct dax_device *dax_dev)
 {
 	struct fuse_conn_dax *fcd;
 	int err;
@@ -1301,6 +1305,7 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
 		return -ENOMEM;
 
 	spin_lock_init(&fcd->lock);
+	fcd->mode = mode;
 	fcd->dev = dax_dev;
 	err = fuse_dax_mem_range_init(fcd);
 	if (err) {
@@ -1339,10 +1344,16 @@ 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);
+	unsigned int mode;
 
 	if (!fc->dax)
 		return false;
 
+	mode = fc->dax->mode;
+
+	if (mode == FUSE_DAX_MOUNT_NEVER)
+		return false;
+
 	return true;
 }
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 07829ce78695..f29018323845 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -487,6 +487,12 @@ struct fuse_dev {
 	struct list_head entry;
 };
 
+enum {
+	FUSE_DAX_MOUNT_INODE,
+	FUSE_DAX_MOUNT_ALWAYS,
+	FUSE_DAX_MOUNT_NEVER,
+};
+
 struct fuse_fs_context {
 	int fd;
 	unsigned int rootmode;
@@ -503,7 +509,7 @@ struct fuse_fs_context {
 	bool no_control:1;
 	bool no_force_umount:1;
 	bool legacy_opts_show:1;
-	bool dax:1;
+	unsigned int dax;
 	unsigned int max_read;
 	unsigned int blksize;
 	const char *subtype;
@@ -1242,7 +1248,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, unsigned int 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 b9beb39a4a18..f6b46395edb2 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1434,7 +1434,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, ctx->dax_dev);
 		if (err)
 			goto err;
 	}
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 8f52cdaa8445..561f711d1945 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[] = {
+	{"inode",	FUSE_DAX_MOUNT_INODE },
+	{"always",	FUSE_DAX_MOUNT_ALWAYS },
+	{"never",	FUSE_DAX_MOUNT_NEVER },
+	{}
+};
+
 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 *fc,
 
 	switch (opt) {
 	case OPT_DAX:
-		ctx->dax = 1;
+		ctx->dax = FUSE_DAX_MOUNT_ALWAYS;
+		break;
+	case OPT_DAX_ENUM:
+		ctx->dax = result.uint_32;
 		break;
 	default:
 		return -EINVAL;
@@ -1326,7 +1338,7 @@ 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 (ctx->dax != FUSE_DAX_MOUNT_NEVER) {
 		if (!fs->dax_dev) {
 			err = -EINVAL;
 			pr_err("virtio-fs: dax can't be enabled as filesystem"
-- 
2.27.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:51PM +0800, Jeffle Xu wrote:
> We add 'always', 'never', and 'inode' (default). '-o dax' continues to
> operate the same which is equivalent to 'always'.
> 
> By the time this patch is applied, 'inode' mode is actually equal to
> 'always' mode, before the per-file DAX flag is introduced in the
> following patch.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/dax.c       | 13 ++++++++++++-
>  fs/fuse/fuse_i.h    | 11 +++++++++--
>  fs/fuse/inode.c     |  2 +-
>  fs/fuse/virtio_fs.c | 16 ++++++++++++++--
>  4 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index c6f4e82e65f3..a478e824c2d0 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -70,6 +70,9 @@ struct fuse_inode_dax {
>  };
>  
>  struct fuse_conn_dax {
> +	/** dax mode: FUSE_DAX_MOUNT_* (always, never or per-file) **/
> +	unsigned int mode;

Why "/**" ?

How about make it something like "enum fuse_dax_mode mode" instead?

enum fuse_dax_mode dax_mode;

> +
>  	/* DAX device */
>  	struct dax_device *dev;
>  
> @@ -1288,7 +1291,8 @@ 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, unsigned int mode,
> +			struct dax_device *dax_dev)
>  {
>  	struct fuse_conn_dax *fcd;
>  	int err;
> @@ -1301,6 +1305,7 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&fcd->lock);
> +	fcd->mode = mode;
>  	fcd->dev = dax_dev;
>  	err = fuse_dax_mem_range_init(fcd);
>  	if (err) {
> @@ -1339,10 +1344,16 @@ 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);
> +	unsigned int mode;
>  
>  	if (!fc->dax)
>  		return false;
>  
> +	mode = fc->dax->mode;
> +
> +	if (mode == FUSE_DAX_MOUNT_NEVER)
> +		return false;
> +
>  	return true;
>  }
>  
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 07829ce78695..f29018323845 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -487,6 +487,12 @@ struct fuse_dev {
>  	struct list_head entry;
>  };
>  
> +enum {
And this becomes.

enum fuse_dax_mode {
};

> +	FUSE_DAX_MOUNT_INODE,
> +	FUSE_DAX_MOUNT_ALWAYS,
> +	FUSE_DAX_MOUNT_NEVER,
> +};

How about getting rid of "MOUNT" and just do.

	FUSE_DAX_INODE,
	FUSE_DAX_ALWAYS,
	FUSE_DAX_NEVER,

> +
>  struct fuse_fs_context {
>  	int fd;
>  	unsigned int rootmode;
> @@ -503,7 +509,7 @@ struct fuse_fs_context {
>  	bool no_control:1;
>  	bool no_force_umount:1;
>  	bool legacy_opts_show:1;
> -	bool dax:1;
> +	unsigned int dax;

enum fuse_dax_mode dax_mode;

>  	unsigned int max_read;
>  	unsigned int blksize;
>  	const char *subtype;
> @@ -1242,7 +1248,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, unsigned int mode,
						   ^^
						enum fuse_dax_mode dax_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 b9beb39a4a18..f6b46395edb2 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1434,7 +1434,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, ctx->dax_dev);
>  		if (err)
>  			goto err;
>  	}
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 8f52cdaa8445..561f711d1945 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[] = {
> +	{"inode",	FUSE_DAX_MOUNT_INODE },
> +	{"always",	FUSE_DAX_MOUNT_ALWAYS },
> +	{"never",	FUSE_DAX_MOUNT_NEVER },
> +	{}
> +};
> +
>  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 *fc,
>  
>  	switch (opt) {
>  	case OPT_DAX:
> -		ctx->dax = 1;
> +		ctx->dax = FUSE_DAX_MOUNT_ALWAYS;
> +		break;
> +	case OPT_DAX_ENUM:
> +		ctx->dax = result.uint_32;

Do we want to check here if result.uint_32 has one of the allowed values.
FUSE_DAX_MOUNT_INODE, FUSE_DAX_MOUNT_ALWAYS or FUSE_DAX_MOUNT_NEVER. Or
VFS has already taken care of that?

Vivek



>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1326,7 +1338,7 @@ 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 (ctx->dax != FUSE_DAX_MOUNT_NEVER) {
>  		if (!fs->dax_dev) {
>  			err = -EINVAL;
>  			pr_err("virtio-fs: dax can't be enabled as filesystem"
> -- 
> 2.27.0
> 

Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:51PM +0800, Jeffle Xu wrote:
> We add 'always', 'never', and 'inode' (default). '-o dax' continues to
> operate the same which is equivalent to 'always'.
> 
> By the time this patch is applied, 'inode' mode is actually equal to
> 'always' mode, before the per-file DAX flag is introduced in the
> following patch.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/dax.c       | 13 ++++++++++++-
>  fs/fuse/fuse_i.h    | 11 +++++++++--
>  fs/fuse/inode.c     |  2 +-
>  fs/fuse/virtio_fs.c | 16 ++++++++++++++--
>  4 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index c6f4e82e65f3..a478e824c2d0 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -70,6 +70,9 @@ struct fuse_inode_dax {
>  };
>  
>  struct fuse_conn_dax {
> +	/** dax mode: FUSE_DAX_MOUNT_* (always, never or per-file) **/
> +	unsigned int mode;

Why "/**" ?

How about make it something like "enum fuse_dax_mode mode" instead?

enum fuse_dax_mode dax_mode;

> +
>  	/* DAX device */
>  	struct dax_device *dev;
>  
> @@ -1288,7 +1291,8 @@ 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, unsigned int mode,
> +			struct dax_device *dax_dev)
>  {
>  	struct fuse_conn_dax *fcd;
>  	int err;
> @@ -1301,6 +1305,7 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&fcd->lock);
> +	fcd->mode = mode;
>  	fcd->dev = dax_dev;
>  	err = fuse_dax_mem_range_init(fcd);
>  	if (err) {
> @@ -1339,10 +1344,16 @@ 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);
> +	unsigned int mode;
>  
>  	if (!fc->dax)
>  		return false;
>  
> +	mode = fc->dax->mode;
> +
> +	if (mode == FUSE_DAX_MOUNT_NEVER)
> +		return false;
> +
>  	return true;
>  }
>  
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 07829ce78695..f29018323845 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -487,6 +487,12 @@ struct fuse_dev {
>  	struct list_head entry;
>  };
>  
> +enum {
And this becomes.

enum fuse_dax_mode {
};

> +	FUSE_DAX_MOUNT_INODE,
> +	FUSE_DAX_MOUNT_ALWAYS,
> +	FUSE_DAX_MOUNT_NEVER,
> +};

How about getting rid of "MOUNT" and just do.

	FUSE_DAX_INODE,
	FUSE_DAX_ALWAYS,
	FUSE_DAX_NEVER,

> +
>  struct fuse_fs_context {
>  	int fd;
>  	unsigned int rootmode;
> @@ -503,7 +509,7 @@ struct fuse_fs_context {
>  	bool no_control:1;
>  	bool no_force_umount:1;
>  	bool legacy_opts_show:1;
> -	bool dax:1;
> +	unsigned int dax;

enum fuse_dax_mode dax_mode;

>  	unsigned int max_read;
>  	unsigned int blksize;
>  	const char *subtype;
> @@ -1242,7 +1248,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, unsigned int mode,
						   ^^
						enum fuse_dax_mode dax_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 b9beb39a4a18..f6b46395edb2 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1434,7 +1434,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, ctx->dax_dev);
>  		if (err)
>  			goto err;
>  	}
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 8f52cdaa8445..561f711d1945 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[] = {
> +	{"inode",	FUSE_DAX_MOUNT_INODE },
> +	{"always",	FUSE_DAX_MOUNT_ALWAYS },
> +	{"never",	FUSE_DAX_MOUNT_NEVER },
> +	{}
> +};
> +
>  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 *fc,
>  
>  	switch (opt) {
>  	case OPT_DAX:
> -		ctx->dax = 1;
> +		ctx->dax = FUSE_DAX_MOUNT_ALWAYS;
> +		break;
> +	case OPT_DAX_ENUM:
> +		ctx->dax = result.uint_32;

Do we want to check here if result.uint_32 has one of the allowed values.
FUSE_DAX_MOUNT_INODE, FUSE_DAX_MOUNT_ALWAYS or FUSE_DAX_MOUNT_NEVER. Or
VFS has already taken care of that?

Vivek



>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1326,7 +1338,7 @@ 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 (ctx->dax != FUSE_DAX_MOUNT_NEVER) {
>  		if (!fs->dax_dev) {
>  			err = -EINVAL;
>  			pr_err("virtio-fs: dax can't be enabled as filesystem"
> -- 
> 2.27.0
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state

[JeffleXu]

On 7/20/21 2:02 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:51PM +0800, Jeffle Xu wrote:
>> We add 'always', 'never', and 'inode' (default). '-o dax' continues to
>> operate the same which is equivalent to 'always'.
>>
>> By the time this patch is applied, 'inode' mode is actually equal to
>> 'always' mode, before the per-file DAX flag is introduced in the
>> following patch.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  fs/fuse/dax.c       | 13 ++++++++++++-
>>  fs/fuse/fuse_i.h    | 11 +++++++++--
>>  fs/fuse/inode.c     |  2 +-
>>  fs/fuse/virtio_fs.c | 16 ++++++++++++++--
>>  4 files changed, 36 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>> index c6f4e82e65f3..a478e824c2d0 100644
>> --- a/fs/fuse/dax.c
>> +++ b/fs/fuse/dax.c
>> @@ -70,6 +70,9 @@ struct fuse_inode_dax {
>>  };
>>  
>>  struct fuse_conn_dax {
>> +	/** dax mode: FUSE_DAX_MOUNT_* (always, never or per-file) **/
>> +	unsigned int mode;
> 
> Why "/**" ?

I copied this comment style from fuse in v4.19... Anyway, I will fix this.

> 
> How about make it something like "enum fuse_dax_mode mode" instead?
> 
> enum fuse_dax_mode dax_mode;

OK.

> 
>> +
>>  	/* DAX device */
>>  	struct dax_device *dev;
>>  
>> @@ -1288,7 +1291,8 @@ 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, unsigned int mode,
>> +			struct dax_device *dax_dev)
>>  {
>>  	struct fuse_conn_dax *fcd;
>>  	int err;
>> @@ -1301,6 +1305,7 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
>>  		return -ENOMEM;
>>  
>>  	spin_lock_init(&fcd->lock);
>> +	fcd->mode = mode;
>>  	fcd->dev = dax_dev;
>>  	err = fuse_dax_mem_range_init(fcd);
>>  	if (err) {
>> @@ -1339,10 +1344,16 @@ 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);
>> +	unsigned int mode;
>>  
>>  	if (!fc->dax)
>>  		return false;
>>  
>> +	mode = fc->dax->mode;
>> +
>> +	if (mode == FUSE_DAX_MOUNT_NEVER)
>> +		return false;
>> +
>>  	return true;
>>  }
>>  
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index 07829ce78695..f29018323845 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -487,6 +487,12 @@ struct fuse_dev {
>>  	struct list_head entry;
>>  };
>>  
>> +enum {
> And this becomes.
> 
> enum fuse_dax_mode {
> };

OK.

> 
>> +	FUSE_DAX_MOUNT_INODE,
>> +	FUSE_DAX_MOUNT_ALWAYS,
>> +	FUSE_DAX_MOUNT_NEVER,
>> +};
> 
> How about getting rid of "MOUNT" and just do.
> 
> 	FUSE_DAX_INODE,
> 	FUSE_DAX_ALWAYS,
> 	FUSE_DAX_NEVER,

OK.

> 
>> +
>>  struct fuse_fs_context {
>>  	int fd;
>>  	unsigned int rootmode;
>> @@ -503,7 +509,7 @@ struct fuse_fs_context {
>>  	bool no_control:1;
>>  	bool no_force_umount:1;
>>  	bool legacy_opts_show:1;
>> -	bool dax:1;
>> +	unsigned int dax;
> 
> enum fuse_dax_mode dax_mode;

OK.

> 
>>  	unsigned int max_read;
>>  	unsigned int blksize;
>>  	const char *subtype;
>> @@ -1242,7 +1248,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, unsigned int mode,
> 						   ^^
> 						enum fuse_dax_mode dax_mode

OK.

>> +			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 b9beb39a4a18..f6b46395edb2 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -1434,7 +1434,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, ctx->dax_dev);
>>  		if (err)
>>  			goto err;
>>  	}
>> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
>> index 8f52cdaa8445..561f711d1945 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[] = {
>> +	{"inode",	FUSE_DAX_MOUNT_INODE },
>> +	{"always",	FUSE_DAX_MOUNT_ALWAYS },
>> +	{"never",	FUSE_DAX_MOUNT_NEVER },
>> +	{}
>> +};
>> +
>>  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 *fc,
>>  
>>  	switch (opt) {
>>  	case OPT_DAX:
>> -		ctx->dax = 1;
>> +		ctx->dax = FUSE_DAX_MOUNT_ALWAYS;
>> +		break;
>> +	case OPT_DAX_ENUM:
>> +		ctx->dax = result.uint_32;
> 
> Do we want to check here if result.uint_32 has one of the allowed values.
> FUSE_DAX_MOUNT_INODE, FUSE_DAX_MOUNT_ALWAYS or FUSE_DAX_MOUNT_NEVER. Or
> VFS has already taken care of that?

VFS will ensure that the returned result.uint_32 can only be among the
values defined in 'struct constant_table', or fs_parse() will return
-EINVAL.

-- 
Thanks,
Jeffle

Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state

[JeffleXu]

On 7/20/21 2:02 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:51PM +0800, Jeffle Xu wrote:
>> We add 'always', 'never', and 'inode' (default). '-o dax' continues to
>> operate the same which is equivalent to 'always'.
>>
>> By the time this patch is applied, 'inode' mode is actually equal to
>> 'always' mode, before the per-file DAX flag is introduced in the
>> following patch.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>  fs/fuse/dax.c       | 13 ++++++++++++-
>>  fs/fuse/fuse_i.h    | 11 +++++++++--
>>  fs/fuse/inode.c     |  2 +-
>>  fs/fuse/virtio_fs.c | 16 ++++++++++++++--
>>  4 files changed, 36 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>> index c6f4e82e65f3..a478e824c2d0 100644
>> --- a/fs/fuse/dax.c
>> +++ b/fs/fuse/dax.c
>> @@ -70,6 +70,9 @@ struct fuse_inode_dax {
>>  };
>>  
>>  struct fuse_conn_dax {
>> +	/** dax mode: FUSE_DAX_MOUNT_* (always, never or per-file) **/
>> +	unsigned int mode;
> 
> Why "/**" ?

I copied this comment style from fuse in v4.19... Anyway, I will fix this.

> 
> How about make it something like "enum fuse_dax_mode mode" instead?
> 
> enum fuse_dax_mode dax_mode;

OK.

> 
>> +
>>  	/* DAX device */
>>  	struct dax_device *dev;
>>  
>> @@ -1288,7 +1291,8 @@ 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, unsigned int mode,
>> +			struct dax_device *dax_dev)
>>  {
>>  	struct fuse_conn_dax *fcd;
>>  	int err;
>> @@ -1301,6 +1305,7 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, struct dax_device *dax_dev)
>>  		return -ENOMEM;
>>  
>>  	spin_lock_init(&fcd->lock);
>> +	fcd->mode = mode;
>>  	fcd->dev = dax_dev;
>>  	err = fuse_dax_mem_range_init(fcd);
>>  	if (err) {
>> @@ -1339,10 +1344,16 @@ 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);
>> +	unsigned int mode;
>>  
>>  	if (!fc->dax)
>>  		return false;
>>  
>> +	mode = fc->dax->mode;
>> +
>> +	if (mode == FUSE_DAX_MOUNT_NEVER)
>> +		return false;
>> +
>>  	return true;
>>  }
>>  
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index 07829ce78695..f29018323845 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -487,6 +487,12 @@ struct fuse_dev {
>>  	struct list_head entry;
>>  };
>>  
>> +enum {
> And this becomes.
> 
> enum fuse_dax_mode {
> };

OK.

> 
>> +	FUSE_DAX_MOUNT_INODE,
>> +	FUSE_DAX_MOUNT_ALWAYS,
>> +	FUSE_DAX_MOUNT_NEVER,
>> +};
> 
> How about getting rid of "MOUNT" and just do.
> 
> 	FUSE_DAX_INODE,
> 	FUSE_DAX_ALWAYS,
> 	FUSE_DAX_NEVER,

OK.

> 
>> +
>>  struct fuse_fs_context {
>>  	int fd;
>>  	unsigned int rootmode;
>> @@ -503,7 +509,7 @@ struct fuse_fs_context {
>>  	bool no_control:1;
>>  	bool no_force_umount:1;
>>  	bool legacy_opts_show:1;
>> -	bool dax:1;
>> +	unsigned int dax;
> 
> enum fuse_dax_mode dax_mode;

OK.

> 
>>  	unsigned int max_read;
>>  	unsigned int blksize;
>>  	const char *subtype;
>> @@ -1242,7 +1248,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, unsigned int mode,
> 						   ^^
> 						enum fuse_dax_mode dax_mode

OK.

>> +			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 b9beb39a4a18..f6b46395edb2 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -1434,7 +1434,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, ctx->dax_dev);
>>  		if (err)
>>  			goto err;
>>  	}
>> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
>> index 8f52cdaa8445..561f711d1945 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[] = {
>> +	{"inode",	FUSE_DAX_MOUNT_INODE },
>> +	{"always",	FUSE_DAX_MOUNT_ALWAYS },
>> +	{"never",	FUSE_DAX_MOUNT_NEVER },
>> +	{}
>> +};
>> +
>>  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 *fc,
>>  
>>  	switch (opt) {
>>  	case OPT_DAX:
>> -		ctx->dax = 1;
>> +		ctx->dax = FUSE_DAX_MOUNT_ALWAYS;
>> +		break;
>> +	case OPT_DAX_ENUM:
>> +		ctx->dax = result.uint_32;
> 
> Do we want to check here if result.uint_32 has one of the allowed values.
> FUSE_DAX_MOUNT_INODE, FUSE_DAX_MOUNT_ALWAYS or FUSE_DAX_MOUNT_NEVER. Or
> VFS has already taken care of that?

VFS will ensure that the returned result.uint_32 can only be among the
values defined in 'struct constant_table', or fs_parse() will return
-EINVAL.

-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2 3/4] fuse: add per-file DAX flag

[Jeffle Xu]

Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
this file.

When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
 fs/fuse/file.c            |  4 ++--
 fs/fuse/fuse_i.h          |  5 +++--
 fs/fuse/inode.c           |  5 ++++-
 include/uapi/linux/fuse.h |  5 +++++
 5 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index a478e824c2d0..0e862119757a 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1341,7 +1341,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);
 	unsigned int mode;
@@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
 	if (mode == FUSE_DAX_MOUNT_NEVER)
 		return false;
 
-	return true;
+	if (mode == FUSE_DAX_MOUNT_ALWAYS)
+		return true;
+
+	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
+	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;
 	inode->i_data.a_ops = &fuse_dax_file_aops;
 }
 
+void fuse_dax_dontcache(struct inode *inode, bool newdax)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+
+	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
+	    IS_DAX(inode) != newdax)
+		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/file.c b/fs/fuse/file.c
index 97f860cfc195..cf42af492146 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -3142,7 +3142,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, struct fuse_attr *attr)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
 
@@ -3156,5 +3156,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, attr->flags);
 }
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f29018323845..0793b93d680a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1000,7 +1000,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, struct fuse_attr *attr);
 
 /**
  * Initialize inode operations on regular files and special files
@@ -1252,8 +1252,9 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, unsigned int 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);
+void fuse_dax_dontcache(struct inode *inode, bool newdax);
 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 f6b46395edb2..2ae92798126e 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -269,6 +269,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 & FUSE_ATTR_DAX);
 }
 
 static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
@@ -281,7 +284,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);
 	} else if (S_ISDIR(inode->i_mode))
 		fuse_init_dir(inode);
 	else if (S_ISLNK(inode->i_mode))
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
  */
 
 #ifndef _LINUX_FUSE_H
@@ -449,8 +452,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-file DAX mode
  */
 #define FUSE_ATTR_SUBMOUNT      (1 << 0)
+#define FUSE_ATTR_DAX		(1 << 1)
 
 /**
  * Open flags
-- 
2.27.0

[PATCH v2 3/4] fuse: add per-file DAX flag

[Jeffle Xu]

Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
this file.

When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
 fs/fuse/file.c            |  4 ++--
 fs/fuse/fuse_i.h          |  5 +++--
 fs/fuse/inode.c           |  5 ++++-
 include/uapi/linux/fuse.h |  5 +++++
 5 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index a478e824c2d0..0e862119757a 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1341,7 +1341,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);
 	unsigned int mode;
@@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
 	if (mode == FUSE_DAX_MOUNT_NEVER)
 		return false;
 
-	return true;
+	if (mode == FUSE_DAX_MOUNT_ALWAYS)
+		return true;
+
+	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
+	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;
 	inode->i_data.a_ops = &fuse_dax_file_aops;
 }
 
+void fuse_dax_dontcache(struct inode *inode, bool newdax)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+
+	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
+	    IS_DAX(inode) != newdax)
+		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/file.c b/fs/fuse/file.c
index 97f860cfc195..cf42af492146 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -3142,7 +3142,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, struct fuse_attr *attr)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
 
@@ -3156,5 +3156,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, attr->flags);
 }
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f29018323845..0793b93d680a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1000,7 +1000,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, struct fuse_attr *attr);
 
 /**
  * Initialize inode operations on regular files and special files
@@ -1252,8 +1252,9 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, unsigned int 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);
+void fuse_dax_dontcache(struct inode *inode, bool newdax);
 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 f6b46395edb2..2ae92798126e 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -269,6 +269,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 & FUSE_ATTR_DAX);
 }
 
 static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
@@ -281,7 +284,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);
 	} else if (S_ISDIR(inode->i_mode))
 		fuse_init_dir(inode);
 	else if (S_ISLNK(inode->i_mode))
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
  */
 
 #ifndef _LINUX_FUSE_H
@@ -449,8 +452,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-file DAX mode
  */
 #define FUSE_ATTR_SUBMOUNT      (1 << 0)
+#define FUSE_ATTR_DAX		(1 << 1)
 
 /**
  * Open flags
-- 
2.27.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> this file.
> 
> When the per-file DAX flag changes for an *opened* file, the 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>

[..]
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -449,8 +452,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-file DAX mode
>   */
>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
> +#define FUSE_ATTR_DAX		(1 << 1)

Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
a separate patch. 

I am not clear on one thing. If we are planning to rely on persistent
inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
why fuse server needs to communicate the state of that attr using a 
flag? Can client directly query it?  I am not sure where at these
attrs stored and if fuse protocol currently supports it.

What about flag STATX_ATTR_DAX. We probably should report that too
in stat if we are using dax on the inode?

Vivek

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> this file.
> 
> When the per-file DAX flag changes for an *opened* file, the 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>

[..]
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -449,8 +452,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-file DAX mode
>   */
>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
> +#define FUSE_ATTR_DAX		(1 << 1)

Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
a separate patch. 

I am not clear on one thing. If we are planning to rely on persistent
inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
why fuse server needs to communicate the state of that attr using a 
flag? Can client directly query it?  I am not sure where at these
attrs stored and if fuse protocol currently supports it.

What about flag STATX_ATTR_DAX. We probably should report that too
in stat if we are using dax on the inode?

Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/20/21 2:41 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>> this file.
>>
>> When the per-file DAX flag changes for an *opened* file, the 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>
> 
> [..]
>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>>   */
>>  
>>  #ifndef _LINUX_FUSE_H
>> @@ -449,8 +452,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-file DAX mode
>>   */
>>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
>> +#define FUSE_ATTR_DAX		(1 << 1)
> 
> Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
> a separate patch. 

Got it.

> 
> I am not clear on one thing. If we are planning to rely on persistent
> inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
> why fuse server needs to communicate the state of that attr using a 
> flag? Can client directly query it?  I am not sure where at these
> attrs stored and if fuse protocol currently supports it.

There are two issues.

1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
file.

As I previously replied in [1], FUSE server must enable DAX if the
backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
previously set by FUSE client effective.

But I will argue that FUSE server also has the flexibility of the
algorithm implementation. Even if guest queries FS_XFLAG_DAX by
GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
backend file is not FS_XFLAG_DAX flagged.


2. The protocol between server and client.

extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl

As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
there will be one more round trip.


[1]
https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac0ef@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e

> 
> What about flag STATX_ATTR_DAX. We probably should report that too
> in stat if we are using dax on the inode?
> 

VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
e.g., in vfs_getattr_nosec().



-- 
Thanks,
Jeffle

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/20/21 2:41 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>> this file.
>>
>> When the per-file DAX flag changes for an *opened* file, the 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>
> 
> [..]
>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>>   */
>>  
>>  #ifndef _LINUX_FUSE_H
>> @@ -449,8 +452,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-file DAX mode
>>   */
>>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
>> +#define FUSE_ATTR_DAX		(1 << 1)
> 
> Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
> a separate patch. 

Got it.

> 
> I am not clear on one thing. If we are planning to rely on persistent
> inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
> why fuse server needs to communicate the state of that attr using a 
> flag? Can client directly query it?  I am not sure where at these
> attrs stored and if fuse protocol currently supports it.

There are two issues.

1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
file.

As I previously replied in [1], FUSE server must enable DAX if the
backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
previously set by FUSE client effective.

But I will argue that FUSE server also has the flexibility of the
algorithm implementation. Even if guest queries FS_XFLAG_DAX by
GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
backend file is not FS_XFLAG_DAX flagged.


2. The protocol between server and client.

extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl

As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
there will be one more round trip.


[1]
https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac0ef@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e

> 
> What about flag STATX_ATTR_DAX. We probably should report that too
> in stat if we are using dax on the inode?
> 

VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
e.g., in vfs_getattr_nosec().



-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Tue, Jul 20, 2021 at 03:19:50PM +0800, JeffleXu wrote:
> 
> 
> On 7/20/21 2:41 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> >> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> >> this file.
> >>
> >> When the per-file DAX flag changes for an *opened* file, the 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>
> > 
> > [..]
> >> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> >> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
> >>   */
> >>  
> >>  #ifndef _LINUX_FUSE_H
> >> @@ -449,8 +452,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-file DAX mode
> >>   */
> >>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
> >> +#define FUSE_ATTR_DAX		(1 << 1)
> > 
> > Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
> > a separate patch. 
> 
> Got it.
> 
> > 
> > I am not clear on one thing. If we are planning to rely on persistent
> > inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
> > why fuse server needs to communicate the state of that attr using a 
> > flag? Can client directly query it?  I am not sure where at these
> > attrs stored and if fuse protocol currently supports it.
> 
> There are two issues.
> 
> 1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
> file.
> 
> As I previously replied in [1], FUSE server must enable DAX if the
> backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
> previously set by FUSE client effective.
> 
> But I will argue that FUSE server also has the flexibility of the
> algorithm implementation. Even if guest queries FS_XFLAG_DAX by
> GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
> backend file is not FS_XFLAG_DAX flagged.
> 
> 
> 2. The protocol between server and client.
> 
> extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl
> 
> As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
> there will be one more round trip.
> 
> 
> [1]
> https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac0ef@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e
> 
> > 
> > What about flag STATX_ATTR_DAX. We probably should report that too
> > in stat if we are using dax on the inode?
> > 
> 
> VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
> e.g., in vfs_getattr_nosec().

Good to know. Given user will know which files are using dax and 
which ones are not, it is even more important to define semantics
properly. In what cases DAX will be driven by FS_XFLAGS_DAX attr
and in what cases DAX will completely be driven by server.

May be we should divide it in two patch series. First patch series
implements "-o dax=inode" and server follows FS_XFLAGS_DAX attr
and reports during lookup/getattr/..... 

And once that is merged this can be ehanced with "-o dax=server" where
server is free to choose what files dax should be used on. Only if
this is still needed.

Vivek

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Tue, Jul 20, 2021 at 03:19:50PM +0800, JeffleXu wrote:
> 
> 
> On 7/20/21 2:41 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> >> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> >> this file.
> >>
> >> When the per-file DAX flag changes for an *opened* file, the 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>
> > 
> > [..]
> >> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> >> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
> >>   */
> >>  
> >>  #ifndef _LINUX_FUSE_H
> >> @@ -449,8 +452,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-file DAX mode
> >>   */
> >>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
> >> +#define FUSE_ATTR_DAX		(1 << 1)
> > 
> > Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
> > a separate patch. 
> 
> Got it.
> 
> > 
> > I am not clear on one thing. If we are planning to rely on persistent
> > inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
> > why fuse server needs to communicate the state of that attr using a 
> > flag? Can client directly query it?  I am not sure where at these
> > attrs stored and if fuse protocol currently supports it.
> 
> There are two issues.
> 
> 1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
> file.
> 
> As I previously replied in [1], FUSE server must enable DAX if the
> backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
> previously set by FUSE client effective.
> 
> But I will argue that FUSE server also has the flexibility of the
> algorithm implementation. Even if guest queries FS_XFLAG_DAX by
> GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
> backend file is not FS_XFLAG_DAX flagged.
> 
> 
> 2. The protocol between server and client.
> 
> extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl
> 
> As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
> there will be one more round trip.
> 
> 
> [1]
> https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac0ef@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e
> 
> > 
> > What about flag STATX_ATTR_DAX. We probably should report that too
> > in stat if we are using dax on the inode?
> > 
> 
> VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
> e.g., in vfs_getattr_nosec().

Good to know. Given user will know which files are using dax and 
which ones are not, it is even more important to define semantics
properly. In what cases DAX will be driven by FS_XFLAGS_DAX attr
and in what cases DAX will completely be driven by server.

May be we should divide it in two patch series. First patch series
implements "-o dax=inode" and server follows FS_XFLAGS_DAX attr
and reports during lookup/getattr/..... 

And once that is merged this can be ehanced with "-o dax=server" where
server is free to choose what files dax should be used on. Only if
this is still needed.

Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/21/21 3:40 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 03:19:50PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 2:41 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>>>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>>>> this file.
>>>>
>>>> When the per-file DAX flag changes for an *opened* file, the 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>
>>>
>>> [..]
>>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>>> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>>>>   */
>>>>  
>>>>  #ifndef _LINUX_FUSE_H
>>>> @@ -449,8 +452,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-file DAX mode
>>>>   */
>>>>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
>>>> +#define FUSE_ATTR_DAX		(1 << 1)
>>>
>>> Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
>>> a separate patch. 
>>
>> Got it.
>>
>>>
>>> I am not clear on one thing. If we are planning to rely on persistent
>>> inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
>>> why fuse server needs to communicate the state of that attr using a 
>>> flag? Can client directly query it?  I am not sure where at these
>>> attrs stored and if fuse protocol currently supports it.
>>
>> There are two issues.
>>
>> 1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
>> file.
>>
>> As I previously replied in [1], FUSE server must enable DAX if the
>> backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
>> previously set by FUSE client effective.
>>
>> But I will argue that FUSE server also has the flexibility of the
>> algorithm implementation. Even if guest queries FS_XFLAG_DAX by
>> GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
>> backend file is not FS_XFLAG_DAX flagged.
>>
>>
>> 2. The protocol between server and client.
>>
>> extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl
>>
>> As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
>> there will be one more round trip.
>>
>>
>> [1]
>> https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac0ef@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e
>>
>>>
>>> What about flag STATX_ATTR_DAX. We probably should report that too
>>> in stat if we are using dax on the inode?
>>>
>>
>> VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
>> e.g., in vfs_getattr_nosec().
> 
> Good to know. Given user will know which files are using dax and 
> which ones are not, it is even more important to define semantics
> properly. In what cases DAX will be driven by FS_XFLAGS_DAX attr
> and in what cases DAX will completely be driven by server.
> 
> May be we should divide it in two patch series. First patch series
> implements "-o dax=inode" and server follows FS_XFLAGS_DAX attr
> and reports during lookup/getattr/..... 

Agreed, '-o dax=inode' and policy upon FS_XFLAG_DAX xattr could be
implemented as the first step.

> 
> And once that is merged this can be ehanced with "-o dax=server" where
> server is free to choose what files dax should be used on. Only if
> this is still needed.

I also need to discuss with my colleagues about our using case, and if
FS_XFLAG_DAX poly is enough.


-- 
Thanks,
Jeffle

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/21/21 3:40 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 03:19:50PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 2:41 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>>>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>>>> this file.
>>>>
>>>> When the per-file DAX flag changes for an *opened* file, the 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>
>>>
>>> [..]
>>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>>> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>>>>   */
>>>>  
>>>>  #ifndef _LINUX_FUSE_H
>>>> @@ -449,8 +452,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-file DAX mode
>>>>   */
>>>>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
>>>> +#define FUSE_ATTR_DAX		(1 << 1)
>>>
>>> Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
>>> a separate patch. 
>>
>> Got it.
>>
>>>
>>> I am not clear on one thing. If we are planning to rely on persistent
>>> inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
>>> why fuse server needs to communicate the state of that attr using a 
>>> flag? Can client directly query it?  I am not sure where at these
>>> attrs stored and if fuse protocol currently supports it.
>>
>> There are two issues.
>>
>> 1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
>> file.
>>
>> As I previously replied in [1], FUSE server must enable DAX if the
>> backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
>> previously set by FUSE client effective.
>>
>> But I will argue that FUSE server also has the flexibility of the
>> algorithm implementation. Even if guest queries FS_XFLAG_DAX by
>> GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
>> backend file is not FS_XFLAG_DAX flagged.
>>
>>
>> 2. The protocol between server and client.
>>
>> extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl
>>
>> As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
>> there will be one more round trip.
>>
>>
>> [1]
>> https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac0ef@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e
>>
>>>
>>> What about flag STATX_ATTR_DAX. We probably should report that too
>>> in stat if we are using dax on the inode?
>>>
>>
>> VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
>> e.g., in vfs_getattr_nosec().
> 
> Good to know. Given user will know which files are using dax and 
> which ones are not, it is even more important to define semantics
> properly. In what cases DAX will be driven by FS_XFLAGS_DAX attr
> and in what cases DAX will completely be driven by server.
> 
> May be we should divide it in two patch series. First patch series
> implements "-o dax=inode" and server follows FS_XFLAGS_DAX attr
> and reports during lookup/getattr/..... 

Agreed, '-o dax=inode' and policy upon FS_XFLAG_DAX xattr could be
implemented as the first step.

> 
> And once that is merged this can be ehanced with "-o dax=server" where
> server is free to choose what files dax should be used on. Only if
> this is still needed.

I also need to discuss with my colleagues about our using case, and if
FS_XFLAG_DAX poly is enough.


-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> this file.
> 
> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>  fs/fuse/file.c            |  4 ++--
>  fs/fuse/fuse_i.h          |  5 +++--
>  fs/fuse/inode.c           |  5 ++++-
>  include/uapi/linux/fuse.h |  5 +++++
>  5 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index a478e824c2d0..0e862119757a 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -1341,7 +1341,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);
>  	unsigned int mode;
> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>  		return false;
>  
> -	return true;
> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
> +		return true;
> +
> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
> +	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;
>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>  }
>  
> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
> +{
> +	struct fuse_conn *fc = get_fuse_conn(inode);
> +
> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
> +	    IS_DAX(inode) != newdax)
> +		d_mark_dontcache(inode);
> +}
> +

This capability to mark an inode dontcache should probably be in a
separate patch. These seem to logically two functionalities. One is
enabling DAX on an inode. And second is making sure how soon you
see the effect of that change and hence marking inode dontcache.

Not sure how useful this is. In cache=none mode we should get rid of
inode ASAP. In cache=auto mode we will get rid of after 1 second (or
after a user specified timeout). So only place this seems to be
useful is cache=always.

Vivek


>  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/file.c b/fs/fuse/file.c
> index 97f860cfc195..cf42af492146 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -3142,7 +3142,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, struct fuse_attr *attr)
>  {
>  	struct fuse_inode *fi = get_fuse_inode(inode);
>  
> @@ -3156,5 +3156,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, attr->flags);
>  }
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index f29018323845..0793b93d680a 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1000,7 +1000,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, struct fuse_attr *attr);
>  
>  /**
>   * Initialize inode operations on regular files and special files
> @@ -1252,8 +1252,9 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, unsigned int 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);
> +void fuse_dax_dontcache(struct inode *inode, bool newdax);
>  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 f6b46395edb2..2ae92798126e 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -269,6 +269,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 & FUSE_ATTR_DAX);
>  }
>  
>  static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
> @@ -281,7 +284,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);
>  	} else if (S_ISDIR(inode->i_mode))
>  		fuse_init_dir(inode);
>  	else if (S_ISLNK(inode->i_mode))
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -449,8 +452,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-file DAX mode
>   */
>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
> +#define FUSE_ATTR_DAX		(1 << 1)
>  
>  /**
>   * Open flags
> -- 
> 2.27.0
> 

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> this file.
> 
> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>  fs/fuse/file.c            |  4 ++--
>  fs/fuse/fuse_i.h          |  5 +++--
>  fs/fuse/inode.c           |  5 ++++-
>  include/uapi/linux/fuse.h |  5 +++++
>  5 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index a478e824c2d0..0e862119757a 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -1341,7 +1341,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);
>  	unsigned int mode;
> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>  		return false;
>  
> -	return true;
> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
> +		return true;
> +
> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
> +	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;
>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>  }
>  
> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
> +{
> +	struct fuse_conn *fc = get_fuse_conn(inode);
> +
> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
> +	    IS_DAX(inode) != newdax)
> +		d_mark_dontcache(inode);
> +}
> +

This capability to mark an inode dontcache should probably be in a
separate patch. These seem to logically two functionalities. One is
enabling DAX on an inode. And second is making sure how soon you
see the effect of that change and hence marking inode dontcache.

Not sure how useful this is. In cache=none mode we should get rid of
inode ASAP. In cache=auto mode we will get rid of after 1 second (or
after a user specified timeout). So only place this seems to be
useful is cache=always.

Vivek


>  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/file.c b/fs/fuse/file.c
> index 97f860cfc195..cf42af492146 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -3142,7 +3142,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, struct fuse_attr *attr)
>  {
>  	struct fuse_inode *fi = get_fuse_inode(inode);
>  
> @@ -3156,5 +3156,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, attr->flags);
>  }
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index f29018323845..0793b93d680a 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1000,7 +1000,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, struct fuse_attr *attr);
>  
>  /**
>   * Initialize inode operations on regular files and special files
> @@ -1252,8 +1252,9 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, unsigned int 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);
> +void fuse_dax_dontcache(struct inode *inode, bool newdax);
>  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 f6b46395edb2..2ae92798126e 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -269,6 +269,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 & FUSE_ATTR_DAX);
>  }
>  
>  static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
> @@ -281,7 +284,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);
>  	} else if (S_ISDIR(inode->i_mode))
>  		fuse_init_dir(inode);
>  	else if (S_ISLNK(inode->i_mode))
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 36ed092227fa..90c9df10d37a 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_ATTR_DAX
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -449,8 +452,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-file DAX mode
>   */
>  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
> +#define FUSE_ATTR_DAX		(1 << 1)
>  
>  /**
>   * Open flags
> -- 
> 2.27.0
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/20/21 3:44 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>> this file.
>>
>> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>>  fs/fuse/file.c            |  4 ++--
>>  fs/fuse/fuse_i.h          |  5 +++--
>>  fs/fuse/inode.c           |  5 ++++-
>>  include/uapi/linux/fuse.h |  5 +++++
>>  5 files changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>> index a478e824c2d0..0e862119757a 100644
>> --- a/fs/fuse/dax.c
>> +++ b/fs/fuse/dax.c
>> @@ -1341,7 +1341,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);
>>  	unsigned int mode;
>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>>  		return false;
>>  
>> -	return true;
>> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
>> +		return true;
>> +
>> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>> +	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;
>>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>>  }
>>  
>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>> +{
>> +	struct fuse_conn *fc = get_fuse_conn(inode);
>> +
>> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>> +	    IS_DAX(inode) != newdax)
>> +		d_mark_dontcache(inode);
>> +}
>> +
> 
> This capability to mark an inode dontcache should probably be in a
> separate patch. These seem to logically two functionalities. One is
> enabling DAX on an inode. And second is making sure how soon you
> see the effect of that change and hence marking inode dontcache.

OK, sounds reasonable.

> 
> Not sure how useful this is. In cache=none mode we should get rid of
> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
> after a user specified timeout). So only place this seems to be
> useful is cache=always.

Actually dontcache here is used to avoid dynamic switching between DAX
and non-DAX state while file is opened. The complexity of dynamic
switching is that, you have to clear the address_space, since page cache
and DAX entry can not coexist in the address space. Besides,
inode->a_ops also needs to be changed dynamically.

With dontcache, dynamic switching is no longer needed and the DAX state
will be decided only when inode (in memory) is initialized. The downside
is that the new DAX state won't be updated until the file is closed and
reopened later.

'cache=none' only invalidates dentry, while the inode (in memory) is
still there (with address_space uncleared and a_ops unchanged).

The dynamic switching may be done, though it's not such straightforward.
Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
new DAX state won't be seen until the file is closed and reopened later.

-- 
Thanks,
Jeffle

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/20/21 3:44 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>> this file.
>>
>> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>>  fs/fuse/file.c            |  4 ++--
>>  fs/fuse/fuse_i.h          |  5 +++--
>>  fs/fuse/inode.c           |  5 ++++-
>>  include/uapi/linux/fuse.h |  5 +++++
>>  5 files changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>> index a478e824c2d0..0e862119757a 100644
>> --- a/fs/fuse/dax.c
>> +++ b/fs/fuse/dax.c
>> @@ -1341,7 +1341,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);
>>  	unsigned int mode;
>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>>  		return false;
>>  
>> -	return true;
>> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
>> +		return true;
>> +
>> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>> +	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;
>>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>>  }
>>  
>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>> +{
>> +	struct fuse_conn *fc = get_fuse_conn(inode);
>> +
>> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>> +	    IS_DAX(inode) != newdax)
>> +		d_mark_dontcache(inode);
>> +}
>> +
> 
> This capability to mark an inode dontcache should probably be in a
> separate patch. These seem to logically two functionalities. One is
> enabling DAX on an inode. And second is making sure how soon you
> see the effect of that change and hence marking inode dontcache.

OK, sounds reasonable.

> 
> Not sure how useful this is. In cache=none mode we should get rid of
> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
> after a user specified timeout). So only place this seems to be
> useful is cache=always.

Actually dontcache here is used to avoid dynamic switching between DAX
and non-DAX state while file is opened. The complexity of dynamic
switching is that, you have to clear the address_space, since page cache
and DAX entry can not coexist in the address space. Besides,
inode->a_ops also needs to be changed dynamically.

With dontcache, dynamic switching is no longer needed and the DAX state
will be decided only when inode (in memory) is initialized. The downside
is that the new DAX state won't be updated until the file is closed and
reopened later.

'cache=none' only invalidates dentry, while the inode (in memory) is
still there (with address_space uncleared and a_ops unchanged).

The dynamic switching may be done, though it's not such straightforward.
Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
new DAX state won't be seen until the file is closed and reopened later.

-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/20/21 2:51 PM, JeffleXu wrote:
> 
> 
> On 7/20/21 3:44 AM, Vivek Goyal wrote:
>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>>> this file.
>>>
>>> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>>>  fs/fuse/file.c            |  4 ++--
>>>  fs/fuse/fuse_i.h          |  5 +++--
>>>  fs/fuse/inode.c           |  5 ++++-
>>>  include/uapi/linux/fuse.h |  5 +++++
>>>  5 files changed, 31 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>>> index a478e824c2d0..0e862119757a 100644
>>> --- a/fs/fuse/dax.c
>>> +++ b/fs/fuse/dax.c
>>> @@ -1341,7 +1341,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);
>>>  	unsigned int mode;
>>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>>>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>>>  		return false;
>>>  
>>> -	return true;
>>> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
>>> +		return true;
>>> +
>>> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>>> +	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;
>>>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>>>  }
>>>  
>>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>>> +{
>>> +	struct fuse_conn *fc = get_fuse_conn(inode);
>>> +
>>> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>>> +	    IS_DAX(inode) != newdax)
>>> +		d_mark_dontcache(inode);
>>> +}
>>> +
>>
>> This capability to mark an inode dontcache should probably be in a
>> separate patch. These seem to logically two functionalities. One is
>> enabling DAX on an inode. And second is making sure how soon you
>> see the effect of that change and hence marking inode dontcache.
> 
> OK, sounds reasonable.
> 
>>
>> Not sure how useful this is. In cache=none mode we should get rid of
>> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
>> after a user specified timeout). So only place this seems to be
>> useful is cache=always.
> 
> Actually dontcache here is used to avoid dynamic switching between DAX
> and non-DAX state while file is opened. The complexity of dynamic
> switching is that, you have to clear the address_space, since page cache
> and DAX entry can not coexist in the address space. Besides,
> inode->a_ops also needs to be changed dynamically.

FYI some context on the complication of switching DAX state dynamically,
when Ira Weiny was working on per-file DAX of ext4/xfs.

> At LSF/MM we discussed the difficulties of switching the DAX state of
a file with active mappings / page cache.  It was thought the races
could be avoided by limiting DAX state flips to 0-length files.
>
> However, this turns out to not be true.[3][5] This is because address
space operations (a_ops) may be in use at any time the inode is referenced.
> from Ira Weiny

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

> 
> With dontcache, dynamic switching is no longer needed and the DAX state
> will be decided only when inode (in memory) is initialized. The downside
> is that the new DAX state won't be updated until the file is closed and
> reopened later.
> 
> 'cache=none' only invalidates dentry, while the inode (in memory) is
> still there (with address_space uncleared and a_ops unchanged).
> 
> The dynamic switching may be done, though it's not such straightforward.
> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
> new DAX state won't be seen until the file is closed and reopened later.
> 

-- 
Thanks,
Jeffle

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/20/21 2:51 PM, JeffleXu wrote:
> 
> 
> On 7/20/21 3:44 AM, Vivek Goyal wrote:
>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>>> this file.
>>>
>>> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>>>  fs/fuse/file.c            |  4 ++--
>>>  fs/fuse/fuse_i.h          |  5 +++--
>>>  fs/fuse/inode.c           |  5 ++++-
>>>  include/uapi/linux/fuse.h |  5 +++++
>>>  5 files changed, 31 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>>> index a478e824c2d0..0e862119757a 100644
>>> --- a/fs/fuse/dax.c
>>> +++ b/fs/fuse/dax.c
>>> @@ -1341,7 +1341,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);
>>>  	unsigned int mode;
>>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>>>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>>>  		return false;
>>>  
>>> -	return true;
>>> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
>>> +		return true;
>>> +
>>> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>>> +	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;
>>>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>>>  }
>>>  
>>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>>> +{
>>> +	struct fuse_conn *fc = get_fuse_conn(inode);
>>> +
>>> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>>> +	    IS_DAX(inode) != newdax)
>>> +		d_mark_dontcache(inode);
>>> +}
>>> +
>>
>> This capability to mark an inode dontcache should probably be in a
>> separate patch. These seem to logically two functionalities. One is
>> enabling DAX on an inode. And second is making sure how soon you
>> see the effect of that change and hence marking inode dontcache.
> 
> OK, sounds reasonable.
> 
>>
>> Not sure how useful this is. In cache=none mode we should get rid of
>> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
>> after a user specified timeout). So only place this seems to be
>> useful is cache=always.
> 
> Actually dontcache here is used to avoid dynamic switching between DAX
> and non-DAX state while file is opened. The complexity of dynamic
> switching is that, you have to clear the address_space, since page cache
> and DAX entry can not coexist in the address space. Besides,
> inode->a_ops also needs to be changed dynamically.

FYI some context on the complication of switching DAX state dynamically,
when Ira Weiny was working on per-file DAX of ext4/xfs.

> At LSF/MM we discussed the difficulties of switching the DAX state of
a file with active mappings / page cache.  It was thought the races
could be avoided by limiting DAX state flips to 0-length files.
>
> However, this turns out to not be true.[3][5] This is because address
space operations (a_ops) may be in use at any time the inode is referenced.
> from Ira Weiny

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

> 
> With dontcache, dynamic switching is no longer needed and the DAX state
> will be decided only when inode (in memory) is initialized. The downside
> is that the new DAX state won't be updated until the file is closed and
> reopened later.
> 
> 'cache=none' only invalidates dentry, while the inode (in memory) is
> still there (with address_space uncleared and a_ops unchanged).
> 
> The dynamic switching may be done, though it's not such straightforward.
> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
> new DAX state won't be seen until the file is closed and reopened later.
> 

-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Tue, Jul 20, 2021 at 02:51:34PM +0800, JeffleXu wrote:
> 
> 
> On 7/20/21 3:44 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> >> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> >> this file.
> >>
> >> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
> >>  fs/fuse/file.c            |  4 ++--
> >>  fs/fuse/fuse_i.h          |  5 +++--
> >>  fs/fuse/inode.c           |  5 ++++-
> >>  include/uapi/linux/fuse.h |  5 +++++
> >>  5 files changed, 31 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> >> index a478e824c2d0..0e862119757a 100644
> >> --- a/fs/fuse/dax.c
> >> +++ b/fs/fuse/dax.c
> >> @@ -1341,7 +1341,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);
> >>  	unsigned int mode;
> >> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
> >>  	if (mode == FUSE_DAX_MOUNT_NEVER)
> >>  		return false;
> >>  
> >> -	return true;
> >> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
> >> +		return true;
> >> +
> >> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
> >> +	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;
> >>  	inode->i_data.a_ops = &fuse_dax_file_aops;
> >>  }
> >>  
> >> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
> >> +{
> >> +	struct fuse_conn *fc = get_fuse_conn(inode);
> >> +
> >> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
> >> +	    IS_DAX(inode) != newdax)
> >> +		d_mark_dontcache(inode);
> >> +}
> >> +
> > 
> > This capability to mark an inode dontcache should probably be in a
> > separate patch. These seem to logically two functionalities. One is
> > enabling DAX on an inode. And second is making sure how soon you
> > see the effect of that change and hence marking inode dontcache.
> 
> OK, sounds reasonable.
> 
> > 
> > Not sure how useful this is. In cache=none mode we should get rid of
> > inode ASAP. In cache=auto mode we will get rid of after 1 second (or
> > after a user specified timeout). So only place this seems to be
> > useful is cache=always.
> 
> Actually dontcache here is used to avoid dynamic switching between DAX
> and non-DAX state while file is opened. The complexity of dynamic
> switching is that, you have to clear the address_space, since page cache
> and DAX entry can not coexist in the address space. Besides,
> inode->a_ops also needs to be changed dynamically.
> 
> With dontcache, dynamic switching is no longer needed and the DAX state
> will be decided only when inode (in memory) is initialized. The downside
> is that the new DAX state won't be updated until the file is closed and
> reopened later.
> 
> 'cache=none' only invalidates dentry, while the inode (in memory) is
> still there (with address_space uncleared and a_ops unchanged).

Aha.., that's a good point.
> 
> The dynamic switching may be done, though it's not such straightforward.
> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
> new DAX state won't be seen until the file is closed and reopened later.

Got it. Agreed that dontcache seems reasonable if file's DAX state
has changed. Keep it in separate patch though with proper commit
logs.

Also, please copy virtiofs list (virtio-fs@redhat.com) when you post
patches next time.

Vivek

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Tue, Jul 20, 2021 at 02:51:34PM +0800, JeffleXu wrote:
> 
> 
> On 7/20/21 3:44 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> >> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> >> this file.
> >>
> >> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
> >>  fs/fuse/file.c            |  4 ++--
> >>  fs/fuse/fuse_i.h          |  5 +++--
> >>  fs/fuse/inode.c           |  5 ++++-
> >>  include/uapi/linux/fuse.h |  5 +++++
> >>  5 files changed, 31 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> >> index a478e824c2d0..0e862119757a 100644
> >> --- a/fs/fuse/dax.c
> >> +++ b/fs/fuse/dax.c
> >> @@ -1341,7 +1341,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);
> >>  	unsigned int mode;
> >> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
> >>  	if (mode == FUSE_DAX_MOUNT_NEVER)
> >>  		return false;
> >>  
> >> -	return true;
> >> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
> >> +		return true;
> >> +
> >> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
> >> +	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;
> >>  	inode->i_data.a_ops = &fuse_dax_file_aops;
> >>  }
> >>  
> >> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
> >> +{
> >> +	struct fuse_conn *fc = get_fuse_conn(inode);
> >> +
> >> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
> >> +	    IS_DAX(inode) != newdax)
> >> +		d_mark_dontcache(inode);
> >> +}
> >> +
> > 
> > This capability to mark an inode dontcache should probably be in a
> > separate patch. These seem to logically two functionalities. One is
> > enabling DAX on an inode. And second is making sure how soon you
> > see the effect of that change and hence marking inode dontcache.
> 
> OK, sounds reasonable.
> 
> > 
> > Not sure how useful this is. In cache=none mode we should get rid of
> > inode ASAP. In cache=auto mode we will get rid of after 1 second (or
> > after a user specified timeout). So only place this seems to be
> > useful is cache=always.
> 
> Actually dontcache here is used to avoid dynamic switching between DAX
> and non-DAX state while file is opened. The complexity of dynamic
> switching is that, you have to clear the address_space, since page cache
> and DAX entry can not coexist in the address space. Besides,
> inode->a_ops also needs to be changed dynamically.
> 
> With dontcache, dynamic switching is no longer needed and the DAX state
> will be decided only when inode (in memory) is initialized. The downside
> is that the new DAX state won't be updated until the file is closed and
> reopened later.
> 
> 'cache=none' only invalidates dentry, while the inode (in memory) is
> still there (with address_space uncleared and a_ops unchanged).

Aha.., that's a good point.
> 
> The dynamic switching may be done, though it's not such straightforward.
> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
> new DAX state won't be seen until the file is closed and reopened later.

Got it. Agreed that dontcache seems reasonable if file's DAX state
has changed. Keep it in separate patch though with proper commit
logs.

Also, please copy virtiofs list (virtio-fs@redhat.com) when you post
patches next time.

Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/21/21 3:27 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 02:51:34PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 3:44 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>>>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>>>> this file.
>>>>
>>>> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>>>>  fs/fuse/file.c            |  4 ++--
>>>>  fs/fuse/fuse_i.h          |  5 +++--
>>>>  fs/fuse/inode.c           |  5 ++++-
>>>>  include/uapi/linux/fuse.h |  5 +++++
>>>>  5 files changed, 31 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>>>> index a478e824c2d0..0e862119757a 100644
>>>> --- a/fs/fuse/dax.c
>>>> +++ b/fs/fuse/dax.c
>>>> @@ -1341,7 +1341,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);
>>>>  	unsigned int mode;
>>>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>>>>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>>>>  		return false;
>>>>  
>>>> -	return true;
>>>> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
>>>> +		return true;
>>>> +
>>>> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>>>> +	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;
>>>>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>>>>  }
>>>>  
>>>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>>>> +{
>>>> +	struct fuse_conn *fc = get_fuse_conn(inode);
>>>> +
>>>> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>>>> +	    IS_DAX(inode) != newdax)
>>>> +		d_mark_dontcache(inode);
>>>> +}
>>>> +
>>>
>>> This capability to mark an inode dontcache should probably be in a
>>> separate patch. These seem to logically two functionalities. One is
>>> enabling DAX on an inode. And second is making sure how soon you
>>> see the effect of that change and hence marking inode dontcache.
>>
>> OK, sounds reasonable.
>>
>>>
>>> Not sure how useful this is. In cache=none mode we should get rid of
>>> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
>>> after a user specified timeout). So only place this seems to be
>>> useful is cache=always.
>>
>> Actually dontcache here is used to avoid dynamic switching between DAX
>> and non-DAX state while file is opened. The complexity of dynamic
>> switching is that, you have to clear the address_space, since page cache
>> and DAX entry can not coexist in the address space. Besides,
>> inode->a_ops also needs to be changed dynamically.
>>
>> With dontcache, dynamic switching is no longer needed and the DAX state
>> will be decided only when inode (in memory) is initialized. The downside
>> is that the new DAX state won't be updated until the file is closed and
>> reopened later.
>>
>> 'cache=none' only invalidates dentry, while the inode (in memory) is
>> still there (with address_space uncleared and a_ops unchanged).
> 
> Aha.., that's a good point.
>>
>> The dynamic switching may be done, though it's not such straightforward.
>> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
>> new DAX state won't be seen until the file is closed and reopened later.
> 
> Got it. Agreed that dontcache seems reasonable if file's DAX state
> has changed. Keep it in separate patch though with proper commit
> logs.
> 
> Also, please copy virtiofs list (virtio-fs@redhat.com) when you post
> patches next time.
> 

Got it. By the way, what's the git repository of virtiofsd? AFAIK,
virtiofsd included in qemu (git@github.com:qemu/qemu.git) doesn't
support DAX yet?

-- 
Thanks,
Jeffle

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[JeffleXu]

On 7/21/21 3:27 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 02:51:34PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 3:44 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>>>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>>>> this file.
>>>>
>>>> When the per-file DAX flag changes for an *opened* file, the 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             | 21 +++++++++++++++++----
>>>>  fs/fuse/file.c            |  4 ++--
>>>>  fs/fuse/fuse_i.h          |  5 +++--
>>>>  fs/fuse/inode.c           |  5 ++++-
>>>>  include/uapi/linux/fuse.h |  5 +++++
>>>>  5 files changed, 31 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>>>> index a478e824c2d0..0e862119757a 100644
>>>> --- a/fs/fuse/dax.c
>>>> +++ b/fs/fuse/dax.c
>>>> @@ -1341,7 +1341,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);
>>>>  	unsigned int mode;
>>>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
>>>>  	if (mode == FUSE_DAX_MOUNT_NEVER)
>>>>  		return false;
>>>>  
>>>> -	return true;
>>>> +	if (mode == FUSE_DAX_MOUNT_ALWAYS)
>>>> +		return true;
>>>> +
>>>> +	WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>>>> +	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;
>>>>  	inode->i_data.a_ops = &fuse_dax_file_aops;
>>>>  }
>>>>  
>>>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>>>> +{
>>>> +	struct fuse_conn *fc = get_fuse_conn(inode);
>>>> +
>>>> +	if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>>>> +	    IS_DAX(inode) != newdax)
>>>> +		d_mark_dontcache(inode);
>>>> +}
>>>> +
>>>
>>> This capability to mark an inode dontcache should probably be in a
>>> separate patch. These seem to logically two functionalities. One is
>>> enabling DAX on an inode. And second is making sure how soon you
>>> see the effect of that change and hence marking inode dontcache.
>>
>> OK, sounds reasonable.
>>
>>>
>>> Not sure how useful this is. In cache=none mode we should get rid of
>>> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
>>> after a user specified timeout). So only place this seems to be
>>> useful is cache=always.
>>
>> Actually dontcache here is used to avoid dynamic switching between DAX
>> and non-DAX state while file is opened. The complexity of dynamic
>> switching is that, you have to clear the address_space, since page cache
>> and DAX entry can not coexist in the address space. Besides,
>> inode->a_ops also needs to be changed dynamically.
>>
>> With dontcache, dynamic switching is no longer needed and the DAX state
>> will be decided only when inode (in memory) is initialized. The downside
>> is that the new DAX state won't be updated until the file is closed and
>> reopened later.
>>
>> 'cache=none' only invalidates dentry, while the inode (in memory) is
>> still there (with address_space uncleared and a_ops unchanged).
> 
> Aha.., that's a good point.
>>
>> The dynamic switching may be done, though it's not such straightforward.
>> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
>> new DAX state won't be seen until the file is closed and reopened later.
> 
> Got it. Agreed that dontcache seems reasonable if file's DAX state
> has changed. Keep it in separate patch though with proper commit
> logs.
> 
> Also, please copy virtiofs list (virtio-fs@redhat.com) when you post
> patches next time.
> 

Got it. By the way, what's the git repository of virtiofsd? AFAIK,
virtiofsd included in qemu (git@github.com:qemu/qemu.git) doesn't
support DAX yet?

-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Wed, Jul 21, 2021 at 10:14:44PM +0800, JeffleXu wrote:
[..]
> > Also, please copy virtiofs list (virtio-fs@redhat.com) when you post
> > patches next time.
> > 
> 
> Got it. By the way, what's the git repository of virtiofsd? AFAIK,
> virtiofsd included in qemu (git@github.com:qemu/qemu.git) doesn't
> support DAX yet?

Yes virtiofsd got merged in qemu upstream. And it does not support dax
yet. David is still sorting out couple of issues based on feedback. I
think following is the branch where he had pushed his latest patches.

https://gitlab.com/virtio-fs/qemu/-/tree/virtio-fs-dev

David, please correct me if that's not the case.

Vivek

Re: [PATCH v2 3/4] fuse: add per-file DAX flag

[Vivek Goyal]

On Wed, Jul 21, 2021 at 10:14:44PM +0800, JeffleXu wrote:
[..]
> > Also, please copy virtiofs list (virtio-fs@redhat.com) when you post
> > patches next time.
> > 
> 
> Got it. By the way, what's the git repository of virtiofsd? AFAIK,
> virtiofsd included in qemu (git@github.com:qemu/qemu.git) doesn't
> support DAX yet?

Yes virtiofsd got merged in qemu upstream. And it does not support dax
yet. David is still sorting out couple of issues based on feedback. I
think following is the branch where he had pushed his latest patches.

https://gitlab.com/virtio-fs/qemu/-/tree/virtio-fs-dev

David, please correct me if that's not the case.

Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest

[Jeffle Xu]

Fuse client can enable or disable per-file DAX inside guest by
chattr(1). Similarly the new state won't be updated until the file is
closed and reopened later.

It is worth nothing that it is a best-effort style, since whether
per-file DAX is enabled or not is controlled by fuse_attr.flags retrieved
by FUSE LOOKUP routine, while the algorithm constructing fuse_attr.flags
is totally fuse server specific, not to mention ioctl may not be
supported by fuse server at all.

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

diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
index 546ea3d58fb4..172e05c3f038 100644
--- a/fs/fuse/ioctl.c
+++ b/fs/fuse/ioctl.c
@@ -460,6 +460,7 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
 	struct fuse_file *ff;
 	unsigned int flags = fa->flags;
 	struct fsxattr xfa;
+	bool newdax;
 	int err;
 
 	ff = fuse_priv_ioctl_prepare(inode);
@@ -467,10 +468,9 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
 		return PTR_ERR(ff);
 
 	if (fa->flags_valid) {
+		newdax = flags & FS_DAX_FL;
 		err = fuse_priv_ioctl(inode, ff, FS_IOC_SETFLAGS,
 				      &flags, sizeof(flags));
-		if (err)
-			goto cleanup;
 	} else {
 		memset(&xfa, 0, sizeof(xfa));
 		xfa.fsx_xflags = fa->fsx_xflags;
@@ -479,11 +479,14 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
 		xfa.fsx_projid = fa->fsx_projid;
 		xfa.fsx_cowextsize = fa->fsx_cowextsize;
 
+		newdax = fa->fsx_xflags & FS_XFLAG_DAX;
 		err = fuse_priv_ioctl(inode, ff, FS_IOC_FSSETXATTR,
 				      &xfa, sizeof(xfa));
 	}
 
-cleanup:
+	if (!err && IS_ENABLED(CONFIG_FUSE_DAX))
+		fuse_dax_dontcache(inode, newdax);
+
 	fuse_priv_ioctl_cleanup(inode, ff);
 
 	return err;
-- 
2.27.0

[PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest

[Jeffle Xu]

Fuse client can enable or disable per-file DAX inside guest by
chattr(1). Similarly the new state won't be updated until the file is
closed and reopened later.

It is worth nothing that it is a best-effort style, since whether
per-file DAX is enabled or not is controlled by fuse_attr.flags retrieved
by FUSE LOOKUP routine, while the algorithm constructing fuse_attr.flags
is totally fuse server specific, not to mention ioctl may not be
supported by fuse server at all.

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

diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
index 546ea3d58fb4..172e05c3f038 100644
--- a/fs/fuse/ioctl.c
+++ b/fs/fuse/ioctl.c
@@ -460,6 +460,7 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
 	struct fuse_file *ff;
 	unsigned int flags = fa->flags;
 	struct fsxattr xfa;
+	bool newdax;
 	int err;
 
 	ff = fuse_priv_ioctl_prepare(inode);
@@ -467,10 +468,9 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
 		return PTR_ERR(ff);
 
 	if (fa->flags_valid) {
+		newdax = flags & FS_DAX_FL;
 		err = fuse_priv_ioctl(inode, ff, FS_IOC_SETFLAGS,
 				      &flags, sizeof(flags));
-		if (err)
-			goto cleanup;
 	} else {
 		memset(&xfa, 0, sizeof(xfa));
 		xfa.fsx_xflags = fa->fsx_xflags;
@@ -479,11 +479,14 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
 		xfa.fsx_projid = fa->fsx_projid;
 		xfa.fsx_cowextsize = fa->fsx_cowextsize;
 
+		newdax = fa->fsx_xflags & FS_XFLAG_DAX;
 		err = fuse_priv_ioctl(inode, ff, FS_IOC_FSSETXATTR,
 				      &xfa, sizeof(xfa));
 	}
 
-cleanup:
+	if (!err && IS_ENABLED(CONFIG_FUSE_DAX))
+		fuse_dax_dontcache(inode, newdax);
+
 	fuse_priv_ioctl_cleanup(inode, ff);
 
 	return err;
-- 
2.27.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:53PM +0800, Jeffle Xu wrote:
> Fuse client can enable or disable per-file DAX inside guest by
> chattr(1). Similarly the new state won't be updated until the file is
> closed and reopened later.
> 
> It is worth nothing that it is a best-effort style, since whether
> per-file DAX is enabled or not is controlled by fuse_attr.flags retrieved
> by FUSE LOOKUP routine, while the algorithm constructing fuse_attr.flags
> is totally fuse server specific, not to mention ioctl may not be
> supported by fuse server at all.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/ioctl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
> index 546ea3d58fb4..172e05c3f038 100644
> --- a/fs/fuse/ioctl.c
> +++ b/fs/fuse/ioctl.c
> @@ -460,6 +460,7 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
>  	struct fuse_file *ff;
>  	unsigned int flags = fa->flags;
>  	struct fsxattr xfa;
> +	bool newdax;
>  	int err;
>  
>  	ff = fuse_priv_ioctl_prepare(inode);
> @@ -467,10 +468,9 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
>  		return PTR_ERR(ff);
>  
>  	if (fa->flags_valid) {
> +		newdax = flags & FS_DAX_FL;
>  		err = fuse_priv_ioctl(inode, ff, FS_IOC_SETFLAGS,
>  				      &flags, sizeof(flags));
> -		if (err)
> -			goto cleanup;
>  	} else {
>  		memset(&xfa, 0, sizeof(xfa));
>  		xfa.fsx_xflags = fa->fsx_xflags;
> @@ -479,11 +479,14 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
>  		xfa.fsx_projid = fa->fsx_projid;
>  		xfa.fsx_cowextsize = fa->fsx_cowextsize;
>  
> +		newdax = fa->fsx_xflags & FS_XFLAG_DAX;
>  		err = fuse_priv_ioctl(inode, ff, FS_IOC_FSSETXATTR,
>  				      &xfa, sizeof(xfa));
>  	}
>  
> -cleanup:
> +	if (!err && IS_ENABLED(CONFIG_FUSE_DAX))
> +		fuse_dax_dontcache(inode, newdax);

This assumes that server will set ATTR_DAX flag for inode based on
whether inode has FS_DAX_FL set or not.

So that means server first will have to know that client has DAX enabled
so that it can query FS_DAX_FL. And in current framework we don't have
a way for server to know if client is using DAX or not.

I think there is little disconnect here. So either client should be
checking FS_DAX_FL flag on inode. But we probably don't want to pay
extra round trip cost for this. 

That means somehow server should return this information as part of
inode attrs only if client wants this extra file attr informaiton. So
may be GETATTR should be enhanced instead to return file attr information
too if client asked for it?

I have not looked what it takes to implement this. If this is too 
complicated, then alternate approach will be that it is up to the
server to decide what inodes should use DAX and there is no guarantee
that server will make sue of FS_DAX_FL flag. fuse will still support
setting FS_DAX_FL but server could choose to not use it at all. In
that case fuse client will not have to query file attrs in GETATTR
and just rely on ATTR_DAX flag set by server. I think that's what
you are implementing.  If that's the case then dontcache does not make
much sense because you don't even know if server is looking at
FS_DAX_FL to decide whether DAX should be used or not.

Thanks
Vivek

> +
>  	fuse_priv_ioctl_cleanup(inode, ff);
>  
>  	return err;
> -- 
> 2.27.0
> 

Re: [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:53PM +0800, Jeffle Xu wrote:
> Fuse client can enable or disable per-file DAX inside guest by
> chattr(1). Similarly the new state won't be updated until the file is
> closed and reopened later.
> 
> It is worth nothing that it is a best-effort style, since whether
> per-file DAX is enabled or not is controlled by fuse_attr.flags retrieved
> by FUSE LOOKUP routine, while the algorithm constructing fuse_attr.flags
> is totally fuse server specific, not to mention ioctl may not be
> supported by fuse server at all.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/ioctl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
> index 546ea3d58fb4..172e05c3f038 100644
> --- a/fs/fuse/ioctl.c
> +++ b/fs/fuse/ioctl.c
> @@ -460,6 +460,7 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
>  	struct fuse_file *ff;
>  	unsigned int flags = fa->flags;
>  	struct fsxattr xfa;
> +	bool newdax;
>  	int err;
>  
>  	ff = fuse_priv_ioctl_prepare(inode);
> @@ -467,10 +468,9 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
>  		return PTR_ERR(ff);
>  
>  	if (fa->flags_valid) {
> +		newdax = flags & FS_DAX_FL;
>  		err = fuse_priv_ioctl(inode, ff, FS_IOC_SETFLAGS,
>  				      &flags, sizeof(flags));
> -		if (err)
> -			goto cleanup;
>  	} else {
>  		memset(&xfa, 0, sizeof(xfa));
>  		xfa.fsx_xflags = fa->fsx_xflags;
> @@ -479,11 +479,14 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
>  		xfa.fsx_projid = fa->fsx_projid;
>  		xfa.fsx_cowextsize = fa->fsx_cowextsize;
>  
> +		newdax = fa->fsx_xflags & FS_XFLAG_DAX;
>  		err = fuse_priv_ioctl(inode, ff, FS_IOC_FSSETXATTR,
>  				      &xfa, sizeof(xfa));
>  	}
>  
> -cleanup:
> +	if (!err && IS_ENABLED(CONFIG_FUSE_DAX))
> +		fuse_dax_dontcache(inode, newdax);

This assumes that server will set ATTR_DAX flag for inode based on
whether inode has FS_DAX_FL set or not.

So that means server first will have to know that client has DAX enabled
so that it can query FS_DAX_FL. And in current framework we don't have
a way for server to know if client is using DAX or not.

I think there is little disconnect here. So either client should be
checking FS_DAX_FL flag on inode. But we probably don't want to pay
extra round trip cost for this. 

That means somehow server should return this information as part of
inode attrs only if client wants this extra file attr informaiton. So
may be GETATTR should be enhanced instead to return file attr information
too if client asked for it?

I have not looked what it takes to implement this. If this is too 
complicated, then alternate approach will be that it is up to the
server to decide what inodes should use DAX and there is no guarantee
that server will make sue of FS_DAX_FL flag. fuse will still support
setting FS_DAX_FL but server could choose to not use it at all. In
that case fuse client will not have to query file attrs in GETATTR
and just rely on ATTR_DAX flag set by server. I think that's what
you are implementing.  If that's the case then dontcache does not make
much sense because you don't even know if server is looking at
FS_DAX_FL to decide whether DAX should be used or not.

Thanks
Vivek

> +
>  	fuse_priv_ioctl_cleanup(inode, ff);
>  
>  	return err;
> -- 
> 2.27.0
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
> This patchset adds support of per-file DAX for virtiofs, which is
> inspired by Ira Weiny's work on ext4[1] and xfs[2].
> 
> There are three related scenarios:
> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
> In this case, the dentry and inode are all marked as DONT_CACHE, and
> the DAX state won't be updated until the file is closed and reopened
> later.
> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
> (patch 4)
> 4. Create new files under directories with DAX enabled. When creating
> new files in ext4/xfs on host, the new created files will inherit the
> per-file DAX flag from the directory, and thus the new created files in
> virtiofs will also inherit the per-file DAX flag if the fuse server
> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
> DAX flag.

Thinking little bit more about this from requirement perspective. I think
we are trying to address two use cases here.

A. Client does not know which files DAX should be used on. Only server
   knows it and server passes this information to client. I suspect
   that's your primary use case.

B. Client is driving which files are supposed to be using DAX. This is
   exactly same as the model ext4/xfs are using by storing a persistent
   flag on inode. 

Current patches seem to be a hybrid of both approach A and B. 

If we were to implement B, then fuse client probably needs to have the
capability to query FS_XFLAG_DAX on inode and decide whether to
enable DAX or not. (Without extra round trip). Or know it indirectly
by extending GETATTR and requesting this explicitly.

If we were only implementing A, then server does not have a way to 
tell client to enable DAX. Server can either look at FS_XFLAG_DAX
and decide to enable DAX or use some other property. Given querying
FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
it probably will be a server option. But enabling on server does not
mean client will enable it.

I think my primary concern with this patch right now is trying to
figure out which requirement we are trying to cater to first and how
to connect server and client well so they both understand what mode
they are operating in and interact well.

Vivek

> 
> 
> 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")
> 
> 
> changes since v1:
> - add support for changing per-file DAX flags inside guest (patch 4)
> 
> v1:https://www.spinics.net/lists/linux-virtualization/msg51008.html
> 
> Jeffle Xu (4):
>   fuse: add fuse_should_enable_dax() helper
>   fuse: Make DAX mount option a tri-state
>   fuse: add per-file DAX flag
>   fuse: support changing per-file DAX flag inside guest
> 
>  fs/fuse/dax.c             | 36 ++++++++++++++++++++++++++++++++++--
>  fs/fuse/file.c            |  4 ++--
>  fs/fuse/fuse_i.h          | 16 ++++++++++++----
>  fs/fuse/inode.c           |  7 +++++--
>  fs/fuse/ioctl.c           |  9 ++++++---
>  fs/fuse/virtio_fs.c       | 16 ++++++++++++++--
>  include/uapi/linux/fuse.h |  5 +++++
>  7 files changed, 78 insertions(+), 15 deletions(-)
> 
> -- 
> 2.27.0
> 

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
> This patchset adds support of per-file DAX for virtiofs, which is
> inspired by Ira Weiny's work on ext4[1] and xfs[2].
> 
> There are three related scenarios:
> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
> In this case, the dentry and inode are all marked as DONT_CACHE, and
> the DAX state won't be updated until the file is closed and reopened
> later.
> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
> (patch 4)
> 4. Create new files under directories with DAX enabled. When creating
> new files in ext4/xfs on host, the new created files will inherit the
> per-file DAX flag from the directory, and thus the new created files in
> virtiofs will also inherit the per-file DAX flag if the fuse server
> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
> DAX flag.

Thinking little bit more about this from requirement perspective. I think
we are trying to address two use cases here.

A. Client does not know which files DAX should be used on. Only server
   knows it and server passes this information to client. I suspect
   that's your primary use case.

B. Client is driving which files are supposed to be using DAX. This is
   exactly same as the model ext4/xfs are using by storing a persistent
   flag on inode. 

Current patches seem to be a hybrid of both approach A and B. 

If we were to implement B, then fuse client probably needs to have the
capability to query FS_XFLAG_DAX on inode and decide whether to
enable DAX or not. (Without extra round trip). Or know it indirectly
by extending GETATTR and requesting this explicitly.

If we were only implementing A, then server does not have a way to 
tell client to enable DAX. Server can either look at FS_XFLAG_DAX
and decide to enable DAX or use some other property. Given querying
FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
it probably will be a server option. But enabling on server does not
mean client will enable it.

I think my primary concern with this patch right now is trying to
figure out which requirement we are trying to cater to first and how
to connect server and client well so they both understand what mode
they are operating in and interact well.

Vivek

> 
> 
> 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")
> 
> 
> changes since v1:
> - add support for changing per-file DAX flags inside guest (patch 4)
> 
> v1:https://www.spinics.net/lists/linux-virtualization/msg51008.html
> 
> Jeffle Xu (4):
>   fuse: add fuse_should_enable_dax() helper
>   fuse: Make DAX mount option a tri-state
>   fuse: add per-file DAX flag
>   fuse: support changing per-file DAX flag inside guest
> 
>  fs/fuse/dax.c             | 36 ++++++++++++++++++++++++++++++++++--
>  fs/fuse/file.c            |  4 ++--
>  fs/fuse/fuse_i.h          | 16 ++++++++++++----
>  fs/fuse/inode.c           |  7 +++++--
>  fs/fuse/ioctl.c           |  9 ++++++---
>  fs/fuse/virtio_fs.c       | 16 ++++++++++++++--
>  include/uapi/linux/fuse.h |  5 +++++
>  7 files changed, 78 insertions(+), 15 deletions(-)
> 
> -- 
> 2.27.0
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[JeffleXu]

On 7/20/21 5:30 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
>> This patchset adds support of per-file DAX for virtiofs, which is
>> inspired by Ira Weiny's work on ext4[1] and xfs[2].
>>
>> There are three related scenarios:
>> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
>> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
>> In this case, the dentry and inode are all marked as DONT_CACHE, and
>> the DAX state won't be updated until the file is closed and reopened
>> later.
>> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
>> (patch 4)
>> 4. Create new files under directories with DAX enabled. When creating
>> new files in ext4/xfs on host, the new created files will inherit the
>> per-file DAX flag from the directory, and thus the new created files in
>> virtiofs will also inherit the per-file DAX flag if the fuse server
>> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
>> DAX flag.
> 
> Thinking little bit more about this from requirement perspective. I think
> we are trying to address two use cases here.
> 
> A. Client does not know which files DAX should be used on. Only server
>    knows it and server passes this information to client. I suspect
>    that's your primary use case.

Yes, this is the starting point of this patch set.

> 
> B. Client is driving which files are supposed to be using DAX. This is
>    exactly same as the model ext4/xfs are using by storing a persistent
>    flag on inode. 
> 
> Current patches seem to be a hybrid of both approach A and B. 
> 
> If we were to implement B, then fuse client probably needs to have the
> capability to query FS_XFLAG_DAX on inode and decide whether to
> enable DAX or not. (Without extra round trip). Or know it indirectly
> by extending GETATTR and requesting this explicitly.

If guest queries if the file is DAX capable or not by an extra GETATTR,
I'm afraid this will add extra round trip.

Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
this patch set, then the FUSE server needs to set ATTR_DAX according to
the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
previously set by FUSE client work*. Then it becomes a *mandatory*
requirement when implementing FUSE server. i.e., it becomes part of the
FUSE protocol rather than implementation specific. FUSE server can still
implement some other algorithm deciding whether to set ATTR_DAX or not,
though it must set ATTR_DAX once the backend file is flagged with
FS_XFLAG_DAX.

Besides, as you said, FUSE server needs to add one extra
GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
cost, we could negotiate the per-file DAX capability during FUSE_INIT.
Only when the per-file DAX capability is negotiated, will the FUSE
server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
doing LOOKUP.


Personally, I prefer the second way, i.e., by extending LOOKUP (adding
ATTR_DAX), to eliminate the extra round trip.

> 
> If we were only implementing A, then server does not have a way to 
> tell client to enable DAX. Server can either look at FS_XFLAG_DAX
> and decide to enable DAX or use some other property. Given querying
> FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
> it probably will be a server option. But enabling on server does not
> mean client will enable it.

As I said previously, it can be negotiated whether this per-file DAX
capability is needed. Guest can advertise this capability when '-o
dax=inode' is configured.

> 
> I think my primary concern with this patch right now is trying to
> figure out which requirement we are trying to cater to first and how
> to connect server and client well so they both understand what mode
> they are operating in and interact well.
> 


-- 
Thanks,
Jeffle

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[JeffleXu]

On 7/20/21 5:30 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
>> This patchset adds support of per-file DAX for virtiofs, which is
>> inspired by Ira Weiny's work on ext4[1] and xfs[2].
>>
>> There are three related scenarios:
>> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
>> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
>> In this case, the dentry and inode are all marked as DONT_CACHE, and
>> the DAX state won't be updated until the file is closed and reopened
>> later.
>> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
>> (patch 4)
>> 4. Create new files under directories with DAX enabled. When creating
>> new files in ext4/xfs on host, the new created files will inherit the
>> per-file DAX flag from the directory, and thus the new created files in
>> virtiofs will also inherit the per-file DAX flag if the fuse server
>> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
>> DAX flag.
> 
> Thinking little bit more about this from requirement perspective. I think
> we are trying to address two use cases here.
> 
> A. Client does not know which files DAX should be used on. Only server
>    knows it and server passes this information to client. I suspect
>    that's your primary use case.

Yes, this is the starting point of this patch set.

> 
> B. Client is driving which files are supposed to be using DAX. This is
>    exactly same as the model ext4/xfs are using by storing a persistent
>    flag on inode. 
> 
> Current patches seem to be a hybrid of both approach A and B. 
> 
> If we were to implement B, then fuse client probably needs to have the
> capability to query FS_XFLAG_DAX on inode and decide whether to
> enable DAX or not. (Without extra round trip). Or know it indirectly
> by extending GETATTR and requesting this explicitly.

If guest queries if the file is DAX capable or not by an extra GETATTR,
I'm afraid this will add extra round trip.

Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
this patch set, then the FUSE server needs to set ATTR_DAX according to
the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
previously set by FUSE client work*. Then it becomes a *mandatory*
requirement when implementing FUSE server. i.e., it becomes part of the
FUSE protocol rather than implementation specific. FUSE server can still
implement some other algorithm deciding whether to set ATTR_DAX or not,
though it must set ATTR_DAX once the backend file is flagged with
FS_XFLAG_DAX.

Besides, as you said, FUSE server needs to add one extra
GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
cost, we could negotiate the per-file DAX capability during FUSE_INIT.
Only when the per-file DAX capability is negotiated, will the FUSE
server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
doing LOOKUP.


Personally, I prefer the second way, i.e., by extending LOOKUP (adding
ATTR_DAX), to eliminate the extra round trip.

> 
> If we were only implementing A, then server does not have a way to 
> tell client to enable DAX. Server can either look at FS_XFLAG_DAX
> and decide to enable DAX or use some other property. Given querying
> FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
> it probably will be a server option. But enabling on server does not
> mean client will enable it.

As I said previously, it can be negotiated whether this per-file DAX
capability is needed. Guest can advertise this capability when '-o
dax=inode' is configured.

> 
> I think my primary concern with this patch right now is trying to
> figure out which requirement we are trying to cater to first and how
> to connect server and client well so they both understand what mode
> they are operating in and interact well.
> 


-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Tue, Jul 20, 2021 at 01:25:11PM +0800, JeffleXu wrote:
> 
> 
> On 7/20/21 5:30 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
> >> This patchset adds support of per-file DAX for virtiofs, which is
> >> inspired by Ira Weiny's work on ext4[1] and xfs[2].
> >>
> >> There are three related scenarios:
> >> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
> >> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
> >> In this case, the dentry and inode are all marked as DONT_CACHE, and
> >> the DAX state won't be updated until the file is closed and reopened
> >> later.
> >> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
> >> (patch 4)
> >> 4. Create new files under directories with DAX enabled. When creating
> >> new files in ext4/xfs on host, the new created files will inherit the
> >> per-file DAX flag from the directory, and thus the new created files in
> >> virtiofs will also inherit the per-file DAX flag if the fuse server
> >> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
> >> DAX flag.
> > 
> > Thinking little bit more about this from requirement perspective. I think
> > we are trying to address two use cases here.
> > 
> > A. Client does not know which files DAX should be used on. Only server
> >    knows it and server passes this information to client. I suspect
> >    that's your primary use case.
> 
> Yes, this is the starting point of this patch set.
> 
> > 
> > B. Client is driving which files are supposed to be using DAX. This is
> >    exactly same as the model ext4/xfs are using by storing a persistent
> >    flag on inode. 
> > 
> > Current patches seem to be a hybrid of both approach A and B. 
> > 
> > If we were to implement B, then fuse client probably needs to have the
> > capability to query FS_XFLAG_DAX on inode and decide whether to
> > enable DAX or not. (Without extra round trip). Or know it indirectly
> > by extending GETATTR and requesting this explicitly.
> 
> If guest queries if the file is DAX capable or not by an extra GETATTR,
> I'm afraid this will add extra round trip.
> 
> Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
> this patch set, then the FUSE server needs to set ATTR_DAX according to
> the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
> previously set by FUSE client work*. Then it becomes a *mandatory*
> requirement when implementing FUSE server. i.e., it becomes part of the
> FUSE protocol rather than implementation specific. FUSE server can still
> implement some other algorithm deciding whether to set ATTR_DAX or not,
> though it must set ATTR_DAX once the backend file is flagged with
> FS_XFLAG_DAX.
> 
> Besides, as you said, FUSE server needs to add one extra
> GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
> cost, we could negotiate the per-file DAX capability during FUSE_INIT.
> Only when the per-file DAX capability is negotiated, will the FUSE
> server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
> doing LOOKUP.
> 
> 
> Personally, I prefer the second way, i.e., by extending LOOKUP (adding
> ATTR_DAX), to eliminate the extra round trip.

Negotiating a fuse feature say FUSE_FS_XFLAG_DAX makes sense. If
client is mounted with "-o dax=inode", then client will negotitate
this feature and if server does not support it, mount can fail.

But this probably will be binding on server that it needs to return
the state of FS_XFLAG_DAX attr on inode upon lookup/getattr. I don't
this will allow server to implement its own separate policy which
does not follow FS_XFLAG_DAX xattr. 

IOW, I don't think server can choose to implement its own policy
for enabling dax for "-o dax=inode".

If there is really a need to for something new where server needs
to dynamically decide which inodes should use dax (and not use
FS_XFLAG_FS), I guess that probably should be a separate mount
option say "-o dax=server" and it negotiates a separate feature
say FUSE_DAX_SERVER. Once that's negotiated, now both client
and server know that DAX will be used on files as determined
by server and not necessarily by using file attr FS_XFLAG_DAX.

So is "dax=inode" enough for your needs? What's your requirement,
can you give little bit of more details.

Thanks
Vivek

> 
> > 
> > If we were only implementing A, then server does not have a way to 
> > tell client to enable DAX. Server can either look at FS_XFLAG_DAX
> > and decide to enable DAX or use some other property. Given querying
> > FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
> > it probably will be a server option. But enabling on server does not
> > mean client will enable it.
> 
> As I said previously, it can be negotiated whether this per-file DAX
> capability is needed. Guest can advertise this capability when '-o
> dax=inode' is configured.
> 
> > 
> > I think my primary concern with this patch right now is trying to
> > figure out which requirement we are trying to cater to first and how
> > to connect server and client well so they both understand what mode
> > they are operating in and interact well.
> > 
> 
> 
> -- 
> Thanks,
> Jeffle
> 

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Tue, Jul 20, 2021 at 01:25:11PM +0800, JeffleXu wrote:
> 
> 
> On 7/20/21 5:30 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
> >> This patchset adds support of per-file DAX for virtiofs, which is
> >> inspired by Ira Weiny's work on ext4[1] and xfs[2].
> >>
> >> There are three related scenarios:
> >> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
> >> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
> >> In this case, the dentry and inode are all marked as DONT_CACHE, and
> >> the DAX state won't be updated until the file is closed and reopened
> >> later.
> >> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
> >> (patch 4)
> >> 4. Create new files under directories with DAX enabled. When creating
> >> new files in ext4/xfs on host, the new created files will inherit the
> >> per-file DAX flag from the directory, and thus the new created files in
> >> virtiofs will also inherit the per-file DAX flag if the fuse server
> >> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
> >> DAX flag.
> > 
> > Thinking little bit more about this from requirement perspective. I think
> > we are trying to address two use cases here.
> > 
> > A. Client does not know which files DAX should be used on. Only server
> >    knows it and server passes this information to client. I suspect
> >    that's your primary use case.
> 
> Yes, this is the starting point of this patch set.
> 
> > 
> > B. Client is driving which files are supposed to be using DAX. This is
> >    exactly same as the model ext4/xfs are using by storing a persistent
> >    flag on inode. 
> > 
> > Current patches seem to be a hybrid of both approach A and B. 
> > 
> > If we were to implement B, then fuse client probably needs to have the
> > capability to query FS_XFLAG_DAX on inode and decide whether to
> > enable DAX or not. (Without extra round trip). Or know it indirectly
> > by extending GETATTR and requesting this explicitly.
> 
> If guest queries if the file is DAX capable or not by an extra GETATTR,
> I'm afraid this will add extra round trip.
> 
> Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
> this patch set, then the FUSE server needs to set ATTR_DAX according to
> the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
> previously set by FUSE client work*. Then it becomes a *mandatory*
> requirement when implementing FUSE server. i.e., it becomes part of the
> FUSE protocol rather than implementation specific. FUSE server can still
> implement some other algorithm deciding whether to set ATTR_DAX or not,
> though it must set ATTR_DAX once the backend file is flagged with
> FS_XFLAG_DAX.
> 
> Besides, as you said, FUSE server needs to add one extra
> GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
> cost, we could negotiate the per-file DAX capability during FUSE_INIT.
> Only when the per-file DAX capability is negotiated, will the FUSE
> server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
> doing LOOKUP.
> 
> 
> Personally, I prefer the second way, i.e., by extending LOOKUP (adding
> ATTR_DAX), to eliminate the extra round trip.

Negotiating a fuse feature say FUSE_FS_XFLAG_DAX makes sense. If
client is mounted with "-o dax=inode", then client will negotitate
this feature and if server does not support it, mount can fail.

But this probably will be binding on server that it needs to return
the state of FS_XFLAG_DAX attr on inode upon lookup/getattr. I don't
this will allow server to implement its own separate policy which
does not follow FS_XFLAG_DAX xattr. 

IOW, I don't think server can choose to implement its own policy
for enabling dax for "-o dax=inode".

If there is really a need to for something new where server needs
to dynamically decide which inodes should use dax (and not use
FS_XFLAG_FS), I guess that probably should be a separate mount
option say "-o dax=server" and it negotiates a separate feature
say FUSE_DAX_SERVER. Once that's negotiated, now both client
and server know that DAX will be used on files as determined
by server and not necessarily by using file attr FS_XFLAG_DAX.

So is "dax=inode" enough for your needs? What's your requirement,
can you give little bit of more details.

Thanks
Vivek

> 
> > 
> > If we were only implementing A, then server does not have a way to 
> > tell client to enable DAX. Server can either look at FS_XFLAG_DAX
> > and decide to enable DAX or use some other property. Given querying
> > FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
> > it probably will be a server option. But enabling on server does not
> > mean client will enable it.
> 
> As I said previously, it can be negotiated whether this per-file DAX
> capability is needed. Guest can advertise this capability when '-o
> dax=inode' is configured.
> 
> > 
> > I think my primary concern with this patch right now is trying to
> > figure out which requirement we are trying to cater to first and how
> > to connect server and client well so they both understand what mode
> > they are operating in and interact well.
> > 
> 
> 
> -- 
> Thanks,
> Jeffle
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[JeffleXu]

On 7/21/21 3:18 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 01:25:11PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 5:30 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
>>>> This patchset adds support of per-file DAX for virtiofs, which is
>>>> inspired by Ira Weiny's work on ext4[1] and xfs[2].
>>>>
>>>> There are three related scenarios:
>>>> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
>>>> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
>>>> In this case, the dentry and inode are all marked as DONT_CACHE, and
>>>> the DAX state won't be updated until the file is closed and reopened
>>>> later.
>>>> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
>>>> (patch 4)
>>>> 4. Create new files under directories with DAX enabled. When creating
>>>> new files in ext4/xfs on host, the new created files will inherit the
>>>> per-file DAX flag from the directory, and thus the new created files in
>>>> virtiofs will also inherit the per-file DAX flag if the fuse server
>>>> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
>>>> DAX flag.
>>>
>>> Thinking little bit more about this from requirement perspective. I think
>>> we are trying to address two use cases here.
>>>
>>> A. Client does not know which files DAX should be used on. Only server
>>>    knows it and server passes this information to client. I suspect
>>>    that's your primary use case.
>>
>> Yes, this is the starting point of this patch set.
>>
>>>
>>> B. Client is driving which files are supposed to be using DAX. This is
>>>    exactly same as the model ext4/xfs are using by storing a persistent
>>>    flag on inode. 
>>>
>>> Current patches seem to be a hybrid of both approach A and B. 
>>>
>>> If we were to implement B, then fuse client probably needs to have the
>>> capability to query FS_XFLAG_DAX on inode and decide whether to
>>> enable DAX or not. (Without extra round trip). Or know it indirectly
>>> by extending GETATTR and requesting this explicitly.
>>
>> If guest queries if the file is DAX capable or not by an extra GETATTR,
>> I'm afraid this will add extra round trip.
>>
>> Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
>> this patch set, then the FUSE server needs to set ATTR_DAX according to
>> the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
>> previously set by FUSE client work*. Then it becomes a *mandatory*
>> requirement when implementing FUSE server. i.e., it becomes part of the
>> FUSE protocol rather than implementation specific. FUSE server can still
>> implement some other algorithm deciding whether to set ATTR_DAX or not,
>> though it must set ATTR_DAX once the backend file is flagged with
>> FS_XFLAG_DAX.
>>
>> Besides, as you said, FUSE server needs to add one extra
>> GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
>> cost, we could negotiate the per-file DAX capability during FUSE_INIT.
>> Only when the per-file DAX capability is negotiated, will the FUSE
>> server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
>> doing LOOKUP.
>>
>>
>> Personally, I prefer the second way, i.e., by extending LOOKUP (adding
>> ATTR_DAX), to eliminate the extra round trip.
> 
> Negotiating a fuse feature say FUSE_FS_XFLAG_DAX makes sense. If
> client is mounted with "-o dax=inode", then client will negotitate
> this feature and if server does not support it, mount can fail.
> 
> But this probably will be binding on server that it needs to return
> the state of FS_XFLAG_DAX attr on inode upon lookup/getattr. I don't
> this will allow server to implement its own separate policy which
> does not follow FS_XFLAG_DAX xattr. 

That means the backend fs must be ext4/xfs supporting per-file DAX feature.

If given more right to construct its own policy, FUSE server could
support per-file DAX upon other backend fs, though it will always fail
when virtiofs wants to set FS_XFLAG_DAX inside guest.

> 
> IOW, I don't think server can choose to implement its own policy
> for enabling dax for "-o dax=inode".
> 
> If there is really a need to for something new where server needs
> to dynamically decide which inodes should use dax (and not use
> FS_XFLAG_FS), I guess that probably should be a separate mount
> option say "-o dax=server" and it negotiates a separate feature
> say FUSE_DAX_SERVER. Once that's negotiated, now both client
> and server know that DAX will be used on files as determined
> by server and not necessarily by using file attr FS_XFLAG_DAX.
> 
> So is "dax=inode" enough for your needs? What's your requirement,
> can you give little bit of more details.

In our use case, the backend fs is something like SquashFS on host. The
content of the file on host is downloaded *as needed*. When the file is
not completely ready (completely downloaded), the guest will follow the
normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
is completely ready, per-file DAX is enabled for this file. IOW the FUSE
server need to dynamically decide if per-file DAX shall be enabled,
depending on if the file is completely downloaded.

Our strategy and design is still under discussion and may change. Any
comment and discussion is welcomed.

> 
>>
>>>
>>> If we were only implementing A, then server does not have a way to 
>>> tell client to enable DAX. Server can either look at FS_XFLAG_DAX
>>> and decide to enable DAX or use some other property. Given querying
>>> FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
>>> it probably will be a server option. But enabling on server does not
>>> mean client will enable it.
>>
>> As I said previously, it can be negotiated whether this per-file DAX
>> capability is needed. Guest can advertise this capability when '-o
>> dax=inode' is configured.
>>
>>>
>>> I think my primary concern with this patch right now is trying to
>>> figure out which requirement we are trying to cater to first and how
>>> to connect server and client well so they both understand what mode
>>> they are operating in and interact well.
>>>
>>
>>
>> -- 
>> Thanks,
>> Jeffle
>>

-- 
Thanks,
Jeffle

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[JeffleXu]

On 7/21/21 3:18 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 01:25:11PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 5:30 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
>>>> This patchset adds support of per-file DAX for virtiofs, which is
>>>> inspired by Ira Weiny's work on ext4[1] and xfs[2].
>>>>
>>>> There are three related scenarios:
>>>> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
>>>> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
>>>> In this case, the dentry and inode are all marked as DONT_CACHE, and
>>>> the DAX state won't be updated until the file is closed and reopened
>>>> later.
>>>> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
>>>> (patch 4)
>>>> 4. Create new files under directories with DAX enabled. When creating
>>>> new files in ext4/xfs on host, the new created files will inherit the
>>>> per-file DAX flag from the directory, and thus the new created files in
>>>> virtiofs will also inherit the per-file DAX flag if the fuse server
>>>> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
>>>> DAX flag.
>>>
>>> Thinking little bit more about this from requirement perspective. I think
>>> we are trying to address two use cases here.
>>>
>>> A. Client does not know which files DAX should be used on. Only server
>>>    knows it and server passes this information to client. I suspect
>>>    that's your primary use case.
>>
>> Yes, this is the starting point of this patch set.
>>
>>>
>>> B. Client is driving which files are supposed to be using DAX. This is
>>>    exactly same as the model ext4/xfs are using by storing a persistent
>>>    flag on inode. 
>>>
>>> Current patches seem to be a hybrid of both approach A and B. 
>>>
>>> If we were to implement B, then fuse client probably needs to have the
>>> capability to query FS_XFLAG_DAX on inode and decide whether to
>>> enable DAX or not. (Without extra round trip). Or know it indirectly
>>> by extending GETATTR and requesting this explicitly.
>>
>> If guest queries if the file is DAX capable or not by an extra GETATTR,
>> I'm afraid this will add extra round trip.
>>
>> Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
>> this patch set, then the FUSE server needs to set ATTR_DAX according to
>> the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
>> previously set by FUSE client work*. Then it becomes a *mandatory*
>> requirement when implementing FUSE server. i.e., it becomes part of the
>> FUSE protocol rather than implementation specific. FUSE server can still
>> implement some other algorithm deciding whether to set ATTR_DAX or not,
>> though it must set ATTR_DAX once the backend file is flagged with
>> FS_XFLAG_DAX.
>>
>> Besides, as you said, FUSE server needs to add one extra
>> GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
>> cost, we could negotiate the per-file DAX capability during FUSE_INIT.
>> Only when the per-file DAX capability is negotiated, will the FUSE
>> server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
>> doing LOOKUP.
>>
>>
>> Personally, I prefer the second way, i.e., by extending LOOKUP (adding
>> ATTR_DAX), to eliminate the extra round trip.
> 
> Negotiating a fuse feature say FUSE_FS_XFLAG_DAX makes sense. If
> client is mounted with "-o dax=inode", then client will negotitate
> this feature and if server does not support it, mount can fail.
> 
> But this probably will be binding on server that it needs to return
> the state of FS_XFLAG_DAX attr on inode upon lookup/getattr. I don't
> this will allow server to implement its own separate policy which
> does not follow FS_XFLAG_DAX xattr. 

That means the backend fs must be ext4/xfs supporting per-file DAX feature.

If given more right to construct its own policy, FUSE server could
support per-file DAX upon other backend fs, though it will always fail
when virtiofs wants to set FS_XFLAG_DAX inside guest.

> 
> IOW, I don't think server can choose to implement its own policy
> for enabling dax for "-o dax=inode".
> 
> If there is really a need to for something new where server needs
> to dynamically decide which inodes should use dax (and not use
> FS_XFLAG_FS), I guess that probably should be a separate mount
> option say "-o dax=server" and it negotiates a separate feature
> say FUSE_DAX_SERVER. Once that's negotiated, now both client
> and server know that DAX will be used on files as determined
> by server and not necessarily by using file attr FS_XFLAG_DAX.
> 
> So is "dax=inode" enough for your needs? What's your requirement,
> can you give little bit of more details.

In our use case, the backend fs is something like SquashFS on host. The
content of the file on host is downloaded *as needed*. When the file is
not completely ready (completely downloaded), the guest will follow the
normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
is completely ready, per-file DAX is enabled for this file. IOW the FUSE
server need to dynamically decide if per-file DAX shall be enabled,
depending on if the file is completely downloaded.

Our strategy and design is still under discussion and may change. Any
comment and discussion is welcomed.

> 
>>
>>>
>>> If we were only implementing A, then server does not have a way to 
>>> tell client to enable DAX. Server can either look at FS_XFLAG_DAX
>>> and decide to enable DAX or use some other property. Given querying
>>> FS_XFLAG_DAX will be an extra ioctl() on every inode lookup/getattr,
>>> it probably will be a server option. But enabling on server does not
>>> mean client will enable it.
>>
>> As I said previously, it can be negotiated whether this per-file DAX
>> capability is needed. Guest can advertise this capability when '-o
>> dax=inode' is configured.
>>
>>>
>>> I think my primary concern with this patch right now is trying to
>>> figure out which requirement we are trying to cater to first and how
>>> to connect server and client well so they both understand what mode
>>> they are operating in and interact well.
>>>
>>
>>
>> -- 
>> Thanks,
>> Jeffle
>>

-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Wed, Jul 21, 2021 at 08:32:19PM +0800, JeffleXu wrote:
> 
> 
> On 7/21/21 3:18 AM, Vivek Goyal wrote:
> > On Tue, Jul 20, 2021 at 01:25:11PM +0800, JeffleXu wrote:
> >>
> >>
> >> On 7/20/21 5:30 AM, Vivek Goyal wrote:
> >>> On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
> >>>> This patchset adds support of per-file DAX for virtiofs, which is
> >>>> inspired by Ira Weiny's work on ext4[1] and xfs[2].
> >>>>
> >>>> There are three related scenarios:
> >>>> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
> >>>> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
> >>>> In this case, the dentry and inode are all marked as DONT_CACHE, and
> >>>> the DAX state won't be updated until the file is closed and reopened
> >>>> later.
> >>>> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
> >>>> (patch 4)
> >>>> 4. Create new files under directories with DAX enabled. When creating
> >>>> new files in ext4/xfs on host, the new created files will inherit the
> >>>> per-file DAX flag from the directory, and thus the new created files in
> >>>> virtiofs will also inherit the per-file DAX flag if the fuse server
> >>>> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
> >>>> DAX flag.
> >>>
> >>> Thinking little bit more about this from requirement perspective. I think
> >>> we are trying to address two use cases here.
> >>>
> >>> A. Client does not know which files DAX should be used on. Only server
> >>>    knows it and server passes this information to client. I suspect
> >>>    that's your primary use case.
> >>
> >> Yes, this is the starting point of this patch set.
> >>
> >>>
> >>> B. Client is driving which files are supposed to be using DAX. This is
> >>>    exactly same as the model ext4/xfs are using by storing a persistent
> >>>    flag on inode. 
> >>>
> >>> Current patches seem to be a hybrid of both approach A and B. 
> >>>
> >>> If we were to implement B, then fuse client probably needs to have the
> >>> capability to query FS_XFLAG_DAX on inode and decide whether to
> >>> enable DAX or not. (Without extra round trip). Or know it indirectly
> >>> by extending GETATTR and requesting this explicitly.
> >>
> >> If guest queries if the file is DAX capable or not by an extra GETATTR,
> >> I'm afraid this will add extra round trip.
> >>
> >> Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
> >> this patch set, then the FUSE server needs to set ATTR_DAX according to
> >> the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
> >> previously set by FUSE client work*. Then it becomes a *mandatory*
> >> requirement when implementing FUSE server. i.e., it becomes part of the
> >> FUSE protocol rather than implementation specific. FUSE server can still
> >> implement some other algorithm deciding whether to set ATTR_DAX or not,
> >> though it must set ATTR_DAX once the backend file is flagged with
> >> FS_XFLAG_DAX.
> >>
> >> Besides, as you said, FUSE server needs to add one extra
> >> GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
> >> cost, we could negotiate the per-file DAX capability during FUSE_INIT.
> >> Only when the per-file DAX capability is negotiated, will the FUSE
> >> server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
> >> doing LOOKUP.
> >>
> >>
> >> Personally, I prefer the second way, i.e., by extending LOOKUP (adding
> >> ATTR_DAX), to eliminate the extra round trip.
> > 
> > Negotiating a fuse feature say FUSE_FS_XFLAG_DAX makes sense. If
> > client is mounted with "-o dax=inode", then client will negotitate
> > this feature and if server does not support it, mount can fail.
> > 
> > But this probably will be binding on server that it needs to return
> > the state of FS_XFLAG_DAX attr on inode upon lookup/getattr. I don't
> > this will allow server to implement its own separate policy which
> > does not follow FS_XFLAG_DAX xattr. 
> 
> That means the backend fs must be ext4/xfs supporting per-file DAX feature.

Yes. This probably will be a requirement because we are dependent on
file attr FS_XFLAG_DAX to decide whether to enable DAX or not. And
if underlying filesystem does not support this attr, then it will
not work.

> 
> If given more right to construct its own policy, FUSE server could
> support per-file DAX upon other backend fs, though it will always fail
> when virtiofs wants to set FS_XFLAG_DAX inside guest.
> 
> > 
> > IOW, I don't think server can choose to implement its own policy
> > for enabling dax for "-o dax=inode".
> > 
> > If there is really a need to for something new where server needs
> > to dynamically decide which inodes should use dax (and not use
> > FS_XFLAG_FS), I guess that probably should be a separate mount
> > option say "-o dax=server" and it negotiates a separate feature
> > say FUSE_DAX_SERVER. Once that's negotiated, now both client
> > and server know that DAX will be used on files as determined
> > by server and not necessarily by using file attr FS_XFLAG_DAX.
> > 
> > So is "dax=inode" enough for your needs? What's your requirement,
> > can you give little bit of more details.
> 
> In our use case, the backend fs is something like SquashFS on host. The
> content of the file on host is downloaded *as needed*. When the file is
> not completely ready (completely downloaded), the guest will follow the
> normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
> is completely ready, per-file DAX is enabled for this file. IOW the FUSE
> server need to dynamically decide if per-file DAX shall be enabled,
> depending on if the file is completely downloaded.

So you don't want to enable DAX yet because guest might fault on
a section of file which has not been downloaded yet?

I am wondering if somehow user fault handling can help with this.
If we could handle faults for this file in user space, then you
should be able to download that particular page[s] and resolve
the fault?

Thanks
Vivek

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Wed, Jul 21, 2021 at 08:32:19PM +0800, JeffleXu wrote:
> 
> 
> On 7/21/21 3:18 AM, Vivek Goyal wrote:
> > On Tue, Jul 20, 2021 at 01:25:11PM +0800, JeffleXu wrote:
> >>
> >>
> >> On 7/20/21 5:30 AM, Vivek Goyal wrote:
> >>> On Fri, Jul 16, 2021 at 06:47:49PM +0800, Jeffle Xu wrote:
> >>>> This patchset adds support of per-file DAX for virtiofs, which is
> >>>> inspired by Ira Weiny's work on ext4[1] and xfs[2].
> >>>>
> >>>> There are three related scenarios:
> >>>> 1. Alloc inode: get per-file DAX flag from fuse_attr.flags. (patch 3)
> >>>> 2. Per-file DAX flag changes when the file has been opened. (patch 3)
> >>>> In this case, the dentry and inode are all marked as DONT_CACHE, and
> >>>> the DAX state won't be updated until the file is closed and reopened
> >>>> later.
> >>>> 3. Users can change the per-file DAX flag inside the guest by chattr(1).
> >>>> (patch 4)
> >>>> 4. Create new files under directories with DAX enabled. When creating
> >>>> new files in ext4/xfs on host, the new created files will inherit the
> >>>> per-file DAX flag from the directory, and thus the new created files in
> >>>> virtiofs will also inherit the per-file DAX flag if the fuse server
> >>>> derives fuse_attr.flags from the underlying ext4/xfs inode's per-file
> >>>> DAX flag.
> >>>
> >>> Thinking little bit more about this from requirement perspective. I think
> >>> we are trying to address two use cases here.
> >>>
> >>> A. Client does not know which files DAX should be used on. Only server
> >>>    knows it and server passes this information to client. I suspect
> >>>    that's your primary use case.
> >>
> >> Yes, this is the starting point of this patch set.
> >>
> >>>
> >>> B. Client is driving which files are supposed to be using DAX. This is
> >>>    exactly same as the model ext4/xfs are using by storing a persistent
> >>>    flag on inode. 
> >>>
> >>> Current patches seem to be a hybrid of both approach A and B. 
> >>>
> >>> If we were to implement B, then fuse client probably needs to have the
> >>> capability to query FS_XFLAG_DAX on inode and decide whether to
> >>> enable DAX or not. (Without extra round trip). Or know it indirectly
> >>> by extending GETATTR and requesting this explicitly.
> >>
> >> If guest queries if the file is DAX capable or not by an extra GETATTR,
> >> I'm afraid this will add extra round trip.
> >>
> >> Or if we add the DAX flag (ATTR_DAX) by extending LOOKUP, as done by
> >> this patch set, then the FUSE server needs to set ATTR_DAX according to
> >> the FS_XFLAG_DAX of the backend files, *to make the FS_XFLAG_DAX flag
> >> previously set by FUSE client work*. Then it becomes a *mandatory*
> >> requirement when implementing FUSE server. i.e., it becomes part of the
> >> FUSE protocol rather than implementation specific. FUSE server can still
> >> implement some other algorithm deciding whether to set ATTR_DAX or not,
> >> though it must set ATTR_DAX once the backend file is flagged with
> >> FS_XFLAG_DAX.
> >>
> >> Besides, as you said, FUSE server needs to add one extra
> >> GETFLAGS/FSGETXATTR ioctl per LOOKUP in this case. To eliminate this
> >> cost, we could negotiate the per-file DAX capability during FUSE_INIT.
> >> Only when the per-file DAX capability is negotiated, will the FUSE
> >> server do extra GETFLAGS/FSGETXATTR ioctl and set ATTR_DAX flag when
> >> doing LOOKUP.
> >>
> >>
> >> Personally, I prefer the second way, i.e., by extending LOOKUP (adding
> >> ATTR_DAX), to eliminate the extra round trip.
> > 
> > Negotiating a fuse feature say FUSE_FS_XFLAG_DAX makes sense. If
> > client is mounted with "-o dax=inode", then client will negotitate
> > this feature and if server does not support it, mount can fail.
> > 
> > But this probably will be binding on server that it needs to return
> > the state of FS_XFLAG_DAX attr on inode upon lookup/getattr. I don't
> > this will allow server to implement its own separate policy which
> > does not follow FS_XFLAG_DAX xattr. 
> 
> That means the backend fs must be ext4/xfs supporting per-file DAX feature.

Yes. This probably will be a requirement because we are dependent on
file attr FS_XFLAG_DAX to decide whether to enable DAX or not. And
if underlying filesystem does not support this attr, then it will
not work.

> 
> If given more right to construct its own policy, FUSE server could
> support per-file DAX upon other backend fs, though it will always fail
> when virtiofs wants to set FS_XFLAG_DAX inside guest.
> 
> > 
> > IOW, I don't think server can choose to implement its own policy
> > for enabling dax for "-o dax=inode".
> > 
> > If there is really a need to for something new where server needs
> > to dynamically decide which inodes should use dax (and not use
> > FS_XFLAG_FS), I guess that probably should be a separate mount
> > option say "-o dax=server" and it negotiates a separate feature
> > say FUSE_DAX_SERVER. Once that's negotiated, now both client
> > and server know that DAX will be used on files as determined
> > by server and not necessarily by using file attr FS_XFLAG_DAX.
> > 
> > So is "dax=inode" enough for your needs? What's your requirement,
> > can you give little bit of more details.
> 
> In our use case, the backend fs is something like SquashFS on host. The
> content of the file on host is downloaded *as needed*. When the file is
> not completely ready (completely downloaded), the guest will follow the
> normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
> is completely ready, per-file DAX is enabled for this file. IOW the FUSE
> server need to dynamically decide if per-file DAX shall be enabled,
> depending on if the file is completely downloaded.

So you don't want to enable DAX yet because guest might fault on
a section of file which has not been downloaded yet?

I am wondering if somehow user fault handling can help with this.
If we could handle faults for this file in user space, then you
should be able to download that particular page[s] and resolve
the fault?

Thanks
Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Wed, Jul 21, 2021 at 08:48:57AM -0400, Vivek Goyal wrote:
[..]
> > > So is "dax=inode" enough for your needs? What's your requirement,
> > > can you give little bit of more details.
> > 
> > In our use case, the backend fs is something like SquashFS on host. The
> > content of the file on host is downloaded *as needed*. When the file is
> > not completely ready (completely downloaded), the guest will follow the
> > normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
> > is completely ready, per-file DAX is enabled for this file. IOW the FUSE
> > server need to dynamically decide if per-file DAX shall be enabled,
> > depending on if the file is completely downloaded.
> 
> So you don't want to enable DAX yet because guest might fault on
> a section of file which has not been downloaded yet?
> 
> I am wondering if somehow user fault handling can help with this.
> If we could handle faults for this file in user space, then you
> should be able to download that particular page[s] and resolve
> the fault?

Stefan mentioned that can't we block when fuse mmap request comes
in and download corresponding section of file. Or do whatever you
are doing in FUSE_READ. 

IOW, even if you enable dax in your use case on all files,
FUSE_SETUPMAPPING request will give you control to make sure 
file section being mmaped has been downloaded.

Vivek

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[Vivek Goyal]

On Wed, Jul 21, 2021 at 08:48:57AM -0400, Vivek Goyal wrote:
[..]
> > > So is "dax=inode" enough for your needs? What's your requirement,
> > > can you give little bit of more details.
> > 
> > In our use case, the backend fs is something like SquashFS on host. The
> > content of the file on host is downloaded *as needed*. When the file is
> > not completely ready (completely downloaded), the guest will follow the
> > normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
> > is completely ready, per-file DAX is enabled for this file. IOW the FUSE
> > server need to dynamically decide if per-file DAX shall be enabled,
> > depending on if the file is completely downloaded.
> 
> So you don't want to enable DAX yet because guest might fault on
> a section of file which has not been downloaded yet?
> 
> I am wondering if somehow user fault handling can help with this.
> If we could handle faults for this file in user space, then you
> should be able to download that particular page[s] and resolve
> the fault?

Stefan mentioned that can't we block when fuse mmap request comes
in and download corresponding section of file. Or do whatever you
are doing in FUSE_READ. 

IOW, even if you enable dax in your use case on all files,
FUSE_SETUPMAPPING request will give you control to make sure 
file section being mmaped has been downloaded.

Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[JeffleXu]

On 7/21/21 10:42 PM, Vivek Goyal wrote:
> On Wed, Jul 21, 2021 at 08:48:57AM -0400, Vivek Goyal wrote:
> [..]
>>>> So is "dax=inode" enough for your needs? What's your requirement,
>>>> can you give little bit of more details.
>>>
>>> In our use case, the backend fs is something like SquashFS on host. The
>>> content of the file on host is downloaded *as needed*. When the file is
>>> not completely ready (completely downloaded), the guest will follow the
>>> normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
>>> is completely ready, per-file DAX is enabled for this file. IOW the FUSE
>>> server need to dynamically decide if per-file DAX shall be enabled,
>>> depending on if the file is completely downloaded.
>>
>> So you don't want to enable DAX yet because guest might fault on
>> a section of file which has not been downloaded yet?
>>
>> I am wondering if somehow user fault handling can help with this.
>> If we could handle faults for this file in user space, then you
>> should be able to download that particular page[s] and resolve
>> the fault?
> 
> Stefan mentioned that can't we block when fuse mmap request comes
> in and download corresponding section of file. Or do whatever you
> are doing in FUSE_READ. 
> 
> IOW, even if you enable dax in your use case on all files,
> FUSE_SETUPMAPPING request will give you control to make sure 
> file section being mmaped has been downloaded.
> 

Sorry for the late reply. I missed this mail as it is classified into
the mailing list folder.

The idea you mentioned may works. Anyway, the implementation details of
the FUSE server is not strongly binding to the FUSE protocol changes in
kernel. The protocol only requires that FUSE client shall be able to
store FS_DAX_FL attr persistently in *some way*. The changes in kernel
shall be general, no matter whether the FUSE server is FS_DAX_FL attr
based or something else.


-- 
Thanks,
Jeffle

Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX

[JeffleXu]

On 7/21/21 10:42 PM, Vivek Goyal wrote:
> On Wed, Jul 21, 2021 at 08:48:57AM -0400, Vivek Goyal wrote:
> [..]
>>>> So is "dax=inode" enough for your needs? What's your requirement,
>>>> can you give little bit of more details.
>>>
>>> In our use case, the backend fs is something like SquashFS on host. The
>>> content of the file on host is downloaded *as needed*. When the file is
>>> not completely ready (completely downloaded), the guest will follow the
>>> normal IO routine, i.e., by FUSE_READ/FUSE_WRITE request. While the file
>>> is completely ready, per-file DAX is enabled for this file. IOW the FUSE
>>> server need to dynamically decide if per-file DAX shall be enabled,
>>> depending on if the file is completely downloaded.
>>
>> So you don't want to enable DAX yet because guest might fault on
>> a section of file which has not been downloaded yet?
>>
>> I am wondering if somehow user fault handling can help with this.
>> If we could handle faults for this file in user space, then you
>> should be able to download that particular page[s] and resolve
>> the fault?
> 
> Stefan mentioned that can't we block when fuse mmap request comes
> in and download corresponding section of file. Or do whatever you
> are doing in FUSE_READ. 
> 
> IOW, even if you enable dax in your use case on all files,
> FUSE_SETUPMAPPING request will give you control to make sure 
> file section being mmaped has been downloaded.
> 

Sorry for the late reply. I missed this mail as it is classified into
the mailing list folder.

The idea you mentioned may works. Anyway, the implementation details of
the FUSE server is not strongly binding to the FUSE protocol changes in
kernel. The protocol only requires that FUSE client shall be able to
store FS_DAX_FL attr persistently in *some way*. The changes in kernel
shall be general, no matter whether the FUSE server is FS_DAX_FL attr
based or something else.


-- 
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest

[kernel test robot]

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

CC: clang-built-linux(a)googlegroups.com
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210716104753.74377-5-jefflexu@linux.alibaba.com>
References: <20210716104753.74377-5-jefflexu@linux.alibaba.com>
TO: Jeffle Xu <jefflexu@linux.alibaba.com>
TO: vgoyal(a)redhat.com
TO: stefanha(a)redhat.com
TO: miklos(a)szeredi.hu
CC: linux-fsdevel(a)vger.kernel.org
CC: virtualization(a)lists.linux-foundation.org
CC: bo.liu(a)linux.alibaba.com
CC: joseph.qi(a)linux.alibaba.com

Hi Jeffle,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on fuse/for-next]
[also build test WARNING on v5.14-rc3 next-20210726]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jeffle-Xu/virtiofs-fuse-support-per-file-DAX/20210718-102250
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
:::::: branch date: 10 days ago
:::::: commit date: 10 days ago
config: x86_64-randconfig-c001-20210727 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project c658b472f3e61e1818e1909bf02f3d65470018a5)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/a6ac625f19c4c4de28ee8a466c1bab8824b7042e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jeffle-Xu/virtiofs-fuse-support-per-file-DAX/20210718-102250
        git checkout a6ac625f19c4c4de28ee8a466c1bab8824b7042e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)
   drivers/regulator/qcom_spmi-regulator.c:1155:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
           ret = spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_ENABLE,
           ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/regulator/qcom_spmi-regulator.c:1155:2: note: Value stored to 'ret' is never read
           ret = spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_ENABLE,
           ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   drivers/regulator/palmas-regulator.c:1598:36: warning: Value stored to 'pdata' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
           struct palmas_pmic_platform_data *pdata = dev_get_platdata(&pdev->dev);
                                             ^~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/regulator/palmas-regulator.c:1598:36: note: Value stored to 'pdata' during its initialization is never read
           struct palmas_pmic_platform_data *pdata = dev_get_platdata(&pdev->dev);
                                             ^~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:49:39: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
                           const struct nvkm_vmm_desc *pair = page[-1].desc;
                                                              ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1145:8: note: Assuming  is non-null
           if (!(*pvmm = kzalloc(sizeof(**pvmm), GFP_KERNEL)))
                 ^~~~~
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1145:2: note: Taking false branch
           if (!(*pvmm = kzalloc(sizeof(**pvmm), GFP_KERNEL)))
           ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1147:9: note: Calling 'nvkm_vmm_ctor'
           return nvkm_vmm_ctor(func, mmu, hdr, managed, addr, size, key, name, *pvmm);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1051:43: note: Assuming 'key' is null
           __mutex_init(&vmm->mutex, "&vmm->mutex", key ? key : &_key);
                                                    ^~~
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1051:43: note: '?' condition is false
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1056:2: note: Loop condition is false. Execution continues on line 1063
           while (page[1].shift)
           ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1063:2: note: Loop condition is false. Execution continues on line 1065
           for (levels = 0, desc = page->desc; desc->bits; desc++, levels++)
           ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1068:6: note: Taking false branch
           if (WARN_ON(levels > NVKM_VMM_LEVELS_MAX))
               ^
   include/asm-generic/bug.h:120:2: note: expanded from macro 'WARN_ON'
           if (unlikely(__ret_warn_on))                                    \
           ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1068:2: note: Taking false branch
           if (WARN_ON(levels > NVKM_VMM_LEVELS_MAX))
           ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1072:41: note: Passing null pointer value via 3rd parameter 'page'
           vmm->pd = nvkm_vmm_pt_new(desc, false, NULL);
                                                  ^
   include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
   #define NULL ((void *)0)
                ^~~~~~~~~~~
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1072:12: note: Calling 'nvkm_vmm_pt_new'
           vmm->pd = nvkm_vmm_pt_new(desc, false, NULL);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:47:6: note: Assuming field 'type' is > PGT
           if (desc->type > PGT) {
               ^~~~~~~~~~~~~~~~
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:47:2: note: Taking true branch
           if (desc->type > PGT) {
           ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:48:7: note: Assuming field 'type' is equal to SPT
                   if (desc->type == SPT) {
                       ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:48:3: note: Taking true branch
                   if (desc->type == SPT) {
                   ^
   drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:49:39: note: Dereference of null pointer
                           const struct nvkm_vmm_desc *pair = page[-1].desc;
                                                              ^~~~~~~~~~~~~
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   8 warnings generated.
>> fs/fuse/ioctl.c:471:3: warning: Value stored to 'newdax' is never read [clang-analyzer-deadcode.DeadStores]
                   newdax = flags & FS_DAX_FL;
                   ^        ~~~~~~~~~~~~~~~~~
   fs/fuse/ioctl.c:471:3: note: Value stored to 'newdax' is never read
                   newdax = flags & FS_DAX_FL;
                   ^        ~~~~~~~~~~~~~~~~~
   fs/fuse/ioctl.c:482:3: warning: Value stored to 'newdax' is never read [clang-analyzer-deadcode.DeadStores]
                   newdax = fa->fsx_xflags & FS_XFLAG_DAX;
                   ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/fuse/ioctl.c:482:3: note: Value stored to 'newdax' is never read
                   newdax = fa->fsx_xflags & FS_XFLAG_DAX;
                   ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   8 warnings generated.
   fs/fuse/cuse.c:359:2: warning: Value stored to 'rc' is never read [clang-analyzer-deadcode.DeadStores]
           rc = -ENOMEM;
           ^    ~~~~~~~
   fs/fuse/cuse.c:359:2: note: Value stored to 'rc' is never read
           rc = -ENOMEM;
           ^    ~~~~~~~
   fs/fuse/cuse.c:386:2: warning: Value stored to 'rc' is never read [clang-analyzer-deadcode.DeadStores]
           rc = -ENOMEM;
           ^    ~~~~~~~
   fs/fuse/cuse.c:386:2: note: Value stored to 'rc' is never read
           rc = -ENOMEM;
           ^    ~~~~~~~
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   8 warnings generated.
   fs/fuse/virtio_fs.c:912:2: warning: Attempt to free released memory [clang-analyzer-unix.Malloc]
           kfree(fs->vqs);
           ^~~~~~~~~~~~~~
   fs/fuse/virtio_fs.c:879:6: note: Assuming 'fs' is non-null
           if (!fs)
               ^~~
   fs/fuse/virtio_fs.c:879:2: note: Taking false branch
           if (!fs)
           ^
   fs/fuse/virtio_fs.c:885:6: note: 'ret' is >= 0
           if (ret < 0)
               ^~~
   fs/fuse/virtio_fs.c:885:2: note: Taking false branch
           if (ret < 0)
           ^
   fs/fuse/virtio_fs.c:888:8: note: Calling 'virtio_fs_setup_vqs'
           ret = virtio_fs_setup_vqs(vdev, fs);
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/fuse/virtio_fs.c:691:2: note: Loop condition is false.  Exiting loop
           virtio_cread_le(vdev, struct virtio_fs_config, num_request_queues,
           ^
   include/linux/virtio_config.h:396:3: note: expanded from macro 'virtio_cread_le'
                   might_sleep();                                          \
                   ^
   include/linux/kernel.h:119:2: note: expanded from macro 'might_sleep'
           do { __might_sleep(__FILE__, __LINE__, 0); might_resched(); } while (0)
           ^
   fs/fuse/virtio_fs.c:691:2: note: Control jumps to 'case 4:' @line 691
           virtio_cread_le(vdev, struct virtio_fs_config, num_request_queues,
           ^
   include/linux/virtio_config.h:400:3: note: expanded from macro 'virtio_cread_le'
                   switch (sizeof(virtio_cread_v)) {                       \
                   ^
   fs/fuse/virtio_fs.c:691:2: note:  Execution continues on line 691
           virtio_cread_le(vdev, struct virtio_fs_config, num_request_queues,
           ^
   include/linux/virtio_config.h:408:4: note: expanded from macro 'virtio_cread_le'
                           break;                                          \
                           ^
   fs/fuse/virtio_fs.c:691:2: note: Loop condition is false.  Exiting loop
           virtio_cread_le(vdev, struct virtio_fs_config, num_request_queues,
           ^
   include/linux/virtio_config.h:393:2: note: expanded from macro 'virtio_cread_le'
           do {                                                            \
           ^
   fs/fuse/virtio_fs.c:693:6: note: Assuming field 'num_request_queues' is not equal to 0
           if (fs->num_request_queues == 0)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/fuse/virtio_fs.c:693:2: note: Taking false branch
           if (fs->num_request_queues == 0)
           ^
   fs/fuse/virtio_fs.c:698:6: note: Assuming field 'vqs' is non-null
           if (!fs->vqs)
               ^~~~~~~~
   fs/fuse/virtio_fs.c:698:2: note: Taking false branch
           if (!fs->vqs)
           ^
   fs/fuse/virtio_fs.c:705:6: note: Assuming 'vqs' is non-null
           if (!vqs || !callbacks || !names) {
               ^~~~
   fs/fuse/virtio_fs.c:705:6: note: Left side of '||' is false
   fs/fuse/virtio_fs.c:705:14: note: Assuming 'callbacks' is non-null
           if (!vqs || !callbacks || !names) {
                       ^~~~~~~~~~
   fs/fuse/virtio_fs.c:705:6: note: Left side of '||' is false
           if (!vqs || !callbacks || !names) {
               ^
   fs/fuse/virtio_fs.c:705:28: note: Assuming 'names' is non-null
           if (!vqs || !callbacks || !names) {
                                     ^~~~~~
   fs/fuse/virtio_fs.c:705:2: note: Taking false branch

vim +/newdax +471 fs/fuse/ioctl.c

72227eac177dd1 Miklos Szeredi 2021-04-08  455  
72227eac177dd1 Miklos Szeredi 2021-04-08  456  int fuse_fileattr_set(struct user_namespace *mnt_userns,
72227eac177dd1 Miklos Szeredi 2021-04-08  457  		      struct dentry *dentry, struct fileattr *fa)
72227eac177dd1 Miklos Szeredi 2021-04-08  458  {
72227eac177dd1 Miklos Szeredi 2021-04-08  459  	struct inode *inode = d_inode(dentry);
72227eac177dd1 Miklos Szeredi 2021-04-08  460  	struct fuse_file *ff;
72227eac177dd1 Miklos Szeredi 2021-04-08  461  	unsigned int flags = fa->flags;
72227eac177dd1 Miklos Szeredi 2021-04-08  462  	struct fsxattr xfa;
a6ac625f19c4c4 Jeffle Xu      2021-07-16  463  	bool newdax;
72227eac177dd1 Miklos Szeredi 2021-04-08  464  	int err;
72227eac177dd1 Miklos Szeredi 2021-04-08  465  
72227eac177dd1 Miklos Szeredi 2021-04-08  466  	ff = fuse_priv_ioctl_prepare(inode);
72227eac177dd1 Miklos Szeredi 2021-04-08  467  	if (IS_ERR(ff))
72227eac177dd1 Miklos Szeredi 2021-04-08  468  		return PTR_ERR(ff);
72227eac177dd1 Miklos Szeredi 2021-04-08  469  
72227eac177dd1 Miklos Szeredi 2021-04-08  470  	if (fa->flags_valid) {
a6ac625f19c4c4 Jeffle Xu      2021-07-16 @471  		newdax = flags & FS_DAX_FL;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34402 bytes --]

Re: [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest

[kernel test robot]

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


Hi Jeffle,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on fuse/for-next]
[also build test WARNING on v5.14-rc3 next-20210726]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: 
https://github.com/0day-ci/linux/commits/Jeffle-Xu/virtiofs-fuse-support-per-file-DAX/20210718-102250
base: 
https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
:::::: branch date: 10 days ago
:::::: commit date: 10 days ago
config: x86_64-randconfig-c001-20210727 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 
c658b472f3e61e1818e1909bf02f3d65470018a5)
reproduce (this is a W=1 build):
         wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # install x86_64 cross compiling tool for clang build
         # apt-get install binutils-x86-64-linux-gnu
         # 
https://github.com/0day-ci/linux/commit/a6ac625f19c4c4de28ee8a466c1bab8824b7042e
         git remote add linux-review https://github.com/0day-ci/linux
         git fetch --no-tags linux-review 
Jeffle-Xu/virtiofs-fuse-support-per-file-DAX/20210718-102250
         git checkout a6ac625f19c4c4de28ee8a466c1bab8824b7042e
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross 
ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)
    drivers/regulator/qcom_spmi-regulator.c:1155:2: warning: Value 
stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
            ret = spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_ENABLE,
            ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/regulator/qcom_spmi-regulator.c:1155:2: note: Value stored 
to 'ret' is never read
            ret = spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_ENABLE,
            ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    7 warnings generated.
    drivers/regulator/palmas-regulator.c:1598:36: warning: Value stored 
to 'pdata' during its initialization is never read 
[clang-analyzer-deadcode.DeadStores]
            struct palmas_pmic_platform_data *pdata = 
dev_get_platdata(&pdev->dev);
                                              ^~~~~ 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/regulator/palmas-regulator.c:1598:36: note: Value stored to 
'pdata' during its initialization is never read
            struct palmas_pmic_platform_data *pdata = 
dev_get_platdata(&pdev->dev);
                                              ^~~~~ 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    5 warnings generated.
    Suppressed 5 warnings (5 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    5 warnings generated.
    Suppressed 5 warnings (5 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    7 warnings generated.
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:49:39: warning: 
Dereference of null pointer [clang-analyzer-core.NullDereference]
                            const struct nvkm_vmm_desc *pair = 
page[-1].desc;
                                                               ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1145:8: note: Assuming 
  is non-null
            if (!(*pvmm = kzalloc(sizeof(**pvmm), GFP_KERNEL)))
                  ^~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1145:2: note: Taking 
false branch
            if (!(*pvmm = kzalloc(sizeof(**pvmm), GFP_KERNEL)))
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1147:9: note: Calling 
'nvkm_vmm_ctor'
            return nvkm_vmm_ctor(func, mmu, hdr, managed, addr, size, 
key, name, *pvmm);
 
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1051:43: note: 
Assuming 'key' is null
            __mutex_init(&vmm->mutex, "&vmm->mutex", key ? key : &_key);
                                                     ^~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1051:43: note: '?' 
condition is false
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1056:2: note: Loop 
condition is false. Execution continues on line 1063
            while (page[1].shift)
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1063:2: note: Loop 
condition is false. Execution continues on line 1065
            for (levels = 0, desc = page->desc; desc->bits; desc++, 
levels++)
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1068:6: note: Taking 
false branch
            if (WARN_ON(levels > NVKM_VMM_LEVELS_MAX))
                ^
    include/asm-generic/bug.h:120:2: note: expanded from macro 'WARN_ON'
            if (unlikely(__ret_warn_on)) 
     \
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1068:2: note: Taking 
false branch
            if (WARN_ON(levels > NVKM_VMM_LEVELS_MAX))
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1072:41: note: Passing 
null pointer value via 3rd parameter 'page'
            vmm->pd = nvkm_vmm_pt_new(desc, false, NULL);
                                                   ^
    include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
    #define NULL ((void *)0)
                 ^~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1072:12: note: Calling 
'nvkm_vmm_pt_new'
            vmm->pd = nvkm_vmm_pt_new(desc, false, NULL);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:47:6: note: Assuming 
field 'type' is > PGT
            if (desc->type > PGT) {
                ^~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:47:2: note: Taking 
true branch
            if (desc->type > PGT) {
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:48:7: note: Assuming 
field 'type' is equal to SPT
                    if (desc->type == SPT) {
                        ^~~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:48:3: note: Taking 
true branch
                    if (desc->type == SPT) {
                    ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:49:39: note: 
Dereference of null pointer
                            const struct nvkm_vmm_desc *pair = 
page[-1].desc;
                                                               ^~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    8 warnings generated.
>> fs/fuse/ioctl.c:471:3: warning: Value stored to 'newdax' is never read [clang-analyzer-deadcode.DeadStores]
                    newdax = flags & FS_DAX_FL;
                    ^        ~~~~~~~~~~~~~~~~~
    fs/fuse/ioctl.c:471:3: note: Value stored to 'newdax' is never read
                    newdax = flags & FS_DAX_FL;
                    ^        ~~~~~~~~~~~~~~~~~
    fs/fuse/ioctl.c:482:3: warning: Value stored to 'newdax' is never 
read [clang-analyzer-deadcode.DeadStores]
                    newdax = fa->fsx_xflags & FS_XFLAG_DAX;
                    ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    fs/fuse/ioctl.c:482:3: note: Value stored to 'newdax' is never read
                    newdax = fa->fsx_xflags & FS_XFLAG_DAX;
                    ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    8 warnings generated.
    fs/fuse/cuse.c:359:2: warning: Value stored to 'rc' is never read 
[clang-analyzer-deadcode.DeadStores]
            rc = -ENOMEM;
            ^    ~~~~~~~
    fs/fuse/cuse.c:359:2: note: Value stored to 'rc' is never read
            rc = -ENOMEM;
            ^    ~~~~~~~
    fs/fuse/cuse.c:386:2: warning: Value stored to 'rc' is never read 
[clang-analyzer-deadcode.DeadStores]
            rc = -ENOMEM;
            ^    ~~~~~~~
    fs/fuse/cuse.c:386:2: note: Value stored to 'rc' is never read
            rc = -ENOMEM;
            ^    ~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    8 warnings generated.
    fs/fuse/virtio_fs.c:912:2: warning: Attempt to free released memory 
[clang-analyzer-unix.Malloc]
            kfree(fs->vqs);
            ^~~~~~~~~~~~~~
    fs/fuse/virtio_fs.c:879:6: note: Assuming 'fs' is non-null
            if (!fs)
                ^~~
    fs/fuse/virtio_fs.c:879:2: note: Taking false branch
            if (!fs)
            ^
    fs/fuse/virtio_fs.c:885:6: note: 'ret' is >= 0
            if (ret < 0)
                ^~~
    fs/fuse/virtio_fs.c:885:2: note: Taking false branch
            if (ret < 0)
            ^
    fs/fuse/virtio_fs.c:888:8: note: Calling 'virtio_fs_setup_vqs'
            ret = virtio_fs_setup_vqs(vdev, fs);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    fs/fuse/virtio_fs.c:691:2: note: Loop condition is false.  Exiting loop
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:396:3: note: expanded from macro 
'virtio_cread_le'
                    might_sleep(); 
     \
                    ^
    include/linux/kernel.h:119:2: note: expanded from macro 'might_sleep'
            do { __might_sleep(__FILE__, __LINE__, 0); might_resched(); 
} while (0)
            ^
    fs/fuse/virtio_fs.c:691:2: note: Control jumps to 'case 4:'  at line 691
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:400:3: note: expanded from macro 
'virtio_cread_le'
                    switch (sizeof(virtio_cread_v)) { 
     \
                    ^
    fs/fuse/virtio_fs.c:691:2: note:  Execution continues on line 691
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:408:4: note: expanded from macro 
'virtio_cread_le'
                            break; 
     \
                            ^
    fs/fuse/virtio_fs.c:691:2: note: Loop condition is false.  Exiting loop
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:393:2: note: expanded from macro 
'virtio_cread_le'
            do { 
     \
            ^
    fs/fuse/virtio_fs.c:693:6: note: Assuming field 'num_request_queues' 
is not equal to 0
            if (fs->num_request_queues == 0)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
    fs/fuse/virtio_fs.c:693:2: note: Taking false branch
            if (fs->num_request_queues == 0)
            ^
    fs/fuse/virtio_fs.c:698:6: note: Assuming field 'vqs' is non-null
            if (!fs->vqs)
                ^~~~~~~~
    fs/fuse/virtio_fs.c:698:2: note: Taking false branch
            if (!fs->vqs)
            ^
    fs/fuse/virtio_fs.c:705:6: note: Assuming 'vqs' is non-null
            if (!vqs || !callbacks || !names) {
                ^~~~
    fs/fuse/virtio_fs.c:705:6: note: Left side of '||' is false
    fs/fuse/virtio_fs.c:705:14: note: Assuming 'callbacks' is non-null
            if (!vqs || !callbacks || !names) {
                        ^~~~~~~~~~
    fs/fuse/virtio_fs.c:705:6: note: Left side of '||' is false
            if (!vqs || !callbacks || !names) {
                ^
    fs/fuse/virtio_fs.c:705:28: note: Assuming 'names' is non-null
            if (!vqs || !callbacks || !names) {
                                      ^~~~~~
    fs/fuse/virtio_fs.c:705:2: note: Taking false branch

vim +/newdax +471 fs/fuse/ioctl.c

72227eac177dd1 Miklos Szeredi 2021-04-08  455  72227eac177dd1 Miklos 
Szeredi 2021-04-08  456  int fuse_fileattr_set(struct user_namespace 
*mnt_userns,
72227eac177dd1 Miklos Szeredi 2021-04-08  457  		      struct dentry 
*dentry, struct fileattr *fa)
72227eac177dd1 Miklos Szeredi 2021-04-08  458  {
72227eac177dd1 Miklos Szeredi 2021-04-08  459  	struct inode *inode = 
d_inode(dentry);
72227eac177dd1 Miklos Szeredi 2021-04-08  460  	struct fuse_file *ff;
72227eac177dd1 Miklos Szeredi 2021-04-08  461  	unsigned int flags = 
fa->flags;
72227eac177dd1 Miklos Szeredi 2021-04-08  462  	struct fsxattr xfa;
a6ac625f19c4c4 Jeffle Xu      2021-07-16  463  	bool newdax;
72227eac177dd1 Miklos Szeredi 2021-04-08  464  	int err;
72227eac177dd1 Miklos Szeredi 2021-04-08  465  72227eac177dd1 Miklos 
Szeredi 2021-04-08  466  	ff = fuse_priv_ioctl_prepare(inode);
72227eac177dd1 Miklos Szeredi 2021-04-08  467  	if (IS_ERR(ff))
72227eac177dd1 Miklos Szeredi 2021-04-08  468  		return PTR_ERR(ff);
72227eac177dd1 Miklos Szeredi 2021-04-08  469  72227eac177dd1 Miklos 
Szeredi 2021-04-08  470  	if (fa->flags_valid) {
a6ac625f19c4c4 Jeffle Xu      2021-07-16 @471  		newdax = flags & FS_DAX_FL;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34402 bytes --]

[-- Attachment #3: Attached Message Part --]
[-- Type: text/plain, Size: 150 bytes --]

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org

Re: [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest

[kernel test robot]

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


Hi Jeffle,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on fuse/for-next]
[also build test WARNING on v5.14-rc3 next-20210726]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: 
https://github.com/0day-ci/linux/commits/Jeffle-Xu/virtiofs-fuse-support-per-file-DAX/20210718-102250
base: 
https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
:::::: branch date: 10 days ago
:::::: commit date: 10 days ago
config: x86_64-randconfig-c001-20210727 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 
c658b472f3e61e1818e1909bf02f3d65470018a5)
reproduce (this is a W=1 build):
         wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # install x86_64 cross compiling tool for clang build
         # apt-get install binutils-x86-64-linux-gnu
         # 
https://github.com/0day-ci/linux/commit/a6ac625f19c4c4de28ee8a466c1bab8824b7042e
         git remote add linux-review https://github.com/0day-ci/linux
         git fetch --no-tags linux-review 
Jeffle-Xu/virtiofs-fuse-support-per-file-DAX/20210718-102250
         git checkout a6ac625f19c4c4de28ee8a466c1bab8824b7042e
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross 
ARCH=x86_64 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)
    drivers/regulator/qcom_spmi-regulator.c:1155:2: warning: Value 
stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
            ret = spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_ENABLE,
            ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/regulator/qcom_spmi-regulator.c:1155:2: note: Value stored 
to 'ret' is never read
            ret = spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_ENABLE,
            ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    7 warnings generated.
    drivers/regulator/palmas-regulator.c:1598:36: warning: Value stored 
to 'pdata' during its initialization is never read 
[clang-analyzer-deadcode.DeadStores]
            struct palmas_pmic_platform_data *pdata = 
dev_get_platdata(&pdev->dev);
                                              ^~~~~ 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/regulator/palmas-regulator.c:1598:36: note: Value stored to 
'pdata' during its initialization is never read
            struct palmas_pmic_platform_data *pdata = 
dev_get_platdata(&pdev->dev);
                                              ^~~~~ 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    5 warnings generated.
    Suppressed 5 warnings (5 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    5 warnings generated.
    Suppressed 5 warnings (5 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    7 warnings generated.
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:49:39: warning: 
Dereference of null pointer [clang-analyzer-core.NullDereference]
                            const struct nvkm_vmm_desc *pair = 
page[-1].desc;
                                                               ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1145:8: note: Assuming 
  is non-null
            if (!(*pvmm = kzalloc(sizeof(**pvmm), GFP_KERNEL)))
                  ^~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1145:2: note: Taking 
false branch
            if (!(*pvmm = kzalloc(sizeof(**pvmm), GFP_KERNEL)))
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1147:9: note: Calling 
'nvkm_vmm_ctor'
            return nvkm_vmm_ctor(func, mmu, hdr, managed, addr, size, 
key, name, *pvmm);
 
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1051:43: note: 
Assuming 'key' is null
            __mutex_init(&vmm->mutex, "&vmm->mutex", key ? key : &_key);
                                                     ^~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1051:43: note: '?' 
condition is false
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1056:2: note: Loop 
condition is false. Execution continues on line 1063
            while (page[1].shift)
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1063:2: note: Loop 
condition is false. Execution continues on line 1065
            for (levels = 0, desc = page->desc; desc->bits; desc++, 
levels++)
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1068:6: note: Taking 
false branch
            if (WARN_ON(levels > NVKM_VMM_LEVELS_MAX))
                ^
    include/asm-generic/bug.h:120:2: note: expanded from macro 'WARN_ON'
            if (unlikely(__ret_warn_on)) 
     \
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1068:2: note: Taking 
false branch
            if (WARN_ON(levels > NVKM_VMM_LEVELS_MAX))
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1072:41: note: Passing 
null pointer value via 3rd parameter 'page'
            vmm->pd = nvkm_vmm_pt_new(desc, false, NULL);
                                                   ^
    include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
    #define NULL ((void *)0)
                 ^~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:1072:12: note: Calling 
'nvkm_vmm_pt_new'
            vmm->pd = nvkm_vmm_pt_new(desc, false, NULL);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:47:6: note: Assuming 
field 'type' is > PGT
            if (desc->type > PGT) {
                ^~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:47:2: note: Taking 
true branch
            if (desc->type > PGT) {
            ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:48:7: note: Assuming 
field 'type' is equal to SPT
                    if (desc->type == SPT) {
                        ^~~~~~~~~~~~~~~~~
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:48:3: note: Taking 
true branch
                    if (desc->type == SPT) {
                    ^
    drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:49:39: note: 
Dereference of null pointer
                            const struct nvkm_vmm_desc *pair = 
page[-1].desc;
                                                               ^~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    6 warnings generated.
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    8 warnings generated.
>> fs/fuse/ioctl.c:471:3: warning: Value stored to 'newdax' is never read [clang-analyzer-deadcode.DeadStores]
                    newdax = flags & FS_DAX_FL;
                    ^        ~~~~~~~~~~~~~~~~~
    fs/fuse/ioctl.c:471:3: note: Value stored to 'newdax' is never read
                    newdax = flags & FS_DAX_FL;
                    ^        ~~~~~~~~~~~~~~~~~
    fs/fuse/ioctl.c:482:3: warning: Value stored to 'newdax' is never 
read [clang-analyzer-deadcode.DeadStores]
                    newdax = fa->fsx_xflags & FS_XFLAG_DAX;
                    ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    fs/fuse/ioctl.c:482:3: note: Value stored to 'newdax' is never read
                    newdax = fa->fsx_xflags & FS_XFLAG_DAX;
                    ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    8 warnings generated.
    fs/fuse/cuse.c:359:2: warning: Value stored to 'rc' is never read 
[clang-analyzer-deadcode.DeadStores]
            rc = -ENOMEM;
            ^    ~~~~~~~
    fs/fuse/cuse.c:359:2: note: Value stored to 'rc' is never read
            rc = -ENOMEM;
            ^    ~~~~~~~
    fs/fuse/cuse.c:386:2: warning: Value stored to 'rc' is never read 
[clang-analyzer-deadcode.DeadStores]
            rc = -ENOMEM;
            ^    ~~~~~~~
    fs/fuse/cuse.c:386:2: note: Value stored to 'rc' is never read
            rc = -ENOMEM;
            ^    ~~~~~~~
    Suppressed 6 warnings (6 in non-user code).
    Use -header-filter=.* to display errors from all non-system headers. 
Use -system-headers to display errors from system headers as well.
    8 warnings generated.
    fs/fuse/virtio_fs.c:912:2: warning: Attempt to free released memory 
[clang-analyzer-unix.Malloc]
            kfree(fs->vqs);
            ^~~~~~~~~~~~~~
    fs/fuse/virtio_fs.c:879:6: note: Assuming 'fs' is non-null
            if (!fs)
                ^~~
    fs/fuse/virtio_fs.c:879:2: note: Taking false branch
            if (!fs)
            ^
    fs/fuse/virtio_fs.c:885:6: note: 'ret' is >= 0
            if (ret < 0)
                ^~~
    fs/fuse/virtio_fs.c:885:2: note: Taking false branch
            if (ret < 0)
            ^
    fs/fuse/virtio_fs.c:888:8: note: Calling 'virtio_fs_setup_vqs'
            ret = virtio_fs_setup_vqs(vdev, fs);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    fs/fuse/virtio_fs.c:691:2: note: Loop condition is false.  Exiting loop
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:396:3: note: expanded from macro 
'virtio_cread_le'
                    might_sleep(); 
     \
                    ^
    include/linux/kernel.h:119:2: note: expanded from macro 'might_sleep'
            do { __might_sleep(__FILE__, __LINE__, 0); might_resched(); 
} while (0)
            ^
    fs/fuse/virtio_fs.c:691:2: note: Control jumps to 'case 4:' @line 691
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:400:3: note: expanded from macro 
'virtio_cread_le'
                    switch (sizeof(virtio_cread_v)) { 
     \
                    ^
    fs/fuse/virtio_fs.c:691:2: note:  Execution continues on line 691
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:408:4: note: expanded from macro 
'virtio_cread_le'
                            break; 
     \
                            ^
    fs/fuse/virtio_fs.c:691:2: note: Loop condition is false.  Exiting loop
            virtio_cread_le(vdev, struct virtio_fs_config, 
num_request_queues,
            ^
    include/linux/virtio_config.h:393:2: note: expanded from macro 
'virtio_cread_le'
            do { 
     \
            ^
    fs/fuse/virtio_fs.c:693:6: note: Assuming field 'num_request_queues' 
is not equal to 0
            if (fs->num_request_queues == 0)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
    fs/fuse/virtio_fs.c:693:2: note: Taking false branch
            if (fs->num_request_queues == 0)
            ^
    fs/fuse/virtio_fs.c:698:6: note: Assuming field 'vqs' is non-null
            if (!fs->vqs)
                ^~~~~~~~
    fs/fuse/virtio_fs.c:698:2: note: Taking false branch
            if (!fs->vqs)
            ^
    fs/fuse/virtio_fs.c:705:6: note: Assuming 'vqs' is non-null
            if (!vqs || !callbacks || !names) {
                ^~~~
    fs/fuse/virtio_fs.c:705:6: note: Left side of '||' is false
    fs/fuse/virtio_fs.c:705:14: note: Assuming 'callbacks' is non-null
            if (!vqs || !callbacks || !names) {
                        ^~~~~~~~~~
    fs/fuse/virtio_fs.c:705:6: note: Left side of '||' is false
            if (!vqs || !callbacks || !names) {
                ^
    fs/fuse/virtio_fs.c:705:28: note: Assuming 'names' is non-null
            if (!vqs || !callbacks || !names) {
                                      ^~~~~~
    fs/fuse/virtio_fs.c:705:2: note: Taking false branch

vim +/newdax +471 fs/fuse/ioctl.c

72227eac177dd1 Miklos Szeredi 2021-04-08  455  72227eac177dd1 Miklos 
Szeredi 2021-04-08  456  int fuse_fileattr_set(struct user_namespace 
*mnt_userns,
72227eac177dd1 Miklos Szeredi 2021-04-08  457  		      struct dentry 
*dentry, struct fileattr *fa)
72227eac177dd1 Miklos Szeredi 2021-04-08  458  {
72227eac177dd1 Miklos Szeredi 2021-04-08  459  	struct inode *inode = 
d_inode(dentry);
72227eac177dd1 Miklos Szeredi 2021-04-08  460  	struct fuse_file *ff;
72227eac177dd1 Miklos Szeredi 2021-04-08  461  	unsigned int flags = 
fa->flags;
72227eac177dd1 Miklos Szeredi 2021-04-08  462  	struct fsxattr xfa;
a6ac625f19c4c4 Jeffle Xu      2021-07-16  463  	bool newdax;
72227eac177dd1 Miklos Szeredi 2021-04-08  464  	int err;
72227eac177dd1 Miklos Szeredi 2021-04-08  465  72227eac177dd1 Miklos 
Szeredi 2021-04-08  466  	ff = fuse_priv_ioctl_prepare(inode);
72227eac177dd1 Miklos Szeredi 2021-04-08  467  	if (IS_ERR(ff))
72227eac177dd1 Miklos Szeredi 2021-04-08  468  		return PTR_ERR(ff);
72227eac177dd1 Miklos Szeredi 2021-04-08  469  72227eac177dd1 Miklos 
Szeredi 2021-04-08  470  	if (fa->flags_valid) {
a6ac625f19c4c4 Jeffle Xu      2021-07-16 @471  		newdax = flags & FS_DAX_FL;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org


[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34402 bytes --]

[-- Attachment #3: AttachedMessagePart.ksh --]
[-- Type: text/plain, Size: 150 bytes --]

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org