All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5 linux-next V2] cifs: Replace CIFSSMBSetEOF() with smb_set_file_size()
@ 2013-12-08 21:08 ` Tim Gardner
  0 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Steve French, Tim Gardner, Dean Gehnert, Jeff Layton

According to Microsoft documentation:
		http://msdn.microsoft.com/en-us/library/ee441943.aspx
		http://msdn.microsoft.com/en-us/library/ff469854.aspx

The information level codes used in CIFSSMBSetEOF() are not
legal. In practice we have found that they really don't work either.
The specification clearly states that none of the SMB_SET_FILE infornmation
level codes are supported for TRANS2_SET_PATH_INFORMATION.

You can trigger this function with the following C code. Note that O_CREAT|O_TRUNC
is not a valid combination, but it does serve to illustrate the bug.

cat <<EOF > truncate.c

int main(int argc, char *argv[])
{
	char *fname;
	int fd;
	mode_t mode = S_IRUSR|S_IWUSR;
	int flags = O_CREAT|O_TRUNC;

	if (argc == 2) {
		fname = argv[1];
	} else {
		printf("You must supply a file name.\n");
		exit(1);
	}

	if ((fd=open(fname,flags,mode))  < 0)
	{
		printf("could not open %s with flags %08x\n",fname,flags);
		exit(1);
	}
	printf("Opened %s with flags %08x\n",fname,flags);

	close(fd);
}
EOF

I used this script:

cat <<EOF > truncate.sh
LOG=log.txt
SRV=10.0.0.184
MP=/tmp/mnt
TD=$MP/temp

gcc -o truncate truncate.c
rm -f $LOG

sudo mkdir -p $MP
sudo mount -t cifs //$SRV/test $MP --verbose -o noserverino,nounix,user=test,pass=test

mkdir -p $TD
sudo rm -f $TD/junk.txt
echo 1 > junk.txt
sudo cp junk.txt $TD/junk.txt
sudo dmesg -c > /dev/null

echo 1 | sudo tee /proc/fs/cifs/cifsFYI
sudo ./truncate $TD/junk.txt
echo 0 | sudo tee /proc/fs/cifs/cifsFYI

sudo umount $MP
sudo dmesg -c > $LOG
EOF

The resulting log file:

[179777.458893] fs/cifs/file.c:cifs_open:447: CIFS VFS: in cifs_open as Xid: 654 with uid: 0
[179777.458903] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
[179777.458907] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
[179777.458922] fs/cifs/file.c:cifs_open:465: inode = 0xffff88005a42b828 file flags are 0x8240 for \temp\junk.txt
[179777.458935] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 162
[179777.458938] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=114
[179777.460089] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x67
[179777.460109] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x6b, smb_buf_length: 0x67
[179777.460125] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=162 mid=20 state=4
[179777.460134] fs/cifs/inode.c:cifs_get_inode_info:646: Getting info on \temp\junk.txt
[179777.460138] fs/cifs/inode.c:cifs_revalidate_cache:95: cifs_revalidate_cache: revalidating inode 320
[179777.460141] fs/cifs/inode.c:cifs_revalidate_cache:119: cifs_revalidate_cache: invalidating inode 320 mapping
[179777.460148] fs/cifs/file.c:cifs_open:545: CIFS VFS: leaving cifs_open (xid = 654) rc = 0
[179777.460153] fs/cifs/inode.c:cifs_setattr:2280: name junk.txt size 0
[179777.460157] fs/cifs/inode.c:cifs_setattr_nounix:2126: CIFS VFS: in cifs_setattr_nounix as Xid: 655 with uid: 0
[179777.460160] fs/cifs/inode.c:cifs_setattr_nounix:2129: setattr on file junk.txt attrs->iavalid 0xa068
[179777.460163] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
[179777.460166] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
[179777.460170] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5403: In SetEOF
[179777.460175] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 50
[179777.460178] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=112
[179777.460810] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
[179777.460827] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
[179777.460830] fs/cifs/smb1ops.c:check2ndT2:255: invalid transact2 word count
[179777.460841] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=50 mid=21 state=4
[179777.460845] fs/cifs/netmisc.c:map_smb_to_linux_error:891: Mapping smb error code 0xc0000022 to POSIX err -13
[179777.460849] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5469: SetPathInfo (file size) returned -13
[179777.460851] fs/cifs/inode.c:cifs_set_file_size:1934: SetEOF by path (setattrs) rc = -13
[179777.460855] fs/cifs/inode.c:cifs_setattr_nounix:2269: CIFS VFS: leaving cifs_setattr_nounix (xid = 655) rc = -13
[179777.460863] fs/cifs/file.c:cifsFileInfo_put:385: closing last open instance for inode ffff88005a42b828
[179777.460866] fs/cifs/file.c:cifsFileInfo_put:403: CIFS VFS: in cifsFileInfo_put as Xid: 656 with uid: 0
[179777.460869] fs/cifs/cifssmb.c:CIFSSMBClose:2457: In CIFSSMBClose
[179777.460873] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 4
[179777.460876] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=41
[179777.461257] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
[179777.461267] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
[179777.461280] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=4 mid=22 state=4

We encountered this problem whilst backporting the CIFS driver to a kernel and VFS old enough
to still call inode_operations.setattr() when truncating a file. We're not sure it is the
right upstream solution, but it worked for us on this older kernel.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - There really is no substantive change from V1. Rather, this patch is now
the first in a series that acheives the review comments set out by Jeff Layton
for the V1 patch.

 fs/cifs/cifsproto.h |    3 --
 fs/cifs/cifssmb.c   |   98 ---------------------------------------------------
 fs/cifs/smb1ops.c   |   36 ++++++++++++++++++-
 3 files changed, 35 insertions(+), 102 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index aa33976..fe69b9c 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -296,9 +296,6 @@ extern int CIFSSMBSetAttrLegacy(unsigned int xid, struct cifs_tcon *tcon,
 			char *fileName, __u16 dos_attributes,
 			const struct nls_table *nls_codepage);
 #endif /* possibly unneeded function */
-extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
-			 const char *file_name, __u64 size,
-			 struct cifs_sb_info *cifs_sb, bool set_allocation);
 extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
 			      struct cifsFileInfo *cfile, __u64 size,
 			      bool set_allocation);
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 124aa02..53f1b9b 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -5453,104 +5453,6 @@ QFSPosixRetry:
 	return rc;
 }
 
-
-/*
- * We can not use write of zero bytes trick to set file size due to need for
- * large file support. Also note that this SetPathInfo is preferred to
- * SetFileInfo based method in next routine which is only needed to work around
- * a sharing violation bugin Samba which this routine can run into.
- */
-int
-CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
-	      const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
-	      bool set_allocation)
-{
-	struct smb_com_transaction2_spi_req *pSMB = NULL;
-	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
-	struct file_end_of_file_info *parm_data;
-	int name_len;
-	int rc = 0;
-	int bytes_returned = 0;
-	int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
-
-	__u16 params, byte_count, data_count, param_offset, offset;
-
-	cifs_dbg(FYI, "In SetEOF\n");
-SetEOFRetry:
-	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
-		      (void **) &pSMBr);
-	if (rc)
-		return rc;
-
-	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
-		name_len =
-		    cifsConvertToUTF16((__le16 *) pSMB->FileName, file_name,
-				       PATH_MAX, cifs_sb->local_nls, remap);
-		name_len++;	/* trailing null */
-		name_len *= 2;
-	} else {	/* BB improve the check for buffer overruns BB */
-		name_len = strnlen(file_name, PATH_MAX);
-		name_len++;	/* trailing null */
-		strncpy(pSMB->FileName, file_name, name_len);
-	}
-	params = 6 + name_len;
-	data_count = sizeof(struct file_end_of_file_info);
-	pSMB->MaxParameterCount = cpu_to_le16(2);
-	pSMB->MaxDataCount = cpu_to_le16(4100);
-	pSMB->MaxSetupCount = 0;
-	pSMB->Reserved = 0;
-	pSMB->Flags = 0;
-	pSMB->Timeout = 0;
-	pSMB->Reserved2 = 0;
-	param_offset = offsetof(struct smb_com_transaction2_spi_req,
-				InformationLevel) - 4;
-	offset = param_offset + params;
-	if (set_allocation) {
-		if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
-			pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO2);
-		else
-			pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO);
-	} else /* Set File Size */  {
-	    if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
-		    pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO2);
-	    else
-		    pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO);
-	}
-
-	parm_data =
-	    (struct file_end_of_file_info *) (((char *) &pSMB->hdr.Protocol) +
-				       offset);
-	pSMB->ParameterOffset = cpu_to_le16(param_offset);
-	pSMB->DataOffset = cpu_to_le16(offset);
-	pSMB->SetupCount = 1;
-	pSMB->Reserved3 = 0;
-	pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
-	byte_count = 3 /* pad */  + params + data_count;
-	pSMB->DataCount = cpu_to_le16(data_count);
-	pSMB->TotalDataCount = pSMB->DataCount;
-	pSMB->ParameterCount = cpu_to_le16(params);
-	pSMB->TotalParameterCount = pSMB->ParameterCount;
-	pSMB->Reserved4 = 0;
-	inc_rfc1001_len(pSMB, byte_count);
-	parm_data->FileSize = cpu_to_le64(size);
-	pSMB->ByteCount = cpu_to_le16(byte_count);
-	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
-	if (rc)
-		cifs_dbg(FYI, "SetPathInfo (file size) returned %d\n", rc);
-
-	cifs_buf_release(pSMB);
-
-	if (rc == -EAGAIN)
-		goto SetEOFRetry;
-
-	return rc;
-}
-
 int
 CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
 		   struct cifsFileInfo *cfile, __u64 size, bool set_allocation)
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 5f5ba0d..14d4832 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -756,6 +756,40 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
 }
 
 static int
+smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
+	      const char *full_path, __u64 size, struct cifs_sb_info *cifs_sb,
+	      bool set_allocation)
+{
+	int oplock = 0;
+	int rc;
+	__u16 netfid;
+	struct cifsFileInfo cfile;
+
+	cifs_dbg(FYI, "smb_set_file_size: %s\n", full_path);
+
+	rc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
+			 FILE_WRITE_DATA, CREATE_NOT_DIR,
+			 &netfid, &oplock, NULL, cifs_sb->local_nls,
+			 cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
+	if (rc)
+		return rc;
+
+	/*
+	 * Only 2 fields are required to set the file size.
+	 */
+	memset(&cfile, 0, sizeof(cfile));
+	cfile.pid = current->tgid;
+	cfile.fid.netfid = netfid;
+
+	rc = CIFSSMBSetFileSize(xid, tcon, &cfile, size, set_allocation);
+
+	if (!rc)
+		rc = CIFSSMBClose(xid, tcon, netfid);
+
+	return rc;
+}
+
+static int
 smb_set_file_info(struct inode *inode, const char *full_path,
 		  FILE_BASIC_INFO *buf, const unsigned int xid)
 {
@@ -979,7 +1013,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_path_size = CIFSSMBSetEOF,
+	.set_path_size = smb_set_file_size,
 	.set_file_size = CIFSSMBSetFileSize,
 	.set_file_info = smb_set_file_info,
 	.set_compression = cifs_set_compression,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 1/5 linux-next V2] cifs: Replace CIFSSMBSetEOF() with smb_set_file_size()
@ 2013-12-08 21:08 ` Tim Gardner
  0 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Tim Gardner, Steve French, Jeff Layton, Dean Gehnert

According to Microsoft documentation:
		http://msdn.microsoft.com/en-us/library/ee441943.aspx
		http://msdn.microsoft.com/en-us/library/ff469854.aspx

