[PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Pavel Shilovsky]

Simplify many places when we call cifs_revalidate/invalidate to make
it does what it exactly needs.

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/cifsfs.c |   31 ++++++++++-----
 fs/cifs/cifsfs.h |    4 +-
 fs/cifs/file.c   |   16 ++++++--
 fs/cifs/inode.c  |  114 ++++++++++++++++++++++++++++++++++--------------------
 4 files changed, 108 insertions(+), 57 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index fb6a2ad..1646360 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -623,16 +623,27 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
 {
 	/* origin == SEEK_END => we must revalidate the cached file length */
 	if (origin == SEEK_END) {
-		int retval;
-
-		/* some applications poll for the file length in this strange
-		   way so we must seek to end on non-oplocked files by
-		   setting the revalidate time to zero */
-		CIFS_I(file->f_path.dentry->d_inode)->time = 0;
-
-		retval = cifs_revalidate_file(file);
-		if (retval < 0)
-			return (loff_t)retval;
+		int rc;
+		struct inode *inode = file->f_path.dentry->d_inode;
+
+		/* Need to flush all dirty pages to get a right file length */
+		if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
+			rc = filemap_write_and_wait(inode->i_mapping);
+			if (rc) {
+				mapping_set_error(inode->i_mapping, rc);
+				return (loff_t)rc;
+			}
+		}
+		/*
+		 * Some applications poll for the file length in this strange
+		 * way so we must seek to end on non-oplocked files by
+		 * setting the revalidate time to zero.
+		 */
+		CIFS_I(inode)->time = 0;
+
+		rc = cifs_revalidate_file_attr(file);
+		if (rc < 0)
+			return (loff_t)rc;
 	}
 	return generic_file_llseek_unlocked(file, offset, origin);
 }
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index bb64313..d304584 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -59,9 +59,11 @@ extern int cifs_mkdir(struct inode *, struct dentry *, int);
 extern int cifs_rmdir(struct inode *, struct dentry *);
 extern int cifs_rename(struct inode *, struct dentry *, struct inode *,
 		       struct dentry *);
+extern int cifs_revalidate_file_attr(struct file *filp);
+extern int cifs_revalidate_dentry_attr(struct dentry *);
 extern int cifs_revalidate_file(struct file *filp);
 extern int cifs_revalidate_dentry(struct dentry *);
-extern void cifs_invalidate_mapping(struct inode *inode);
+extern int cifs_invalidate_mapping(struct inode *inode);
 extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
 extern int cifs_setattr(struct dentry *, struct iattr *);
 
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 613f965..3ceef8a 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1445,8 +1445,13 @@ int cifs_strict_fsync(struct file *file, int datasync)
 	cFYI(1, "Sync file - name: %s datasync: 0x%x",
 		file->f_path.dentry->d_name.name, datasync);
 
-	if (!CIFS_I(inode)->clientCanCacheRead)
-		cifs_invalidate_mapping(inode);
+	if (!CIFS_I(inode)->clientCanCacheRead) {
+		rc = cifs_invalidate_mapping(inode);
+		if (rc) {
+			cFYI(1, "rc: %d during invalidate phase", rc);
+			rc = 0; /* don't care about it in fsync */
+		}
+	}
 
 	tcon = tlink_tcon(smbfile->tlink);
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
@@ -1931,8 +1936,11 @@ int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma)
 
 	xid = GetXid();
 
-	if (!CIFS_I(inode)->clientCanCacheRead)
-		cifs_invalidate_mapping(inode);
+	if (!CIFS_I(inode)->clientCanCacheRead) {
+		rc = cifs_invalidate_mapping(inode);
+		if (rc)
+			return rc;
+	}
 
 	rc = generic_file_mmap(file, vma);
 	if (rc == 0)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index adb6324..5f71e11 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1683,18 +1683,15 @@ cifs_inode_needs_reval(struct inode *inode)
 /*
  * Zap the cache. Called when invalid_mapping flag is set.
  */
