Re: [PATCH 5/5 linux-next V2 (resend)] cifs: Combine set_file_sizeby handle and path into one function

[Jeff Layton]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/