The information level codes used in CIFSSMBSetEOF() are not
legal. In practice we have found that they really don't work either.
The specification clearly states that none of the SMB_SET_FILE infornmation
level codes are supported for TRANS2_SET_PATH_INFORMATION.

You can trigger this function with the following C code. Note that O_CREAT|O_TRUNC
is not a valid combination, but it does serve to illustrate the bug.

cat <<EOF > truncate.c

int main(int argc, char *argv[])
{
	char *fname;
	int fd;
	mode_t mode = S_IRUSR|S_IWUSR;
	int flags = O_CREAT|O_TRUNC;

	if (argc == 2) {
		fname = argv[1];
	} else {
		printf("You must supply a file name.\n");
		exit(1);
	}

	if ((fd=open(fname,flags,mode))  < 0)
	{
		printf("could not open %s with flags %08x\n",fname,flags);
		exit(1);
	}
	printf("Opened %s with flags %08x\n",fname,flags);

	close(fd);
}
EOF

I used this script:

cat <<EOF > truncate.sh
LOG=log.txt
SRV=10.0.0.184
MP=/tmp/mnt
TD=$MP/temp

gcc -o truncate truncate.c
rm -f $LOG

sudo mkdir -p $MP
sudo mount -t cifs //$SRV/test $MP --verbose -o noserverino,nounix,user=test,pass=test

mkdir -p $TD
sudo rm -f $TD/junk.txt
echo 1 > junk.txt
sudo cp junk.txt $TD/junk.txt
sudo dmesg -c > /dev/null

echo 1 | sudo tee /proc/fs/cifs/cifsFYI
sudo ./truncate $TD/junk.txt
echo 0 | sudo tee /proc/fs/cifs/cifsFYI

sudo umount $MP
sudo dmesg -c > $LOG
EOF

The resulting log file:

[179777.458893] fs/cifs/file.c:cifs_open:447: CIFS VFS: in cifs_open as Xid: 654 with uid: 0
[179777.458903] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
[179777.458907] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
[179777.458922] fs/cifs/file.c:cifs_open:465: inode = 0xffff88005a42b828 file flags are 0x8240 for \temp\junk.txt
[179777.458935] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 162
[179777.458938] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=114
[179777.460089] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x67
[179777.460109] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x6b, smb_buf_length: 0x67
[179777.460125] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=162 mid=20 state=4
[179777.460134] fs/cifs/inode.c:cifs_get_inode_info:646: Getting info on \temp\junk.txt
[179777.460138] fs/cifs/inode.c:cifs_revalidate_cache:95: cifs_revalidate_cache: revalidating inode 320
[179777.460141] fs/cifs/inode.c:cifs_revalidate_cache:119: cifs_revalidate_cache: invalidating inode 320 mapping
[179777.460148] fs/cifs/file.c:cifs_open:545: CIFS VFS: leaving cifs_open (xid = 654) rc = 0
[179777.460153] fs/cifs/inode.c:cifs_setattr:2280: name junk.txt size 0
[179777.460157] fs/cifs/inode.c:cifs_setattr_nounix:2126: CIFS VFS: in cifs_setattr_nounix as Xid: 655 with uid: 0
[179777.460160] fs/cifs/inode.c:cifs_setattr_nounix:2129: setattr on file junk.txt attrs->iavalid 0xa068
[179777.460163] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
[179777.460166] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
[179777.460170] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5403: In SetEOF
[179777.460175] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 50
[179777.460178] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=112
[179777.460810] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
[179777.460827] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
[179777.460830] fs/cifs/smb1ops.c:check2ndT2:255: invalid transact2 word count
[179777.460841] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=50 mid=21 state=4
[179777.460845] fs/cifs/netmisc.c:map_smb_to_linux_error:891: Mapping smb error code 0xc0000022 to POSIX err -13
[179777.460849] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5469: SetPathInfo (file size) returned -13
[179777.460851] fs/cifs/inode.c:cifs_set_file_size:1934: SetEOF by path (setattrs) rc = -13
[179777.460855] fs/cifs/inode.c:cifs_setattr_nounix:2269: CIFS VFS: leaving cifs_setattr_nounix (xid = 655) rc = -13
[179777.460863] fs/cifs/file.c:cifsFileInfo_put:385: closing last open instance for inode ffff88005a42b828
[179777.460866] fs/cifs/file.c:cifsFileInfo_put:403: CIFS VFS: in cifsFileInfo_put as Xid: 656 with uid: 0
[179777.460869] fs/cifs/cifssmb.c:CIFSSMBClose:2457: In CIFSSMBClose
[179777.460873] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 4
[179777.460876] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=41
[179777.461257] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
[179777.461267] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
[179777.461280] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=4 mid=22 state=4

We encountered this problem whilst backporting the CIFS driver to a kernel and VFS old enough
to still call inode_operations.setattr() when truncating a file. We're not sure it is the
right upstream solution, but it worked for us on this older kernel.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - There really is no substantive change from V1. Rather, this patch is now
the first in a series that acheives the review comments set out by Jeff Layton
for the V1 patch.

 fs/cifs/cifsproto.h |    3 --
 fs/cifs/cifssmb.c   |   98 ---------------------------------------------------
 fs/cifs/smb1ops.c   |   36 ++++++++++++++++++-
 3 files changed, 35 insertions(+), 102 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index aa33976..fe69b9c 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -296,9 +296,6 @@ extern int CIFSSMBSetAttrLegacy(unsigned int xid, struct cifs_tcon *tcon,
 			char *fileName, __u16 dos_attributes,
 			const struct nls_table *nls_codepage);
 #endif /* possibly unneeded function */
-extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
-			 const char *file_name, __u64 size,
-			 struct cifs_sb_info *cifs_sb, bool set_allocation);
 extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
 			      struct cifsFileInfo *cfile, __u64 size,
 			      bool set_allocation);
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 124aa02..53f1b9b 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -5453,104 +5453,6 @@ QFSPosixRetry:
 	return rc;
 }
 
-
-/*
- * We can not use write of zero bytes trick to set file size due to need for
- * large file support. Also note that this SetPathInfo is preferred to
- * SetFileInfo based method in next routine which is only needed to work around
- * a sharing violation bugin Samba which this routine can run into.
- */
-int
-CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
-	      const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
-	      bool set_allocation)
-{
-	struct smb_com_transaction2_spi_req *pSMB = NULL;
-	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
-	struct file_end_of_file_info *parm_data;
-	int name_len;
-	int rc = 0;
-	int bytes_returned = 0;
-	int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
-
-	__u16 params, byte_count, data_count, param_offset, offset;
-
-	cifs_dbg(FYI, "In SetEOF\n");
-SetEOFRetry:
-	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
-		      (void **) &pSMBr);
-	if (rc)
-		return rc;
-
-	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
-		name_len =
-		    cifsConvertToUTF16((__le16 *) pSMB->FileName, file_name,
-				       PATH_MAX, cifs_sb->local_nls, remap);
-		name_len++;	/* trailing null */
-		name_len *= 2;
-	} else {	/* BB improve the check for buffer overruns BB */
-		name_len = strnlen(file_name, PATH_MAX);
-		name_len++;	/* trailing null */
-		strncpy(pSMB->FileName, file_name, name_len);
-	}
-	params = 6 + name_len;
-	data_count = sizeof(struct file_end_of_file_info);
-	pSMB->MaxParameterCount = cpu_to_le16(2);
-	pSMB->MaxDataCount = cpu_to_le16(4100);
-	pSMB->MaxSetupCount = 0;
-	pSMB->Reserved = 0;
-	pSMB->Flags = 0;
-	pSMB->Timeout = 0;
-	pSMB->Reserved2 = 0;
-	param_offset = offsetof(struct smb_com_transaction2_spi_req,
-				InformationLevel) - 4;
-	offset = param_offset + params;
-	if (set_allocation) {
-		if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
-			pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO2);
-		else
-			pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO);
-	} else /* Set File Size */  {
-	    if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
-		    pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO2);
-	    else
-		    pSMB->InformationLevel =
-				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO);
-	}
-
-	parm_data =
-	    (struct file_end_of_file_info *) (((char *) &pSMB->hdr.Protocol) +
-				       offset);
-	pSMB->ParameterOffset = cpu_to_le16(param_offset);
-	pSMB->DataOffset = cpu_to_le16(offset);
-	pSMB->SetupCount = 1;
-	pSMB->Reserved3 = 0;
-	pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
-	byte_count = 3 /* pad */  + params + data_count;
-	pSMB->DataCount = cpu_to_le16(data_count);
-	pSMB->TotalDataCount = pSMB->DataCount;
-	pSMB->ParameterCount = cpu_to_le16(params);
-	pSMB->TotalParameterCount = pSMB->ParameterCount;
-	pSMB->Reserved4 = 0;
-	inc_rfc1001_len(pSMB, byte_count);
-	parm_data->FileSize = cpu_to_le64(size);
-	pSMB->ByteCount = cpu_to_le16(byte_count);
-	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
-	if (rc)
-		cifs_dbg(FYI, "SetPathInfo (file size) returned %d\n", rc);
-
-	cifs_buf_release(pSMB);
-
-	if (rc == -EAGAIN)
-		goto SetEOFRetry;
-
-	return rc;
-}
-
 int
 CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
 		   struct cifsFileInfo *cfile, __u64 size, bool set_allocation)
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 5f5ba0d..14d4832 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -756,6 +756,40 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
 }
 
 static int
+smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
+	      const char *full_path, __u64 size, struct cifs_sb_info *cifs_sb,
+	      bool set_allocation)
+{
+	int oplock = 0;
+	int rc;
+	__u16 netfid;
+	struct cifsFileInfo cfile;
+
+	cifs_dbg(FYI, "smb_set_file_size: %s\n", full_path);
+
+	rc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
+			 FILE_WRITE_DATA, CREATE_NOT_DIR,
+			 &netfid, &oplock, NULL, cifs_sb->local_nls,
+			 cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
+	if (rc)
+		return rc;
+
+	/*
+	 * Only 2 fields are required to set the file size.
+	 */
+	memset(&cfile, 0, sizeof(cfile));
+	cfile.pid = current->tgid;
+	cfile.fid.netfid = netfid;
+
+	rc = CIFSSMBSetFileSize(xid, tcon, &cfile, size, set_allocation);
+
+	if (!rc)
+		rc = CIFSSMBClose(xid, tcon, netfid);
+
+	return rc;
+}
+
+static int
 smb_set_file_info(struct inode *inode, const char *full_path,
 		  FILE_BASIC_INFO *buf, const unsigned int xid)
 {
@@ -979,7 +1013,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_path_size = CIFSSMBSetEOF,
+	.set_path_size = smb_set_file_size,
 	.set_file_size = CIFSSMBSetFileSize,
 	.set_file_info = smb_set_file_info,
 	.set_compression = cifs_set_compression,
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 2/5 linux-next V2] cifs: rename set_path_size and set_file_size
  2013-12-08 21:08 ` Tim Gardner
@ 2013-12-08 21:08   ` Tim Gardner
  -1 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Steve French, Tim Gardner, Dean Gehnert, Jeff Layton

These 2 method names are a bit confusing. Rename them
so that one can tell at a glance how they are to be used.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - this is a new patch in the V2 series.

 fs/cifs/cifsglob.h  |    4 ++--
 fs/cifs/inode.c     |   15 +++++++++------
 fs/cifs/smb1ops.c   |    6 +++---
 fs/cifs/smb2inode.c |    2 +-
 fs/cifs/smb2ops.c   |   14 +++++++-------
 fs/cifs/smb2proto.h |    8 +++++---
 6 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f918a99..8fd8eb2 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -270,10 +270,10 @@ struct smb_version_operations {
 			    struct cifs_sb_info *, const char *,
 			    u64 *uniqueid, FILE_ALL_INFO *);
 	/* set size by path */
