RE: [PATCH v6 11/15] cifs: When caching, try to open O_WRONLY file rdwr on server

[Naveen Mamindlapalli <naveenm@marvell.com>]

> -----Original Message-----
> From: David Howells <dhowells@redhat.com>
> Sent: Thursday, March 28, 2024 10:28 PM
> To: Steve French <smfrench@gmail.com>
> Cc: David Howells <dhowells@redhat.com>; Jeff Layton <jlayton@kernel.org>;
> Matthew Wilcox <willy@infradead.org>; Paulo Alcantara <pc@manguebit.com>;
> Shyam Prasad N <sprasad@microsoft.com>; Tom Talpey <tom@talpey.com>;
> Christian Brauner <christian@brauner.io>; netfs@lists.linux.dev; linux-
> cifs@vger.kernel.org; linux-fsdevel@vger.kernel.org; linux-mm@kvack.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Steve French
> <sfrench@samba.org>; Shyam Prasad N <nspmangalore@gmail.com>; Rohith
> Surabattula <rohiths.msft@gmail.com>
> Subject: [PATCH v6 11/15] cifs: When caching, try to open
> O_WRONLY file rdwr on server
> 
> When we're engaged in local caching of a cifs filesystem, we cannot perform
> caching of a partially written cache granule unless we can read the rest of the
> granule.  To deal with this, if a file is opened O_WRONLY locally, but the mount
> was given the "-o fsc" flag, try first opening the remote file with
> GENERIC_READ|GENERIC_WRITE and if that returns -EACCES, try dropping
> the GENERIC_READ and doing the open again.  If that last succeeds, invalidate
> the cache for that file as for O_DIRECT.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Steve French <sfrench@samba.org>
> cc: Shyam Prasad N <nspmangalore@gmail.com>
> cc: Rohith Surabattula <rohiths.msft@gmail.com>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: linux-cifs@vger.kernel.org
> cc: netfs@lists.linux.dev
> cc: linux-fsdevel@vger.kernel.org
> cc: linux-mm@kvack.org
> ---
>  fs/smb/client/dir.c     | 15 ++++++++++++
>  fs/smb/client/file.c    | 51 +++++++++++++++++++++++++++++++++--------
>  fs/smb/client/fscache.h |  6 +++++
>  3 files changed, 62 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c index
> 89333d9bce36..37897b919dd5 100644
> --- a/fs/smb/client/dir.c
> +++ b/fs/smb/client/dir.c
> @@ -189,6 +189,7 @@ static int cifs_do_create(struct inode *inode, struct dentry
> *direntry, unsigned
>  	int disposition;
>  	struct TCP_Server_Info *server = tcon->ses->server;
>  	struct cifs_open_parms oparms;
> +	int rdwr_for_fscache = 0;
> 
>  	*oplock = 0;
>  	if (tcon->ses->server->oplocks)
> @@ -200,6 +201,10 @@ static int cifs_do_create(struct inode *inode, struct
> dentry *direntry, unsigned
>  		return PTR_ERR(full_path);
>  	}
> 
> +	/* If we're caching, we need to be able to fill in around partial writes. */
> +	if (cifs_fscache_enabled(inode) && (oflags & O_ACCMODE) ==
> O_WRONLY)
> +		rdwr_for_fscache = 1;
> +
>  #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
>  	if (tcon->unix_ext && cap_unix(tcon->ses) && !tcon->broken_posix_open
> &&
>  	    (CIFS_UNIX_POSIX_PATH_OPS_CAP &
> @@ -276,6 +281,8 @@ static int cifs_do_create(struct inode *inode, struct dentry
> *direntry, unsigned
>  		desired_access |= GENERIC_READ; /* is this too little? */
>  	if (OPEN_FMODE(oflags) & FMODE_WRITE)
>  		desired_access |= GENERIC_WRITE;
> +	if (rdwr_for_fscache == 1)
> +		desired_access |= GENERIC_READ;
> 
>  	disposition = FILE_OVERWRITE_IF;
>  	if ((oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL)) @@ -
> 304,6 +311,7 @@ static int cifs_do_create(struct inode *inode, struct dentry
> *direntry, unsigned
>  	if (!tcon->unix_ext && (mode & S_IWUGO) == 0)
>  		create_options |= CREATE_OPTION_READONLY;
> 
> +retry_open:
>  	oparms = (struct cifs_open_parms) {
>  		.tcon = tcon,
>  		.cifs_sb = cifs_sb,
> @@ -317,8 +325,15 @@ static int cifs_do_create(struct inode *inode, struct
> dentry *direntry, unsigned
>  	rc = server->ops->open(xid, &oparms, oplock, buf);
>  	if (rc) {
>  		cifs_dbg(FYI, "cifs_create returned 0x%x\n", rc);
> +		if (rc == -EACCES && rdwr_for_fscache == 1) {
> +			desired_access &= ~GENERIC_READ;
> +			rdwr_for_fscache = 2;
> +			goto retry_open;
> +		}
>  		goto out;
>  	}
> +	if (rdwr_for_fscache == 2)
> +		cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
> 
>  #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
>  	/*
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c index
> 73573dadf90e..761a80963f76 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -521,12 +521,12 @@ cifs_mark_open_files_invalid(struct cifs_tcon *tcon)
>  	 */
>  }
> 
> -static inline int cifs_convert_flags(unsigned int flags)
> +static inline int cifs_convert_flags(unsigned int flags, int
> +rdwr_for_fscache)
>  {
>  	if ((flags & O_ACCMODE) == O_RDONLY)
>  		return GENERIC_READ;
>  	else if ((flags & O_ACCMODE) == O_WRONLY)
> -		return GENERIC_WRITE;
> +		return rdwr_for_fscache == 1 ? (GENERIC_READ |
> GENERIC_WRITE) :
> +GENERIC_WRITE;
>  	else if ((flags & O_ACCMODE) == O_RDWR) {
>  		/* GENERIC_ALL is too much permission to request
>  		   can cause unnecessary access denied on create */ @@ -
> 663,11 +663,16 @@ static int cifs_nt_open(const char *full_path, struct inode
> *inode, struct cifs_
>  	int create_options = CREATE_NOT_DIR;
>  	struct TCP_Server_Info *server = tcon->ses->server;
>  	struct cifs_open_parms oparms;
> +	int rdwr_for_fscache = 0;
> 
>  	if (!server->ops->open)
>  		return -ENOSYS;
> 
> -	desired_access = cifs_convert_flags(f_flags);
> +	/* If we're caching, we need to be able to fill in around partial writes. */
> +	if (cifs_fscache_enabled(inode) && (f_flags & O_ACCMODE) ==
> O_WRONLY)
> +		rdwr_for_fscache = 1;
> +
> +	desired_access = cifs_convert_flags(f_flags, rdwr_for_fscache);
> 
>  /*********************************************************************
>   *  open flag mapping table:
> @@ -704,6 +709,7 @@ static int cifs_nt_open(const char *full_path, struct inode
> *inode, struct cifs_
>  	if (f_flags & O_DIRECT)
>  		create_options |= CREATE_NO_BUFFER;
> 
> +retry_open:
>  	oparms = (struct cifs_open_parms) {
>  		.tcon = tcon,
>  		.cifs_sb = cifs_sb,
> @@ -715,8 +721,16 @@ static int cifs_nt_open(const char *full_path, struct inode
> *inode, struct cifs_
>  	};
> 
>  	rc = server->ops->open(xid, &oparms, oplock, buf);
> -	if (rc)
> +	if (rc) {
> +		if (rc == -EACCES && rdwr_for_fscache == 1) {
> +			desired_access = cifs_convert_flags(f_flags, 0);
> +			rdwr_for_fscache = 2;
> +			goto retry_open;
> +		}
>  		return rc;
> +	}
> +	if (rdwr_for_fscache == 2)
> +		cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
> 
>  	/* TODO: Add support for calling posix query info but with passing in fid */
>  	if (tcon->unix_ext)
> @@ -1149,11 +1163,14 @@ int cifs_open(struct inode *inode, struct file *file)
>  use_cache:
>  	fscache_use_cookie(cifs_inode_cookie(file_inode(file)),
>  			   file->f_mode & FMODE_WRITE);
> -	if (file->f_flags & O_DIRECT &&
> -	    (!((file->f_flags & O_ACCMODE) != O_RDONLY) ||
> -	     file->f_flags & O_APPEND))
> -		cifs_invalidate_cache(file_inode(file),
> -				      FSCACHE_INVAL_DIO_WRITE);
> +	//if ((file->f_flags & O_ACCMODE) == O_WRONLY)
> +	//	goto inval;

Why to keep unused code?

Thanks,
Naveen

> +	if (!(file->f_flags & O_DIRECT))
> +		goto out;
> +	if ((file->f_flags & (O_ACCMODE | O_APPEND)) == O_RDONLY)
> +		goto out;
> +//inval:
> +	cifs_invalidate_cache(file_inode(file), FSCACHE_INVAL_DIO_WRITE);
> 
>  out:
>  	free_dentry_path(page);
> @@ -1218,6 +1235,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool
> can_flush)
>  	int disposition = FILE_OPEN;
>  	int create_options = CREATE_NOT_DIR;
>  	struct cifs_open_parms oparms;
> +	int rdwr_for_fscache = 0;
> 
>  	xid = get_xid();
>  	mutex_lock(&cfile->fh_mutex);
> @@ -1281,7 +1299,11 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool
> can_flush)
>  	}
>  #endif /* CONFIG_CIFS_ALLOW_INSECURE_LEGACY */
> 
> -	desired_access = cifs_convert_flags(cfile->f_flags);
> +	/* If we're caching, we need to be able to fill in around partial writes. */
> +	if (cifs_fscache_enabled(inode) && (cfile->f_flags & O_ACCMODE) ==
> O_WRONLY)
> +		rdwr_for_fscache = 1;
> +
> +	desired_access = cifs_convert_flags(cfile->f_flags, rdwr_for_fscache);
> 
>  	/* O_SYNC also has bit for O_DSYNC so following check picks up either
> */
>  	if (cfile->f_flags & O_SYNC)
> @@ -1293,6 +1315,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool
> can_flush)
>  	if (server->ops->get_lease_key)
>  		server->ops->get_lease_key(inode, &cfile->fid);
> 
> +retry_open:
>  	oparms = (struct cifs_open_parms) {
>  		.tcon = tcon,
>  		.cifs_sb = cifs_sb,
> @@ -1318,6 +1341,11 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool
> can_flush)
>  		/* indicate that we need to relock the file */
>  		oparms.reconnect = true;
>  	}
> +	if (rc == -EACCES && rdwr_for_fscache == 1) {
> +		desired_access = cifs_convert_flags(cfile->f_flags, 0);
> +		rdwr_for_fscache = 2;
> +		goto retry_open;
> +	}
> 
>  	if (rc) {
>  		mutex_unlock(&cfile->fh_mutex);
> @@ -1326,6 +1354,9 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool
> can_flush)
>  		goto reopen_error_exit;
>  	}
> 
> +	if (rdwr_for_fscache == 2)
> +		cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
> +
>  #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
>  reopen_success:
>  #endif /* CONFIG_CIFS_ALLOW_INSECURE_LEGACY */ diff --git
> a/fs/smb/client/fscache.h b/fs/smb/client/fscache.h index
> a3d73720914f..1f2ea9f5cc9a 100644
> --- a/fs/smb/client/fscache.h
> +++ b/fs/smb/client/fscache.h
> @@ -109,6 +109,11 @@ static inline void cifs_readahead_to_fscache(struct
> inode *inode,
>  		__cifs_readahead_to_fscache(inode, pos, len);  }
> 
> +static inline bool cifs_fscache_enabled(struct inode *inode) {
> +	return fscache_cookie_enabled(cifs_inode_cookie(inode));
> +}
> +
>  #else /* CONFIG_CIFS_FSCACHE */
>  static inline
>  void cifs_fscache_fill_coherency(struct inode *inode, @@ -124,6 +129,7 @@
> static inline void cifs_fscache_release_inode_cookie(struct inode *inode) {}  static
> inline void cifs_fscache_unuse_inode_cookie(struct inode *inode, bool update) {}
> static inline struct fscache_cookie *cifs_inode_cookie(struct inode *inode) { return
> NULL; }  static inline void cifs_invalidate_cache(struct inode *inode, unsigned int
> flags) {}
> +static inline bool cifs_fscache_enabled(struct inode *inode) { return
> +false; }
> 
>  static inline int cifs_fscache_query_occupancy(struct inode *inode,
>  					       pgoff_t first, unsigned int nr_pages,
>