diff mbox series

9p: Fix initialisation of netfs_inode for 9p

Message ID 292837.1704232179@warthog.procyon.org.uk
State Superseded
Headers show
Series 9p: Fix initialisation of netfs_inode for 9p | expand

Commit Message

David Howells Jan. 2, 2024, 9:49 p.m. UTC
This needs a fix that I would fold in.  Somehow it gets through xfstests
without it, but it seems problems can be caused with executables.

David
---
9p: Fix initialisation of netfs_inode for 9p

The 9p filesystem is calling netfs_inode_init() in v9fs_init_inode() -
before the struct inode fields have been initialised from the obtained file
stats (ie. after v9fs_stat2inode*() has been called), but netfslib wants to
set a couple of its fields from i_size.

Reported-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Van Hensbergen <ericvh@kernel.org>
cc: Latchesar Ionkov <lucho@ionkov.net>
cc: Dominique Martinet <asmadeus@codewreck.org>
cc: Christian Schoenebeck <linux_oss@crudebyte.com>
cc: v9fs@lists.linux.dev
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
---
 fs/9p/v9fs_vfs.h       |    1 +
 fs/9p/vfs_inode.c      |    6 +++---
 fs/9p/vfs_inode_dotl.c |    1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

Comments

Dominique Martinet Jan. 3, 2024, 1:10 p.m. UTC | #1
David Howells wrote on Tue, Jan 02, 2024 at 09:49:39PM +0000:
> This needs a fix that I would fold in.  Somehow it gets through xfstests
> without it, but it seems problems can be caused with executables.

Looking at a file without that patch we seem to be reading just zeroes
off pre-existing files; I'm surprised xfstest doesn't have a write
something/umount/mount/check content is identical test...

(You're probably aware of this, but note for others this breaks with the
rest of the patch series even if the big 9p patch isn't applied -- this
is the main reason I'd rather just get the patch in this cycle, as the
new patches got more tests with the full series than with the 9p writes
patch dropped.)


> 9p: Fix initialisation of netfs_inode for 9p
> 
> The 9p filesystem is calling netfs_inode_init() in v9fs_init_inode() -
> before the struct inode fields have been initialised from the obtained file
> stats (ie. after v9fs_stat2inode*() has been called), but netfslib wants to
> set a couple of its fields from i_size.

Would it make sense to just always update netfs's ctx->remote_i_size in
the various stat2inode calls instead?
We don't have any cache coherency so if a file changes beneath us
through an edit on the server (or through another client) hell will
break loose anyway, but stat would update the inode's i_size so it'll
likely be weird that the remote_i_size doesn't get updated.


> Reported-by: Marc Dionne <marc.dionne@auristor.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Tested-by: Marc Dionne <marc.dionne@auristor.com>
> cc: Eric Van Hensbergen <ericvh@kernel.org>
> cc: Latchesar Ionkov <lucho@ionkov.net>
> cc: Dominique Martinet <asmadeus@codewreck.org>

With that being said, it seems to do the immediate job:
Acked-by: Dominique Martinet <asmadeus@codewreck.org>
David Howells Jan. 3, 2024, 1:59 p.m. UTC | #2
Dominique Martinet <asmadeus@codewreck.org> wrote:

> Would it make sense to just always update netfs's ctx->remote_i_size in
> the various stat2inode calls instead?

Yeah, that's probably a good idea.

David
David Howells Jan. 3, 2024, 2:04 p.m. UTC | #3
Dominique Martinet <asmadeus@codewreck.org> wrote:

> Would it make sense to just always update netfs's ctx->remote_i_size in
> the various stat2inode calls instead?

Btw, v9fs_i_size_write() should be redundant.  It should be sufficient to just
use i_size_write() as long as you use i_size_read().

David
diff mbox series

Patch

diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
index 731e3d14b67d..0e8418066a48 100644
--- a/fs/9p/v9fs_vfs.h
+++ b/fs/9p/v9fs_vfs.h
@@ -42,6 +42,7 @@  struct inode *v9fs_alloc_inode(struct super_block *sb);
 void v9fs_free_inode(struct inode *inode);
 struct inode *v9fs_get_inode(struct super_block *sb, umode_t mode,
 			     dev_t rdev);
+void v9fs_set_netfs_context(struct inode *inode);
 int v9fs_init_inode(struct v9fs_session_info *v9ses,
 		    struct inode *inode, umode_t mode, dev_t rdev);
 void v9fs_evict_inode(struct inode *inode);
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index b66466e97459..32572982f72e 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -246,7 +246,7 @@  void v9fs_free_inode(struct inode *inode)
 /*
  * Set parameters for the netfs library
  */
-static void v9fs_set_netfs_context(struct inode *inode)
+void v9fs_set_netfs_context(struct inode *inode)
 {
 	struct v9fs_inode *v9inode = V9FS_I(inode);
 	netfs_inode_init(&v9inode->netfs, &v9fs_req_ops, true);
@@ -326,8 +326,6 @@  int v9fs_init_inode(struct v9fs_session_info *v9ses,
 		err = -EINVAL;
 		goto error;
 	}
-
-	v9fs_set_netfs_context(inode);
 error:
 	return err;
 
@@ -359,6 +357,7 @@  struct inode *v9fs_get_inode(struct super_block *sb, umode_t mode, dev_t rdev)
 		iput(inode);
 		return ERR_PTR(err);
 	}
+	v9fs_set_netfs_context(inode);
 	return inode;
 }
 
@@ -461,6 +460,7 @@  static struct inode *v9fs_qid_iget(struct super_block *sb,
 		goto error;
 
 	v9fs_stat2inode(st, inode, sb, 0);
+	v9fs_set_netfs_context(inode);
 	v9fs_cache_inode_get_cookie(inode);
 	unlock_new_inode(inode);
 	return inode;
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index e25fbc988f09..3505227e1704 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -128,6 +128,7 @@  static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
 		goto error;
 
 	v9fs_stat2inode_dotl(st, inode, 0);
+	v9fs_set_netfs_context(inode);
 	v9fs_cache_inode_get_cookie(inode);
 	retval = v9fs_get_acl(inode, fid);
 	if (retval)