-	int (*set_path_size)(const unsigned int, struct cifs_tcon *,
+	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)(const unsigned int, struct cifs_tcon *,
+	int (*set_file_size_by_handle)(const unsigned int, struct cifs_tcon *,
 			     struct cifsFileInfo *, __u64, bool);
 	/* set attributes */
 	int (*set_file_info)(struct inode *, const char *, FILE_BASIC_INFO *,
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 36f9ebb..7181516 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1925,9 +1925,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 	if (open_file) {
 		tcon = tlink_tcon(open_file->tlink);
 		server = tcon->ses->server;
-		if (server->ops->set_file_size)
-			rc = server->ops->set_file_size(xid, tcon, open_file,
-							attrs->ia_size, false);
+		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);
@@ -1963,9 +1965,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 	 * valid, writeable file handle for it was found or because there was
 	 * an error setting it by handle.
 	 */
-	if (server->ops->set_path_size)
-		rc = server->ops->set_path_size(xid, tcon, full_path,
-						attrs->ia_size, cifs_sb, false);
+	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);
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 14d4832..c5d9ec6 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -756,7 +756,7 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
 }
 
 static int
-smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
+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,
 	      bool set_allocation)
 {
@@ -1013,8 +1013,8 @@ 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_path_size = smb_set_file_size,
-	.set_file_size = CIFSSMBSetFileSize,
+	.set_file_size_by_path = smb_set_file_size_by_path,
+	.set_file_size_by_handle = CIFSSMBSetFileSize,
 	.set_file_info = smb_set_file_info,
 	.set_compression = cifs_set_compression,
 	.echo = CIFSSMBEcho,
diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
index 84c012a..78b590f 100644
--- a/fs/cifs/smb2inode.c
+++ b/fs/cifs/smb2inode.c
@@ -244,7 +244,7 @@ smb2_create_hardlink(const unsigned int xid, struct cifs_tcon *tcon,
 }
 
 int
-smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
+smb2_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, bool set_alloc)
 {
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 757da3e..dc7b1cb 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -688,7 +688,7 @@ smb2_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
 }
 
 static int
-smb2_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
+smb2_set_file_size_by_handle(const unsigned int xid, struct cifs_tcon *tcon,
 		   struct cifsFileInfo *cfile, __u64 size, bool set_alloc)
 {
 	__le64 eof = cpu_to_le64(size);
@@ -1131,8 +1131,8 @@ 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_path_size = smb2_set_path_size,
-	.set_file_size = smb2_set_file_size,
+	.set_file_size_by_path = smb2_set_file_size_by_path,
+	.set_file_size_by_handle = smb2_set_file_size_by_handle,
 	.set_file_info = smb2_set_file_info,
 	.set_compression = smb2_set_compression,
 	.mkdir = smb2_mkdir,
@@ -1205,8 +1205,8 @@ 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_path_size = smb2_set_path_size,
-	.set_file_size = smb2_set_file_size,
+	.set_file_size_by_path = smb2_set_file_size_by_path,
+	.set_file_size_by_handle = smb2_set_file_size_by_handle,
 	.set_file_info = smb2_set_file_info,
 	.set_compression = smb2_set_compression,
 	.mkdir = smb2_mkdir,
@@ -1280,8 +1280,8 @@ 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_path_size = smb2_set_path_size,
-	.set_file_size = smb2_set_file_size,
+	.set_file_size_by_path = smb2_set_file_size_by_path,
+	.set_file_size_by_handle = smb2_set_file_size_by_handle,
 	.set_file_info = smb2_set_file_info,
 	.set_compression = smb2_set_compression,
 	.mkdir = smb2_mkdir,
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index 93adc64..b6f3c13 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -62,9 +62,11 @@ extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
 				struct cifs_sb_info *cifs_sb,
 				const char *full_path, FILE_ALL_INFO *data,
 				bool *adjust_tz, bool *symlink);
-extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
-			      const char *full_path, __u64 size,
-			      struct cifs_sb_info *cifs_sb, bool set_alloc);
+extern int smb2_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,
+				      bool set_alloc);
 extern int smb2_set_file_info(struct inode *inode, const char *full_path,
 			      FILE_BASIC_INFO *buf, const unsigned int xid);
 extern int smb2_mkdir(const unsigned int xid, struct cifs_tcon *tcon,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 2/5 linux-next V2] cifs: rename set_path_size and set_file_size
@ 2013-12-08 21:08   ` Tim Gardner
  0 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Tim Gardner, Steve French, Jeff Layton, Dean Gehnert

These 2 method names are a bit confusing. Rename them
so that one can tell at a glance how they are to be used.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - this is a new patch in the V2 series.

 fs/cifs/cifsglob.h  |    4 ++--
 fs/cifs/inode.c     |   15 +++++++++------
 fs/cifs/smb1ops.c   |    6 +++---
 fs/cifs/smb2inode.c |    2 +-
 fs/cifs/smb2ops.c   |   14 +++++++-------
 fs/cifs/smb2proto.h |    8 +++++---
 6 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f918a99..8fd8eb2 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -270,10 +270,10 @@ struct smb_version_operations {
 			    struct cifs_sb_info *, const char *,
 			    u64 *uniqueid, FILE_ALL_INFO *);
 	/* set size by path */
-	int (*set_path_size)(const unsigned int, struct cifs_tcon *,
+	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)(const unsigned int, struct cifs_tcon *,
+	int (*set_file_size_by_handle)(const unsigned int, struct cifs_tcon *,
 			     struct cifsFileInfo *, __u64, bool);
 	/* set attributes */
 	int (*set_file_info)(struct inode *, const char *, FILE_BASIC_INFO *,
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 36f9ebb..7181516 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1925,9 +1925,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 	if (open_file) {
 		tcon = tlink_tcon(open_file->tlink);
 		server = tcon->ses->server;
-		if (server->ops->set_file_size)
-			rc = server->ops->set_file_size(xid, tcon, open_file,
-							attrs->ia_size, false);
+		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);
@@ -1963,9 +1965,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 	 * valid, writeable file handle for it was found or because there was
 	 * an error setting it by handle.
 	 */
-	if (server->ops->set_path_size)
-		rc = server->ops->set_path_size(xid, tcon, full_path,
-						attrs->ia_size, cifs_sb, false);
+	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);
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 14d4832..c5d9ec6 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -756,7 +756,7 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
 }
 
 static int
-smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
+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,
 	      bool set_allocation)
 {
@@ -1013,8 +1013,8 @@ 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_path_size = smb_set_file_size,
-	.set_file_size = CIFSSMBSetFileSize,
+	.set_file_size_by_path = smb_set_file_size_by_path,
+	.set_file_size_by_handle = CIFSSMBSetFileSize,
 	.set_file_info = smb_set_file_info,
 	.set_compression = cifs_set_compression,
 	.echo = CIFSSMBEcho,
diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
index 84c012a..78b590f 100644
--- a/fs/cifs/smb2inode.c
+++ b/fs/cifs/smb2inode.c
@@ -244,7 +244,7 @@ smb2_create_hardlink(const unsigned int xid, struct cifs_tcon *tcon,
 }
 
 int
-smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
+smb2_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, bool set_alloc)
 {
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 757da3e..dc7b1cb 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -688,7 +688,7 @@ smb2_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
 }
 
 static int
-smb2_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
+smb2_set_file_size_by_handle(const unsigned int xid, struct cifs_tcon *tcon,
 		   struct cifsFileInfo *cfile, __u64 size, bool set_alloc)
 {
 	__le64 eof = cpu_to_le64(size);
@@ -1131,8 +1131,8 @@ 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_path_size = smb2_set_path_size,
-	.set_file_size = smb2_set_file_size,
+	.set_file_size_by_path = smb2_set_file_size_by_path,
+	.set_file_size_by_handle = smb2_set_file_size_by_handle,
 	.set_file_info = smb2_set_file_info,
 	.set_compression = smb2_set_compression,
 	.mkdir = smb2_mkdir,
@@ -1205,8 +1205,8 @@ 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_path_size = smb2_set_path_size,
-	.set_file_size = smb2_set_file_size,
+	.set_file_size_by_path = smb2_set_file_size_by_path,
+	.set_file_size_by_handle = smb2_set_file_size_by_handle,
 	.set_file_info = smb2_set_file_info,
 	.set_compression = smb2_set_compression,
 	.mkdir = smb2_mkdir,
@@ -1280,8 +1280,8 @@ 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_path_size = smb2_set_path_size,
-	.set_file_size = smb2_set_file_size,
+	.set_file_size_by_path = smb2_set_file_size_by_path,
+	.set_file_size_by_handle = smb2_set_file_size_by_handle,
 	.set_file_info = smb2_set_file_info,
 	.set_compression = smb2_set_compression,
 	.mkdir = smb2_mkdir,
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index 93adc64..b6f3c13 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -62,9 +62,11 @@ extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
 				struct cifs_sb_info *cifs_sb,
 				const char *full_path, FILE_ALL_INFO *data,
 				bool *adjust_tz, bool *symlink);
-extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
-			      const char *full_path, __u64 size,
-			      struct cifs_sb_info *cifs_sb, bool set_alloc);
+extern int smb2_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,
+				      bool set_alloc);
 extern int smb2_set_file_info(struct inode *inode, const char *full_path,
 			      FILE_BASIC_INFO *buf, const unsigned int xid);
 extern int smb2_mkdir(const unsigned int xid, struct cifs_tcon *tcon,
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 3/5 linux-next V2] cifs: Introduce cifs_legacy_set_file_size()
  2013-12-08 21:08 ` Tim Gardner
@ 2013-12-08 21:08   ` Tim Gardner
  -1 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Steve French, Tim Gardner, Dean Gehnert, Jeff Layton

Consolidates some duplicate code.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - this is a new patch in the V2 series.

 fs/cifs/inode.c |   54 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 7181516..3f710c6 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1899,6 +1899,30 @@ 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)
@@ -1910,7 +1934,6 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 	struct tcon_link *tlink = NULL;
 	struct cifs_tcon *tcon = NULL;
 	struct TCP_Server_Info *server;
-	struct cifs_io_parms io_parms;
 
 	/*
 	 * To avoid spurious oplock breaks from server, in the case of
@@ -1935,16 +1958,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 		cifsFileInfo_put(open_file);
 		cifs_dbg(FYI, "SetFSize for attrs rc = %d\n", rc);
 		if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
-			unsigned int bytes_written;
-
-			io_parms.netfid = open_file->fid.netfid;
-			io_parms.pid = open_file->pid;
-			io_parms.tcon = tcon;
-			io_parms.offset = 0;
-			io_parms.length = attrs->ia_size;
-			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written,
-					  NULL, NULL, 1);
-			cifs_dbg(FYI, "Wrt seteof rc %d\n", rc);
+			rc = cifs_legacy_set_file_size(xid,
+						       open_file->fid.netfid,
+						       open_file->pid,
+						       attrs->ia_size,
+						       tcon, full_path);
 		}
 	} else
 		rc = -EINVAL;
@@ -1982,16 +2000,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 				   cifs_sb->mnt_cifs_flags &
 						CIFS_MOUNT_MAP_SPECIAL_CHR);
 		if (rc == 0) {
-			unsigned int bytes_written;
-
-			io_parms.netfid = netfid;
-			io_parms.pid = current->tgid;
-			io_parms.tcon = tcon;
-			io_parms.offset = 0;
-			io_parms.length = attrs->ia_size;
-			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written, NULL,
-					  NULL,  1);
-			cifs_dbg(FYI, "wrt seteof rc %d\n", rc);
+			rc = cifs_legacy_set_file_size(xid, netfid,
+						       current->tgid,
+						       attrs->ia_size, tcon,
+						       full_path);
 			CIFSSMBClose(xid, tcon, netfid);
 		}
 	}
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 3/5 linux-next V2] cifs: Introduce cifs_legacy_set_file_size()
@ 2013-12-08 21:08   ` Tim Gardner
  0 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Tim Gardner, Steve French, Jeff Layton, Dean Gehnert