-void
+int
 cifs_invalidate_mapping(struct inode *inode)
 {
-	int rc;
+	int rc = 0;
 	struct cifsInodeInfo *cifs_i = CIFS_I(inode);
 
 	cifs_i->invalid_mapping = false;
 
 	if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
-		/* write back any cached data */
-		rc = filemap_write_and_wait(inode->i_mapping);
-		mapping_set_error(inode->i_mapping, rc);
 		rc = invalidate_inode_pages2(inode->i_mapping);
 		if (rc) {
 			cERROR(1, "%s: could not invalidate inode %p", __func__,
@@ -1704,56 +1701,52 @@ cifs_invalidate_mapping(struct inode *inode)
 	}
 
 	cifs_fscache_reset_inode_cookie(inode);
+	return rc;
 }
 
-int cifs_revalidate_file(struct file *filp)
+int cifs_revalidate_file_attr(struct file *filp)
 {
 	int rc = 0;
 	struct inode *inode = filp->f_path.dentry->d_inode;
 	struct cifsFileInfo *cfile = (struct cifsFileInfo *) filp->private_data;
 
 	if (!cifs_inode_needs_reval(inode))
-		goto check_inval;
+		return rc;
 
 	if (tlink_tcon(cfile->tlink)->unix_ext)
 		rc = cifs_get_file_info_unix(filp);
 	else
 		rc = cifs_get_file_info(filp);
 
-check_inval:
-	if (CIFS_I(inode)->invalid_mapping)
-		cifs_invalidate_mapping(inode);
-
 	return rc;
 }
 
-/* revalidate a dentry's inode attributes */
-int cifs_revalidate_dentry(struct dentry *dentry)
+int cifs_revalidate_dentry_attr(struct dentry *dentry)
 {
 	int xid;
 	int rc = 0;
-	char *full_path = NULL;
 	struct inode *inode = dentry->d_inode;
 	struct super_block *sb = dentry->d_sb;
+	char *full_path = NULL;
 
 	if (inode == NULL)
 		return -ENOENT;
 
-	xid = GetXid();
-
 	if (!cifs_inode_needs_reval(inode))
-		goto check_inval;
+		return rc;
+
+	xid = GetXid();
 
 	/* can not safely grab the rename sem here if rename calls revalidate
 	   since that would deadlock */
 	full_path = build_path_from_dentry(dentry);
 	if (full_path == NULL) {
 		rc = -ENOMEM;
-		goto check_inval;
+		goto out;
 	}
 
-	cFYI(1, "Revalidate: %s inode 0x%p count %d dentry: 0x%p d_time %ld "
-		 "jiffies %ld", full_path, inode, inode->i_count.counter,
+	cFYI(1, "Update attributes: %s inode 0x%p count %d dentry: 0x%p d_time "
+		 "%ld jiffies %ld", full_path, inode, inode->i_count.counter,
 		 dentry, dentry->d_time, jiffies);
 
 	if (cifs_sb_master_tcon(CIFS_SB(sb))->unix_ext)
@@ -1762,41 +1755,78 @@ int cifs_revalidate_dentry(struct dentry *dentry)
 		rc = cifs_get_inode_info(&inode, full_path, NULL, sb,
 					 xid, NULL);
 
-check_inval:
-	if (CIFS_I(inode)->invalid_mapping)
-		cifs_invalidate_mapping(inode);
-
+out:
 	kfree(full_path);
 	FreeXid(xid);
 	return rc;
 }
 
+int cifs_revalidate_file(struct file *filp)
+{
+	int rc;
+	struct inode *inode = filp->f_path.dentry->d_inode;
+
+	rc = cifs_revalidate_file_attr(filp);
+	if (rc)
+		return rc;
+
+	if (CIFS_I(inode)->invalid_mapping)
+		rc = cifs_invalidate_mapping(inode);
+	return rc;
+}
+
+/* revalidate a dentry's inode attributes */
+int cifs_revalidate_dentry(struct dentry *dentry)
+{
+	int rc;
+	struct inode *inode = dentry->d_inode;
+
+	rc = cifs_revalidate_dentry_attr(dentry);
+	if (rc)
+		return rc;
+
+	if (CIFS_I(inode)->invalid_mapping)
+		rc = cifs_invalidate_mapping(inode);
+	return rc;
+}
+
 int cifs_getattr(struct vfsmount *mnt, struct dentry *dentry,
 		 struct kstat *stat)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
-	int err = cifs_revalidate_dentry(dentry);
-
-	if (!err) {
-		generic_fillattr(dentry->d_inode, stat);
-		stat->blksize = CIFS_MAX_MSGSIZE;
-		stat->ino = CIFS_I(dentry->d_inode)->uniqueid;
+	struct inode *inode = dentry->d_inode;
+	int rc;
 
-		/*
-		 * If on a multiuser mount without unix extensions, and the
-		 * admin hasn't overridden them, set the ownership to the
-		 * fsuid/fsgid of the current process.
-		 */
-		if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER) &&
-		    !tcon->unix_ext) {
-			if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID))
-				stat->uid = current_fsuid();
-			if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID))
-				stat->gid = current_fsgid();
+	if (inode && inode->i_mapping && inode->i_mapping->nrpages != 0) {
+		rc = filemap_write_and_wait(inode->i_mapping);
+		if (rc) {
+			mapping_set_error(inode->i_mapping, rc);
+			return rc;
 		}
 	}
-	return err;
+
+	rc = cifs_revalidate_dentry_attr(dentry);
+	if (rc)
+		return rc;
+
+	generic_fillattr(dentry->d_inode, stat);
+	stat->blksize = CIFS_MAX_MSGSIZE;
+	stat->ino = CIFS_I(dentry->d_inode)->uniqueid;
+
+	/*
+	 * If on a multiuser mount without unix extensions, and the admin hasn't
+	 * overridden them, set the ownership to the fsuid/fsgid of the current
+	 * process.
+	 */
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER) &&
+	    !tcon->unix_ext) {
+		if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID))
+			stat->uid = current_fsuid();
+		if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID))
+			stat->gid = current_fsgid();
+	}
+	return rc;
 }
 
 static int cifs_truncate_page(struct address_space *mapping, loff_t from)
-- 
1.7.1

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Pavel Shilovsky]

