[RFC PATCH 0/2] fuse: Enable SB_NOSEC if filesystem is not shared

[RFC PATCH 0/2] fuse: Enable SB_NOSEC if filesystem is not shared

[Vivek Goyal]

Hi,

I want to enable SB_NOSEC in fuse in some form so that performance of
small random writes can be improved. As of now, we call file_remove_privs(),
which results in fuse always sending getxattr(security.capability) to
server to figure out if security.capability has been set on file or not.
If it has been set, it needs to be cleared. This slows down small
random writes tremendously.

I posted couple of proposals in the past here.

Proposal 1:

https://lore.kernel.org/linux-fsdevel/20200716144032.GC422759@redhat.com/

Proposal 2:

https://lore.kernel.org/linux-fsdevel/20200724183812.19573-1-vgoyal@redhat.com/

This is 3rd proposal now. One of the roadblocks in enabling SB_NOSEC
is shared filesystem. It is possible that another client modified the
file data and this client does not know about it. So we might regress
if we don't fetch security.capability always.

So looks like this needs to be handled different for shared filesystems
and non-shared filesystems. non-shared filesystems will be more like
local filesystems where fuse does not expect file data/metadata to
change outside the fuse. And we should be able to enable SB_NOSEC
optimization. This is what this proposal does.

It does not handle the case of shared filesystems. I believe solution
to that will depend on filesystem based on what's the cache coherency
guarantees filesystem provides and what's the cache invalidation 
mechanism it uses.

For now, all filesystems which are not shared can benefit from this
optimization. I am interested in virtiofs which is not shared in
many of the cases. In fact we don't even support shared mode yet. 

Any comments or feedback is welcome.

Thanks
Vivek

Vivek Goyal (2):
  fuse: Add a flag FUSE_NONSHARED_FS
  fuse: Enable SB_NOSEC if filesystem is not shared

 fs/fuse/fuse_i.h          |  3 +++
 fs/fuse/inode.c           | 12 +++++++++++-
 include/uapi/linux/fuse.h |  4 ++++
 3 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.25.4

[PATCH 1/2] fuse: Add a flag FUSE_NONSHARED_FS

[Vivek Goyal]

FUSE_NONSHARED_FS will signify that filesystem is not shared. That is
all fuse modifications are going thourgh this single fuse connection. It
should not happen that file's data/metadata changed without the knowledge
of fuse. Automatic file time stamp changes will probably be an exception
to this rule.

If filesystem is shared between different clients, then this flag should
not be set.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/fuse_i.h          | 3 +++
 fs/fuse/inode.c           | 6 +++++-
 include/uapi/linux/fuse.h | 4 ++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 740a8a7d7ae6..3ace15488eb6 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -720,6 +720,9 @@ struct fuse_conn {
 	/* Do not show mount options */
 	unsigned int no_mount_options:1;
 
+	/** This is not a shared filesystem */
+	unsigned int nonshared_fs:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index bba747520e9b..088faa3e352c 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -965,6 +965,9 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_args *args,
 					min_t(unsigned int, FUSE_MAX_MAX_PAGES,
 					max_t(unsigned int, arg->max_pages, 1));
 			}
+			if (arg->flags & FUSE_NONSHARED_FS) {
+				fc->nonshared_fs = 1;
+			}
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1002,7 +1005,8 @@ void fuse_send_init(struct fuse_conn *fc)
 		FUSE_WRITEBACK_CACHE | FUSE_NO_OPEN_SUPPORT |
 		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
 		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
-		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA;
+		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
+		FUSE_NONSHARED_FS;
 	ia->args.opcode = FUSE_INIT;
 	ia->args.in_numargs = 1;
 	ia->args.in_args[0].size = sizeof(ia->in);
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 373cada89815..bdb106d9f10b 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -172,6 +172,8 @@
  *  - add FUSE_WRITE_KILL_PRIV flag
  *  - add FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING
  *  - add map_alignment to fuse_init_out, add FUSE_MAP_ALIGNMENT flag
+ *  7.32
+ *  - add FUSE_NONSHARED_FS flag
  */
 
 #ifndef _LINUX_FUSE_H