Consolidates some duplicate code.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - this is a new patch in the V2 series.

 fs/cifs/inode.c |   54 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 7181516..3f710c6 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1899,6 +1899,30 @@ 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)
@@ -1910,7 +1934,6 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 	struct tcon_link *tlink = NULL;
 	struct cifs_tcon *tcon = NULL;
 	struct TCP_Server_Info *server;
-	struct cifs_io_parms io_parms;
 
 	/*
 	 * To avoid spurious oplock breaks from server, in the case of
@@ -1935,16 +1958,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 		cifsFileInfo_put(open_file);
 		cifs_dbg(FYI, "SetFSize for attrs rc = %d\n", rc);
 		if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
-			unsigned int bytes_written;
-
-			io_parms.netfid = open_file->fid.netfid;
-			io_parms.pid = open_file->pid;
-			io_parms.tcon = tcon;
-			io_parms.offset = 0;
-			io_parms.length = attrs->ia_size;
-			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written,
-					  NULL, NULL, 1);
-			cifs_dbg(FYI, "Wrt seteof rc %d\n", rc);
+			rc = cifs_legacy_set_file_size(xid,
+						       open_file->fid.netfid,
+						       open_file->pid,
+						       attrs->ia_size,
+						       tcon, full_path);
 		}
 	} else
 		rc = -EINVAL;
@@ -1982,16 +2000,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 				   cifs_sb->mnt_cifs_flags &
 						CIFS_MOUNT_MAP_SPECIAL_CHR);
 		if (rc == 0) {
-			unsigned int bytes_written;
-
-			io_parms.netfid = netfid;
-			io_parms.pid = current->tgid;
-			io_parms.tcon = tcon;
-			io_parms.offset = 0;
-			io_parms.length = attrs->ia_size;
-			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written, NULL,
-					  NULL,  1);
-			cifs_dbg(FYI, "wrt seteof rc %d\n", rc);
+			rc = cifs_legacy_set_file_size(xid, netfid,
+						       current->tgid,
+						       attrs->ia_size, tcon,
+						       full_path);
 			CIFSSMBClose(xid, tcon, netfid);
 		}
 	}
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 4/5 linux-next V2] cifs: fix incorrect reference count check
  2013-12-08 21:08 ` Tim Gardner
@ 2013-12-08 21:08   ` Tim Gardner
  -1 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Steve French, Tim Gardner, Dean Gehnert, Jeff Layton

The reference count on tlink can only be decremented if
cifs_sb_tlink(cifs_sb) was used to acquire it. That only
happens if open_file==NULL.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - this is a new patch in the V2 series.

 fs/cifs/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3f710c6..e332038 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2007,7 +2007,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 			CIFSSMBClose(xid, tcon, netfid);
 		}
 	}
-	if (tlink)
+	if (!open_file)
 		cifs_put_tlink(tlink);
 
 set_size_out:
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 4/5 linux-next V2] cifs: fix incorrect reference count check
@ 2013-12-08 21:08   ` Tim Gardner
  0 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Tim Gardner, Steve French, Jeff Layton, Dean Gehnert

The reference count on tlink can only be decremented if
cifs_sb_tlink(cifs_sb) was used to acquire it. That only
happens if open_file==NULL.

Cc: Steve French <sfrench@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

V2 - this is a new patch in the V2 series.

 fs/cifs/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3f710c6..e332038 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2007,7 +2007,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 			CIFSSMBClose(xid, tcon, netfid);
 		}
 	}
-	if (tlink)
+	if (!open_file)
 		cifs_put_tlink(tlink);
 
 set_size_out:
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 5/5 linux-next V2] cifs: Combine set_file_size by handle and path into one function
  2013-12-08 21:08 ` Tim Gardner
                   ` (3 preceding siblings ...)
  (?)
@ 2013-12-08 21:08 ` Tim Gardner
  -1 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:08 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel; +Cc: Tim Gardner

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.

Signed-off-by: Tim Gardner <timg@tpi.com>
---

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).

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);
+	} 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,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 5/5 linux-next V2 (resend)] cifs: Combine set_file_size by handle and path into one function
  2013-12-08 21:08 ` Tim Gardner
@ 2013-12-08 21:13     ` Tim Gardner
  -1 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:13 UTC (permalink / raw)
  To: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Tim Gardner, Steve French, Jeff Layton, Dean Gehnert

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-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Dean Gehnert <deang-l6nL5VImRDY@public.gmane.org>
Signed-off-by: Tim Gardner <timg-l6nL5VImRDY@public.gmane.org>
---

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);
+	} 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,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 23+ messages in thread
* [PATCH 5/5 linux-next V2 (resend)] cifs: Combine set_file_size by handle and path into one function
@ 2013-12-08 21:13     ` Tim Gardner
  0 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-08 21:13 UTC (permalink / raw)
  To: linux-cifs, samba-technical, linux-kernel
  Cc: Tim Gardner, Steve French, Jeff Layton, Dean Gehnert

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@samba.org>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: Dean Gehnert <deang@tpi.com>
Signed-off-by: Tim Gardner <timg@tpi.com>
---

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);
+	} 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,
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread
* Re: [PATCH 1/5 linux-next V2] cifs: Replace CIFSSMBSetEOF() with smb_set_file_size()
  2013-12-08 21:08 ` Tim Gardner
@ 2013-12-09 10:59   ` Jeff Layton
  -1 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 10:59 UTC (permalink / raw)
  To: Tim Gardner
  Cc: Steve French, linux-cifs, Dean Gehnert, samba-technical, linux-kernel

On Sun,  8 Dec 2013 14:08:40 -0700
Tim Gardner <timg@tpi.com> wrote:

> According to Microsoft documentation:
> 		http://msdn.microsoft.com/en-us/library/ee441943.aspx
> 		http://msdn.microsoft.com/en-us/library/ff469854.aspx
> 
> The information level codes used in CIFSSMBSetEOF() are not
> legal. In practice we have found that they really don't work either.
> The specification clearly states that none of the SMB_SET_FILE infornmation
> level codes are supported for TRANS2_SET_PATH_INFORMATION.
> 
> You can trigger this function with the following C code. Note that O_CREAT|O_TRUNC
> is not a valid combination, but it does serve to illustrate the bug.
> 
> cat <<EOF > truncate.c
> 
> int main(int argc, char *argv[])
> {
> 	char *fname;
> 	int fd;
> 	mode_t mode = S_IRUSR|S_IWUSR;
> 	int flags = O_CREAT|O_TRUNC;
> 
> 	if (argc == 2) {
> 		fname = argv[1];
> 	} else {
> 		printf("You must supply a file name.\n");
> 		exit(1);
> 	}
> 
> 	if ((fd=open(fname,flags,mode))  < 0)
> 	{
> 		printf("could not open %s with flags %08x\n",fname,flags);
> 		exit(1);
> 	}
> 	printf("Opened %s with flags %08x\n",fname,flags);
> 
> 	close(fd);
> }
> EOF
> 
> I used this script:
> 
> cat <<EOF > truncate.sh
> LOG=log.txt
> SRV=10.0.0.184
> MP=/tmp/mnt
> TD=$MP/temp
> 
> gcc -o truncate truncate.c
> rm -f $LOG
> 
> sudo mkdir -p $MP
> sudo mount -t cifs //$SRV/test $MP --verbose -o noserverino,nounix,user=test,pass=test
> 
> mkdir -p $TD
> sudo rm -f $TD/junk.txt
> echo 1 > junk.txt
> sudo cp junk.txt $TD/junk.txt
> sudo dmesg -c > /dev/null
> 
> echo 1 | sudo tee /proc/fs/cifs/cifsFYI
> sudo ./truncate $TD/junk.txt
> echo 0 | sudo tee /proc/fs/cifs/cifsFYI
> 
> sudo umount $MP
> sudo dmesg -c > $LOG
> EOF
> 
> The resulting log file:
> 
> [179777.458893] fs/cifs/file.c:cifs_open:447: CIFS VFS: in cifs_open as Xid: 654 with uid: 0
> [179777.458903] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
> [179777.458907] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
> [179777.458922] fs/cifs/file.c:cifs_open:465: inode = 0xffff88005a42b828 file flags are 0x8240 for \temp\junk.txt
> [179777.458935] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 162
> [179777.458938] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=114
> [179777.460089] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x67
> [179777.460109] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x6b, smb_buf_length: 0x67
> [179777.460125] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=162 mid=20 state=4
> [179777.460134] fs/cifs/inode.c:cifs_get_inode_info:646: Getting info on \temp\junk.txt
> [179777.460138] fs/cifs/inode.c:cifs_revalidate_cache:95: cifs_revalidate_cache: revalidating inode 320
> [179777.460141] fs/cifs/inode.c:cifs_revalidate_cache:119: cifs_revalidate_cache: invalidating inode 320 mapping
> [179777.460148] fs/cifs/file.c:cifs_open:545: CIFS VFS: leaving cifs_open (xid = 654) rc = 0
> [179777.460153] fs/cifs/inode.c:cifs_setattr:2280: name junk.txt size 0
> [179777.460157] fs/cifs/inode.c:cifs_setattr_nounix:2126: CIFS VFS: in cifs_setattr_nounix as Xid: 655 with uid: 0
> [179777.460160] fs/cifs/inode.c:cifs_setattr_nounix:2129: setattr on file junk.txt attrs->iavalid 0xa068
> [179777.460163] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
> [179777.460166] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
> [179777.460170] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5403: In SetEOF
> [179777.460175] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 50
> [179777.460178] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=112
> [179777.460810] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
> [179777.460827] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
> [179777.460830] fs/cifs/smb1ops.c:check2ndT2:255: invalid transact2 word count
> [179777.460841] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=50 mid=21 state=4
> [179777.460845] fs/cifs/netmisc.c:map_smb_to_linux_error:891: Mapping smb error code 0xc0000022 to POSIX err -13
> [179777.460849] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5469: SetPathInfo (file size) returned -13
> [179777.460851] fs/cifs/inode.c:cifs_set_file_size:1934: SetEOF by path (setattrs) rc = -13
> [179777.460855] fs/cifs/inode.c:cifs_setattr_nounix:2269: CIFS VFS: leaving cifs_setattr_nounix (xid = 655) rc = -13
> [179777.460863] fs/cifs/file.c:cifsFileInfo_put:385: closing last open instance for inode ffff88005a42b828
> [179777.460866] fs/cifs/file.c:cifsFileInfo_put:403: CIFS VFS: in cifsFileInfo_put as Xid: 656 with uid: 0
> [179777.460869] fs/cifs/cifssmb.c:CIFSSMBClose:2457: In CIFSSMBClose
> [179777.460873] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 4
> [179777.460876] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=41
> [179777.461257] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
> [179777.461267] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
> [179777.461280] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=4 mid=22 state=4
> 
> We encountered this problem whilst backporting the CIFS driver to a kernel and VFS old enough
> to still call inode_operations.setattr() when truncating a file. We're not sure it is the
> right upstream solution, but it worked for us on this older kernel.
> 
> Cc: Steve French <sfrench@samba.org>
> Cc: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Dean Gehnert <deang@tpi.com>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
> 
> V2 - There really is no substantive change from V1. Rather, this patch is now
> the first in a series that acheives the review comments set out by Jeff Layton
> for the V1 patch.
> 
>  fs/cifs/cifsproto.h |    3 --
>  fs/cifs/cifssmb.c   |   98 ---------------------------------------------------
>  fs/cifs/smb1ops.c   |   36 ++++++++++++++++++-
>  3 files changed, 35 insertions(+), 102 deletions(-)
> 
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index aa33976..fe69b9c 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -296,9 +296,6 @@ extern int CIFSSMBSetAttrLegacy(unsigned int xid, struct cifs_tcon *tcon,
>  			char *fileName, __u16 dos_attributes,
>  			const struct nls_table *nls_codepage);
>  #endif /* possibly unneeded function */
> -extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> -			 const char *file_name, __u64 size,
> -			 struct cifs_sb_info *cifs_sb, bool set_allocation);
>  extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
>  			      struct cifsFileInfo *cfile, __u64 size,
>  			      bool set_allocation);
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 124aa02..53f1b9b 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -5453,104 +5453,6 @@ QFSPosixRetry:
>  	return rc;
>  }
>  
> -
> -/*
> - * We can not use write of zero bytes trick to set file size due to need for
> - * large file support. Also note that this SetPathInfo is preferred to
> - * SetFileInfo based method in next routine which is only needed to work around
> - * a sharing violation bugin Samba which this routine can run into.
> - */
> -int
> -CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> -	      const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
> -	      bool set_allocation)
> -{
> -	struct smb_com_transaction2_spi_req *pSMB = NULL;
> -	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
> -	struct file_end_of_file_info *parm_data;
> -	int name_len;
> -	int rc = 0;
> -	int bytes_returned = 0;
> -	int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
> -
> -	__u16 params, byte_count, data_count, param_offset, offset;
> -
> -	cifs_dbg(FYI, "In SetEOF\n");
> -SetEOFRetry:
> -	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
> -		      (void **) &pSMBr);
> -	if (rc)
> -		return rc;
> -
> -	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
> -		name_len =
> -		    cifsConvertToUTF16((__le16 *) pSMB->FileName, file_name,
> -				       PATH_MAX, cifs_sb->local_nls, remap);
> -		name_len++;	/* trailing null */
> -		name_len *= 2;
> -	} else {	/* BB improve the check for buffer overruns BB */
> -		name_len = strnlen(file_name, PATH_MAX);
> -		name_len++;	/* trailing null */
> -		strncpy(pSMB->FileName, file_name, name_len);
> -	}
> -	params = 6 + name_len;
> -	data_count = sizeof(struct file_end_of_file_info);
> -	pSMB->MaxParameterCount = cpu_to_le16(2);
> -	pSMB->MaxDataCount = cpu_to_le16(4100);
> -	pSMB->MaxSetupCount = 0;
> -	pSMB->Reserved = 0;
> -	pSMB->Flags = 0;
> -	pSMB->Timeout = 0;
> -	pSMB->Reserved2 = 0;
> -	param_offset = offsetof(struct smb_com_transaction2_spi_req,
> -				InformationLevel) - 4;
> -	offset = param_offset + params;
> -	if (set_allocation) {
> -		if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
> -			pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO2);
> -		else
> -			pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO);
> -	} else /* Set File Size */  {
> -	    if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
> -		    pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO2);
> -	    else
> -		    pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO);
> -	}
> -
> -	parm_data =
> -	    (struct file_end_of_file_info *) (((char *) &pSMB->hdr.Protocol) +
> -				       offset);
> -	pSMB->ParameterOffset = cpu_to_le16(param_offset);
> -	pSMB->DataOffset = cpu_to_le16(offset);
> -	pSMB->SetupCount = 1;
> -	pSMB->Reserved3 = 0;
> -	pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
> -	byte_count = 3 /* pad */  + params + data_count;
> -	pSMB->DataCount = cpu_to_le16(data_count);
> -	pSMB->TotalDataCount = pSMB->DataCount;
> -	pSMB->ParameterCount = cpu_to_le16(params);
> -	pSMB->TotalParameterCount = pSMB->ParameterCount;
> -	pSMB->Reserved4 = 0;
> -	inc_rfc1001_len(pSMB, byte_count);
> -	parm_data->FileSize = cpu_to_le64(size);
> -	pSMB->ByteCount = cpu_to_le16(byte_count);
> -	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
> -			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
> -	if (rc)
> -		cifs_dbg(FYI, "SetPathInfo (file size) returned %d\n", rc);
> -
> -	cifs_buf_release(pSMB);
> -
> -	if (rc == -EAGAIN)
> -		goto SetEOFRetry;
> -
> -	return rc;
> -}
> -
>  int
>  CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
>  		   struct cifsFileInfo *cfile, __u64 size, bool set_allocation)
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 5f5ba0d..14d4832 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -756,6 +756,40 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
>  }
>  
>  static int
> +smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
> +	      const char *full_path, __u64 size, struct cifs_sb_info *cifs_sb,
> +	      bool set_allocation)
> +{
> +	int oplock = 0;
> +	int rc;
> +	__u16 netfid;
> +	struct cifsFileInfo cfile;
> +
> +	cifs_dbg(FYI, "smb_set_file_size: %s\n", full_path);
> +
> +	rc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> +			 FILE_WRITE_DATA, CREATE_NOT_DIR,
> +			 &netfid, &oplock, NULL, cifs_sb->local_nls,
> +			 cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Only 2 fields are required to set the file size.
> +	 */
> +	memset(&cfile, 0, sizeof(cfile));
> +	cfile.pid = current->tgid;
> +	cfile.fid.netfid = netfid;
> +
> +	rc = CIFSSMBSetFileSize(xid, tcon, &cfile, size, set_allocation);
> +
> +	if (!rc)
> +		rc = CIFSSMBClose(xid, tcon, netfid);
> +
> +	return rc;
> +}
> +
> +static int
>  smb_set_file_info(struct inode *inode, const char *full_path,
>  		  FILE_BASIC_INFO *buf, const unsigned int xid)
>  {
> @@ -979,7 +1013,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_path_size = CIFSSMBSetEOF,
> +	.set_path_size = smb_set_file_size,
>  	.set_file_size = CIFSSMBSetFileSize,
>  	.set_file_info = smb_set_file_info,
>  	.set_compression = cifs_set_compression,

Reviewed-by: Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 1/5 linux-next V2] cifs: Replace CIFSSMBSetEOF() with smb_set_file_size()
@ 2013-12-09 10:59   ` Jeff Layton
  0 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 10:59 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs, samba-technical, linux-kernel, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:08:40 -0700
Tim Gardner <timg@tpi.com> wrote:

> According to Microsoft documentation:
> 		http://msdn.microsoft.com/en-us/library/ee441943.aspx
> 		http://msdn.microsoft.com/en-us/library/ff469854.aspx
> 
> The information level codes used in CIFSSMBSetEOF() are not
> legal. In practice we have found that they really don't work either.
> The specification clearly states that none of the SMB_SET_FILE infornmation
> level codes are supported for TRANS2_SET_PATH_INFORMATION.
> 
> You can trigger this function with the following C code. Note that O_CREAT|O_TRUNC
> is not a valid combination, but it does serve to illustrate the bug.
> 
> cat <<EOF > truncate.c
> 
> int main(int argc, char *argv[])
> {
> 	char *fname;
> 	int fd;
> 	mode_t mode = S_IRUSR|S_IWUSR;
> 	int flags = O_CREAT|O_TRUNC;
> 
> 	if (argc == 2) {
> 		fname = argv[1];
> 	} else {
> 		printf("You must supply a file name.\n");
> 		exit(1);
> 	}
> 
> 	if ((fd=open(fname,flags,mode))  < 0)
> 	{
> 		printf("could not open %s with flags %08x\n",fname,flags);
> 		exit(1);
> 	}
> 	printf("Opened %s with flags %08x\n",fname,flags);
> 
> 	close(fd);
> }
> EOF
> 
> I used this script:
> 
> cat <<EOF > truncate.sh
> LOG=log.txt
> SRV=10.0.0.184
> MP=/tmp/mnt
> TD=$MP/temp
> 
> gcc -o truncate truncate.c
> rm -f $LOG
> 
> sudo mkdir -p $MP
> sudo mount -t cifs //$SRV/test $MP --verbose -o noserverino,nounix,user=test,pass=test
> 
> mkdir -p $TD
> sudo rm -f $TD/junk.txt
> echo 1 > junk.txt
> sudo cp junk.txt $TD/junk.txt
> sudo dmesg -c > /dev/null
> 
> echo 1 | sudo tee /proc/fs/cifs/cifsFYI
> sudo ./truncate $TD/junk.txt
> echo 0 | sudo tee /proc/fs/cifs/cifsFYI
> 
> sudo umount $MP
> sudo dmesg -c > $LOG
> EOF
> 
> The resulting log file:
> 
> [179777.458893] fs/cifs/file.c:cifs_open:447: CIFS VFS: in cifs_open as Xid: 654 with uid: 0
> [179777.458903] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
> [179777.458907] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
> [179777.458922] fs/cifs/file.c:cifs_open:465: inode = 0xffff88005a42b828 file flags are 0x8240 for \temp\junk.txt
> [179777.458935] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 162
> [179777.458938] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=114
> [179777.460089] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x67
> [179777.460109] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x6b, smb_buf_length: 0x67
> [179777.460125] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=162 mid=20 state=4
> [179777.460134] fs/cifs/inode.c:cifs_get_inode_info:646: Getting info on \temp\junk.txt
> [179777.460138] fs/cifs/inode.c:cifs_revalidate_cache:95: cifs_revalidate_cache: revalidating inode 320
> [179777.460141] fs/cifs/inode.c:cifs_revalidate_cache:119: cifs_revalidate_cache: invalidating inode 320 mapping
> [179777.460148] fs/cifs/file.c:cifs_open:545: CIFS VFS: leaving cifs_open (xid = 654) rc = 0
> [179777.460153] fs/cifs/inode.c:cifs_setattr:2280: name junk.txt size 0
> [179777.460157] fs/cifs/inode.c:cifs_setattr_nounix:2126: CIFS VFS: in cifs_setattr_nounix as Xid: 655 with uid: 0
> [179777.460160] fs/cifs/inode.c:cifs_setattr_nounix:2129: setattr on file junk.txt attrs->iavalid 0xa068
> [179777.460163] fs/cifs/dir.c:build_path_from_dentry:127: name: \junk.txt
> [179777.460166] fs/cifs/dir.c:build_path_from_dentry:127: name: \temp\junk.txt
> [179777.460170] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5403: In SetEOF
> [179777.460175] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 50
> [179777.460178] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=112
> [179777.460810] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
> [179777.460827] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
> [179777.460830] fs/cifs/smb1ops.c:check2ndT2:255: invalid transact2 word count
> [179777.460841] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=50 mid=21 state=4
> [179777.460845] fs/cifs/netmisc.c:map_smb_to_linux_error:891: Mapping smb error code 0xc0000022 to POSIX err -13
> [179777.460849] fs/cifs/cifssmb.c:CIFSSMBSetEOF:5469: SetPathInfo (file size) returned -13
> [179777.460851] fs/cifs/inode.c:cifs_set_file_size:1934: SetEOF by path (setattrs) rc = -13
> [179777.460855] fs/cifs/inode.c:cifs_setattr_nounix:2269: CIFS VFS: leaving cifs_setattr_nounix (xid = 655) rc = -13
> [179777.460863] fs/cifs/file.c:cifsFileInfo_put:385: closing last open instance for inode ffff88005a42b828
> [179777.460866] fs/cifs/file.c:cifsFileInfo_put:403: CIFS VFS: in cifsFileInfo_put as Xid: 656 with uid: 0
> [179777.460869] fs/cifs/cifssmb.c:CIFSSMBClose:2457: In CIFSSMBClose
> [179777.460873] fs/cifs/transport.c:AllocMidQEntry:64: For smb_command 4
> [179777.460876] fs/cifs/transport.c:smb_send_rqst:288: Sending smb: smb_len=41
> [179777.461257] fs/cifs/connect.c:cifs_demultiplex_thread:874: RFC1002 header 0x23
> [179777.461267] fs/cifs/misc.c:checkSMB:316: checkSMB Length: 0x27, smb_buf_length: 0x23
> [179777.461280] fs/cifs/transport.c:cifs_sync_mid_result:571: cifs_sync_mid_result: cmd=4 mid=22 state=4
> 
> We encountered this problem whilst backporting the CIFS driver to a kernel and VFS old enough
> to still call inode_operations.setattr() when truncating a file. We're not sure it is the
> right upstream solution, but it worked for us on this older kernel.
> 
> Cc: Steve French <sfrench@samba.org>
> Cc: Jeff Layton <jlayton@redhat.com>
> Signed-off-by: Dean Gehnert <deang@tpi.com>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
> 
> V2 - There really is no substantive change from V1. Rather, this patch is now
> the first in a series that acheives the review comments set out by Jeff Layton
> for the V1 patch.
> 
>  fs/cifs/cifsproto.h |    3 --
>  fs/cifs/cifssmb.c   |   98 ---------------------------------------------------
>  fs/cifs/smb1ops.c   |   36 ++++++++++++++++++-
>  3 files changed, 35 insertions(+), 102 deletions(-)
> 
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index aa33976..fe69b9c 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -296,9 +296,6 @@ extern int CIFSSMBSetAttrLegacy(unsigned int xid, struct cifs_tcon *tcon,
>  			char *fileName, __u16 dos_attributes,
>  			const struct nls_table *nls_codepage);
>  #endif /* possibly unneeded function */
> -extern int CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> -			 const char *file_name, __u64 size,
> -			 struct cifs_sb_info *cifs_sb, bool set_allocation);
>  extern int CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
>  			      struct cifsFileInfo *cfile, __u64 size,
>  			      bool set_allocation);
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 124aa02..53f1b9b 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -5453,104 +5453,6 @@ QFSPosixRetry:
>  	return rc;
>  }
>  
> -
> -/*
> - * We can not use write of zero bytes trick to set file size due to need for
> - * large file support. Also note that this SetPathInfo is preferred to
> - * SetFileInfo based method in next routine which is only needed to work around
> - * a sharing violation bugin Samba which this routine can run into.
> - */
> -int
> -CIFSSMBSetEOF(const unsigned int xid, struct cifs_tcon *tcon,
> -	      const char *file_name, __u64 size, struct cifs_sb_info *cifs_sb,
> -	      bool set_allocation)
> -{
> -	struct smb_com_transaction2_spi_req *pSMB = NULL;
> -	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
> -	struct file_end_of_file_info *parm_data;
> -	int name_len;
> -	int rc = 0;
> -	int bytes_returned = 0;
> -	int remap = cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR;
> -
> -	__u16 params, byte_count, data_count, param_offset, offset;
> -
> -	cifs_dbg(FYI, "In SetEOF\n");
> -SetEOFRetry:
> -	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
> -		      (void **) &pSMBr);
> -	if (rc)
> -		return rc;
> -
> -	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
> -		name_len =
> -		    cifsConvertToUTF16((__le16 *) pSMB->FileName, file_name,
> -				       PATH_MAX, cifs_sb->local_nls, remap);
> -		name_len++;	/* trailing null */
> -		name_len *= 2;
> -	} else {	/* BB improve the check for buffer overruns BB */
> -		name_len = strnlen(file_name, PATH_MAX);
> -		name_len++;	/* trailing null */
> -		strncpy(pSMB->FileName, file_name, name_len);
> -	}
> -	params = 6 + name_len;
> -	data_count = sizeof(struct file_end_of_file_info);
> -	pSMB->MaxParameterCount = cpu_to_le16(2);
> -	pSMB->MaxDataCount = cpu_to_le16(4100);
> -	pSMB->MaxSetupCount = 0;
> -	pSMB->Reserved = 0;
> -	pSMB->Flags = 0;
> -	pSMB->Timeout = 0;
> -	pSMB->Reserved2 = 0;
> -	param_offset = offsetof(struct smb_com_transaction2_spi_req,
> -				InformationLevel) - 4;
> -	offset = param_offset + params;
> -	if (set_allocation) {
> -		if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
> -			pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO2);
> -		else
> -			pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_ALLOCATION_INFO);
> -	} else /* Set File Size */  {
> -	    if (tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)
> -		    pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO2);
> -	    else
> -		    pSMB->InformationLevel =
> -				cpu_to_le16(SMB_SET_FILE_END_OF_FILE_INFO);
> -	}
> -
> -	parm_data =
> -	    (struct file_end_of_file_info *) (((char *) &pSMB->hdr.Protocol) +
> -				       offset);
> -	pSMB->ParameterOffset = cpu_to_le16(param_offset);
> -	pSMB->DataOffset = cpu_to_le16(offset);
> -	pSMB->SetupCount = 1;
> -	pSMB->Reserved3 = 0;
> -	pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
> -	byte_count = 3 /* pad */  + params + data_count;
> -	pSMB->DataCount = cpu_to_le16(data_count);
> -	pSMB->TotalDataCount = pSMB->DataCount;
> -	pSMB->ParameterCount = cpu_to_le16(params);
> -	pSMB->TotalParameterCount = pSMB->ParameterCount;
> -	pSMB->Reserved4 = 0;
> -	inc_rfc1001_len(pSMB, byte_count);
> -	parm_data->FileSize = cpu_to_le64(size);
> -	pSMB->ByteCount = cpu_to_le16(byte_count);
> -	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
> -			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
> -	if (rc)
> -		cifs_dbg(FYI, "SetPathInfo (file size) returned %d\n", rc);
> -
> -	cifs_buf_release(pSMB);
> -
> -	if (rc == -EAGAIN)
> -		goto SetEOFRetry;
> -
> -	return rc;
> -}
> -
>  int
>  CIFSSMBSetFileSize(const unsigned int xid, struct cifs_tcon *tcon,
>  		   struct cifsFileInfo *cfile, __u64 size, bool set_allocation)
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 5f5ba0d..14d4832 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -756,6 +756,40 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
>  }
>  
>  static int
> +smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
> +	      const char *full_path, __u64 size, struct cifs_sb_info *cifs_sb,
> +	      bool set_allocation)
> +{
> +	int oplock = 0;
> +	int rc;
> +	__u16 netfid;
> +	struct cifsFileInfo cfile;
> +
> +	cifs_dbg(FYI, "smb_set_file_size: %s\n", full_path);
> +
> +	rc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> +			 FILE_WRITE_DATA, CREATE_NOT_DIR,
> +			 &netfid, &oplock, NULL, cifs_sb->local_nls,
> +			 cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Only 2 fields are required to set the file size.
> +	 */
> +	memset(&cfile, 0, sizeof(cfile));
> +	cfile.pid = current->tgid;
> +	cfile.fid.netfid = netfid;
> +
> +	rc = CIFSSMBSetFileSize(xid, tcon, &cfile, size, set_allocation);
> +
> +	if (!rc)
> +		rc = CIFSSMBClose(xid, tcon, netfid);
> +
> +	return rc;
> +}
> +
> +static int
>  smb_set_file_info(struct inode *inode, const char *full_path,
>  		  FILE_BASIC_INFO *buf, const unsigned int xid)
>  {
> @@ -979,7 +1013,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_path_size = CIFSSMBSetEOF,
> +	.set_path_size = smb_set_file_size,
>  	.set_file_size = CIFSSMBSetFileSize,
>  	.set_file_info = smb_set_file_info,
>  	.set_compression = cifs_set_compression,

Reviewed-by: Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 2/5 linux-next V2] cifs: rename set_path_size and set_file_size
  2013-12-08 21:08   ` Tim Gardner