2011/4/12 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> Simplify many places when we call cifs_revalidate/invalidate to make
> it does what it exactly needs.
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |   31 ++++++++++-----
>  fs/cifs/cifsfs.h |    4 +-
>  fs/cifs/file.c   |   16 ++++++--
>  fs/cifs/inode.c  |  114 ++++++++++++++++++++++++++++++++++--------------------
>  4 files changed, 108 insertions(+), 57 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index fb6a2ad..1646360 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -623,16 +623,27 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
>  {
>        /* origin == SEEK_END => we must revalidate the cached file length */
>        if (origin == SEEK_END) {
> -               int retval;
> -
> -               /* some applications poll for the file length in this strange
> -                  way so we must seek to end on non-oplocked files by
> -                  setting the revalidate time to zero */
> -               CIFS_I(file->f_path.dentry->d_inode)->time = 0;
> -
> -               retval = cifs_revalidate_file(file);
> -               if (retval < 0)
> -                       return (loff_t)retval;
> +               int rc;
> +               struct inode *inode = file->f_path.dentry->d_inode;
> +
> +               /* Need to flush all dirty pages to get a right file length */
> +               if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
> +                       rc = filemap_write_and_wait(inode->i_mapping);
> +                       if (rc) {
> +                               mapping_set_error(inode->i_mapping, rc);
> +                               return (loff_t)rc;
> +                       }
> +               }
> +               /*
> +                * Some applications poll for the file length in this strange
> +                * way so we must seek to end on non-oplocked files by
> +                * setting the revalidate time to zero.
> +                */
> +               CIFS_I(inode)->time = 0;
> +
> +               rc = cifs_revalidate_file_attr(file);
> +               if (rc < 0)
> +                       return (loff_t)rc;
>        }
>        return generic_file_llseek_unlocked(file, offset, origin);
>  }
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index bb64313..d304584 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -59,9 +59,11 @@ extern int cifs_mkdir(struct inode *, struct dentry *, int);
>  extern int cifs_rmdir(struct inode *, struct dentry *);
>  extern int cifs_rename(struct inode *, struct dentry *, struct inode *,
>                       struct dentry *);
> +extern int cifs_revalidate_file_attr(struct file *filp);
> +extern int cifs_revalidate_dentry_attr(struct dentry *);
>  extern int cifs_revalidate_file(struct file *filp);
>  extern int cifs_revalidate_dentry(struct dentry *);
> -extern void cifs_invalidate_mapping(struct inode *inode);
> +extern int cifs_invalidate_mapping(struct inode *inode);
>  extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
>  extern int cifs_setattr(struct dentry *, struct iattr *);
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 613f965..3ceef8a 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1445,8 +1445,13 @@ int cifs_strict_fsync(struct file *file, int datasync)
>        cFYI(1, "Sync file - name: %s datasync: 0x%x",
>                file->f_path.dentry->d_name.name, datasync);
>
> -       if (!CIFS_I(inode)->clientCanCacheRead)
> -               cifs_invalidate_mapping(inode);
> +       if (!CIFS_I(inode)->clientCanCacheRead) {
> +               rc = cifs_invalidate_mapping(inode);
> +               if (rc) {
> +                       cFYI(1, "rc: %d during invalidate phase", rc);
> +                       rc = 0; /* don't care about it in fsync */
> +               }
> +       }
>
>        tcon = tlink_tcon(smbfile->tlink);
>        if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> @@ -1931,8 +1936,11 @@ int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma)
>
>        xid = GetXid();
>
> -       if (!CIFS_I(inode)->clientCanCacheRead)
> -               cifs_invalidate_mapping(inode);
> +       if (!CIFS_I(inode)->clientCanCacheRead) {
> +               rc = cifs_invalidate_mapping(inode);
> +               if (rc)
> +                       return rc;
> +       }
>
>        rc = generic_file_mmap(file, vma);
>        if (rc == 0)
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index adb6324..5f71e11 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1683,18 +1683,15 @@ cifs_inode_needs_reval(struct inode *inode)
>  /*
>  * Zap the cache. Called when invalid_mapping flag is set.
>  */
> -void
> +int
>  cifs_invalidate_mapping(struct inode *inode)
>  {
> -       int rc;
> +       int rc = 0;
>        struct cifsInodeInfo *cifs_i = CIFS_I(inode);
>
>        cifs_i->invalid_mapping = false;
>
>        if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
> -               /* write back any cached data */
> -               rc = filemap_write_and_wait(inode->i_mapping);
> -               mapping_set_error(inode->i_mapping, rc);
>                rc = invalidate_inode_pages2(inode->i_mapping);
>                if (rc) {
>                        cERROR(1, "%s: could not invalidate inode %p", __func__,
> @@ -1704,56 +1701,52 @@ cifs_invalidate_mapping(struct inode *inode)
>        }
>
>        cifs_fscache_reset_inode_cookie(inode);
> +       return rc;
>  }
>
> -int cifs_revalidate_file(struct file *filp)
> +int cifs_revalidate_file_attr(struct file *filp)
>  {
>        int rc = 0;
>        struct inode *inode = filp->f_path.dentry->d_inode;
>        struct cifsFileInfo *cfile = (struct cifsFileInfo *) filp->private_data;
>
>        if (!cifs_inode_needs_reval(inode))
> -               goto check_inval;
> +               return rc;
>
>        if (tlink_tcon(cfile->tlink)->unix_ext)
>                rc = cifs_get_file_info_unix(filp);
>        else
>                rc = cifs_get_file_info(filp);
>
> -check_inval:
> -       if (CIFS_I(inode)->invalid_mapping)
> -               cifs_invalidate_mapping(inode);
> -
>        return rc;
>  }
>
> -/* revalidate a dentry's inode attributes */
> -int cifs_revalidate_dentry(struct dentry *dentry)
> +int cifs_revalidate_dentry_attr(struct dentry *dentry)
>  {
>        int xid;
>        int rc = 0;
> -       char *full_path = NULL;
>        struct inode *inode = dentry->d_inode;
>        struct super_block *sb = dentry->d_sb;
> +       char *full_path = NULL;
>
>        if (inode == NULL)
>                return -ENOENT;
>
> -       xid = GetXid();
> -
>        if (!cifs_inode_needs_reval(inode))
> -               goto check_inval;
> +               return rc;
> +
> +       xid = GetXid();
>
>        /* can not safely grab the rename sem here if rename calls revalidate
>           since that would deadlock */
>        full_path = build_path_from_dentry(dentry);
>        if (full_path == NULL) {
>                rc = -ENOMEM;
> -               goto check_inval;
> +               goto out;
>        }
>
> -       cFYI(1, "Revalidate: %s inode 0x%p count %d dentry: 0x%p d_time %ld "
> -                "jiffies %ld", full_path, inode, inode->i_count.counter,
> +       cFYI(1, "Update attributes: %s inode 0x%p count %d dentry: 0x%p d_time "
> +                "%ld jiffies %ld", full_path, inode, inode->i_count.counter,
>                 dentry, dentry->d_time, jiffies);
>
>        if (cifs_sb_master_tcon(CIFS_SB(sb))->unix_ext)
> @@ -1762,41 +1755,78 @@ int cifs_revalidate_dentry(struct dentry *dentry)
>                rc = cifs_get_inode_info(&inode, full_path, NULL, sb,
>                                         xid, NULL);
>
> -check_inval:
> -       if (CIFS_I(inode)->invalid_mapping)
> -               cifs_invalidate_mapping(inode);
> -
> +out:
>        kfree(full_path);
>        FreeXid(xid);
>        return rc;
>  }
>
> +int cifs_revalidate_file(struct file *filp)
> +{
> +       int rc;
> +       struct inode *inode = filp->f_path.dentry->d_inode;
> +
> +       rc = cifs_revalidate_file_attr(filp);
> +       if (rc)
> +               return rc;
> +
> +       if (CIFS_I(inode)->invalid_mapping)
> +               rc = cifs_invalidate_mapping(inode);
> +       return rc;
> +}
> +
> +/* revalidate a dentry's inode attributes */
> +int cifs_revalidate_dentry(struct dentry *dentry)
> +{
> +       int rc;
> +       struct inode *inode = dentry->d_inode;
> +
> +       rc = cifs_revalidate_dentry_attr(dentry);
> +       if (rc)
> +               return rc;
> +
> +       if (CIFS_I(inode)->invalid_mapping)
> +               rc = cifs_invalidate_mapping(inode);
> +       return rc;
> +}
> +
>  int cifs_getattr(struct vfsmount *mnt, struct dentry *dentry,
>                 struct kstat *stat)
>  {
>        struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
>        struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> -       int err = cifs_revalidate_dentry(dentry);
> -
> -       if (!err) {
> -               generic_fillattr(dentry->d_inode, stat);
> -               stat->blksize = CIFS_MAX_MSGSIZE;
> -               stat->ino = CIFS_I(dentry->d_inode)->uniqueid;
> +       struct inode *inode = dentry->d_inode;
> +       int rc;
>
> -               /*
> -                * If on a multiuser mount without unix extensions, and the
> -                * admin hasn't overridden them, set the ownership to the
> -                * fsuid/fsgid of the current process.
> -                */
> -               if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER) &&
> -                   !tcon->unix_ext) {
> -                       if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID))
> -                               stat->uid = current_fsuid();
> -                       if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID))
> -                               stat->gid = current_fsgid();
> +       if (inode && inode->i_mapping && inode->i_mapping->nrpages != 0) {
> +               rc = filemap_write_and_wait(inode->i_mapping);
> +               if (rc) {
> +                       mapping_set_error(inode->i_mapping, rc);
> +                       return rc;
>                }
>        }
> -       return err;
> +
> +       rc = cifs_revalidate_dentry_attr(dentry);
> +       if (rc)
> +               return rc;
> +
> +       generic_fillattr(dentry->d_inode, stat);
> +       stat->blksize = CIFS_MAX_MSGSIZE;
> +       stat->ino = CIFS_I(dentry->d_inode)->uniqueid;
> +
> +       /*
> +        * If on a multiuser mount without unix extensions, and the admin hasn't
> +        * overridden them, set the ownership to the fsuid/fsgid of the current
> +        * process.
> +        */
> +       if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER) &&
> +           !tcon->unix_ext) {
> +               if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_UID))
> +                       stat->uid = current_fsuid();
> +               if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID))
> +                       stat->gid = current_fsgid();
> +       }
> +       return rc;
>  }
>
>  static int cifs_truncate_page(struct address_space *mapping, loff_t from)
> --
> 1.7.1
>
>

