On Thu, 24 Mar 2011 17:39:47 +0300
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> Base of approach for splitting all calls that uses cifs_invalidate_mapping
> (or cifs_revalidate_dentry, cifs_revalidate_file) into two groups:
> 1) aware about -EBUSY error code and report it back (cifs_d_revalidate,
> cifs_strict_fsync, cifs_file_strict_mmap);
> 2) don't do it (cifs_getattrs, cifs_lseek).
>
> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> ---
> fs/cifs/cifsfs.c | 8 +++++++-
> fs/cifs/cifsfs.h | 2 +-
> fs/cifs/file.c | 18 ++++++++++++++----
> fs/cifs/inode.c | 30 ++++++++++++++++++------------
> 4 files changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index de49fbb..b275d76 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -634,7 +634,13 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int origin)
> CIFS_I(file->f_path.dentry->d_inode)->time = 0;
>
> retval = cifs_revalidate_file(file);
> - if (retval < 0)
> + /*
> + * We only need to get right file length and don't need to
> + * aware about busy pages (-EBUSY error code).
> + */
> + if (retval == -EBUSY)
> + retval = 0;
> + else if (retval < 0)
> return (loff_t)retval;
> }
> return generic_file_llseek_unlocked(file, offset, origin);
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index bb64313..f4391ff 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -61,7 +61,7 @@ extern int cifs_rename(struct inode *, struct dentry *, struct inode *,
> 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 b9731c9..d99cf48 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1448,8 +1448,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) {
> + FreeXid(xid);
> + return rc;
> + }
> + }
>
Hmm...this put us in danger of reporting writeback errors twice? Note
that cifs_invalidate_mapping resets the error on the mapping if
writeback fails. So imagine:
t1: do the regular filemap_write_and_wait in vfs_fsync
t2: another thread dirties pages on the inode
t1: call cifs_invalidate_mapping which does filemap_write_and_wait.
That fails, so fsync returns an error.
t2: now it does an fsync and the error is reported again
> tcon = tlink_tcon(smbfile->tlink);
> if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
> @@ -1916,8 +1921,13 @@ 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) {
> + FreeXid(xid);
> + return rc;
> + }
> + }
>
> rc = generic_file_mmap(file, vma);
> FreeXid(xid);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index adb6324..d3abfc5 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1683,27 +1683,25 @@ 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__,
> - inode);
> - cifs_i->invalid_mapping = true;
> - }
> + if (rc)
> + cFYI(1, "%s: could not invalidate inode %p", __func__,
> + inode);
> + cifs_i->invalid_mapping = (rc != 0) ? true : false;
This is racy. You could end up writing back, invalidating pages, etc.
successfully. Another thread could come along afterward and then set
invalid_mapping = true. Then you go and set it to false, thinking that
everything is OK.
I think what you really want to do is mapping to false first, then
reset it to true if writeback or page invalidation fails.
> }
>
> cifs_fscache_reset_inode_cookie(inode);
> + return rc;
> }
>
> int cifs_revalidate_file(struct file *filp)
> @@ -1722,7 +1720,7 @@ int cifs_revalidate_file(struct file *filp)
>
> check_inval:
> if (CIFS_I(inode)->invalid_mapping)
> - cifs_invalidate_mapping(inode);
> + rc = cifs_invalidate_mapping(inode);
>
> return rc;
> }
> @@ -1764,7 +1762,7 @@ int cifs_revalidate_dentry(struct dentry *dentry)
>
> check_inval:
> if (CIFS_I(inode)->invalid_mapping)
> - cifs_invalidate_mapping(inode);
> + rc = cifs_invalidate_mapping(inode);
>
> kfree(full_path);
> FreeXid(xid);
> @@ -1776,7 +1774,15 @@ int cifs_getattr(struct vfsmount *mnt, struct dentry *dentry,
> {
> 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);
> + int err;
> +
> + err = cifs_revalidate_dentry(dentry);
> + /*
> + * We only need to get file attributes and don't need to
> + * aware about busy pages (-EBUSY error code).
> + */
> + if (err == -EBUSY)
> + err = 0;
>
> if (!err) {
> generic_fillattr(dentry->d_inode, stat);
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