@ 2013-12-09 10:59     ` Jeff Layton
  -1 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 10:59 UTC (permalink / raw)
  To: Tim Gardner
  Cc: Steve French, linux-cifs, Dean Gehnert, samba-technical, linux-kernel

On Sun,  8 Dec 2013 14:08:41 -0700
Tim Gardner <timg@tpi.com> wrote:

> These 2 method names are a bit confusing. Rename them
> so that one can tell at a glance how they are to be used.
> 
> Cc: Steve French <sfrench@samba.org>
> Cc: Jeff Layton <jlayton@redhat.com>
> Cc: Dean Gehnert <deang@tpi.com>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
> 
> V2 - this is a new patch in the V2 series.
> 
>  fs/cifs/cifsglob.h  |    4 ++--
>  fs/cifs/inode.c     |   15 +++++++++------
>  fs/cifs/smb1ops.c   |    6 +++---
>  fs/cifs/smb2inode.c |    2 +-
>  fs/cifs/smb2ops.c   |   14 +++++++-------
>  fs/cifs/smb2proto.h |    8 +++++---
>  6 files changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f918a99..8fd8eb2 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -270,10 +270,10 @@ struct smb_version_operations {
>  			    struct cifs_sb_info *, const char *,
>  			    u64 *uniqueid, FILE_ALL_INFO *);
>  	/* set size by path */
> -	int (*set_path_size)(const unsigned int, struct cifs_tcon *,
> +	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)(const unsigned int, struct cifs_tcon *,
> +	int (*set_file_size_by_handle)(const unsigned int, struct cifs_tcon *,
>  			     struct cifsFileInfo *, __u64, bool);
>  	/* set attributes */
>  	int (*set_file_info)(struct inode *, const char *, FILE_BASIC_INFO *,
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 36f9ebb..7181516 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1925,9 +1925,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  	if (open_file) {
>  		tcon = tlink_tcon(open_file->tlink);
>  		server = tcon->ses->server;
> -		if (server->ops->set_file_size)
> -			rc = server->ops->set_file_size(xid, tcon, open_file,
> -							attrs->ia_size, false);
> +		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);
> @@ -1963,9 +1965,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  	 * valid, writeable file handle for it was found or because there was
>  	 * an error setting it by handle.
>  	 */
> -	if (server->ops->set_path_size)
> -		rc = server->ops->set_path_size(xid, tcon, full_path,
> -						attrs->ia_size, cifs_sb, false);
> +	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);
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 14d4832..c5d9ec6 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -756,7 +756,7 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
>  }
>  
>  static int
> -smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
> +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,
>  	      bool set_allocation)
>  {
> @@ -1013,8 +1013,8 @@ 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_path_size = smb_set_file_size,
> -	.set_file_size = CIFSSMBSetFileSize,
> +	.set_file_size_by_path = smb_set_file_size_by_path,
> +	.set_file_size_by_handle = CIFSSMBSetFileSize,
>  	.set_file_info = smb_set_file_info,
>  	.set_compression = cifs_set_compression,
>  	.echo = CIFSSMBEcho,
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 84c012a..78b590f 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -244,7 +244,7 @@ smb2_create_hardlink(const unsigned int xid, struct cifs_tcon *tcon,
>  }
>  
>  int
> -smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> +smb2_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, bool set_alloc)
>  {
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 757da3e..dc7b1cb 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -688,7 +688,7 @@ smb2_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
>  }
>  
>  static int
> -smb2_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
> +smb2_set_file_size_by_handle(const unsigned int xid, struct cifs_tcon *tcon,
>  		   struct cifsFileInfo *cfile, __u64 size, bool set_alloc)
>  {
>  	__le64 eof = cpu_to_le64(size);
> @@ -1131,8 +1131,8 @@ 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_path_size = smb2_set_path_size,
> -	.set_file_size = smb2_set_file_size,
> +	.set_file_size_by_path = smb2_set_file_size_by_path,
> +	.set_file_size_by_handle = smb2_set_file_size_by_handle,
>  	.set_file_info = smb2_set_file_info,
>  	.set_compression = smb2_set_compression,
>  	.mkdir = smb2_mkdir,
> @@ -1205,8 +1205,8 @@ 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_path_size = smb2_set_path_size,
> -	.set_file_size = smb2_set_file_size,
> +	.set_file_size_by_path = smb2_set_file_size_by_path,
> +	.set_file_size_by_handle = smb2_set_file_size_by_handle,
>  	.set_file_info = smb2_set_file_info,
>  	.set_compression = smb2_set_compression,
>  	.mkdir = smb2_mkdir,
> @@ -1280,8 +1280,8 @@ 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_path_size = smb2_set_path_size,
> -	.set_file_size = smb2_set_file_size,
> +	.set_file_size_by_path = smb2_set_file_size_by_path,
> +	.set_file_size_by_handle = smb2_set_file_size_by_handle,
>  	.set_file_info = smb2_set_file_info,
>  	.set_compression = smb2_set_compression,
>  	.mkdir = smb2_mkdir,
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 93adc64..b6f3c13 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -62,9 +62,11 @@ extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>  				struct cifs_sb_info *cifs_sb,
>  				const char *full_path, FILE_ALL_INFO *data,
>  				bool *adjust_tz, bool *symlink);
> -extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> -			      const char *full_path, __u64 size,
> -			      struct cifs_sb_info *cifs_sb, bool set_alloc);
> +extern int smb2_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,
> +				      bool set_alloc);
>  extern int smb2_set_file_info(struct inode *inode, const char *full_path,
>  			      FILE_BASIC_INFO *buf, const unsigned int xid);
>  extern int smb2_mkdir(const unsigned int xid, struct cifs_tcon *tcon,

Reviewed-by: Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 2/5 linux-next V2] cifs: rename set_path_size and set_file_size
@ 2013-12-09 10:59     ` Jeff Layton
  0 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 10:59 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs, samba-technical, linux-kernel, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:08:41 -0700