What's about this?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Pavel Shilovsky]

2011/4/12 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> Simplify many places when we call cifs_revalidate/invalidate to make
> it does what it exactly needs.
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>

What's about this patch?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Pavel Shilovsky]

After conversation with Steve, we decided to drop
filemap_write_and_wait from getattr, because we already do it in
cifs_file_aio_write. I also think that we should drop it from
cifs_llseek in this case too. I will repost this patch later (launder
page operation patch was merged earlier).

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Jeff Layton]

On Fri, 22 Apr 2011 12:09:20 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> After conversation with Steve, we decided to drop
> filemap_write_and_wait from getattr, because we already do it in
> cifs_file_aio_write. I also think that we should drop it from
> cifs_llseek in this case too. I will repost this patch later (launder
> page operation patch was merged earlier).
> 

I wasn't privy to this discussion, but that makes no sense to me. Just
because we initiated writeout in cifs_file_aio_write, does not mean
that it's complete. If it's not complete then the size returned by the
server may be bogus.

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

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Pavel Shilovsky]

2011/4/22 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Fri, 22 Apr 2011 12:09:20 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> After conversation with Steve, we decided to drop
>> filemap_write_and_wait from getattr, because we already do it in
>> cifs_file_aio_write. I also think that we should drop it from
>> cifs_llseek in this case too. I will repost this patch later (launder
>> page operation patch was merged earlier).
>>
>
> I wasn't privy to this discussion, but that makes no sense to me. Just
> because we initiated writeout in cifs_file_aio_write, does not mean
> that it's complete. If it's not complete then the size returned by the
> server may be bogus.
>

