diff mbox series

[v4,04/11] fs/9p: Remove unnecessary superblock flags

Message ID 20230218003323.2322580-5-ericvh@kernel.org
State New
Headers
Series Performance fixes for 9p filesystem |

Commit Message

Eric Van Hensbergen Feb. 18, 2023, 12:33 a.m. UTC 
  These flags just add unnecessary extra operations.
When 9p is run without cache, it inherently implements
these options so we don't need them in the superblock
(which ends up sending extraneous fsyncs, etc.).  User
can still request these options on mount, but we don't
need to set them as default.

Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
---
 fs/9p/vfs_super.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Dominique Martinet Feb. 18, 2023, 9:33 a.m. UTC | #1 
Eric Van Hensbergen wrote on Sat, Feb 18, 2023 at 12:33:16AM +0000:
> These flags just add unnecessary extra operations.
> When 9p is run without cache, it inherently implements
> these options so we don't need them in the superblock
> (which ends up sending extraneous fsyncs, etc.).  User
> can still request these options on mount, but we don't
> need to set them as default.

Hm, I don't see where they'd add any operations -- if you have time
would you mind pointing me at some?

As far as I can see, it's just about 'sync' or 'dirsync' in /proc/mounts
and the ST_SYNCHRONOUS statvfs flag; that looks harmless to me and it
looks more correct to keep to me.

(Sorry, didn't take the time to actually try taking a trace; I've
checked the flag itself and the IS_SYNC/IS_DIRSYNC -> inode_needs_sync
wrappers and that only seems used by specific filesystems who'd care
about users setting the mount options, not the other way aorund.)
Eric Van Hensbergen Feb. 18, 2023, 4:24 p.m. UTC | #2 
That's fair -- and it didn't seem to hurt anything to have DIRSYNC at
the moment, so I can drop this patch if we think its too much noise.
I guess it was more of a reaction the filesystem implicitly setting
mount flags which might override whatever the user intended.  FWIW
SB_SYNCHRONOUS did seem to have an effect on behavior (although I
didn't specifically track down where) -- I noticed this because the
problems Christian found seemed to go away if I mounted the filesystem
with sync (which basically ended up overriding aspects of the cache
configuration I guess).

     -eric

On Sat, Feb 18, 2023 at 3:34 AM <asmadeus@codewreck.org> wrote:
>
> Eric Van Hensbergen wrote on Sat, Feb 18, 2023 at 12:33:16AM +0000:
> > These flags just add unnecessary extra operations.
> > When 9p is run without cache, it inherently implements
> > these options so we don't need them in the superblock
> > (which ends up sending extraneous fsyncs, etc.).  User
> > can still request these options on mount, but we don't
> > need to set them as default.
>
> Hm, I don't see where they'd add any operations -- if you have time
> would you mind pointing me at some?
>
> As far as I can see, it's just about 'sync' or 'dirsync' in /proc/mounts
> and the ST_SYNCHRONOUS statvfs flag; that looks harmless to me and it
> looks more correct to keep to me.
>
> (Sorry, didn't take the time to actually try taking a trace; I've
> checked the flag itself and the IS_SYNC/IS_DIRSYNC -> inode_needs_sync
> wrappers and that only seems used by specific filesystems who'd care
> about users setting the mount options, not the other way aorund.)
>
> --
> Dominique
Dominique Martinet Feb. 18, 2023, 8:30 p.m. UTC | #3 
Eric Van Hensbergen wrote on Sat, Feb 18, 2023 at 10:24:17AM -0600:
> That's fair -- and it didn't seem to hurt anything to have DIRSYNC at
> the moment, so I can drop this patch if we think its too much noise.
> I guess it was more of a reaction the filesystem implicitly setting
> mount flags which might override whatever the user intended.  FWIW
> SB_SYNCHRONOUS did seem to have an effect on behavior (although I
> didn't specifically track down where) -- I noticed this because the
> problems Christian found seemed to go away if I mounted the filesystem
> with sync (which basically ended up overriding aspects of the cache
> configuration I guess).

I guess it doesn't hurt either way; happy to keep this patch in doubt.
diff mbox series

Patch

diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index 266c4693e20c..65d96fa94ba2 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -84,9 +84,7 @@  v9fs_fill_super(struct super_block *sb, struct v9fs_session_info *v9ses,
 		sb->s_bdi->io_pages = v9ses->maxdata >> PAGE_SHIFT;
 	}
 
-	sb->s_flags |= SB_ACTIVE | SB_DIRSYNC;
-	if (!v9ses->cache)
-		sb->s_flags |= SB_SYNCHRONOUS;
+	sb->s_flags |= SB_ACTIVE;
 
 #ifdef CONFIG_9P_FS_POSIX_ACL
 	if ((v9ses->flags & V9FS_ACL_MASK) == V9FS_POSIX_ACL)