Tim Gardner <timg@tpi.com> wrote:

> These 2 method names are a bit confusing. Rename them
> so that one can tell at a glance how they are to be used.
> 
> Cc: Steve French <sfrench@samba.org>
> Cc: Jeff Layton <jlayton@redhat.com>
> Cc: Dean Gehnert <deang@tpi.com>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
> 
> V2 - this is a new patch in the V2 series.
> 
>  fs/cifs/cifsglob.h  |    4 ++--
>  fs/cifs/inode.c     |   15 +++++++++------
>  fs/cifs/smb1ops.c   |    6 +++---
>  fs/cifs/smb2inode.c |    2 +-
>  fs/cifs/smb2ops.c   |   14 +++++++-------
>  fs/cifs/smb2proto.h |    8 +++++---
>  6 files changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index f918a99..8fd8eb2 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -270,10 +270,10 @@ struct smb_version_operations {
>  			    struct cifs_sb_info *, const char *,
>  			    u64 *uniqueid, FILE_ALL_INFO *);
>  	/* set size by path */
> -	int (*set_path_size)(const unsigned int, struct cifs_tcon *,
> +	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)(const unsigned int, struct cifs_tcon *,
> +	int (*set_file_size_by_handle)(const unsigned int, struct cifs_tcon *,
>  			     struct cifsFileInfo *, __u64, bool);
>  	/* set attributes */
>  	int (*set_file_info)(struct inode *, const char *, FILE_BASIC_INFO *,
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 36f9ebb..7181516 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1925,9 +1925,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  	if (open_file) {
>  		tcon = tlink_tcon(open_file->tlink);
>  		server = tcon->ses->server;
> -		if (server->ops->set_file_size)
> -			rc = server->ops->set_file_size(xid, tcon, open_file,
> -							attrs->ia_size, false);
> +		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);
> @@ -1963,9 +1965,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  	 * valid, writeable file handle for it was found or because there was
>  	 * an error setting it by handle.
>  	 */
> -	if (server->ops->set_path_size)
> -		rc = server->ops->set_path_size(xid, tcon, full_path,
> -						attrs->ia_size, cifs_sb, false);
> +	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);
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 14d4832..c5d9ec6 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -756,7 +756,7 @@ cifs_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
>  }
>  
>  static int
> -smb_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
> +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,
>  	      bool set_allocation)
>  {
> @@ -1013,8 +1013,8 @@ 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_path_size = smb_set_file_size,
> -	.set_file_size = CIFSSMBSetFileSize,
> +	.set_file_size_by_path = smb_set_file_size_by_path,
> +	.set_file_size_by_handle = CIFSSMBSetFileSize,
>  	.set_file_info = smb_set_file_info,
>  	.set_compression = cifs_set_compression,
>  	.echo = CIFSSMBEcho,
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 84c012a..78b590f 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -244,7 +244,7 @@ smb2_create_hardlink(const unsigned int xid, struct cifs_tcon *tcon,
>  }
>  
>  int
> -smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> +smb2_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, bool set_alloc)
>  {
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 757da3e..dc7b1cb 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -688,7 +688,7 @@ smb2_sync_write(const unsigned int xid, struct cifsFileInfo *cfile,
>  }
>  
>  static int
> -smb2_set_file_size(const unsigned int xid, struct cifs_tcon *tcon,
> +smb2_set_file_size_by_handle(const unsigned int xid, struct cifs_tcon *tcon,
>  		   struct cifsFileInfo *cfile, __u64 size, bool set_alloc)
>  {
>  	__le64 eof = cpu_to_le64(size);
> @@ -1131,8 +1131,8 @@ 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_path_size = smb2_set_path_size,
> -	.set_file_size = smb2_set_file_size,
> +	.set_file_size_by_path = smb2_set_file_size_by_path,
> +	.set_file_size_by_handle = smb2_set_file_size_by_handle,
>  	.set_file_info = smb2_set_file_info,
>  	.set_compression = smb2_set_compression,
>  	.mkdir = smb2_mkdir,
> @@ -1205,8 +1205,8 @@ 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_path_size = smb2_set_path_size,
> -	.set_file_size = smb2_set_file_size,
> +	.set_file_size_by_path = smb2_set_file_size_by_path,
> +	.set_file_size_by_handle = smb2_set_file_size_by_handle,
>  	.set_file_info = smb2_set_file_info,
>  	.set_compression = smb2_set_compression,
>  	.mkdir = smb2_mkdir,
> @@ -1280,8 +1280,8 @@ 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_path_size = smb2_set_path_size,
> -	.set_file_size = smb2_set_file_size,
> +	.set_file_size_by_path = smb2_set_file_size_by_path,
> +	.set_file_size_by_handle = smb2_set_file_size_by_handle,
>  	.set_file_info = smb2_set_file_info,
>  	.set_compression = smb2_set_compression,
>  	.mkdir = smb2_mkdir,
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 93adc64..b6f3c13 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -62,9 +62,11 @@ extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>  				struct cifs_sb_info *cifs_sb,
>  				const char *full_path, FILE_ALL_INFO *data,
>  				bool *adjust_tz, bool *symlink);
> -extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> -			      const char *full_path, __u64 size,
> -			      struct cifs_sb_info *cifs_sb, bool set_alloc);
> +extern int smb2_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,
> +				      bool set_alloc);
>  extern int smb2_set_file_info(struct inode *inode, const char *full_path,
>  			      FILE_BASIC_INFO *buf, const unsigned int xid);
>  extern int smb2_mkdir(const unsigned int xid, struct cifs_tcon *tcon,

Reviewed-by: Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 3/5 linux-next V2] cifs: Introduce cifs_legacy_set_file_size()
  2013-12-08 21:08   ` Tim Gardner
@ 2013-12-09 11:00       ` Jeff Layton
  -1 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 11:00 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:08:42 -0700
