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

From: Naveen Mamindlapalli
Date: Fri Mar 29 2024 - 05:59:24 EST

Next message: Masahiro Yamada: "Re: [PATCH] scripts/unifdef: avoid constexpr keyword"
Previous message: David Lin: "RE: [EXT] Re: [PATCH v9 0/2] wifi: mwifiex: add code to support host mlme"
In reply to: David Howells: "[PATCH v6 11/15] cifs: When caching, try to open O_WRONLY file rdwr on server"
Next in thread: David Howells: "[PATCH v6 12/15] cifs: Cut over to using netfslib"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: David Howells <dhowells@xxxxxxxxxx>
> Sent: Thursday, March 28, 2024 10:28 PM
> To: Steve French <smfrench@xxxxxxxxx>
> Cc: David Howells <dhowells@xxxxxxxxxx>; Jeff Layton <jlayton@xxxxxxxxxx>;
> Matthew Wilcox <willy@xxxxxxxxxxxxx>; Paulo Alcantara <pc@xxxxxxxxxxxxx>;
> Shyam Prasad N <sprasad@xxxxxxxxxxxxx>; Tom Talpey <tom@xxxxxxxxxx>;
> Christian Brauner <christian@xxxxxxxxxx>; netfs@xxxxxxxxxxxxxxx; linux-
> cifs@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Steve French
> <sfrench@xxxxxxxxx>; Shyam Prasad N <nspmangalore@xxxxxxxxx>; Rohith
> Surabattula <rohiths.msft@xxxxxxxxx>
> 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@xxxxxxxxxx>
> cc: Steve French <sfrench@xxxxxxxxx>
> cc: Shyam Prasad N <nspmangalore@xxxxxxxxx>
> cc: Rohith Surabattula <rohiths.msft@xxxxxxxxx>
> cc: Jeff Layton <jlayton@xxxxxxxxxx>
> cc: linux-cifs@xxxxxxxxxxxxxxx
> cc: netfs@xxxxxxxxxxxxxxx
> cc: linux-fsdevel@xxxxxxxxxxxxxxx
> cc: linux-mm@xxxxxxxxx
> ---
> 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,
>

Next message: Masahiro Yamada: "Re: [PATCH] scripts/unifdef: avoid constexpr keyword"
Previous message: David Lin: "RE: [EXT] Re: [PATCH v9 0/2] wifi: mwifiex: add code to support host mlme"
In reply to: David Howells: "[PATCH v6 11/15] cifs: When caching, try to open O_WRONLY file rdwr on server"
Next in thread: David Howells: "[PATCH v6 12/15] cifs: Cut over to using netfslib"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]