@@ -314,6 +316,7 @@ struct fuse_file_lock {
  * FUSE_NO_OPENDIR_SUPPORT: kernel supports zero-message opendir
  * FUSE_EXPLICIT_INVAL_DATA: only invalidate cached pages on explicit request
  * FUSE_MAP_ALIGNMENT: map_alignment field is valid
+ * FUSE_NONSHARED_FS: Filesystem is not shared.
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -342,6 +345,7 @@ struct fuse_file_lock {
 #define FUSE_NO_OPENDIR_SUPPORT (1 << 24)
 #define FUSE_EXPLICIT_INVAL_DATA (1 << 25)
 #define FUSE_MAP_ALIGNMENT	(1 << 26)
+#define FUSE_NONSHARED_FS	(1 << 27)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.25.4

[PATCH 2/2] fuse: Enable SB_NOSEC if filesystem is not shared

[Vivek Goyal]

We don't enable SB_NOSEC on fuse filesystems thinking filesystem is
shared and files attrs setuid/setgid/capabilities can change without
fuse knowing about it. This means on every WRITE, file_remove_privs(),
is called and that calls into fuse server to figure out if security.capability
xattr has been set on file. Most of the time this is a performance hog,
specially for small writes done at high frequency.

Enable SB_NOSEC if fuse filesystem sets flag FS_NONSHARED_FS. This means,
do not expect file attrs/xattrs to change without the knowledge of
fuse. In this case it should be possible to enable SB_NOSEC.

For the case of shared filesystems, we will have to come up with a different
mechanism to enable SB_NOSEC. I guess it will depend on invalidation
mechanisms implemented by filesystem and cache coherency guarantees.

I do clear inode S_NOSEC flag whenever file attrs are being refreshed. So
this still honors attr timeout protocol.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 088faa3e352c..2da13fe25417 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -187,6 +187,9 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
 		inode->i_mode &= ~S_ISVTX;
 
 	fi->orig_ino = attr->ino;
+
+	/* Clear S_NOSEC whenever cached attrs are being refreshed */
+	inode->i_flags &= ~S_NOSEC;
 }
 
 void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
@@ -967,6 +970,9 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_args *args,
 			}
 			if (arg->flags & FUSE_NONSHARED_FS) {
 				fc->nonshared_fs = 1;
+				down_write(&fc->sb->s_umount);
+				fc->sb->s_flags |= SB_NOSEC;
+				up_write(&fc->sb->s_umount);
 			}
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
-- 
2.25.4

Re: [RFC PATCH 0/2] fuse: Enable SB_NOSEC if filesystem is not shared

[Vivek Goyal]

On Tue, Sep 01, 2020 at 04:40:43PM -0400, Vivek Goyal wrote:
> Hi,
> 
> I want to enable SB_NOSEC in fuse in some form so that performance of
> small random writes can be improved. As of now, we call file_remove_privs(),
> which results in fuse always sending getxattr(security.capability) to
> server to figure out if security.capability has been set on file or not.
> If it has been set, it needs to be cleared. This slows down small
> random writes tremendously.
> 
> I posted couple of proposals in the past here.
> 
> Proposal 1:
> 
> https://lore.kernel.org/linux-fsdevel/20200716144032.GC422759@redhat.com/
> 
> Proposal 2:
> 
> https://lore.kernel.org/linux-fsdevel/20200724183812.19573-1-vgoyal@redhat.com/
> 
> This is 3rd proposal now. One of the roadblocks in enabling SB_NOSEC
> is shared filesystem. It is possible that another client modified the
> file data and this client does not know about it. So we might regress
> if we don't fetch security.capability always.
> 
> So looks like this needs to be handled different for shared filesystems
> and non-shared filesystems. non-shared filesystems will be more like
> local filesystems where fuse does not expect file data/metadata to
> change outside the fuse. And we should be able to enable SB_NOSEC
> optimization. This is what this proposal does.
> 
> It does not handle the case of shared filesystems. I believe solution
> to that will depend on filesystem based on what's the cache coherency
> guarantees filesystem provides and what's the cache invalidation 
> mechanism it uses.
> 
> For now, all filesystems which are not shared can benefit from this
> optimization. I am interested in virtiofs which is not shared in
> many of the cases. In fact we don't even support shared mode yet. 

Here is the corresponding qemu virtiofsd patch which can enable this
feature.


Subject: virtiofsd: Enable FUSE_NONSHARED_FS by default

Set FUSE_NONSHARED_FS flag by default as we don't support sharing of
virtiofs filesystem yet. Once that is supported, it should not be set
for shared mode.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/standard-headers/linux/fuse.h |    4 ++++
 tools/virtiofsd/fuse_lowlevel.c       |    9 +++++++++
 2 files changed, 13 insertions(+)

