* [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-16 10:47 ` Jeffle Xu
0 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-16 10:47 ` Jeffle Xu
0 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, joseph.qi, bo.liu, virtualization
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 1/4] fuse: add fuse_should_enable_dax() helper
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-16 10:47 ` Jeffle Xu
-1 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v2 1/4] fuse: add fuse_should_enable_dax() helper
@ 2021-07-16 10:47 ` Jeffle Xu
0 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, joseph.qi, bo.liu, virtualization
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v2 2/4] fuse: Make DAX mount option a tri-state
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-16 10:47 ` Jeffle Xu
-1 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v2 2/4] fuse: Make DAX mount option a tri-state
@ 2021-07-16 10:47 ` Jeffle Xu
0 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, joseph.qi, bo.liu, virtualization
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-16 10:47 ` Jeffle Xu
-1 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-16 10:47 ` Jeffle Xu
0 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, joseph.qi, bo.liu, virtualization
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-16 10:47 ` Jeffle Xu
-1 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest
@ 2021-07-16 10:47 ` Jeffle Xu
0 siblings, 0 replies; 50+ messages in thread
From: Jeffle Xu @ 2021-07-16 10:47 UTC (permalink / raw)
To: vgoyal, stefanha, miklos; +Cc: linux-fsdevel, joseph.qi, bo.liu, virtualization
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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-19 18:02 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 18:02 UTC (permalink / raw)
To: Jeffle Xu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state
@ 2021-07-19 18:02 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 18:02 UTC (permalink / raw)
To: Jeffle Xu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-19 18:41 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 18:41 UTC (permalink / raw)
To: Jeffle Xu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-19 18:41 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 18:41 UTC (permalink / raw)
To: Jeffle Xu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-19 19:44 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 19:44 UTC (permalink / raw)
To: Jeffle Xu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-19 19:44 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 19:44 UTC (permalink / raw)
To: Jeffle Xu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-19 19:54 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 19:54 UTC (permalink / raw)
To: Jeffle Xu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest
@ 2021-07-19 19:54 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 19:54 UTC (permalink / raw)
To: Jeffle Xu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
2021-07-16 10:47 ` Jeffle Xu
@ 2021-07-19 21:30 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 21:30 UTC (permalink / raw)
To: Jeffle Xu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-19 21:30 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-19 21:30 UTC (permalink / raw)
To: Jeffle Xu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
2021-07-19 21:30 ` Vivek Goyal
@ 2021-07-20 5:25 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 5:25 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-20 5:25 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 5:25 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state
2021-07-19 18:02 ` Vivek Goyal
@ 2021-07-20 5:54 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 5:54 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 2/4] fuse: Make DAX mount option a tri-state
@ 2021-07-20 5:54 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 5:54 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-19 19:44 ` Vivek Goyal
@ 2021-07-20 6:51 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 6:51 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-20 6:51 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 6:51 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-19 18:41 ` Vivek Goyal
@ 2021-07-20 7:19 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 7:19 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-20 7:19 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 7:19 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-20 6:51 ` JeffleXu
@ 2021-07-20 9:22 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 9:22 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-20 9:22 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-20 9:22 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
2021-07-20 5:25 ` JeffleXu
@ 2021-07-20 19:18 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-20 19:18 UTC (permalink / raw)
To: JeffleXu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-20 19:18 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-20 19:18 UTC (permalink / raw)
To: JeffleXu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-20 6:51 ` JeffleXu
@ 2021-07-20 19:27 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-20 19:27 UTC (permalink / raw)
To: JeffleXu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-20 19:27 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-20 19:27 UTC (permalink / raw)
To: JeffleXu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-20 7:19 ` JeffleXu
@ 2021-07-20 19:40 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-20 19:40 UTC (permalink / raw)
To: JeffleXu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-20 19:40 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-20 19:40 UTC (permalink / raw)
To: JeffleXu
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
2021-07-20 19:18 ` Vivek Goyal
@ 2021-07-21 12:32 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-21 12:32 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-21 12:32 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-21 12:32 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-20 19:40 ` Vivek Goyal
@ 2021-07-21 12:35 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-21 12:35 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-21 12:35 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-21 12:35 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
2021-07-21 12:32 ` JeffleXu
@ 2021-07-21 12:48 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-21 12:48 UTC (permalink / raw)
To: JeffleXu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu,
joseph.qi, Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-21 12:48 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-21 12:48 UTC (permalink / raw)
To: JeffleXu
Cc: miklos, Dr. David Alan Gilbert, virtualization, joseph.qi,
bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-20 19:27 ` Vivek Goyal
@ 2021-07-21 14:14 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-21 14:14 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu, joseph.qi
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-21 14:14 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-07-21 14:14 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, virtualization, joseph.qi, bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
2021-07-21 14:14 ` JeffleXu
@ 2021-07-21 14:40 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-21 14:40 UTC (permalink / raw)
To: JeffleXu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu,
joseph.qi, Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 3/4] fuse: add per-file DAX flag
@ 2021-07-21 14:40 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-21 14:40 UTC (permalink / raw)
To: JeffleXu
Cc: miklos, Dr. David Alan Gilbert, virtualization, joseph.qi,
bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
2021-07-21 12:48 ` Vivek Goyal
@ 2021-07-21 14:42 ` Vivek Goyal
-1 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-21 14:42 UTC (permalink / raw)
To: JeffleXu
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu,
joseph.qi, Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-07-21 14:42 ` Vivek Goyal
0 siblings, 0 replies; 50+ messages in thread
From: Vivek Goyal @ 2021-07-21 14:42 UTC (permalink / raw)
To: JeffleXu
Cc: miklos, Dr. David Alan Gilbert, virtualization, joseph.qi,
bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
2021-07-21 14:42 ` Vivek Goyal
@ 2021-08-04 6:51 ` JeffleXu
-1 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-08-04 6:51 UTC (permalink / raw)
To: Vivek Goyal
Cc: stefanha, miklos, linux-fsdevel, virtualization, bo.liu,
joseph.qi, Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 0/4] virtiofs,fuse: support per-file DAX
@ 2021-08-04 6:51 ` JeffleXu
0 siblings, 0 replies; 50+ messages in thread
From: JeffleXu @ 2021-08-04 6:51 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, Dr. David Alan Gilbert, virtualization, joseph.qi,
bo.liu, stefanha, linux-fsdevel
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
^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2021-08-04 6:52 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-16 10:47 [PATCH v2 0/4] virtiofs,fuse: support per-file DAX Jeffle Xu
2021-07-16 10:47 ` Jeffle Xu
2021-07-16 10:47 ` [PATCH v2 1/4] fuse: add fuse_should_enable_dax() helper Jeffle Xu
2021-07-16 10:47 ` Jeffle Xu
2021-07-16 10:47 ` [PATCH v2 2/4] fuse: Make DAX mount option a tri-state Jeffle Xu
2021-07-16 10:47 ` Jeffle Xu
2021-07-19 18:02 ` Vivek Goyal
2021-07-19 18:02 ` Vivek Goyal
2021-07-20 5:54 ` JeffleXu
2021-07-20 5:54 ` JeffleXu
2021-07-16 10:47 ` [PATCH v2 3/4] fuse: add per-file DAX flag Jeffle Xu
2021-07-16 10:47 ` Jeffle Xu
2021-07-19 18:41 ` Vivek Goyal
2021-07-19 18:41 ` Vivek Goyal
2021-07-20 7:19 ` JeffleXu
2021-07-20 7:19 ` JeffleXu
2021-07-20 19:40 ` Vivek Goyal
2021-07-20 19:40 ` Vivek Goyal
2021-07-21 12:35 ` JeffleXu
2021-07-21 12:35 ` JeffleXu
2021-07-19 19:44 ` Vivek Goyal
2021-07-19 19:44 ` Vivek Goyal
2021-07-20 6:51 ` JeffleXu
2021-07-20 6:51 ` JeffleXu
2021-07-20 9:22 ` JeffleXu
2021-07-20 9:22 ` JeffleXu
2021-07-20 19:27 ` Vivek Goyal
2021-07-20 19:27 ` Vivek Goyal
2021-07-21 14:14 ` JeffleXu
2021-07-21 14:14 ` JeffleXu
2021-07-21 14:40 ` Vivek Goyal
2021-07-21 14:40 ` Vivek Goyal
2021-07-16 10:47 ` [PATCH v2 4/4] fuse: support changing per-file DAX flag inside guest Jeffle Xu
2021-07-16 10:47 ` Jeffle Xu
2021-07-19 19:54 ` Vivek Goyal
2021-07-19 19:54 ` Vivek Goyal
2021-07-19 21:30 ` [PATCH v2 0/4] virtiofs,fuse: support per-file DAX Vivek Goyal
2021-07-19 21:30 ` Vivek Goyal
2021-07-20 5:25 ` JeffleXu
2021-07-20 5:25 ` JeffleXu
2021-07-20 19:18 ` Vivek Goyal
2021-07-20 19:18 ` Vivek Goyal
2021-07-21 12:32 ` JeffleXu
2021-07-21 12:32 ` JeffleXu
2021-07-21 12:48 ` Vivek Goyal
2021-07-21 12:48 ` Vivek Goyal
2021-07-21 14:42 ` Vivek Goyal
2021-07-21 14:42 ` Vivek Goyal
2021-08-04 6:51 ` JeffleXu
2021-08-04 6:51 ` JeffleXu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.