We call filemap_fdatawrite -> __filemap_fdatawrite(WB_SYNC_ALL) ->
__filemap_fdatawrite_range(WB_SYNC_ALL) -> do_writepages ->
cifs_writepages.

cifs_writepages with WB_SYNC_ALL is sync operations (even with you
async writepages set) - so we always get correct filesize from the
server. If the write from another process is in progress now, we don't
get better results to issues filemap_write_and_wait because another
process can dirty pages after this but before we request getattr to
the server.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Christoph Hellwig]

On Fri, Apr 22, 2011 at 04:05:31PM +0400, Pavel Shilovsky wrote:
> We call filemap_fdatawrite -> __filemap_fdatawrite(WB_SYNC_ALL) ->
> __filemap_fdatawrite_range(WB_SYNC_ALL) -> do_writepages ->
> cifs_writepages.
> 
> cifs_writepages with WB_SYNC_ALL is sync operations (even with you
> async writepages set)

It really shouldn't be synchronous.  The defintion of WB_SYNC_ALL
is that it doesn't skip pages when initiating writeout, but you still
have to wait for them to complete using filemap_fdatawait.

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Jeff Layton]

On Fri, 22 Apr 2011 08:09:12 -0400
Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> On Fri, Apr 22, 2011 at 04:05:31PM +0400, Pavel Shilovsky wrote:
> > We call filemap_fdatawrite -> __filemap_fdatawrite(WB_SYNC_ALL) ->
> > __filemap_fdatawrite_range(WB_SYNC_ALL) -> do_writepages ->
> > cifs_writepages.
> > 
> > cifs_writepages with WB_SYNC_ALL is sync operations (even with you
> > async writepages set)
> 
> It really shouldn't be synchronous.  The defintion of WB_SYNC_ALL
> is that it doesn't skip pages when initiating writeout, but you still
> have to wait for them to complete using filemap_fdatawait.
> 

Yes, I think my current patchset is wrong. It has cifs_writepages wait
for all of the writes to complete before returning in the WB_SYNC_ALL
case. I think it would probably be better to have it just return after
submitting the writes, and have the write completion handler resubmit
them if the error is retryable.

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

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Steve French]

On Fri, Apr 22, 2011 at 6:50 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 22 Apr 2011 12:09:20 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> After conversation with Steve, we decided to drop
>> filemap_write_and_wait from getattr, because we already do it in
>> cifs_file_aio_write. I also think that we should drop it from
>> cifs_llseek in this case too. I will repost this patch later (launder
>> page operation patch was merged earlier).
>>
>
> I wasn't privy to this discussion, but that makes no sense to me. Just
> because we initiated writeout in cifs_file_aio_write, does not mean
> that it's complete. If it's not complete then the size returned by the
> server may be bogus.

What would a local file system do in the case when a write is
racing with a getattr?  In the case of cifs, when we issue
a write, and don't have oplock, we immediately send the
write on the network - but AFAIK posix provides no guarantees
about ordering if they are issued at the same time.



-- 
Thanks,

Steve

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Jeff Layton]

