On Mon, 28 Mar 2011 22:15:53 +0400 Pavel Shilovsky <[email protected]> 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_file_strict_mmap); > 2) don't do it (cifs_getattrs, cifs_strict_fsync, cifs_lseek). > > Signed-off-by: Pavel Shilovsky <[email protected]> > --- > fs/cifs/cifsfs.c | 8 +++++++- > fs/cifs/cifsfs.h | 2 +- > fs/cifs/file.c | 24 ++++++++++++++++++++---- > fs/cifs/inode.c | 23 ++++++++++++++++------- > 4 files changed, 44 insertions(+), 13 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index de49fbb..b76d992 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 should not 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..329081f 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -1448,8 +1448,19 @@ 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); > + /* > + * We only need to flush all dirty pages and should not aware > + * if some of them are busy (-EBUSY error code). > + */ > + if (rc == -EBUSY) > + rc = 0; > + if (rc) { > + FreeXid(xid); > + return rc; > + } > + } > > tcon = tlink_tcon(smbfile->tlink); > if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC)) > @@ -1916,8 +1927,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..7b3a078 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -1683,10 +1683,10 @@ 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; > @@ -1697,13 +1697,14 @@ cifs_invalidate_mapping(struct inode *inode) > 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); > + cFYI(1, "%s: could not invalidate inode %p", __func__, > + inode); > cifs_i->invalid_mapping = true; > } > } > > cifs_fscache_reset_inode_cookie(inode); > + return rc; > } > > int cifs_revalidate_file(struct file *filp) > @@ -1722,7 +1723,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 +1765,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 +1777,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 inode attributes and should not aware about busy > + * pages (-EBUSY error code). > + */ > + if (err == -EBUSY) > + err = 0; > > if (!err) { > generic_fillattr(dentry->d_inode, stat); I really dislike this special casing for EBUSY. That seems like a real red flag that this is the wrong approach. I think the right solution will involve implementing a launder_page operation. With that you shouldn't ever get an EBUSY as long as the launder_page succeeds. If it doesn't then you probably want to return an error in the above cases anyway. -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
