[PATCH] overlayfs: set MS_NOSEC flag for overlayfs

[PATCH] overlayfs: set MS_NOSEC flag for overlayfs

[Jeffle Xu]

Since the stacking of regular file operations [1], the overlayfs
edition of write_iter() is called when writing regular files.

Since then, xattr lookup is needed on every write since file_remove_privs()
is called from ovl_write_iter(), which would become the performance
bottleneck when writing small chunks of data. In my test case,
file_remove_privs() would consume ~15% CPU when running fstime of
unixbench (the workload is repeadly writing 1 KB to the same file) [2].

Set the MS_NOSEC flag for overlayfs superblock. Since then xattr lookup
would be done only once on the first write. Unixbench fstime gets a ~20%
performance gain with this patch.

[1] https://lore.kernel.org/lkml/20180606150905.GC9426@magnolia/T/
[2] https://www.spinics.net/lists/linux-unionfs/msg07153.html

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/overlayfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 732ad54..0b047ce 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1817,7 +1817,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_magic = OVERLAYFS_SUPER_MAGIC;
 	sb->s_xattr = ovl_xattr_handlers;
 	sb->s_fs_info = ofs;
-	sb->s_flags |= SB_POSIXACL;
+	sb->s_flags |= (SB_POSIXACL | SB_NOSEC);
 
 	err = -ENOMEM;
 	root_dentry = ovl_get_root(sb, upperpath.dentry, oe);
-- 
1.8.3.1

Re: [PATCH] overlayfs: set MS_NOSEC flag for overlayfs

[JeffleXu]

It seems that MS_NOSEC flag would be problematic for network filesystems.


@Amir, would you please give some suggestions on if this would break the

permission control down when 'NFS export' feature enabled ?


On 4/23/20 2:57 PM, Jeffle Xu wrote:
> Since the stacking of regular file operations [1], the overlayfs
> edition of write_iter() is called when writing regular files.
>
> Since then, xattr lookup is needed on every write since file_remove_privs()
> is called from ovl_write_iter(), which would become the performance
> bottleneck when writing small chunks of data. In my test case,
> file_remove_privs() would consume ~15% CPU when running fstime of
> unixbench (the workload is repeadly writing 1 KB to the same file) [2].
>
> Set the MS_NOSEC flag for overlayfs superblock. Since then xattr lookup
> would be done only once on the first write. Unixbench fstime gets a ~20%
> performance gain with this patch.
>
> [1] https://lore.kernel.org/lkml/20180606150905.GC9426@magnolia/T/
> [2] https://www.spinics.net/lists/linux-unionfs/msg07153.html
>
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>   fs/overlayfs/super.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 732ad54..0b047ce 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1817,7 +1817,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>   	sb->s_magic = OVERLAYFS_SUPER_MAGIC;
>   	sb->s_xattr = ovl_xattr_handlers;
>   	sb->s_fs_info = ofs;
> -	sb->s_flags |= SB_POSIXACL;
> +	sb->s_flags |= (SB_POSIXACL | SB_NOSEC);
>   
>   	err = -ENOMEM;
>   	root_dentry = ovl_get_root(sb, upperpath.dentry, oe);

Re: [PATCH] overlayfs: set MS_NOSEC flag for overlayfs

[Amir Goldstein]

On Thu, Apr 23, 2020 at 10:06 AM JeffleXu <jefflexu@linux.alibaba.com> wrote:
>
> It seems that MS_NOSEC flag would be problematic for network filesystems.
>
>
> @Amir, would you please give some suggestions on if this would break the
>
> permission control down when 'NFS export' feature enabled ?
>

I cannot think of anything specific to NFS export.
I think you are confusing NFS server with NFS client permissions.
I think network filesystems do not set SB_NOSEC, because client
may not have an coherent state of the xattr on server and other clients.

To reflect on overlayfs, I think overlayfs should inherit the SB_NOSEC
flag from upper fs, which is most likelihood will be set.
The only filesystem I can think of that is used for upper fs without
SB_NOSEC is the recent feature of fuse as upper fs merged to
v5.7-rc1.

Thanks,
Amir.

Re: [PATCH] overlayfs: set MS_NOSEC flag for overlayfs

[JeffleXu]

On 4/23/20 4:27 PM, Amir Goldstein wrote:
> On Thu, Apr 23, 2020 at 10:06 AM JeffleXu <jefflexu@linux.alibaba.com> wrote:
>> It seems that MS_NOSEC flag would be problematic for network filesystems.
>>
>>
>> @Amir, would you please give some suggestions on if this would break the
>>
>> permission control down when 'NFS export' feature enabled ?
>>
> I cannot think of anything specific to NFS export.
> I think you are confusing NFS server with NFS client permissions.
> I think network filesystems do not set SB_NOSEC, because client
> may not have an coherent state of the xattr on server and other clients.
>
> To reflect on overlayfs, I think overlayfs should inherit the SB_NOSEC
> flag from upper fs, which is most likelihood will be set.

Makes sense. So maybe the following patch would be more appropriate. If 
it is OK I will send a v2 patch then.

```

--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1052,6 +1052,10 @@ static int ovl_get_upper(struct super_block *sb, 
struct ovl_fs *ofs,
         upper_mnt->mnt_flags &= ~(MNT_NOATIME | MNT_NODIRATIME | 
MNT_RELATIME);
         ofs->upper_mnt = upper_mnt;

+       /* inherit SB_NOSEC flag from upperdir */
+       if (upper_mnt->mnt_sb->s_flags & SB_NOSEC)
+               sb->s_flags |= SB_NOSEC;
+
         if (ovl_inuse_trylock(ofs->upper_mnt->mnt_root)) {
                 ofs->upperdir_locked = true;
         } else {

```

> The only filesystem I can think of that is used for upper fs without
> SB_NOSEC is the recent feature of fuse as upper fs merged to
> v5.7-rc1.
>
> Thanks,
> Amir.

Re: [PATCH] overlayfs: set MS_NOSEC flag for overlayfs

[Amir Goldstein]

On Thu, Apr 23, 2020 at 1:17 PM JeffleXu <jefflexu@linux.alibaba.com> wrote:
>
>
> On 4/23/20 4:27 PM, Amir Goldstein wrote:
> > On Thu, Apr 23, 2020 at 10:06 AM JeffleXu <jefflexu@linux.alibaba.com> wrote:
> >> It seems that MS_NOSEC flag would be problematic for network filesystems.
> >>
> >>
> >> @Amir, would you please give some suggestions on if this would break the
> >>
> >> permission control down when 'NFS export' feature enabled ?
> >>
> > I cannot think of anything specific to NFS export.
> > I think you are confusing NFS server with NFS client permissions.
> > I think network filesystems do not set SB_NOSEC, because client
> > may not have an coherent state of the xattr on server and other clients.
> >
> > To reflect on overlayfs, I think overlayfs should inherit the SB_NOSEC
> > flag from upper fs, which is most likelihood will be set.
>
> Makes sense. So maybe the following patch would be more appropriate. If
> it is OK I will send a v2 patch then.
>

Yes, it looks better.
But I could be missing other aspects.
Better post v2 and wait for more comments from Miklos.

Thanks,
Amir.