Re: [PATCH v2] fs/9p: Don't open remote file with APPEND mode when writeback cache is used

[Christian Schoenebeck <linux_oss@crudebyte.com>]

On Monday, November 3, 2025 12:56:30 AM CET Tingmao Wang wrote:
> When page cache is used, writebacks are done on a page granularity, and it
> is expected that the underlying filesystem (such as v9fs) should respect
> the write position.  However, currently v9fs will passthrough O_APPEND to
> the server even on cached mode.  This causes data corruption if a sync or
> fstat gets between two writes to the same file.
> 
> This patch removes the APPEND flag from the open request we send to the
> server when writeback caching is involved.  I believe keeping server-side
> APPEND is probably fine for uncached mode (even if two fds are opened, one
> without O_APPEND and one with it, this should still be fine since they
> would use separate fid for the writes).
> 
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
> 
> I haven't done a bisect to figure out if this regression was introduced by
> a change or was this behaviour always present - 6.14 has the same problem
> and 6.13 did not compile for me due to some problem with bool / true /
> false being a keyword in c23, and it could not compile with c17 either.
> 
>  fs/9p/vfs_file.c       | 11 ++++++++---
>  fs/9p/vfs_inode.c      |  3 +--
>  fs/9p/vfs_inode_dotl.c |  2 +-
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 612a230bc012..6f3880208587 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -43,14 +43,18 @@ int v9fs_file_open(struct inode *inode, struct file
> *file) struct v9fs_session_info *v9ses;
>  	struct p9_fid *fid;
>  	int omode;
> +	int o_append;
> 
>  	p9_debug(P9_DEBUG_VFS, "inode: %p file: %p\n", inode, file);
>  	v9ses = v9fs_inode2v9ses(inode);
> -	if (v9fs_proto_dotl(v9ses))
> +	if (v9fs_proto_dotl(v9ses)) {
>  		omode = v9fs_open_to_dotl_flags(file->f_flags);
> -	else
> +		o_append = P9_DOTL_APPEND;
> +	} else {
>  		omode = v9fs_uflags2omode(file->f_flags,
>  					v9fs_proto_dotu(v9ses));
> +		o_append = P9_OAPPEND;
> +	}
>  	fid = file->private_data;
>  	if (!fid) {
>  		fid = v9fs_fid_clone(file_dentry(file));
> @@ -58,9 +62,10 @@ int v9fs_file_open(struct inode *inode, struct file
> *file) return PTR_ERR(fid);
> 
>  		if ((v9ses->cache & CACHE_WRITEBACK) && (omode & P9_OWRITE)) {
> -			int writeback_omode = (omode & ~P9_OWRITE) | P9_ORDWR;
> +			int writeback_omode = (omode & ~(P9_OWRITE | o_append)) | P9_ORDWR;

I guess changes in this function could be simplified by either pulling
O_APPEND at the very beginning right after v9ses = v9fs_inode2v9ses(inode); or
by simply pulling both P9_OAPPEND and P9_DOTL_APPEND here, but it's doing the
job so:

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

/Christian

> 
>  			p9_debug(P9_DEBUG_CACHE, "write-only file with writeback enabled, try
> opening O_RDWR\n"); +
>  			err = p9_client_open(fid, writeback_omode);
>  			if (err < 0) {
>  				p9_debug(P9_DEBUG_CACHE, "could not open O_RDWR, disabling caches\n");
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 8666c9c62258..97abe65bf7c1 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -786,7 +786,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry
> *dentry, p9_omode = v9fs_uflags2omode(flags, v9fs_proto_dotu(v9ses));
> 
>  	if ((v9ses->cache & CACHE_WRITEBACK) && (p9_omode & P9_OWRITE)) {
> -		p9_omode = (p9_omode & ~P9_OWRITE) | P9_ORDWR;
> +		p9_omode = (p9_omode & ~(P9_OWRITE | P9_OAPPEND)) | P9_ORDWR;
>  		p9_debug(P9_DEBUG_CACHE,
>  			"write-only file with writeback enabled, creating w/ O_RDWR\n");
>  	}
> @@ -1393,4 +1393,3 @@ static const struct inode_operations
> v9fs_symlink_inode_operations = { .getattr = v9fs_vfs_getattr,
>  	.setattr = v9fs_vfs_setattr,
>  };
> -
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 1661a25f2772..643e759eacb2 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -282,7 +282,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct
> dentry *dentry, }
> 
>  	if ((v9ses->cache & CACHE_WRITEBACK) && (p9_omode & P9_OWRITE)) {
> -		p9_omode = (p9_omode & ~P9_OWRITE) | P9_ORDWR;
> +		p9_omode = (p9_omode & ~(P9_OWRITE | P9_DOTL_APPEND)) | P9_ORDWR;
>  		p9_debug(P9_DEBUG_CACHE,
>  			"write-only file with writeback enabled, creating w/ O_RDWR\n");
>  	}