Index: qemu/include/standard-headers/linux/fuse.h
===================================================================
--- qemu.orig/include/standard-headers/linux/fuse.h	2020-09-01 15:22:31.449707002 -0400
+++ qemu/include/standard-headers/linux/fuse.h	2020-09-01 15:23:18.776668542 -0400
@@ -172,6 +172,8 @@
  *  - add FUSE_WRITE_KILL_PRIV flag
  *  - add FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING
  *  - add map_alignment to fuse_init_out, add FUSE_MAP_ALIGNMENT flag
+ *  7.32
+ *  - add FUSE_NONSHARED_FS flag
  */
 
 #ifndef _LINUX_FUSE_H
@@ -310,6 +312,7 @@ struct fuse_file_lock {
  * FUSE_NO_OPENDIR_SUPPORT: kernel supports zero-message opendir
  * FUSE_EXPLICIT_INVAL_DATA: only invalidate cached pages on explicit request
  * FUSE_MAP_ALIGNMENT: map_alignment field is valid
+ * FUSE_NONSHARED_FS: Filesystem is not shared.
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -338,6 +341,7 @@ struct fuse_file_lock {
 #define FUSE_NO_OPENDIR_SUPPORT (1 << 24)
 #define FUSE_EXPLICIT_INVAL_DATA (1 << 25)
 #define FUSE_MAP_ALIGNMENT	(1 << 26)
+#define FUSE_NONSHARED_FS	(1 << 27)
 
 /**
  * CUSE INIT request/reply flags
Index: qemu/tools/virtiofsd/fuse_lowlevel.c
===================================================================
--- qemu.orig/tools/virtiofsd/fuse_lowlevel.c	2020-09-01 15:22:31.449707002 -0400
+++ qemu/tools/virtiofsd/fuse_lowlevel.c	2020-09-01 15:23:18.777668583 -0400
@@ -2218,6 +2218,15 @@ static void do_init(fuse_req_t req, fuse
         outarg.map_alignment = ffsl(sysconf(_SC_PAGE_SIZE)) - 1;
     }
 
+    if (arg->flags & FUSE_NONSHARED_FS) {
+        /*
+         * By default virtiofs is not shared. Once we support sharing,
+         * this will be have to a conditional and driven by user's
+         * selection.
+         */
+         outarg.flags |= FUSE_NONSHARED_FS;
+    }
+
     fuse_log(FUSE_LOG_DEBUG, "   INIT: %u.%u\n", outarg.major, outarg.minor);
     fuse_log(FUSE_LOG_DEBUG, "   flags=0x%08x\n", outarg.flags);
     fuse_log(FUSE_LOG_DEBUG, "   max_readahead=0x%08x\n", outarg.max_readahead);

Re: [PATCH 1/2] fuse: Add a flag FUSE_NONSHARED_FS

[Miklos Szeredi]

On Tue, Sep 1, 2020 at 10:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> FUSE_NONSHARED_FS will signify that filesystem is not shared. That is
> all fuse modifications are going thourgh this single fuse connection. It
> should not happen that file's data/metadata changed without the knowledge
> of fuse. Automatic file time stamp changes will probably be an exception
> to this rule.
>
> If filesystem is shared between different clients, then this flag should
> not be set.

I'm thinking maybe this flag should force some other parameters as well:

^FUSE_POSIX_LOCK
^FUSE_FLOCK_LOCKS
^FUSE_AUTO_INVAL_DATA
FUSE_EXPLICIT_INVAL_DATA
FUSE_CACHE_SYMLINKS
attr_valid=inf
entry_valid=inf
FOPEN_KEEP_CACHE
FOPEN_CACHE_DIR

This would make sure that it's really only used in the non-shared case.

Thanks,
Miklos

Re: [PATCH 1/2] fuse: Add a flag FUSE_NONSHARED_FS

[Vivek Goyal]

On Wed, Sep 02, 2020 at 08:57:07AM +0200, Miklos Szeredi wrote:
> On Tue, Sep 1, 2020 at 10:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > FUSE_NONSHARED_FS will signify that filesystem is not shared. That is
> > all fuse modifications are going thourgh this single fuse connection. It
> > should not happen that file's data/metadata changed without the knowledge
> > of fuse. Automatic file time stamp changes will probably be an exception
> > to this rule.
> >
> > If filesystem is shared between different clients, then this flag should
> > not be set.
> 
> I'm thinking maybe this flag should force some other parameters as well:

So you are thinking of fuse enforcing these parameters automatically
without server asking for it. IOW, override what server says about
following parameters and fuse sets their values if
FUSE_NONSHARED_FS is set?

Or should we recommend these for FUSE_NONSHARED_FS and let file server
specify all this.

> 
> ^FUSE_POSIX_LOCK

No remote posix locks. Makes sense. If filesystem is not shared then
there is no need of remote posix locks.

> ^FUSE_FLOCK_LOCKS

No remote BSD style file locks. If filesystem is not shared, then
local locks should work just fine.

> ^FUSE_AUTO_INVAL_DATA

This controls if page cache data should be invalidated upon mtime
change. For non shared fs, mtime should not change outside fuse, 
so makes sense to enforce ^FUSE_AUTO_INVAL_DATA.

> FUSE_EXPLICIT_INVAL_DATA

This tells fuse to not invalidate page cache and not truncate page cache
if attr->size or mtime change is detected. For non-shared fs we don't
expect attr->size or mtime to change, so it does not harm to enfroce
this.

> FUSE_CACHE_SYMLINKS

This caches readlink responses. Makes sense to enable it for
non shared fs. 

> attr_valid=inf
> entry_valid=inf

dentry and attr timeout being infinite should be good for performance
if filesystem is not shared.

> FOPEN_KEEP_CACHE

If this is not set, by default fuse invalidates page cache on open. Makes
sense to not flush page cache on open with FUSE_NONSHARED_FS.

> FOPEN_CACHE_DIR

Caching directory contents for FUSE_NONSHARED_FS makes sense too.

> 
> This would make sure that it's really only used in the non-shared case.

I am little afraid of enforcing this in fuse core because tomorrow
somebody will say hey I need hybrid mode where FUSE_NONSHARED_FS is
set but for some reason I want attrs to expire after some time. I 
don't have a good use case in my mind though.

If I were to choose, I will probably document it and suggest that
file servers sets all the above for the case of FUSE_NONSHARED_FS.

But I am also open to enforcing this in fuse core if you prefer
that option.

Thanks
Vivek

Re: [RFC PATCH 0/2] fuse: Enable SB_NOSEC if filesystem is not shared

[Vivek Goyal]

On Tue, Sep 01, 2020 at 04:40:43PM -0400, Vivek Goyal wrote:
> Hi,
> 
> I want to enable SB_NOSEC in fuse in some form so that performance of
> small random writes can be improved. As of now, we call file_remove_privs(),
> which results in fuse always sending getxattr(security.capability) to
> server to figure out if security.capability has been set on file or not.
> If it has been set, it needs to be cleared. This slows down small
> random writes tremendously.
> 
> I posted couple of proposals in the past here.
> 
> Proposal 1:
> 
> https://lore.kernel.org/linux-fsdevel/20200716144032.GC422759@redhat.com/
> 
> Proposal 2:
> 
> https://lore.kernel.org/linux-fsdevel/20200724183812.19573-1-vgoyal@redhat.com/
> 
> This is 3rd proposal now. One of the roadblocks in enabling SB_NOSEC
> is shared filesystem. It is possible that another client modified the
> file data and this client does not know about it. So we might regress
> if we don't fetch security.capability always.
> 
> So looks like this needs to be handled different for shared filesystems
> and non-shared filesystems. non-shared filesystems will be more like
> local filesystems where fuse does not expect file data/metadata to
> change outside the fuse. And we should be able to enable SB_NOSEC
> optimization. This is what this proposal does.
> 
> It does not handle the case of shared filesystems. I believe solution
> to that will depend on filesystem based on what's the cache coherency
> guarantees filesystem provides and what's the cache invalidation 
> mechanism it uses.
> 
> For now, all filesystems which are not shared can benefit from this
> optimization. I am interested in virtiofs which is not shared in
> many of the cases. In fact we don't even support shared mode yet. 

Well, I was hoping that virtiofs and kata containers can directly
benefit from this mode for root filesystem image. But Eric Ernst
says that kata containers keep bunch of things in a single directory
being exported to guest. And while rootfs image is not expected to
be updated later, it is possile kubernetes updates other parts
later.

And that most likely means kata will not use virtiofs non-shared
mode. 

I guess I need to keep this idea on hold for now because I will not
have any immediate users. And go back to drawing board and figure out
how to not query security.capability on every WRITE.

Thanks
Vivek

> 
> Any comments or feedback is welcome.
> 
> Thanks
> Vivek
> 
> Vivek Goyal (2):
>   fuse: Add a flag FUSE_NONSHARED_FS
>   fuse: Enable SB_NOSEC if filesystem is not shared
> 
>  fs/fuse/fuse_i.h          |  3 +++
>  fs/fuse/inode.c           | 12 +++++++++++-
>  include/uapi/linux/fuse.h |  4 ++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> -- 
> 2.25.4
>