[PATCH 0/7] [CIFS] clean up cifs_setattr and fix setting of mtimes

[Jeff Layton-2]

This patch breaks up the large and unwieldy cifs_setattr function into
multiple pieces. In addition to making the function easier to follow and
reducing indentation, this should also make it easier to implement a
scheme to reduce the amount of times that we retry functions that we
know will fail to a particular server. It also fixes the setting of
mtimes (LastWriteTime) on open files with Windows servers.

This patch is based on top of the "dynperm" patchset that I've been
posting, but which is not yet committed.

Comments and/or suggestions appreciated...

---

Jeff Layton (7):
      [CIFS] turn cifs_setattr into a multiplexor that calls the correct function
      [CIFS] move file time and dos attribute setting logic into new function
      [CIFS] Rename CIFSSMBSetFileTimes to CIFSSMBSetFileInfo and add PID arg
      [CIFS] change CIFSSMBSetTimes to CIFSSMBSetPathInfo
      [CIFS] spin off cifs_setattr with unix extensions to its own function
      [CIFS] bundle up Unix SET_PATH_INFO args into a struct and change name
      [CIFS] break ATTR_SIZE changes out into their own function


 fs/cifs/cifspdu.h   |    2
 fs/cifs/cifsproto.h |   24 ++
 fs/cifs/cifssmb.c   |   43 ++--
 fs/cifs/dir.c       |   58 +++--
 fs/cifs/file.c      |   19 +-
 fs/cifs/inode.c     |  551 ++++++++++++++++++++++++++++++++-------------------
 6 files changed, 422 insertions(+), 275 deletions(-)

--
Signed-off-by: Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

Move the code that handles ATTR_SIZE changes to its own function. This
makes for a smaller function and reduces the level of indentation.

Signed-off-by: Jeff Layton <jlayton@...>
---

 fs/cifs/inode.c |  151 ++++++++++++++++++++++++++++---------------------------
 1 files changed, 78 insertions(+), 73 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 7e27753..c08b061 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1413,6 +1413,82 @@ out_busy:
  return -ETXTBSY;
 }
 
+static int
+cifs_set_file_size(struct inode *inode, struct iattr *attrs,
+   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 cifsTconInfo *pTcon = cifs_sb->tcon;
+
+ /*
+ * 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);
+ if (open_file) {
+ __u16 nfid = open_file->netfid;
+ __u32 npid = open_file->pid;
+ rc = CIFSSMBSetFileSize(xid, pTcon, attrs->ia_size, nfid,
+ npid, false);
+ atomic_dec(&open_file->wrtPending);
+ cFYI(1, ("SetFSize for attrs rc = %d", rc));
+ if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
+ unsigned int bytes_written;
+ rc = CIFSSMBWrite(xid, pTcon, nfid, 0, attrs->ia_size,
+  &bytes_written, NULL, NULL, 1);
+ cFYI(1, ("Wrt seteof rc %d", rc));
+ }
+ } else
+ rc = -EINVAL;
+
+ if (rc != 0) {
+ /* 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 = CIFSSMBSetEOF(xid, pTcon, full_path, attrs->ia_size,
+   false, cifs_sb->local_nls,
+   cifs_sb->mnt_cifs_flags &
+ CIFS_MOUNT_MAP_SPECIAL_CHR);
+ cFYI(1, ("SetEOF by path (setattrs) rc = %d", rc));
+ if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
+ __u16 netfid;
+ int oplock = 0;
+
+ rc = SMBLegacyOpen(xid, pTcon, 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) {
+ unsigned int bytes_written;
+ rc = CIFSSMBWrite(xid, pTcon, netfid, 0,
+  attrs->ia_size,
+  &bytes_written, NULL,
+  NULL, 1);
+ cFYI(1, ("wrt seteof rc %d", rc));
+ CIFSSMBClose(xid, pTcon, netfid);
+ }
+ }
+ }
+
+ if (rc == 0) {
+ rc = cifs_vmtruncate(inode, attrs->ia_size);
+ cifs_truncate_page(inode->i_mapping, inode->i_size);
+ }
+
+ return rc;
+}
+
 int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 {
  int xid;
@@ -1420,7 +1496,6 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  struct cifsTconInfo *pTcon;
  char *full_path = NULL;
  int rc = -EACCES;
- struct cifsFileInfo *open_file = NULL;
  FILE_BASIC_INFO time_buf;
  bool set_time = false;
  bool set_dosattr = false;
@@ -1472,78 +1547,8 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  }
 
  if (attrs->ia_valid & ATTR_SIZE) {
- /* 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);
- if (open_file) {
- __u16 nfid = open_file->netfid;
- __u32 npid = open_file->pid;
- rc = CIFSSMBSetFileSize(xid, pTcon, attrs->ia_size,
- nfid, npid, false);
- atomic_dec(&open_file->wrtPending);
- cFYI(1, ("SetFSize for attrs rc = %d", rc));
- if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
- unsigned int bytes_written;
- rc = CIFSSMBWrite(xid, pTcon,
-  nfid, 0, attrs->ia_size,
-  &bytes_written, NULL, NULL,
-  1 /* 45 seconds */);
- cFYI(1, ("Wrt seteof rc %d", rc));
- }
- } else
- rc = -EINVAL;
-
- if (rc != 0) {
- /* 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 = CIFSSMBSetEOF(xid, pTcon, full_path,
-   attrs->ia_size, false,
-   cifs_sb->local_nls,
-   cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
- cFYI(1, ("SetEOF by path (setattrs) rc = %d", rc));
- if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
- __u16 netfid;
- int oplock = 0;
-
- rc = SMBLegacyOpen(xid, pTcon, 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) {
- unsigned int bytes_written;
- rc = CIFSSMBWrite(xid, pTcon,
- netfid, 0,
- attrs->ia_size,
- &bytes_written, NULL,
- NULL, 1 /* 45 sec */);
- cFYI(1, ("wrt seteof rc %d", rc));
- CIFSSMBClose(xid, pTcon, netfid);
- }
-
- }
- }
-
- /* Server is ok setting allocation size implicitly - no need
-   to call:
- CIFSSMBSetEOF(xid, pTcon, full_path, attrs->ia_size, true,
- cifs_sb->local_nls);
-   */
-
- if (rc == 0) {
- rc = cifs_vmtruncate(inode, attrs->ia_size);
- cifs_truncate_page(inode->i_mapping, inode->i_size);
- } else
+ rc = cifs_set_file_size(inode, attrs, xid, full_path);
+ if (rc != 0)
  goto cifs_setattr_exit;
  }
 

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