Tim Gardner <timg-l6nL5VImRDY@public.gmane.org> wrote:

> Consolidates some duplicate code.
> 
> Cc: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Dean Gehnert <deang-l6nL5VImRDY@public.gmane.org>
> Signed-off-by: Tim Gardner <timg-l6nL5VImRDY@public.gmane.org>
> ---
> 
> V2 - this is a new patch in the V2 series.
> 
>  fs/cifs/inode.c |   54 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 7181516..3f710c6 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1899,6 +1899,30 @@ 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)
> @@ -1910,7 +1934,6 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  	struct tcon_link *tlink = NULL;
>  	struct cifs_tcon *tcon = NULL;
>  	struct TCP_Server_Info *server;
> -	struct cifs_io_parms io_parms;
>  
>  	/*
>  	 * To avoid spurious oplock breaks from server, in the case of
> @@ -1935,16 +1958,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  		cifsFileInfo_put(open_file);
>  		cifs_dbg(FYI, "SetFSize for attrs rc = %d\n", rc);
>  		if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
> -			unsigned int bytes_written;
> -
> -			io_parms.netfid = open_file->fid.netfid;
> -			io_parms.pid = open_file->pid;
> -			io_parms.tcon = tcon;
> -			io_parms.offset = 0;
> -			io_parms.length = attrs->ia_size;
> -			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written,
> -					  NULL, NULL, 1);
> -			cifs_dbg(FYI, "Wrt seteof rc %d\n", rc);
> +			rc = cifs_legacy_set_file_size(xid,
> +						       open_file->fid.netfid,
> +						       open_file->pid,
> +						       attrs->ia_size,
> +						       tcon, full_path);
>  		}
>  	} else
>  		rc = -EINVAL;
> @@ -1982,16 +2000,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  				   cifs_sb->mnt_cifs_flags &
>  						CIFS_MOUNT_MAP_SPECIAL_CHR);
>  		if (rc == 0) {
> -			unsigned int bytes_written;
> -
> -			io_parms.netfid = netfid;
> -			io_parms.pid = current->tgid;
> -			io_parms.tcon = tcon;
> -			io_parms.offset = 0;
> -			io_parms.length = attrs->ia_size;
> -			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written, NULL,
> -					  NULL,  1);
> -			cifs_dbg(FYI, "wrt seteof rc %d\n", rc);
> +			rc = cifs_legacy_set_file_size(xid, netfid,
> +						       current->tgid,
> +						       attrs->ia_size, tcon,
> +						       full_path);
>  			CIFSSMBClose(xid, tcon, netfid);
>  		}
>  	}

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 3/5 linux-next V2] cifs: Introduce cifs_legacy_set_file_size()
@ 2013-12-09 11:00       ` Jeff Layton
  0 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 11:00 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs, samba-technical, linux-kernel, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:08:42 -0700
Tim Gardner <timg@tpi.com> wrote:

> Consolidates some duplicate code.
> 
> Cc: Steve French <sfrench@samba.org>
> Cc: Jeff Layton <jlayton@redhat.com>
> Cc: Dean Gehnert <deang@tpi.com>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
> 
> V2 - this is a new patch in the V2 series.
> 
>  fs/cifs/inode.c |   54 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 7181516..3f710c6 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1899,6 +1899,30 @@ 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)
> @@ -1910,7 +1934,6 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  	struct tcon_link *tlink = NULL;
>  	struct cifs_tcon *tcon = NULL;
>  	struct TCP_Server_Info *server;
> -	struct cifs_io_parms io_parms;
>  
>  	/*
>  	 * To avoid spurious oplock breaks from server, in the case of
> @@ -1935,16 +1958,11 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  		cifsFileInfo_put(open_file);
>  		cifs_dbg(FYI, "SetFSize for attrs rc = %d\n", rc);
>  		if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
> -			unsigned int bytes_written;
> -
> -			io_parms.netfid = open_file->fid.netfid;
> -			io_parms.pid = open_file->pid;
> -			io_parms.tcon = tcon;
> -			io_parms.offset = 0;
> -			io_parms.length = attrs->ia_size;
> -			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written,
> -					  NULL, NULL, 1);
> -			cifs_dbg(FYI, "Wrt seteof rc %d\n", rc);
> +			rc = cifs_legacy_set_file_size(xid,
> +						       open_file->fid.netfid,
> +						       open_file->pid,
> +						       attrs->ia_size,
> +						       tcon, full_path);
>  		}
>  	} else
>  		rc = -EINVAL;
> @@ -1982,16 +2000,10 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  				   cifs_sb->mnt_cifs_flags &
>  						CIFS_MOUNT_MAP_SPECIAL_CHR);
>  		if (rc == 0) {
> -			unsigned int bytes_written;
> -
> -			io_parms.netfid = netfid;
> -			io_parms.pid = current->tgid;
> -			io_parms.tcon = tcon;
> -			io_parms.offset = 0;
> -			io_parms.length = attrs->ia_size;
> -			rc = CIFSSMBWrite(xid, &io_parms, &bytes_written, NULL,
> -					  NULL,  1);
> -			cifs_dbg(FYI, "wrt seteof rc %d\n", rc);
> +			rc = cifs_legacy_set_file_size(xid, netfid,
> +						       current->tgid,
> +						       attrs->ia_size, tcon,
> +						       full_path);
>  			CIFSSMBClose(xid, tcon, netfid);
>  		}
>  	}

Reviewed-by: Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 4/5 linux-next V2] cifs: fix incorrect reference count check
  2013-12-08 21:08   ` Tim Gardner
@ 2013-12-09 11:03       ` Jeff Layton
  -1 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 11:03 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:08:43 -0700
Tim Gardner <timg-l6nL5VImRDY@public.gmane.org> wrote:

> The reference count on tlink can only be decremented if
> cifs_sb_tlink(cifs_sb) was used to acquire it. That only
> happens if open_file==NULL.
> 
> Cc: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Dean Gehnert <deang-l6nL5VImRDY@public.gmane.org>
> Signed-off-by: Tim Gardner <timg-l6nL5VImRDY@public.gmane.org>
> ---
> 
> V2 - this is a new patch in the V2 series.
> 
>  fs/cifs/inode.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 3f710c6..e332038 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2007,7 +2007,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  			CIFSSMBClose(xid, tcon, netfid);
>  		}
>  	}
> -	if (tlink)
> +	if (!open_file)
>  		cifs_put_tlink(tlink);
>  
>  set_size_out:


I don't see the bug here...

The only place tlink gets set to a non-NULL value is where
cifs_sb_tlink gets called. Am I missing something?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 4/5 linux-next V2] cifs: fix incorrect reference count check
@ 2013-12-09 11:03       ` Jeff Layton
  0 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 11:03 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs, samba-technical, linux-kernel, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:08:43 -0700
Tim Gardner <timg@tpi.com> wrote:

> The reference count on tlink can only be decremented if
> cifs_sb_tlink(cifs_sb) was used to acquire it. That only
> happens if open_file==NULL.
> 
> Cc: Steve French <sfrench@samba.org>
> Cc: Jeff Layton <jlayton@redhat.com>
> Cc: Dean Gehnert <deang@tpi.com>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
> 
> V2 - this is a new patch in the V2 series.
> 
>  fs/cifs/inode.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 3f710c6..e332038 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2007,7 +2007,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>  			CIFSSMBClose(xid, tcon, netfid);
>  		}
>  	}
> -	if (tlink)
> +	if (!open_file)
>  		cifs_put_tlink(tlink);
>  
>  set_size_out:


I don't see the bug here...

The only place tlink gets set to a non-NULL value is where
cifs_sb_tlink gets called. Am I missing something?

-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 5/5 linux-next V2 (resend)] cifs: Combine set_file_size by handle and path into one function
  2013-12-08 21:13     ` Tim Gardner
@ 2013-12-09 11:10         ` Jeff Layton
  -1 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 11:10 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:13:37 -0700
Tim Gardner <timg-l6nL5VImRDY@public.gmane.org> 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-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Dean Gehnert <deang-l6nL5VImRDY@public.gmane.org>
> Signed-off-by: Tim Gardner <timg-l6nL5VImRDY@public.gmane.org>
> ---
> 
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 5/5 linux-next V2 (resend)] cifs: Combine set_file_size by handle and path into one function
@ 2013-12-09 11:10         ` Jeff Layton
  0 siblings, 0 replies; 23+ messages in thread
From: Jeff Layton @ 2013-12-09 11:10 UTC (permalink / raw)
  To: Tim Gardner
  Cc: linux-cifs, samba-technical, linux-kernel, Steve French, Dean Gehnert

On Sun,  8 Dec 2013 14:13:37 -0700
Tim Gardner <timg@tpi.com> 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@samba.org>
> Cc: Jeff Layton <jlayton@redhat.com>
> Cc: Dean Gehnert <deang@tpi.com>
> Signed-off-by: Tim Gardner <timg@tpi.com>
> ---
> 
> 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@redhat.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 4/5 linux-next V2] cifs: fix incorrect reference count check
  2013-12-09 11:03       ` Jeff Layton
@ 2013-12-09 19:21         ` Tim Gardner
  -1 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-09 19:21 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Steve French, linux-cifs, Dean Gehnert, samba-technical, linux-kernel

On 12/09/2013 04:03 AM, Jeff Layton wrote:
> On Sun,  8 Dec 2013 14:08:43 -0700
> Tim Gardner <timg@tpi.com> wrote:
> 
>> The reference count on tlink can only be decremented if
>> cifs_sb_tlink(cifs_sb) was used to acquire it. That only
>> happens if open_file==NULL.
>>
>> Cc: Steve French <sfrench@samba.org>
>> Cc: Jeff Layton <jlayton@redhat.com>
>> Cc: Dean Gehnert <deang@tpi.com>
>> Signed-off-by: Tim Gardner <timg@tpi.com>
>> ---
>>
>> V2 - this is a new patch in the V2 series.
>>
>>  fs/cifs/inode.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index 3f710c6..e332038 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -2007,7 +2007,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>>  			CIFSSMBClose(xid, tcon, netfid);
>>  		}
>>  	}
>> -	if (tlink)
>> +	if (!open_file)
>>  		cifs_put_tlink(tlink);
>>  
>>  set_size_out:
> 
> 
> I don't see the bug here...
> 
> The only place tlink gets set to a non-NULL value is where
> cifs_sb_tlink gets called. Am I missing something?
> 

Nope - I think you're correct. For some reason I thought tlink was set
inside the 'if (openfile) {...}' clause. I'll drop this patch from the
V3 series.

rtg
-- 
Tim Gardner timg@tpi.com www.tpi.com
OR 503-601-0234 x102 MT 406-443-5357

^ permalink raw reply	[flat|nested] 23+ messages in thread
* Re: [PATCH 4/5 linux-next V2] cifs: fix incorrect reference count check
@ 2013-12-09 19:21         ` Tim Gardner
  0 siblings, 0 replies; 23+ messages in thread
From: Tim Gardner @ 2013-12-09 19:21 UTC (permalink / raw)
  To: Jeff Layton
  Cc: linux-cifs, samba-technical, linux-kernel, Steve French, Dean Gehnert

On 12/09/2013 04:03 AM, Jeff Layton wrote:
> On Sun,  8 Dec 2013 14:08:43 -0700
> Tim Gardner <timg@tpi.com> wrote:
> 
>> The reference count on tlink can only be decremented if
>> cifs_sb_tlink(cifs_sb) was used to acquire it. That only
>> happens if open_file==NULL.
>>
>> Cc: Steve French <sfrench@samba.org>
>> Cc: Jeff Layton <jlayton@redhat.com>
>> Cc: Dean Gehnert <deang@tpi.com>
>> Signed-off-by: Tim Gardner <timg@tpi.com>
>> ---
>>
>> V2 - this is a new patch in the V2 series.
>>
>>  fs/cifs/inode.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index 3f710c6..e332038 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -2007,7 +2007,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>>  			CIFSSMBClose(xid, tcon, netfid);
>>  		}
>>  	}
>> -	if (tlink)
>> +	if (!open_file)
>>  		cifs_put_tlink(tlink);
>>  
>>  set_size_out:
> 
> 
> I don't see the bug here...
> 
> The only place tlink gets set to a non-NULL value is where
> cifs_sb_tlink gets called. Am I missing something?
> 

Nope - I think you're correct. For some reason I thought tlink was set
inside the 'if (openfile) {...}' clause. I'll drop this patch from the
V3 series.

rtg
-- 
Tim Gardner timg@tpi.com www.tpi.com
OR 503-601-0234 x102 MT 406-443-5357

^ permalink raw reply	[flat|nested] 23+ messages in thread
end of thread, other threads:[~2013-12-09 19:21 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-08 21:08 [PATCH 1/5 linux-next V2] cifs: Replace CIFSSMBSetEOF() with smb_set_file_size() Tim Gardner
2013-12-08 21:08 ` Tim Gardner
2013-12-08 21:08 ` [PATCH 2/5 linux-next V2] cifs: rename set_path_size and set_file_size Tim Gardner
2013-12-08 21:08   ` Tim Gardner
2013-12-09 10:59   ` Jeff Layton
2013-12-09 10:59     ` Jeff Layton
2013-12-08 21:08 ` [PATCH 3/5 linux-next V2] cifs: Introduce cifs_legacy_set_file_size() Tim Gardner
2013-12-08 21:08   ` Tim Gardner
     [not found]   ` <1386536924-51726-3-git-send-email-timg-l6nL5VImRDY@public.gmane.org>
2013-12-09 11:00     ` Jeff Layton
2013-12-09 11:00       ` Jeff Layton
2013-12-08 21:08 ` [PATCH 4/5 linux-next V2] cifs: fix incorrect reference count check Tim Gardner
2013-12-08 21:08   ` Tim Gardner
     [not found]   ` <1386536924-51726-4-git-send-email-timg-l6nL5VImRDY@public.gmane.org>
2013-12-09 11:03     ` Jeff Layton
2013-12-09 11:03       ` Jeff Layton
2013-12-09 19:21       ` Tim Gardner
2013-12-09 19:21         ` Tim Gardner
2013-12-08 21:08 ` [PATCH 5/5 linux-next V2] cifs: Combine set_file_size by handle and path into one function Tim Gardner
     [not found] ` <1386536924-51726-1-git-send-email-timg-l6nL5VImRDY@public.gmane.org>
2013-12-08 21:13   ` [PATCH 5/5 linux-next V2 (resend)] " Tim Gardner
2013-12-08 21:13     ` Tim Gardner
     [not found]     ` <1386537217-52184-1-git-send-email-timg-l6nL5VImRDY@public.gmane.org>
2013-12-09 11:10       ` Jeff Layton
2013-12-09 11:10         ` Jeff Layton
2013-12-09 10:59 ` [PATCH 1/5 linux-next V2] cifs: Replace CIFSSMBSetEOF() with smb_set_file_size() Jeff Layton
2013-12-09 10:59   ` Jeff Layton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.