Re: [V9fs-developer] [PATCH] fs/9p: if O_APPEND, seek to EOF on write, not open

Re: [V9fs-developer] [PATCH] fs/9p: if O_APPEND, seek to EOF on write, not open

[Dominique Martinet]

Hi,

adding linux-fsdevel@vger in Cc, there's more people who know about this
than me there

Nicola Girardi wrote on Mon, Mar 22, 2021 at 04:35:53PM +0000:
> Quoting the POSIX standard:¹
> 
> 	If the O_APPEND flag of the file status flags is set, the file
> 	offset shall be set to the end of the file prior to each write and
> 	no intervening file modification operation shall occur between
> 	changing the file offset and the write operation.
> 
> Previously, the seek to EOF was only done on open instead.
> 
> ¹ https://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
> ---
> 
> I noticed the above minor deviation from POSIX while running a tester
> that looks for differences between ext4 and a fs of mine, mounted
> using v9fs. I wrote a small program to reproduce the problem and
> validate the fix:
> https://raw.githubusercontent.com/nicolagi/fsdiff/master/c/repro-read-append.c.

Sorry for the delay in replying, I just didn't take time to test until
now.
So the thing is given how the current 9p servers I know are implemented,
file IOs will be backed by a file which has been opened in O_APPEND and
the behaviour will transparently be correct for them; that's probably
why nobody ever caught up on this until now.


I think it makes sense however, so I'll take the patch unless someone
complains; please note that in case of concurrent accesses a client-side
implementation will not guarantee proper O_APPEND behaviour so it should
really be enforced by the server; because generic_file_llseek relies on
the size stored in the inode in cache (and it would be racy anyway if it
were to refresh attributes)

e.g. if two clients are opening the same file in O_APPEND and alternate
writing to it, they will just be overwriting each other on your server
implementation.


Well, generic_file_llseek has close to zero extra cost so it doesn't
cost much to include this safety, I'll just adjust the commit message to
warn of this pitfall and include the patch after some more testing.

Thanks,

> 
>  fs/9p/vfs_file.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index be5768949cb1..8e9da3abd498 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -68,9 +68,6 @@ int v9fs_file_open(struct inode *inode, struct file *file)
>  			p9_client_clunk(fid);
>  			return err;
>  		}
> -		if ((file->f_flags & O_APPEND) &&
> -			(!v9fs_proto_dotu(v9ses) && !v9fs_proto_dotl(v9ses)))
> -			generic_file_llseek(file, 0, SEEK_END);
>  	}
>  
>  	file->private_data = fid;
> @@ -419,6 +416,8 @@ v9fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	if (retval <= 0)
>  		return retval;
>  
> +	if (file->f_flags & O_APPEND)
> +		generic_file_llseek(file, 0, SEEK_END);
>  	origin = iocb->ki_pos;
>  	retval = p9_client_write(file->private_data, iocb->ki_pos, from, &err);
>  	if (retval > 0) {
-- 
Dominique