On Fri, 22 Apr 2011 09:02:18 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Apr 22, 2011 at 6:50 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 22 Apr 2011 12:09:20 +0400
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >
> >> After conversation with Steve, we decided to drop
> >> filemap_write_and_wait from getattr, because we already do it in
> >> cifs_file_aio_write. I also think that we should drop it from
> >> cifs_llseek in this case too. I will repost this patch later (launder
> >> page operation patch was merged earlier).
> >>
> >
> > I wasn't privy to this discussion, but that makes no sense to me. Just
> > because we initiated writeout in cifs_file_aio_write, does not mean
> > that it's complete. If it's not complete then the size returned by the
> > server may be bogus.
> 
> What would a local file system do in the case when a write is
> racing with a getattr?  In the case of cifs, when we issue
> a write, and don't have oplock, we immediately send the
> write on the network - but AFAIK posix provides no guarantees
> about ordering if they are issued at the same time.
> 

It's not a problem for a local filesystem as there's only one set of
metadata to deal with. Even if the writes aren't synced out to disk,
you still know how big the file is.

With a client/server setup like cifs or nfs, you have to deal with two,
and when there are buffered writes then there will be discrepancies.
The simplest way to deal with discrepancies there is to make sure
that there aren't any by flushing out any buffered writes before
fetching the attributes.

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

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Steve French]

On Fri, Apr 22, 2011 at 9:10 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 22 Apr 2011 09:02:18 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Fri, Apr 22, 2011 at 6:50 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Fri, 22 Apr 2011 12:09:20 +0400
>> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>> >
>> >> After conversation with Steve, we decided to drop
>> >> filemap_write_and_wait from getattr, because we already do it in
>> >> cifs_file_aio_write. I also think that we should drop it from
>> >> cifs_llseek in this case too. I will repost this patch later (launder
>> >> page operation patch was merged earlier).
>> >>
>> >
>> > I wasn't privy to this discussion, but that makes no sense to me. Just
>> > because we initiated writeout in cifs_file_aio_write, does not mean
>> > that it's complete. If it's not complete then the size returned by the
>> > server may be bogus.
>>
>> What would a local file system do in the case when a write is
>> racing with a getattr?  In the case of cifs, when we issue
>> a write, and don't have oplock, we immediately send the
>> write on the network - but AFAIK posix provides no guarantees
>> about ordering if they are issued at the same time.
>>
>
> It's not a problem for a local filesystem as there's only one set of
> metadata to deal with. Even if the writes aren't synced out to disk,
> you still know how big the file is.
>
> With a client/server setup like cifs or nfs, you have to deal with two,
> and when there are buffered writes then there will be discrepancies.
> The simplest way to deal with discrepancies there is to make sure
> that there aren't any by flushing out any buffered writes before
> fetching the attributes.

it may sound simpler but doesn't prevent a write racing in just
before we send the QueryFileInfo on the wire, and it does
hurt performance to redundantly invoke filemap_fdatawrite.
If the write extends the file size, I don't mind the extra call,
but even in that case, the next revalidate will get the
new file size, and the first revalidate will return the smaller file
size that was current (since the write was only
started but not completed)



-- 
Thanks,

Steve

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Jeff Layton]

On Fri, 22 Apr 2011 09:54:10 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Apr 22, 2011 at 9:10 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 22 Apr 2011 09:02:18 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Fri, Apr 22, 2011 at 6:50 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Fri, 22 Apr 2011 12:09:20 +0400
> >> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >> >
> >> >> After conversation with Steve, we decided to drop
> >> >> filemap_write_and_wait from getattr, because we already do it in
> >> >> cifs_file_aio_write. I also think that we should drop it from
> >> >> cifs_llseek in this case too. I will repost this patch later (launder
> >> >> page operation patch was merged earlier).
> >> >>
> >> >
> >> > I wasn't privy to this discussion, but that makes no sense to me. Just
> >> > because we initiated writeout in cifs_file_aio_write, does not mean
> >> > that it's complete. If it's not complete then the size returned by the
> >> > server may be bogus.
> >>
> >> What would a local file system do in the case when a write is
> >> racing with a getattr?  In the case of cifs, when we issue
> >> a write, and don't have oplock, we immediately send the
> >> write on the network - but AFAIK posix provides no guarantees
> >> about ordering if they are issued at the same time.
> >>
> >
> > It's not a problem for a local filesystem as there's only one set of
> > metadata to deal with. Even if the writes aren't synced out to disk,
> > you still know how big the file is.
> >
> > With a client/server setup like cifs or nfs, you have to deal with two,
> > and when there are buffered writes then there will be discrepancies.
> > The simplest way to deal with discrepancies there is to make sure
> > that there aren't any by flushing out any buffered writes before
> > fetching the attributes.
> 
> it may sound simpler but doesn't prevent a write racing in just
> before we send the QueryFileInfo on the wire, and it does
> hurt performance to redundantly invoke filemap_fdatawrite.
>

IIUC, POSIX says that the st_size must reflect the results of all
writes done up to the point that the stat() call was issued. If some
race in after the fact, then that's generally ok, but we don't want a
situation where someone issues a ton of writes and then the stat() call
doesn't reflect the result of them.

> If the write extends the file size, I don't mind the extra call,
> but even in that case, the next revalidate will get the
> new file size, and the first revalidate will return the smaller file
> size that was current (since the write was only
> started but not completed)
> 

What if someone else extends the file on the server? How do you then
reconcile the difference between the file size as tracked on the client
and that on the server?

