Re: [PATCH 1/1] fs/9p: Do not open remote file with APPEND mode when writeback cache is used

[Dominique Martinet <asmadeus@codewreck.org>]

Tingmao Wang wrote on Sun, Nov 02, 2025 at 08:24:39PM +0000:
> 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.

Ugh.. I'd never expect O_APPEND to have any effect on the server tbh,
9p writes are "pwrite"-like, the offset is always specified -- there is
no plain write(fid, data) call -- so O_APPEND should have nothing to do
with the remote :/

(well, man pwrite(2) does say this... So that explains the behavior if
qemu respects the flag:
BUGS
   POSIX requires that opening a file with the O_APPEND flag should have
   no effect on the location at which pwrite() writes data. However, on
   Linux, if a file is opened with O_APPEND, pwrite() appends data to the
   end of the file, regardless of the value of offset. 
)

I guess I can see this being useful if multiple clients are involved in
cacheless mode, but I agree any kind of data cache should strip this
flag.
(I think this didn't cause problem in other cache modes because writes
through the page cache happen on a writeback fid that's opened with
fixed (no O_APPEND) flags?...)


> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 612a230bc012..ff95dfb30583 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));
> @@ -61,6 +65,11 @@ int v9fs_file_open(struct inode *inode, struct file *file)
>  			int writeback_omode = (omode & ~P9_OWRITE) | P9_ORDWR;
>  
>  			p9_debug(P9_DEBUG_CACHE, "write-only file with writeback enabled, try opening O_RDWR\n");
> +			if (omode & o_append) {
> +				writeback_omode &= ~o_append;
> +				p9_debug(P9_DEBUG_CACHE, "removing append from open mode in writeback cache mode\n");
> +			}

I wouldn't bother with the debug message, just clear it like P9_OWRITE:
   int writebeack_omode = (omode & ~(P9_OWRITE|o_append)) | P9_ORDWR;

(same for other hunks)
-- 
Dominique Martinet | Asmadeus