We'd like to be able to use the unix SET_PATH_INFO_BASIC args to set
file times as well, but that makes the argument list rather long. Bundle
up the args for unix SET_PATH_INFO call into a struct. For now, we don't
actually use the times fields anywhere. That will be done in a follow-on
patch.

Signed-off-by: Jeff Layton <jlayton@...>
---

 fs/cifs/cifspdu.h   |    2 +-
 fs/cifs/cifsproto.h |   17 ++++++++++++---
 fs/cifs/cifssmb.c   |   26 +++++++++++------------
 fs/cifs/dir.c       |   58 ++++++++++++++++++++++++++++-----------------------
 fs/cifs/file.c      |   19 +++++++++--------
 fs/cifs/inode.c     |   54 ++++++++++++++++++++++++++++-------------------
 6 files changed, 102 insertions(+), 74 deletions(-)

diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
index 65d58b4..b7c1db3 100644
--- a/fs/cifs/cifspdu.h
+++ b/fs/cifs/cifspdu.h
@@ -249,7 +249,7 @@
  */
 #define CIFS_NO_HANDLE        0xFFFF
 
-#define NO_CHANGE_64          cpu_to_le64(0xFFFFFFFFFFFFFFFFULL)
+#define NO_CHANGE_64          0xFFFFFFFFFFFFFFFFULL
 #define NO_CHANGE_32          0xFFFFFFFFUL
 
 /* IPC$ in ASCII */
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index b9f5e93..e65ff98 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -191,9 +191,20 @@ extern int CIFSSMBSetEOF(const int xid, struct cifsTconInfo *tcon,
 extern int CIFSSMBSetFileSize(const int xid, struct cifsTconInfo *tcon,
  __u64 size, __u16 fileHandle, __u32 opener_pid,
  bool AllocSizeFlag);
-extern int CIFSSMBUnixSetPerms(const int xid, struct cifsTconInfo *pTcon,
- char *full_path, __u64 mode, __u64 uid,
- __u64 gid, dev_t dev,
+
+struct cifs_unix_set_info_args {
+ __u64 ctime;
+ __u64 atime;
+ __u64 mtime;
+ __u64 mode;
+ __u64 uid;
+ __u64 gid;
+ dev_t device;
+};
+
+extern int CIFSSMBUnixSetInfo(const int xid, struct cifsTconInfo *pTcon,
+ char *fileName,
+ const struct cifs_unix_set_info_args *args,
  const struct nls_table *nls_codepage,
  int remap_special_chars);
 
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 9b8b4cf..29b712a 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -5013,10 +5013,9 @@ SetAttrLgcyRetry:
 #endif /* temporarily unneeded SetAttr legacy function */
 
 int
-CIFSSMBUnixSetPerms(const int xid, struct cifsTconInfo *tcon,
-    char *fileName, __u64 mode, __u64 uid, __u64 gid,
-    dev_t device, const struct nls_table *nls_codepage,
-    int remap)
+CIFSSMBUnixSetInfo(const int xid, struct cifsTconInfo *tcon, char *fileName,
+   const struct cifs_unix_set_info_args *args,
+   const struct nls_table *nls_codepage, int remap)
 {
  TRANSACTION2_SPI_REQ *pSMB = NULL;
  TRANSACTION2_SPI_RSP *pSMBr = NULL;
@@ -5025,6 +5024,7 @@ CIFSSMBUnixSetPerms(const int xid, struct cifsTconInfo *tcon,
  int bytes_returned = 0;
  FILE_UNIX_BASIC_INFO *data_offset;
  __u16 params, param_offset, offset, count, byte_count;
+ __u64 mode = args->mode;
 
  cFYI(1, ("In SetUID/GID/Mode"));
 setPermsRetry:
@@ -5080,16 +5080,16 @@ setPermsRetry:
  set file size and do not want to truncate file size to zero
  accidently as happened on one Samba server beta by putting
  zero instead of -1 here */
- data_offset->EndOfFile = NO_CHANGE_64;
- data_offset->NumOfBytes = NO_CHANGE_64;
- data_offset->LastStatusChange = NO_CHANGE_64;
- data_offset->LastAccessTime = NO_CHANGE_64;
- data_offset->LastModificationTime = NO_CHANGE_64;
- data_offset->Uid = cpu_to_le64(uid);
- data_offset->Gid = cpu_to_le64(gid);
+ data_offset->EndOfFile = cpu_to_le64(NO_CHANGE_64);
+ data_offset->NumOfBytes = cpu_to_le64(NO_CHANGE_64);
+ data_offset->LastStatusChange = cpu_to_le64(args->ctime);
+ data_offset->LastAccessTime = cpu_to_le64(args->atime);
+ data_offset->LastModificationTime = cpu_to_le64(args->mtime);
+ data_offset->Uid = cpu_to_le64(args->uid);
+ data_offset->Gid = cpu_to_le64(args->gid);
  /* better to leave device as zero when it is  */
- data_offset->DevMajor = cpu_to_le64(MAJOR(device));
- data_offset->DevMinor = cpu_to_le64(MINOR(device));
+ data_offset->DevMajor = cpu_to_le64(MAJOR(args->device));
+ data_offset->DevMinor = cpu_to_le64(MINOR(args->device));
  data_offset->Permissions = cpu_to_le64(mode);
 
  if (S_ISREG(mode))
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index fb69c1f..634cf33 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -226,23 +226,26 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
  /* If Open reported that we actually created a file
  then we now have to set the mode if possible */
  if ((pTcon->unix_ext) && (oplock & CIFS_CREATE_ACTION)) {
+ struct cifs_unix_set_info_args args = {
+ .mode = mode,
+ .ctime = NO_CHANGE_64,
+ .atime = NO_CHANGE_64,
+ .mtime = NO_CHANGE_64,
+ .device = 0,
+ };
+
  if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
- CIFSSMBUnixSetPerms(xid, pTcon, full_path, mode,
- (__u64)current->fsuid,
- (__u64)current->fsgid,
- 0 /* dev */,
- cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ args.uid = (__u64) current->fsuid;
+ args.gid = (__u64) current->fsgid;
  } else {
- CIFSSMBUnixSetPerms(xid, pTcon, full_path, mode,
- (__u64)-1,
- (__u64)-1,
- 0 /* dev */,
- cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ args.uid = NO_CHANGE_64;
+ args.gid = NO_CHANGE_64;
  }
+
+ CIFSSMBUnixSetInfo(xid, pTcon, full_path, &args,
+ cifs_sb->local_nls,
+ cifs_sb->mnt_cifs_flags &
+ CIFS_MOUNT_MAP_SPECIAL_CHR);
  } else {
  /* BB implement mode setting via Windows security
    descriptors e.g. */
@@ -357,21 +360,24 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
  if (full_path == NULL)
  rc = -ENOMEM;
  else if (pTcon->unix_ext) {
- mode &= ~current->fs->umask;
+ struct cifs_unix_set_info_args args = {
+ .mode = mode & ~current->fs->umask,
+ .ctime = NO_CHANGE_64,
+ .atime = NO_CHANGE_64,
+ .mtime = NO_CHANGE_64,
+ .device = device_number,
+ };
  if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
- rc = CIFSSMBUnixSetPerms(xid, pTcon, full_path,
- mode, (__u64)current->fsuid,
- (__u64)current->fsgid,
- device_number, cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ args.uid = (__u64) current->fsuid;
+ args.gid = (__u64) current->fsgid;
  } else {
- rc = CIFSSMBUnixSetPerms(xid, pTcon,
- full_path, mode, (__u64)-1, (__u64)-1,
- device_number, cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
+ args.uid = NO_CHANGE_64;
+ args.gid = NO_CHANGE_64;
  }
+ rc = CIFSSMBUnixSetInfo(xid, pTcon, full_path,
+ &args, cifs_sb->local_nls,
+ cifs_sb->mnt_cifs_flags &
+ CIFS_MOUNT_MAP_SPECIAL_CHR);
 
  if (!rc) {
  rc = cifs_get_inode_info_unix(&newinode, full_path,
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 8636cec..8cbf55b 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -310,18 +310,19 @@ int cifs_open(struct inode *inode, struct file *file)
  /* time to set mode which we can not set earlier due to
    problems creating new read-only files */
  if (pTcon->unix_ext) {
- CIFSSMBUnixSetPerms(xid, pTcon, full_path,
-    inode->i_mode,
-    (__u64)-1, (__u64)-1, 0 /* dev */,
+ struct cifs_unix_set_info_args args = {
+ .mode = inode->i_mode,
+ .uid = NO_CHANGE_64,
+ .gid = NO_CHANGE_64,
+ .ctime = NO_CHANGE_64,
+ .atime = NO_CHANGE_64,
+ .mtime = NO_CHANGE_64,
+ .device = 0,
+ };
+ CIFSSMBUnixSetInfo(xid, pTcon, full_path, &args,
     cifs_sb->local_nls,
     cifs_sb->mnt_cifs_flags &
  CIFS_MOUNT_MAP_SPECIAL_CHR);
- } else {
- /* BB implement via Windows security descriptors eg
-   CIFSSMBWinSetPerms(xid, pTcon, full_path, mode,
-      -1, -1, local_nls);
-   in the meantime could set r/o dos attribute when
-   perms are eg: mode & 0222 == 0 */
  }
  }
 
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index c08b061..f67ec8b 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -986,23 +986,24 @@ mkdir_get_info:
  direntry->d_inode->i_nlink = 2;
  mode &= ~current->fs->umask;
  if (pTcon->unix_ext) {
+ struct cifs_unix_set_info_args args = {
+ .mode = mode,
+ .ctime = NO_CHANGE_64,
+ .atime = NO_CHANGE_64,
+ .mtime = NO_CHANGE_64,
+ .device = 0,
+ };
  if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID) {
- CIFSSMBUnixSetPerms(xid, pTcon, full_path,
-    mode,
-    (__u64)current->fsuid,
-    (__u64)current->fsgid,
-    0 /* dev_t */,
-    cifs_sb->local_nls,
-    cifs_sb->mnt_cifs_flags &
-    CIFS_MOUNT_MAP_SPECIAL_CHR);
+ args.uid = (__u64)current->fsuid;
+ args.gid = (__u64)current->fsgid;
  } else {
- CIFSSMBUnixSetPerms(xid, pTcon, full_path,
-    mode, (__u64)-1,
-    (__u64)-1, 0 /* dev_t */,
-    cifs_sb->local_nls,
-    cifs_sb->mnt_cifs_flags &
-    CIFS_MOUNT_MAP_SPECIAL_CHR);
+ args.uid = NO_CHANGE_64;
+ args.gid = NO_CHANGE_64;
  }
+ CIFSSMBUnixSetInfo(xid, pTcon, full_path, &args,
+    cifs_sb->local_nls,
+    cifs_sb->mnt_cifs_flags &
+    CIFS_MOUNT_MAP_SPECIAL_CHR);
  } else {
  if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) &&
     (mode & S_IWUGO) == 0) {
@@ -1499,9 +1500,9 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  FILE_BASIC_INFO time_buf;
  bool set_time = false;
  bool set_dosattr = false;
- __u64 mode = 0xFFFFFFFFFFFFFFFFULL;
- __u64 uid = 0xFFFFFFFFFFFFFFFFULL;
- __u64 gid = 0xFFFFFFFFFFFFFFFFULL;
+ __u64 mode = NO_CHANGE_64;
+ __u64 uid = NO_CHANGE_64;
+ __u64 gid = NO_CHANGE_64;
  struct cifsInodeInfo *cifsInode;
  struct inode *inode = direntry->d_inode;
 
@@ -1585,12 +1586,21 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  }
 
  if ((pTcon->unix_ext)
-    && (attrs->ia_valid & (ATTR_MODE | ATTR_GID | ATTR_UID)))
- rc = CIFSSMBUnixSetPerms(xid, pTcon, full_path, mode, uid, gid,
- 0 /* dev_t */, cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
+    && (attrs->ia_valid & (ATTR_MODE | ATTR_GID | ATTR_UID))) {
+ struct cifs_unix_set_info_args args = {
+ .mode = mode,
+ .uid = uid,
+ .gid = gid,
+ .ctime = NO_CHANGE_64,
+ .atime = NO_CHANGE_64,
+ .mtime = NO_CHANGE_64,
+ .device = 0,
+ };
+ rc = CIFSSMBUnixSetInfo(xid, pTcon, full_path, &args,
+ cifs_sb->local_nls,
+ cifs_sb->mnt_cifs_flags &
  CIFS_MOUNT_MAP_SPECIAL_CHR);
- else if (attrs->ia_valid & ATTR_MODE) {
+ } else if (attrs->ia_valid & ATTR_MODE) {
  rc = 0;
 #ifdef CONFIG_CIFS_EXPERIMENTAL
  if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL)

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

Create a new cifs_setattr_unix function to handle a setattr when unix
extensions are enabled and have cifs_setattr call it. Also, clean up
variable declarations in cifs_setattr.

Signed-off-by: Jeff Layton <jlayton@...>
---

 fs/cifs/inode.c |  157 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 119 insertions(+), 38 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index f67ec8b..bcc493f 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1490,30 +1490,138 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
  return rc;
 }
 
+static int
+cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
+{
+ int rc;
+ int xid;
+ char *full_path = NULL;
+ struct inode *inode = direntry->d_inode;
+ struct cifsInodeInfo *cifsInode = CIFS_I(inode);
+ struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+ struct cifsTconInfo *pTcon = cifs_sb->tcon;
+ struct cifs_unix_set_info_args *args = NULL;
+
+ cFYI(1, ("setattr_unix on file %s attrs->ia_valid=0x%x",
+ direntry->d_name.name, attrs->ia_valid));
+
+ xid = GetXid();
+
+ if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM) == 0) {
+ /* check if we have permission to change attrs */
+ rc = inode_change_ok(inode, attrs);
+ if (rc < 0)
+ goto out;
+ else
+ rc = 0;
+ }
+
+ full_path = build_path_from_dentry(direntry);
+ if (full_path == NULL) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ if ((attrs->ia_valid & ATTR_MTIME) || (attrs->ia_valid & ATTR_SIZE)) {
+ /*
+   Flush data before changing file size or changing the last
+   write time of the file on the server. If the
+   flush returns error, store it to report later and continue.
+   BB: This should be smarter. Why bother flushing pages that
+   will be truncated anyway? Also, should we error out here if
+   the flush returns error?
+ */
+ rc = filemap_write_and_wait(inode->i_mapping);
+ if (rc != 0) {
+ cifsInode->write_behind_rc = rc;
+ rc = 0;
+ }
+ }
+
+ if (attrs->ia_valid & ATTR_SIZE) {
+ rc = cifs_set_file_size(inode, attrs, xid, full_path);
+ if (rc != 0)
+ goto out;
+ }
+
+ /* skip mode change if it's just for clearing setuid/setgid */
+ if (attrs->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
+ attrs->ia_valid &= ~ATTR_MODE;
+
+ args = kmalloc(sizeof(*args), GFP_KERNEL);
+ if (args == NULL) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ /* set up the struct */
+ if (attrs->ia_valid & ATTR_MODE)
+ args->mode = attrs->ia_mode;
+ else
+ args->mode = NO_CHANGE_64;
+
+ if (attrs->ia_valid & ATTR_UID)
+ args->uid = attrs->ia_uid;
+ else
+ args->uid = NO_CHANGE_64;
+
+ if (attrs->ia_valid & ATTR_GID)
+ args->gid = attrs->ia_gid;
+ else
+ args->gid = NO_CHANGE_64;
+
+ if (attrs->ia_valid & ATTR_ATIME)
+ args->atime = cifs_UnixTimeToNT(attrs->ia_atime);
+ else
+ args->atime = NO_CHANGE_64;
+
+ if (attrs->ia_valid & ATTR_MTIME)
+ args->mtime = cifs_UnixTimeToNT(attrs->ia_mtime);
+ else
+ args->mtime = NO_CHANGE_64;
+
+ if (attrs->ia_valid & ATTR_CTIME)
+ args->ctime = cifs_UnixTimeToNT(attrs->ia_ctime);
+ else
+ args->ctime = NO_CHANGE_64;
+
+ args->device = 0;
+ rc = CIFSSMBUnixSetInfo(xid, pTcon, full_path, args,
+ cifs_sb->local_nls,
+ cifs_sb->mnt_cifs_flags &
+ CIFS_MOUNT_MAP_SPECIAL_CHR);
+
+ if (!rc)
+ rc = inode_setattr(inode, attrs);
+out:
+ kfree(args);
+ kfree(full_path);
+ FreeXid(xid);
+ return rc;
+}
+
 int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 {
  int xid;
- struct cifs_sb_info *cifs_sb;
- struct cifsTconInfo *pTcon;
+ struct inode *inode = direntry->d_inode;
+ struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+ struct cifsTconInfo *pTcon = cifs_sb->tcon;
+ struct cifsInodeInfo *cifsInode = CIFS_I(inode);
  char *full_path = NULL;
  int rc = -EACCES;
  FILE_BASIC_INFO time_buf;
  bool set_time = false;
  bool set_dosattr = false;
  __u64 mode = NO_CHANGE_64;
- __u64 uid = NO_CHANGE_64;
- __u64 gid = NO_CHANGE_64;
- struct cifsInodeInfo *cifsInode;
- struct inode *inode = direntry->d_inode;
+
+ if (pTcon->unix_ext)
+ return cifs_setattr_unix(direntry, attrs);
 
  xid = GetXid();
 
  cFYI(1, ("setattr on file %s attrs->iavalid 0x%x",
  direntry->d_name.name, attrs->ia_valid));
 
- cifs_sb = CIFS_SB(inode->i_sb);
- pTcon = cifs_sb->tcon;
-
  if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM) == 0) {
  /* check if we have permission to change attrs */
  rc = inode_change_ok(inode, attrs);
@@ -1529,7 +1637,6 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  FreeXid(xid);
  return -ENOMEM;
  }
- cifsInode = CIFS_I(inode);
 
  if ((attrs->ia_valid & ATTR_MTIME) || (attrs->ia_valid & ATTR_SIZE)) {
  /*
@@ -1560,19 +1667,8 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  * CIFSACL support + proper Windows to Unix idmapping, we may be
  * able to support this in the future.
  */
- if (!pTcon->unix_ext &&
-    !(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID)) {
+ if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID))
  attrs->ia_valid &= ~(ATTR_UID | ATTR_GID);
- } else {
- if (attrs->ia_valid & ATTR_UID) {
- cFYI(1, ("UID changed to %d", attrs->ia_uid));
- uid = attrs->ia_uid;
- }
- if (attrs->ia_valid & ATTR_GID) {
- cFYI(1, ("GID changed to %d", attrs->ia_gid));
- gid = attrs->ia_gid;
- }
- }
 
  time_buf.Attributes = 0;
 
@@ -1585,22 +1681,7 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  mode = attrs->ia_mode;
  }
 
- if ((pTcon->unix_ext)
-    && (attrs->ia_valid & (ATTR_MODE | ATTR_GID | ATTR_UID))) {
- struct cifs_unix_set_info_args args = {
- .mode = mode,
- .uid = uid,
- .gid = gid,
- .ctime = NO_CHANGE_64,
- .atime = NO_CHANGE_64,
- .mtime = NO_CHANGE_64,
- .device = 0,
- };
- rc = CIFSSMBUnixSetInfo(xid, pTcon, full_path, &args,
- cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
- } else if (attrs->ia_valid & ATTR_MODE) {
+ if (attrs->ia_valid & ATTR_MODE) {
  rc = 0;
 #ifdef CONFIG_CIFS_EXPERIMENTAL
  if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL)

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

CIFSSMBSetTimes is a deceptive name. This function does more that just
set file times. Change it to CIFSSMBSetPathInfo, which is closer to its
real purpose.

Signed-off-by: Jeff Layton <jlayton@...>
---

 fs/cifs/cifsproto.h |    2 +-
 fs/cifs/cifssmb.c   |    6 +++---
 fs/cifs/inode.c     |    8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index e65ff98..eb2be99 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -172,7 +172,7 @@ extern int CIFSSMBQFSUnixInfo(const int xid, struct cifsTconInfo *tcon);
 extern int CIFSSMBQFSPosixInfo(const int xid, struct cifsTconInfo *tcon,
  struct kstatfs *FSData);
 
-extern int CIFSSMBSetTimes(const int xid, struct cifsTconInfo *tcon,
+extern int CIFSSMBSetPathInfo(const int xid, struct cifsTconInfo *tcon,
  const char *fileName, const FILE_BASIC_INFO *data,
  const struct nls_table *nls_codepage,
  int remap_special_chars);
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 29b712a..207301e 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -4882,9 +4882,9 @@ CIFSSMBSetFileTimes(const int xid, struct cifsTconInfo *tcon,
 
 
 int
-CIFSSMBSetTimes(const int xid, struct cifsTconInfo *tcon, const char *fileName,
- const FILE_BASIC_INFO *data,
- const struct nls_table *nls_codepage, int remap)
+CIFSSMBSetPathInfo(const int xid, struct cifsTconInfo *tcon,
+   const char *fileName, const FILE_BASIC_INFO *data,
+   const struct nls_table *nls_codepage, int remap)
 {
  TRANSACTION2_SPI_REQ *pSMB = NULL;
  TRANSACTION2_SPI_RSP *pSMBr = NULL;
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index bcc493f..32e34f5 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -737,7 +737,7 @@ psx_del_no_retry:
  /* ATTRS set to normal clears r/o bit */
  pinfo_buf->Attributes = cpu_to_le32(ATTR_NORMAL);
  if (!(pTcon->ses->flags & CIFS_SES_NT4))
- rc = CIFSSMBSetTimes(xid, pTcon, full_path,
+ rc = CIFSSMBSetPathInfo(xid, pTcon, full_path,
      pinfo_buf,
      cifs_sb->local_nls,
      cifs_sb->mnt_cifs_flags &
@@ -1010,7 +1010,7 @@ mkdir_get_info:
  FILE_BASIC_INFO pInfo;
  memset(&pInfo, 0, sizeof(pInfo));
  pInfo.Attributes = cpu_to_le32(ATTR_READONLY);
- CIFSSMBSetTimes(xid, pTcon, full_path,
+ CIFSSMBSetPathInfo(xid, pTcon, full_path,
  &pInfo, cifs_sb->local_nls,
  cifs_sb->mnt_cifs_flags &
  CIFS_MOUNT_MAP_SPECIAL_CHR);
@@ -1760,8 +1760,8 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  /* In the future we should experiment - try setting timestamps
    via Handle (SetFileInfo) instead of by path */
  if (!(pTcon->ses->flags & CIFS_SES_NT4))
- rc = CIFSSMBSetTimes(xid, pTcon, full_path, &time_buf,
-     cifs_sb->local_nls,
+ rc = CIFSSMBSetPathInfo(xid, pTcon, full_path,
+     &time_buf, cifs_sb->local_nls,
      cifs_sb->mnt_cifs_flags &
  CIFS_MOUNT_MAP_SPECIAL_CHR);
  else

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

The new name is more clear since this is also used to set file
attributes. We'll need the pid_of_opener arg so that we can
pass in filehandles of other pids and spare ourselves an open
call.

Signed-off-by: Jeff Layton <jlayton@...>
---

 fs/cifs/cifsproto.h |    5 +++--
 fs/cifs/cifssmb.c   |   11 ++++-------
 fs/cifs/inode.c     |   11 ++++++-----
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index eb2be99..a729d08 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -176,8 +176,9 @@ extern int CIFSSMBSetPathInfo(const int xid, struct cifsTconInfo *tcon,
  const char *fileName, const FILE_BASIC_INFO *data,
  const struct nls_table *nls_codepage,
  int remap_special_chars);
-extern int CIFSSMBSetFileTimes(const int xid, struct cifsTconInfo *tcon,
- const FILE_BASIC_INFO *data, __u16 fid);
+extern int CIFSSMBSetFileInfo(const int xid, struct cifsTconInfo *tcon,
+ const FILE_BASIC_INFO *data, __u16 fid,
+ __u32 pid_of_opener);
 #if 0
 extern int CIFSSMBSetAttrLegacy(int xid, struct cifsTconInfo *tcon,
  char *fileName, __u16 dos_attributes,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 207301e..12be4ad 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -4816,8 +4816,8 @@ CIFSSMBSetFileSize(const int xid, struct cifsTconInfo *tcon, __u64 size,
    time and resort to the original setpathinfo level which takes the ancient
    DOS time format with 2 second granularity */
 int
-CIFSSMBSetFileTimes(const int xid, struct cifsTconInfo *tcon,
-    const FILE_BASIC_INFO *data, __u16 fid)
+CIFSSMBSetFileInfo(const int xid, struct cifsTconInfo *tcon,
+    const FILE_BASIC_INFO *data, __u16 fid, __u32 pid_of_opener)
 {
  struct smb_com_transaction2_sfi_req *pSMB  = NULL;
  char *data_offset;
@@ -4830,11 +4830,8 @@ CIFSSMBSetFileTimes(const int xid, struct cifsTconInfo *tcon,
  if (rc)
  return rc;
 
- /* At this point there is no need to override the current pid
- with the pid of the opener, but that could change if we someday
- use an existing handle (rather than opening one on the fly) */
- /* pSMB->hdr.Pid = cpu_to_le16((__u16)pid_of_opener);
- pSMB->hdr.PidHigh = cpu_to_le16((__u16)(pid_of_opener >> 16));*/
+ pSMB->hdr.Pid = cpu_to_le16((__u16)pid_of_opener);
+ pSMB->hdr.PidHigh = cpu_to_le16((__u16)(pid_of_opener >> 16));
 
  params = 6;
  pSMB->MaxSetupCount = 0;
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 32e34f5..7ccfdaf 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -767,9 +767,10 @@ psx_del_no_retry:
  cifs_sb->mnt_cifs_flags &
     CIFS_MOUNT_MAP_SPECIAL_CHR);
  if (rc == 0) {
- rc = CIFSSMBSetFileTimes(xid, pTcon,
- pinfo_buf,
- netfid);
+ rc = CIFSSMBSetFileInfo(xid, pTcon,
+ pinfo_buf,
+ netfid,
+ current->tgid);
  CIFSSMBClose(xid, pTcon, netfid);
  }
  }
@@ -1782,8 +1783,8 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  cifs_sb->mnt_cifs_flags &
  CIFS_MOUNT_MAP_SPECIAL_CHR);
  if (rc == 0) {
- rc = CIFSSMBSetFileTimes(xid, pTcon, &time_buf,
- netfid);
+ rc = CIFSSMBSetFileInfo(xid, pTcon, &time_buf,
+ netfid, current->tgid);
  CIFSSMBClose(xid, pTcon, netfid);
  } else {
  /* BB For even older servers we could convert time_buf

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

Break up cifs_setattr further by moving the logic that sets file times
and dos attributes into a separate function. This patch also refactors
the logic a bit so that when the file is already open then we go ahead
and do a SetFileInfo call. SetPathInfo seems to be unreliable when
setting times on open files.

Signed-off-by: Jeff Layton <jlayton@...>
---

 fs/cifs/inode.c |  196 +++++++++++++++++++++++++++++++------------------------
 1 files changed, 109 insertions(+), 87 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 7ccfdaf..84e477a 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1492,6 +1492,101 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
 }
 
 static int
+cifs_set_file_info(struct inode *inode, struct iattr *attrs, int xid,
+    char *full_path, __u32 dosattr)
+{
+ int rc;
+ int oplock = 0;
+ __u16 netfid;
+ __u32 netpid;
+ bool set_time = false;
+ struct cifsFileInfo *open_file;
+ struct cifsInodeInfo *cifsInode = CIFS_I(inode);
+ struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+ struct cifsTconInfo *pTcon = cifs_sb->tcon;
+ FILE_BASIC_INFO info_buf;
+
+ if (attrs->ia_valid & ATTR_ATIME) {
+ set_time = true;
+ info_buf.LastAccessTime =
+ cpu_to_le64(cifs_UnixTimeToNT(attrs->ia_atime));
+ } else
+ info_buf.LastAccessTime = 0;
+
+ if (attrs->ia_valid & ATTR_MTIME) {
+ set_time = true;
+ info_buf.LastWriteTime =
+    cpu_to_le64(cifs_UnixTimeToNT(attrs->ia_mtime));
+ } else
+ info_buf.LastWriteTime = 0;
+
+ /*
+ * Samba throws this field away, but windows may actually use it.
+ * Do not set ctime unless other time stamps are changed explicitly
+ * (i.e. by utimes()) since we would then have a mix of client and
+ * server times.
+ */
+ if (set_time && (attrs->ia_valid & ATTR_CTIME)) {
+ cFYI(1, ("CIFS - CTIME changed"));
+ info_buf.ChangeTime =
+    cpu_to_le64(cifs_UnixTimeToNT(attrs->ia_ctime));
+ } else
+ info_buf.ChangeTime = 0;
+
+ info_buf.CreationTime = 0; /* don't change */
+ info_buf.Attributes = cpu_to_le32(dosattr);
+
+ /*
+ * If the file is already open for write, just use that fileid
+ */
+ open_file = find_writable_file(cifsInode);
+ if (open_file) {
+ netfid = open_file->netfid;
+ netpid = open_file->pid;
+ goto set_via_filehandle;
+ }
+
+ /*
+ * NT4 apparently returns success on this call, but it doesn't
+ * really work.
+ */
+ if (!(pTcon->ses->flags & CIFS_SES_NT4)) {
+ rc = CIFSSMBSetPathInfo(xid, pTcon, full_path,
+     &info_buf, cifs_sb->local_nls,
+     cifs_sb->mnt_cifs_flags &
+ CIFS_MOUNT_MAP_SPECIAL_CHR);
+ if (rc != -EOPNOTSUPP && rc != -EINVAL)
+ goto out;
+ }
+
+ cFYI(1, ("calling SetFileInfo since SetPathInfo for "
+ "times not supported by this server"));
+ rc = CIFSSMBOpen(xid, pTcon, full_path, FILE_OPEN,
+ SYNCHRONIZE | FILE_WRITE_ATTRIBUTES,
+ CREATE_NOT_DIR, &netfid, &oplock,
+ NULL, cifs_sb->local_nls,
+ cifs_sb->mnt_cifs_flags &
+ CIFS_MOUNT_MAP_SPECIAL_CHR);
+
+ if (rc != 0) {
+ if (rc == -EIO)
+ rc = -EINVAL;
+ goto out;
+ }
+
+ netpid = current->tgid;
+
+set_via_filehandle:
+ rc = CIFSSMBSetFileInfo(xid, pTcon, &info_buf, netfid, netpid);
+ if (open_file == NULL)
+ CIFSSMBClose(xid, pTcon, netfid);
+ else
+ atomic_dec(&open_file->wrtPending);
+out:
+ return rc;
+}
+
+static int
 cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 {
  int rc;
@@ -1610,9 +1705,7 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  struct cifsInodeInfo *cifsInode = CIFS_I(inode);
  char *full_path = NULL;
  int rc = -EACCES;
- FILE_BASIC_INFO time_buf;
- bool set_time = false;
- bool set_dosattr = false;
+ __u32 dosattr = 0;
  __u64 mode = NO_CHANGE_64;
 
  if (pTcon->unix_ext)
@@ -1671,8 +1764,6 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID))
  attrs->ia_valid &= ~(ATTR_UID | ATTR_GID);
 
- time_buf.Attributes = 0;
-
  /* skip mode change if it's just for clearing setuid/setgid */
  if (attrs->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
  attrs->ia_valid &= ~ATTR_MODE;
@@ -1691,24 +1782,19 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 #endif
  if (((mode & S_IWUGO) == 0) &&
     (cifsInode->cifsAttrs & ATTR_READONLY) == 0) {
- set_dosattr = true;
- time_buf.Attributes = cpu_to_le32(cifsInode->cifsAttrs |
-  ATTR_READONLY);
+
+ dosattr = cifsInode->cifsAttrs | ATTR_READONLY;
+
  /* fix up mode if we're not using dynperm */
  if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM) == 0)
  attrs->ia_mode = inode->i_mode & ~S_IWUGO;
  } else if ((mode & S_IWUGO) &&
    (cifsInode->cifsAttrs & ATTR_READONLY)) {
- /* If file is readonly on server, we would
- not be able to write to it - so if any write
- bit is enabled for user or group or other we
- need to at least try to remove r/o dos attr */
- set_dosattr = true;
- time_buf.Attributes = cpu_to_le32(cifsInode->cifsAttrs &
-    (~ATTR_READONLY));
- /* Windows ignores set to zero */
- if (time_buf.Attributes == 0)
- time_buf.Attributes |= cpu_to_le32(ATTR_NORMAL);
+
+ dosattr = cifsInode->cifsAttrs & ~ATTR_READONLY;
+ /* Attributes of 0 are ignored */
+ if (dosattr == 0)
+ dosattr |= ATTR_NORMAL;
 
  /* reset local inode permissions to normal */
  if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM)) {
@@ -1726,82 +1812,18 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
  }
  }
 
- if (attrs->ia_valid & ATTR_ATIME) {
- set_time = true;
- time_buf.LastAccessTime =
-    cpu_to_le64(cifs_UnixTimeToNT(attrs->ia_atime));
- } else
- time_buf.LastAccessTime = 0;
-
- if (attrs->ia_valid & ATTR_MTIME) {
- set_time = true;
- time_buf.LastWriteTime =
-    cpu_to_le64(cifs_UnixTimeToNT(attrs->ia_mtime));
- } else
- time_buf.LastWriteTime = 0;
- /* Do not set ctime explicitly unless other time
-   stamps are changed explicitly (i.e. by utime()
-   since we would then have a mix of client and
-   server times */
-
- if (set_time && (attrs->ia_valid & ATTR_CTIME)) {
- set_time = true;
- /* Although Samba throws this field away
- it may be useful to Windows - but we do
- not want to set ctime unless some other
- timestamp is changing */
- cFYI(1, ("CIFS - CTIME changed"));
- time_buf.ChangeTime =
-    cpu_to_le64(cifs_UnixTimeToNT(attrs->ia_ctime));
- } else
- time_buf.ChangeTime = 0;
-
- if (set_time || set_dosattr) {
- time_buf.CreationTime = 0; /* do not change */
- /* In the future we should experiment - try setting timestamps
-   via Handle (SetFileInfo) instead of by path */
- if (!(pTcon->ses->flags & CIFS_SES_NT4))
- rc = CIFSSMBSetPathInfo(xid, pTcon, full_path,
-     &time_buf, cifs_sb->local_nls,
-     cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
- else
- rc = -EOPNOTSUPP;
-
- if (rc == -EOPNOTSUPP) {
- int oplock = 0;
- __u16 netfid;
+ if (attrs->ia_valid & (ATTR_MTIME|ATTR_ATIME|ATTR_CTIME) ||
+    ((attrs->ia_valid & ATTR_MODE) && dosattr)) {
+ rc = cifs_set_file_info(inode, attrs, xid, full_path, dosattr);
+ /* BB: check for rc = -EOPNOTSUPP and switch to legacy mode */
 
- cFYI(1, ("calling SetFileInfo since SetPathInfo for "
- "times not supported by this server"));
- /* BB we could scan to see if we already have it open
-   and pass in pid of opener to function */
- rc = CIFSSMBOpen(xid, pTcon, full_path, FILE_OPEN,
- SYNCHRONIZE | FILE_WRITE_ATTRIBUTES,
- CREATE_NOT_DIR, &netfid, &oplock,
- NULL, cifs_sb->local_nls,
- cifs_sb->mnt_cifs_flags &
- CIFS_MOUNT_MAP_SPECIAL_CHR);
- if (rc == 0) {
- rc = CIFSSMBSetFileInfo(xid, pTcon, &time_buf,
- netfid, current->tgid);
- CIFSSMBClose(xid, pTcon, netfid);
- } else {
- /* BB For even older servers we could convert time_buf
-   into old DOS style which uses two second
-   granularity */
-
- /* rc = CIFSSMBSetTimesLegacy(xid, pTcon, full_path,
- &time_buf, cifs_sb->local_nls); */
- }
- }
  /* Even if error on time set, no sense failing the call if
  the server would set the time to a reasonable value anyway,
  and this check ensures that we are not being called from
  sys_utimes in which case we ought to fail the call back to
  the user when the server rejects the call */
  if ((rc) && (attrs->ia_valid &
- (ATTR_MODE | ATTR_GID | ATTR_UID | ATTR_SIZE)))
+ (ATTR_MODE | ATTR_GID | ATTR_UID | ATTR_SIZE)))
  rc = 0;
  }
 

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

Signed-off-by: Jeff Layton <jlayton@...>
---

 fs/cifs/inode.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 84e477a..d27f5dd 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1696,21 +1696,18 @@ out:
  return rc;
 }
 
-int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
+static int
+cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 {
  int xid;
  struct inode *inode = direntry->d_inode;
  struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
- struct cifsTconInfo *pTcon = cifs_sb->tcon;
  struct cifsInodeInfo *cifsInode = CIFS_I(inode);
  char *full_path = NULL;
  int rc = -EACCES;
  __u32 dosattr = 0;
  __u64 mode = NO_CHANGE_64;
 
- if (pTcon->unix_ext)
- return cifs_setattr_unix(direntry, attrs);
-
  xid = GetXid();
 
  cFYI(1, ("setattr on file %s attrs->iavalid 0x%x",
@@ -1837,6 +1834,21 @@ cifs_setattr_exit:
  return rc;
 }
 
+int
+cifs_setattr(struct dentry *direntry, struct iattr *attrs)
+{
+ struct inode *inode = direntry->d_inode;
+ struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+ struct cifsTconInfo *pTcon = cifs_sb->tcon;
+
+ if (pTcon->unix_ext)
+ return cifs_setattr_unix(direntry, attrs);
+
+ return cifs_setattr_nounix(direntry, attrs);
+
+ /* BB: add cifs_setattr_legacy for really old servers */
+}
+
 #if 0
 void cifs_delete_inode(struct inode *inode)
 {

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Günter Kukkukk-2]

Am Freitag, 23. Mai 2008 schrieb Jeff Layton:

> This patch breaks up the large and unwieldy cifs_setattr function into
> multiple pieces. In addition to making the function easier to follow and
> reducing indentation, this should also make it easier to implement a
> scheme to reduce the amount of times that we retry functions that we
> know will fail to a particular server. It also fixes the setting of
> mtimes (LastWriteTime) on open files with Windows servers.
>
> This patch is based on top of the "dynperm" patchset that I've been
> posting, but which is not yet committed.
>
> Comments and/or suggestions appreciated...
>
> ---
>
> Jeff Layton (7):
>       [CIFS] turn cifs_setattr into a multiplexor that calls the correct function
>       [CIFS] move file time and dos attribute setting logic into new function
>       [CIFS] Rename CIFSSMBSetFileTimes to CIFSSMBSetFileInfo and add PID arg
>       [CIFS] change CIFSSMBSetTimes to CIFSSMBSetPathInfo
>       [CIFS] spin off cifs_setattr with unix extensions to its own function
>       [CIFS] bundle up Unix SET_PATH_INFO args into a struct and change name
>       [CIFS] break ATTR_SIZE changes out into their own function
>
>
>  fs/cifs/cifspdu.h   |    2
>  fs/cifs/cifsproto.h |   24 ++
>  fs/cifs/cifssmb.c   |   43 ++--
>  fs/cifs/dir.c       |   58 +++--
>  fs/cifs/file.c      |   19 +-
>  fs/cifs/inode.c     |  551 ++++++++++++++++++++++++++++++++-------------------
>  6 files changed, 422 insertions(+), 275 deletions(-)
>

Hi Jeff,

i fully agree here to "unbundle" the current completely
overloaded function cifs_setattr() - which has to handle
too many _very_ different requests.

As you propose, the very first distinction should be whether
unix extensions are enabled - or not.
Then separate for "attr/mode" and "file times" changes.
Anyway - your approach will help a lot to "implement
missing legacy functionality"!

Ouch - your patch set is really large - and i really
can't tell whether your new stuff is still working...

Jeff, Steve,

what testcases are you using to check for functionality
and compatibilty in your own environments?
Could really help me to do "my own automated tests".

Btw - there are similar problems in

cifs_get_inode_info() ---> CIFSSMBQPathInfo()

... completely broken legacy support (and also
overloaded functions)

Cheers, Günter
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Sat, 24 May 2008 05:54:45 +0200
Günter Kukkukk <linux@...> wrote:

> Am Freitag, 23. Mai 2008 schrieb Jeff Layton:
> > This patch breaks up the large and unwieldy cifs_setattr function into
> > multiple pieces. In addition to making the function easier to follow and
> > reducing indentation, this should also make it easier to implement a
> > scheme to reduce the amount of times that we retry functions that we
> > know will fail to a particular server. It also fixes the setting of
> > mtimes (LastWriteTime) on open files with Windows servers.
> >
> > This patch is based on top of the "dynperm" patchset that I've been
> > posting, but which is not yet committed.
> >
> > Comments and/or suggestions appreciated...
> >
> > ---
> >
> > Jeff Layton (7):
> >       [CIFS] turn cifs_setattr into a multiplexor that calls the correct function
> >       [CIFS] move file time and dos attribute setting logic into new function
> >       [CIFS] Rename CIFSSMBSetFileTimes to CIFSSMBSetFileInfo and add PID arg
> >       [CIFS] change CIFSSMBSetTimes to CIFSSMBSetPathInfo
> >       [CIFS] spin off cifs_setattr with unix extensions to its own function
> >       [CIFS] bundle up Unix SET_PATH_INFO args into a struct and change name
> >       [CIFS] break ATTR_SIZE changes out into their own function
> >
> >
> >  fs/cifs/cifspdu.h   |    2
> >  fs/cifs/cifsproto.h |   24 ++
> >  fs/cifs/cifssmb.c   |   43 ++--
> >  fs/cifs/dir.c       |   58 +++--
> >  fs/cifs/file.c      |   19 +-
> >  fs/cifs/inode.c     |  551 ++++++++++++++++++++++++++++++++-------------------
> >  6 files changed, 422 insertions(+), 275 deletions(-)
> >
>
> Hi Jeff,
>
> i fully agree here to "unbundle" the current completely
> overloaded function cifs_setattr() - which has to handle
> too many _very_ different requests.
>
> As you propose, the very first distinction should be whether
> unix extensions are enabled - or not.
> Then separate for "attr/mode" and "file times" changes.
> Anyway - your approach will help a lot to "implement
> missing legacy functionality"!
>
> Ouch - your patch set is really large - and i really
> can't tell whether your new stuff is still working...
>

Yes -- it is large, but if applied in order they should be bisectable.
The sequence of patches in this set actually does matter...

> Jeff, Steve,
>
> what testcases are you using to check for functionality
> and compatibilty in your own environments?
> Could really help me to do "my own automated tests".
>

Not much, unfortunately. I use connectathon test suite and things like
fsx and iozone, but they aren't really suited for subtle ferreting out
subtle setattr breakages in cifs.

> Btw - there are similar problems in
>
> cifs_get_inode_info() ---> CIFSSMBQPathInfo()
>
> ... completely broken legacy support (and also
> overloaded functions)
>

Yes -- these problems are all over the place. It's only getting
worse as we add more functionality and handle more types of servers.
IMO, we need to step back a bit and rethink how cifs will evolve in the
future.

Steve and I briefly talked about this at Connectathon. What I'd like to
see is something like the smb_ops struct that smbfs uses. In our case
though, rather than using "const" smb_ops structs, ours would be
declared per-server or per-tcon. At mount time, we can do our best to
set this up, but if the lower functions start returning the right
errors, we can change what function gets called.

My thoughts on it are still pretty fuzzy though, so I haven't actually
started implementing it yet. My thinking is that cifs_setattr could be
set up to do something like:

    pTcon->ops->setattr(...)

...so if that's set up to call cifs_setattr_nounix() and that returns
something like -EOPNOTSUPP, then we'd change the setattr pointer to
point to cifs_setattr_legacy() and try again.

The logic of this would need to be worked out, and also we'd want to
try to start with sane pointers in this struct. That seems like the
difficult part since it's tough to tell ahead of time what functions a
server will support. We also may need to deal with the case where some
functions work against some inodes under a mount, but not others (for
instance if it's possible to cross mountpoints on a server within a
share).

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Fri, 23 May 2008 11:50:39 -0400
Jeff Layton <jlayton@...> wrote:

> This patch breaks up the large and unwieldy cifs_setattr function into
> multiple pieces. In addition to making the function easier to follow and
> reducing indentation, this should also make it easier to implement a
> scheme to reduce the amount of times that we retry functions that we
> know will fail to a particular server. It also fixes the setting of
> mtimes (LastWriteTime) on open files with Windows servers.
>
> This patch is based on top of the "dynperm" patchset that I've been
> posting, but which is not yet committed.
>
> Comments and/or suggestions appreciated...
>

Hi Steve,
   Now that the dynperm patchset is in, any thoughts on this one?
Anything amiss, or does it look OK as-is?

If it's OK as-is, then I recommend applying them in order. I'm pretty
sure that that should keep things cleanly bisectable, but I've only
really tested the entire patchset as a whole.

Thanks,
--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client