Also, note that this involves more than the size. There is also the
*time fields to consider. I don't see how to do this safely without
flushing out all of the dirty data on a ->getattr.

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

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Steve French]

On Fri, Apr 22, 2011 at 10:07 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 22 Apr 2011 09:54:10 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Fri, Apr 22, 2011 at 9:10 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Fri, 22 Apr 2011 09:02:18 -0500
>> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> On Fri, Apr 22, 2011 at 6:50 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >> > On Fri, 22 Apr 2011 12:09:20 +0400
>> >> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>> >> >
>> >> >> After conversation with Steve, we decided to drop
>> >> >> filemap_write_and_wait from getattr, because we already do it in
>> >> >> cifs_file_aio_write. I also think that we should drop it from
>> >> >> cifs_llseek in this case too. I will repost this patch later (launder
>> >> >> page operation patch was merged earlier).
>> >> >>
>> >> >
>> >> > I wasn't privy to this discussion, but that makes no sense to me. Just
>> >> > because we initiated writeout in cifs_file_aio_write, does not mean
>> >> > that it's complete. If it's not complete then the size returned by the
>> >> > server may be bogus.
>> >>
>> >> What would a local file system do in the case when a write is
>> >> racing with a getattr?  In the case of cifs, when we issue
>> >> a write, and don't have oplock, we immediately send the
>> >> write on the network - but AFAIK posix provides no guarantees
>> >> about ordering if they are issued at the same time.
>> >>
>> >
>> > It's not a problem for a local filesystem as there's only one set of
>> > metadata to deal with. Even if the writes aren't synced out to disk,
>> > you still know how big the file is.
>> >
>> > With a client/server setup like cifs or nfs, you have to deal with two,
>> > and when there are buffered writes then there will be discrepancies.
>> > The simplest way to deal with discrepancies there is to make sure
>> > that there aren't any by flushing out any buffered writes before
>> > fetching the attributes.
>>
>> it may sound simpler but doesn't prevent a write racing in just
>> before we send the QueryFileInfo on the wire, and it does
>> hurt performance to redundantly invoke filemap_fdatawrite.
>>
>
> IIUC, POSIX says that the st_size must reflect the results of all
> writes done up to the point that the stat() call was issued. If some
> race in after the fact, then that's generally ok, but we don't want a
> situation where someone issues a ton of writes and then the stat() call
> doesn't reflect the result of them.

aio_write doesn't return until filemap_fdatawrite completes and it
calls cifs_writepages which is synchronous - so the st_size
on the server will be uptodate if the write has completed.

-- 
Thanks,

Steve

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Jeff Layton]

On Fri, 22 Apr 2011 11:28:28 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Apr 22, 2011 at 10:07 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 22 Apr 2011 09:54:10 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Fri, Apr 22, 2011 at 9:10 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Fri, 22 Apr 2011 09:02:18 -0500
> >> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> >
> >> >> On Fri, Apr 22, 2011 at 6:50 AM, Jeff Layton <jlayton-H+wXaHxf7aJhl2p70BpVqQ@public.gmane.orgm> wrote:
> >> >> > On Fri, 22 Apr 2011 12:09:20 +0400
> >> >> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >> >> >
> >> >> >> After conversation with Steve, we decided to drop
> >> >> >> filemap_write_and_wait from getattr, because we already do it in
> >> >> >> cifs_file_aio_write. I also think that we should drop it from
> >> >> >> cifs_llseek in this case too. I will repost this patch later (launder
> >> >> >> page operation patch was merged earlier).
> >> >> >>
> >> >> >
> >> >> > I wasn't privy to this discussion, but that makes no sense to me. Just
> >> >> > because we initiated writeout in cifs_file_aio_write, does not mean
> >> >> > that it's complete. If it's not complete then the size returned by the
> >> >> > server may be bogus.
> >> >>
> >> >> What would a local file system do in the case when a write is
> >> >> racing with a getattr?  In the case of cifs, when we issue
> >> >> a write, and don't have oplock, we immediately send the
> >> >> write on the network - but AFAIK posix provides no guarantees
> >> >> about ordering if they are issued at the same time.
> >> >>
> >> >
> >> > It's not a problem for a local filesystem as there's only one set of
> >> > metadata to deal with. Even if the writes aren't synced out to disk,
> >> > you still know how big the file is.
> >> >
> >> > With a client/server setup like cifs or nfs, you have to deal with two,
> >> > and when there are buffered writes then there will be discrepancies.
> >> > The simplest way to deal with discrepancies there is to make sure
> >> > that there aren't any by flushing out any buffered writes before
> >> > fetching the attributes.
> >>
> >> it may sound simpler but doesn't prevent a write racing in just
> >> before we send the QueryFileInfo on the wire, and it does
> >> hurt performance to redundantly invoke filemap_fdatawrite.
> >>
> >
> > IIUC, POSIX says that the st_size must reflect the results of all
> > writes done up to the point that the stat() call was issued. If some
> > race in after the fact, then that's generally ok, but we don't want a
> > situation where someone issues a ton of writes and then the stat() call
> > doesn't reflect the result of them.
> 
> aio_write doesn't return until filemap_fdatawrite completes and it
> calls cifs_writepages which is synchronous - so the st_size
> on the server will be uptodate if the write has completed.
> 

