On Sun, 8 Dec 2013 14:13:37 -0700
Tim Gardner <timg@xxxxxxx> wrote:
> As suggested by Jeff Layton:
>
> --------
> "Now that I look though, it's clear to me that cifs_set_file_size is
> just wrong. Currently it calls ops->set_path_size and if that fails
> with certain error codes it tries to do a SMBLegacyOpen, etc. That's
> going to fall on its face if this is a SMB2/3 connection.
>
> Here's what should be done instead, I think:
>
> - get rid of both set_path_size and set_file_size operations. The
> version specific abstraction was implemented at the wrong level, IMO.
>
> - implement a new protocol level operation that basically takes the
> same args as cifs_set_file_size. cifs_setattr_unix/_nounix should
> call this operation.
>
> - the set_path_size operation for SMB1 should basically be the function
> that is cifs_set_file_size today (though that should probably be
> renamed). That function should be restructured to do the following:
>
> + look for an open filehandle
> + if there isn't one, open the file
> + call CIFSSMBSetFileSize
> + fall back to zero-length write if that fails
> + close the file if we opened it"
> --------
>
> This patch also moves all of the SMBv1 legacy hacks from cifs_set_file_size() into
> the SMBv1 specific handler smb_set_file_size() more or less intact. SMBv2 and higher are
> more regular and orthogonal, so the v2 handler smb2_set_file_size() no longer contains
> the legacy hacks.
>
> Cc: Steve French <sfrench@xxxxxxxxx>
> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
> Cc: Dean Gehnert <deang@xxxxxxx>
> Signed-off-by: Tim Gardner <timg@xxxxxxx>
> ---
>
> V2 - this is a new patch in the V2 series.
>
> I know this is kind of a giant patch, but there really doesn't seem to be any
> intermediate steps. Its pretty much all or nothing.
>
> I've only tested against a Linux CIFS server running a 3.2 kernel since I no longer
> have easy access to smbv2 servers (iOS 10.8 or Windows 7).
>
> resent with corrected Cc's
>
> rtg
>
> fs/cifs/cifsglob.h | 9 ++---
> fs/cifs/inode.c | 108 +++++----------------------------------------------
> fs/cifs/smb1ops.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> fs/cifs/smb2ops.c | 57 ++++++++++++++++++++++++---
> 4 files changed, 170 insertions(+), 113 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 8fd8eb2..6113ce8 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -269,12 +269,9 @@ struct smb_version_operations {
> int (*get_srv_inum)(const unsigned int, struct cifs_tcon *,
> struct cifs_sb_info *, const char *,
> u64 *uniqueid, FILE_ALL_INFO *);
> - /* set size by path */
> - int (*set_file_size_by_path)(const unsigned int, struct cifs_tcon *,
> - const char *, __u64, struct cifs_sb_info *, bool);
> - /* set size by file handle */
> - int (*set_file_size_by_handle)(const unsigned int, struct cifs_tcon *,
> - struct cifsFileInfo *, __u64, bool);
> + /* set file size */
> + int (*set_file_size)(struct inode *inode, struct iattr *attrs,
> + unsigned int xid, char *full_path);
> /* set attributes */
> int (*set_file_info)(struct inode *, const char *, FILE_BASIC_INFO *,
> const unsigned int);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index e332038..3edeafd 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1899,118 +1899,28 @@ static void cifs_setsize(struct inode *inode, loff_t offset)
> truncate_pagecache(inode, offset);
> }
>
> -/*
> - * Legacy hack to set file length.
> - */
> -static int
> -cifs_legacy_set_file_size(unsigned int xid, __u16 netfid, __u32 pid,
> - unsigned int length, struct cifs_tcon *tcon,
> - char *full_path)
> -{
> - int rc;
> - unsigned int bytes_written;
> - struct cifs_io_parms io_parms;
> -
> - io_parms.netfid = netfid;
> - io_parms.pid = pid;
> - io_parms.tcon = tcon;
> - io_parms.offset = 0;
> - io_parms.length = length;
> - rc = CIFSSMBWrite(xid, &io_parms, &bytes_written,
> - NULL, NULL, 1);
> - cifs_dbg(FYI, "%s len %d rc %d on %s\n", __func__, length, rc,
> - full_path);
> - return rc;
> -}
> -
> static int
> cifs_set_file_size(struct inode *inode, struct iattr *attrs,
> unsigned int xid, char *full_path)
> {
> - int rc;
> - struct cifsFileInfo *open_file;
> + int rc = -ENOSYS;
> struct cifsInodeInfo *cifsInode = CIFS_I(inode);
> struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> struct tcon_link *tlink = NULL;
> struct cifs_tcon *tcon = NULL;
> struct TCP_Server_Info *server;
>
> - /*
> - * To avoid spurious oplock breaks from server, in the case of
> - * inodes that we already have open, avoid doing path based
> - * setting of file size if we can do it by handle.
> - * This keeps our caching token (oplock) and avoids timeouts
> - * when the local oplock break takes longer to flush
> - * writebehind data than the SMB timeout for the SetPathInfo
> - * request would allow
> - */
> - open_file = find_writable_file(cifsInode, true);
> - if (open_file) {
> - tcon = tlink_tcon(open_file->tlink);
> - server = tcon->ses->server;
> - if (server->ops->set_file_size_by_handle)
> - rc = server->ops->set_file_size_by_handle(xid, tcon,
> - open_file,
> - attrs->ia_size,
> - false);
> - else
> - rc = -ENOSYS;
> - cifsFileInfo_put(open_file);
> - cifs_dbg(FYI, "SetFSize for attrs rc = %d\n", rc);
> - if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
> - rc = cifs_legacy_set_file_size(xid,
> - open_file->fid.netfid,
> - open_file->pid,
> - attrs->ia_size,
> - tcon, full_path);
> - }
> - } else
> - rc = -EINVAL;
> -
> - if (!rc)
> - goto set_size_out;
> + tlink = cifs_sb_tlink(cifs_sb);
> + if (IS_ERR(tlink))
> + return PTR_ERR(tlink);
> + tcon = tlink_tcon(tlink);
> + server = tcon->ses->server;
>
> - if (tcon == NULL) {
> - tlink = cifs_sb_tlink(cifs_sb);
> - if (IS_ERR(tlink))
> - return PTR_ERR(tlink);
> - tcon = tlink_tcon(tlink);
> - server = tcon->ses->server;
> - }
> + if (server->ops->set_file_size)
> + rc = server->ops->set_file_size(inode, attrs, xid, full_path);
>
> - /*
> - * Set file size by pathname rather than by handle either because no
> - * valid, writeable file handle for it was found or because there was
> - * an error setting it by handle.
> - */
> - if (server->ops->set_file_size_by_path)
> - rc = server->ops->set_file_size_by_path(xid, tcon, full_path,
> - attrs->ia_size, cifs_sb,
> - false);
> - else
> - rc = -ENOSYS;
> - cifs_dbg(FYI, "SetEOF by path (setattrs) rc = %d\n", rc);
> - if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
> - __u16 netfid;
> - int oplock = 0;
> -
> - rc = SMBLegacyOpen(xid, tcon, full_path, FILE_OPEN,
> - GENERIC_WRITE, CREATE_NOT_DIR, &netfid,
> - &oplock, NULL, cifs_sb->local_nls,
> - cifs_sb->mnt_cifs_flags &
> - CIFS_MOUNT_MAP_SPECIAL_CHR);
> - if (rc == 0) {
> - rc = cifs_legacy_set_file_size(xid, netfid,
> - current->tgid,
> - attrs->ia_size, tcon,
> - full_path);
> - CIFSSMBClose(xid, tcon, netfid);
> - }
> - }
> - if (!open_file)
> - cifs_put_tlink(tlink);
> + cifs_put_tlink(tlink);
>
> -set_size_out:
> if (rc == 0) {
> cifsInode->server_eof = attrs->ia_size;
> cifs_setsize(inode, attrs->ia_size);
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index c5d9ec6..63ab3aa 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -755,6 +755,30 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
> return CIFSSMBWrite2(xid, parms, written, iov, nr_segs);
> }
>
> +/*
> + * Legacy hack to set file length.
> + */
> +static int
> +cifs_legacy_set_file_size(unsigned int xid, __u16 netfid, __u32 pid,
> + unsigned int length, struct cifs_tcon *tcon,
> + char *full_path)
> +{
> + int rc;
> + unsigned int bytes_written;
> + struct cifs_io_parms io_parms;
> +
> + io_parms.netfid = netfid;
> + io_parms.pid = pid;
> + io_parms.tcon = tcon;
> + io_parms.offset = 0;
> + io_parms.length = length;
> + rc = CIFSSMBWrite(xid, &io_parms, &bytes_written,
> + NULL, NULL, 1);
> + cifs_dbg(FYI, "%s len %d rc %d on %s\n", __func__, length, rc,
> + full_path);
> + return rc;
> +}
> +
> static int
> smb_set_file_size_by_path(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size, struct cifs_sb_info *cifs_sb,
> @@ -790,6 +814,88 @@ smb_set_file_size_by_path(const unsigned int xid, struct cifs_tcon *tcon,
> }
>
> static int
> +smb_set_file_size(struct inode *inode, struct iattr *attrs, unsigned int xid,
> + char *full_path)
> +{
> + int rc;
> + struct cifsFileInfo *open_file;
> + struct cifsInodeInfo *cifsInode = CIFS_I(inode);
> + struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> + struct tcon_link *tlink = NULL;
> + struct cifs_tcon *tcon = NULL;
> + struct TCP_Server_Info *server;
> +
> + /*
> + * To avoid spurious oplock breaks from server, in the case of
> + * inodes that we already have open, avoid doing path based
> + * setting of file size if we can do it by handle.
> + * This keeps our caching token (oplock) and avoids timeouts
> + * when the local oplock break takes longer to flush
> + * writebehind data than the SMB timeout for the SetPathInfo
> + * request would allow
> + */
> + open_file = find_writable_file(cifsInode, true);
> + if (open_file) {
> + tcon = tlink_tcon(open_file->tlink);
> + server = tcon->ses->server;
> + rc = CIFSSMBSetFileSize(xid, tcon, open_file, attrs->ia_size,
> + false);
> + cifsFileInfo_put(open_file);
> + cifs_dbg(FYI, "%s by handle rc = %d for %s\n", __func__, rc,
> + full_path);
> + if ((rc == -EINVAL) || (rc == -EOPNOTSUPP))
> + rc = cifs_legacy_set_file_size(xid,
> + open_file->fid.netfid,
> + open_file->pid,
> + attrs->ia_size,
> + tcon, full_path);
Ouch, there's an existing bug here that's getting copy and pasted...
We're calling find_writeable_file to get a reference to open_file. Then
we call cifsFileInfo_put to put that reference, and then we dereference
that pointer to pass to cifs_legacy_set_file_size.
That cifsFileInfo_put needs to be moved down to after the
cifs_legacy_set_file_size call. Can you fix that while you're in here?
> + } else
> + rc = -EINVAL;
> +
> + if (!rc)
> + goto set_size_out;
> +
> + if (tcon == NULL) {
> + tlink = cifs_sb_tlink(cifs_sb);
> + if (IS_ERR(tlink))
> + return PTR_ERR(tlink);
> + tcon = tlink_tcon(tlink);
> + server = tcon->ses->server;
> + }
> +
> + /*
> + * Set file size by pathname rather than by handle either because no
> + * valid, writeable file handle for it was found or because there was
> + * an error setting it by handle.
> + */
> + rc = smb_set_file_size_by_path(xid, tcon, full_path, attrs->ia_size,
> + cifs_sb, false);
> + cifs_dbg(FYI, "%s by path rc = %d for %s\n", __func__, rc, full_path);
> + if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
> + __u16 netfid;
> + int oplock = 0;
> +
> + rc = SMBLegacyOpen(xid, tcon, full_path, FILE_OPEN,
> + GENERIC_WRITE, CREATE_NOT_DIR, &netfid,
> + &oplock, NULL, cifs_sb->local_nls,
> + cifs_sb->mnt_cifs_flags &
> + CIFS_MOUNT_MAP_SPECIAL_CHR);
> + if (rc == 0) {
> + rc = cifs_legacy_set_file_size(xid, netfid,
> + current->tgid,
> + attrs->ia_size, tcon,
> + full_path);
> + CIFSSMBClose(xid, tcon, netfid);
> + }
> + }
> + if (!open_file)
> + cifs_put_tlink(tlink);
> +
> +set_size_out:
> + return rc;
> +}
> +
> +static int
> smb_set_file_info(struct inode *inode, const char *full_path,
> FILE_BASIC_INFO *buf, const unsigned int xid)
> {
> @@ -1013,8 +1119,7 @@ struct smb_version_operations smb1_operations = {
> .query_path_info = cifs_query_path_info,
> .query_file_info = cifs_query_file_info,
> .get_srv_inum = cifs_get_srv_inum,
> - .set_file_size_by_path = smb_set_file_size_by_path,
> - .set_file_size_by_handle = CIFSSMBSetFileSize,
> + .set_file_size = smb_set_file_size,
> .set_file_info = smb_set_file_info,
> .set_compression = cifs_set_compression,
> .echo = CIFSSMBEcho,
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index dc7b1cb..206b45f 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -697,6 +697,54 @@ smb2_set_file_size_by_handle(const unsigned int xid, struct cifs_tcon *tcon,
> }
>
> static int
> +smb2_set_file_size(struct inode *inode, struct iattr *attrs, unsigned int xid,
> + char *full_path)
> +{
> + int rc;
> + struct cifsFileInfo *open_file;
> + struct cifsInodeInfo *cifsInode = CIFS_I(inode);
> + struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> + struct tcon_link *tlink;
> + struct cifs_tcon *tcon;
> + struct TCP_Server_Info *server;
> +
> + /*
> + * To avoid spurious oplock breaks from server, in the case of
> + * inodes that we already have open, avoid doing path based
> + * setting of file size if we can do it by handle.
> + * This keeps our caching token (oplock) and avoids timeouts
> + * when the local oplock break takes longer to flush
> + * writebehind data than the SMB timeout for the SetPathInfo
> + * request would allow
> + */
> + open_file = find_writable_file(cifsInode, true);
> + if (open_file) {
> + tcon = tlink_tcon(open_file->tlink);
> + server = tcon->ses->server;
> + rc = smb2_set_file_size_by_handle(xid, tcon, open_file,
> + attrs->ia_size, false);
> + cifsFileInfo_put(open_file);
> + cifs_dbg(FYI, "%s by handle rc = %d on %s\n", __func__, rc,
> + full_path);
> + return rc;
> + }
> +
> + tlink = cifs_sb_tlink(cifs_sb);
> + if (IS_ERR(tlink))
> + return PTR_ERR(tlink);
> + tcon = tlink_tcon(tlink);
> + server = tcon->ses->server;
> +
> + rc = smb2_set_file_size_by_path(xid, tcon, full_path, attrs->ia_size,
> + cifs_sb, false);
> + cifs_dbg(FYI, "%s by path rc = %d on %s\n", __func__, rc, full_path);
> +
> + cifs_put_tlink(tlink);
> +
> + return rc;
> +}
> +
> +static int
> smb2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifsFileInfo *cfile)
> {
> @@ -1131,8 +1179,7 @@ struct smb_version_operations smb20_operations = {
> .query_path_info = smb2_query_path_info,
> .get_srv_inum = smb2_get_srv_inum,
> .query_file_info = smb2_query_file_info,
> - .set_file_size_by_path = smb2_set_file_size_by_path,
> - .set_file_size_by_handle = smb2_set_file_size_by_handle,
> + .set_file_size = smb2_set_file_size,
> .set_file_info = smb2_set_file_info,
> .set_compression = smb2_set_compression,
> .mkdir = smb2_mkdir,
> @@ -1205,8 +1252,7 @@ struct smb_version_operations smb21_operations = {
> .query_path_info = smb2_query_path_info,
> .get_srv_inum = smb2_get_srv_inum,
> .query_file_info = smb2_query_file_info,
> - .set_file_size_by_path = smb2_set_file_size_by_path,
> - .set_file_size_by_handle = smb2_set_file_size_by_handle,
> + .set_file_size = smb2_set_file_size,
> .set_file_info = smb2_set_file_info,
> .set_compression = smb2_set_compression,
> .mkdir = smb2_mkdir,
> @@ -1280,8 +1326,7 @@ struct smb_version_operations smb30_operations = {
> .query_path_info = smb2_query_path_info,
> .get_srv_inum = smb2_get_srv_inum,
> .query_file_info = smb2_query_file_info,
> - .set_file_size_by_path = smb2_set_file_size_by_path,
> - .set_file_size_by_handle = smb2_set_file_size_by_handle,
> + .set_file_size = smb2_set_file_size,
> .set_file_info = smb2_set_file_info,
> .set_compression = smb2_set_compression,
> .mkdir = smb2_mkdir,
The rest of the patch looks good to me though.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