Used to be synchronous. One of the goals of the async write patchset
I'm working on is to change that.

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

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Steve French]

On Fri, Apr 22, 2011 at 11:59 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 22 Apr 2011 11:28:28 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Fri, Apr 22, 2011 at 10:07 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Fri, 22 Apr 2011 09:54:10 -0500
>> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> On Fri, Apr 22, 2011 at 9:10 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >> > On Fri, 22 Apr 2011 09:02:18 -0500
>> >> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> >
>> >> >> On Fri, Apr 22, 2011 at 6:50 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> >> >> > On Fri, 22 Apr 2011 12:09:20 +0400
>> >> >> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>> >> >> >
>> >> >> >> After conversation with Steve, we decided to drop
>> >> >> >> filemap_write_and_wait from getattr, because we already do it in
>> >> >> >> cifs_file_aio_write. I also think that we should drop it from
>> >> >> >> cifs_llseek in this case too. I will repost this patch later (launder
>> >> >> >> page operation patch was merged earlier).
>> >> >> >>
>> >> >> >
>> >> >> > I wasn't privy to this discussion, but that makes no sense to me. Just
>> >> >> > because we initiated writeout in cifs_file_aio_write, does not mean
>> >> >> > that it's complete. If it's not complete then the size returned by the
>> >> >> > server may be bogus.
>> >> >>
>> >> >> What would a local file system do in the case when a write is
>> >> >> racing with a getattr?  In the case of cifs, when we issue
>> >> >> a write, and don't have oplock, we immediately send the
>> >> >> write on the network - but AFAIK posix provides no guarantees
>> >> >> about ordering if they are issued at the same time.
>> >> >>
>> >> >
>> >> > It's not a problem for a local filesystem as there's only one set of
>> >> > metadata to deal with. Even if the writes aren't synced out to disk,
>> >> > you still know how big the file is.
>> >> >
>> >> > With a client/server setup like cifs or nfs, you have to deal with two,
>> >> > and when there are buffered writes then there will be discrepancies.
>> >> > The simplest way to deal with discrepancies there is to make sure
>> >> > that there aren't any by flushing out any buffered writes before
>> >> > fetching the attributes.
>> >>
>> >> it may sound simpler but doesn't prevent a write racing in just
>> >> before we send the QueryFileInfo on the wire, and it does
>> >> hurt performance to redundantly invoke filemap_fdatawrite.
>> >>
>> >
>> > IIUC, POSIX says that the st_size must reflect the results of all
>> > writes done up to the point that the stat() call was issued. If some
>> > race in after the fact, then that's generally ok, but we don't want a
>> > situation where someone issues a ton of writes and then the stat() call
>> > doesn't reflect the result of them.
>>
>> aio_write doesn't return until filemap_fdatawrite completes and it
>> calls cifs_writepages which is synchronous - so the st_size
>> on the server will be uptodate if the write has completed.
>>
>
> Used to be synchronous. One of the goals of the async write patchset
> I'm working on is to change that.

in that case, I don't mind doing the extra fdatawriteandwait before we
send the queryfileinfo to the server, but why not limit the performance
overhead so we only do it when the write would update the filesize
(also have to be careful not to do this when the file is oplocked)


-- 
Thanks,

Steve

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Pavel Shilovsky]

2011/4/22 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Used to be synchronous. One of the goals of the async write patchset
> I'm working on is to change that.

In this case, what do you think about to call filemap_fdatawait before
update attributes in gettattr and llseek? It seems that it makes us
sure that all dirty pages are written and we can get right file
attributes from the server.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Jeff Layton]

On Fri, 22 Apr 2011 23:02:00 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> 2011/4/22 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > Used to be synchronous. One of the goals of the async write patchset
> > I'm working on is to change that.
> 
> In this case, what do you think about to call filemap_fdatawait before
> update attributes in gettattr and llseek? It seems that it makes us
> sure that all dirty pages are written and we can get right file
> attributes from the server.
> 

Yeah, assuming that we write back immediately when notified of an
oplock break, then that should be fine.

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

Re: [PATCH 2/2] CIFS: Simplify invalidate part (try #2)

[Steve French]

On Fri, Apr 22, 2011 at 3:26 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 22 Apr 2011 23:02:00 +0400
> Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
>
>> 2011/4/22 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > Used to be synchronous. One of the goals of the async write patchset
>> > I'm working on is to change that.
>>
>> In this case, what do you think about to call filemap_fdatawait before
>> update attributes in gettattr and llseek? It seems that it makes us
>> sure that all dirty pages are written and we can get right file
>> attributes from the server.
>>
>
> Yeah, assuming that we write back immediately when notified of an
> oplock break, then that should be fine.

we do write back on oplock break (we have to before we can
respond to the oplock break, otherwise the other client would
get stale data).   It is frustrating (about the protocol) that
cifs breaks oplocks even if the second open is from the
same client so in most cases oplock breaks would not
have been needed (since it is a 2nd open from the same client)
but there is nothing we can do about this other than
move to batch oplock, or (for smb2) use the new smb2.1 leases.



-- 
Thanks,

Steve