On Fri, 25 Mar 2011 07:56:07 +0300 Pavel Shilovsky <[email protected]> wrote:
> 2011/3/25 Jeff Layton <[email protected]>: > > On Thu, 24 Mar 2011 17:39:47 +0300 > > 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_strict_fsync, cifs_file_strict_mmap); > >> 2) don't do it (cifs_getattrs, cifs_lseek). > >> > >> Signed-off-by: Pavel Shilovsky <[email protected]> > >> --- > >> 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 > > why does the last line report the error again? what does it prevent to > return OK? > cifs_invalidate_inode does this: rc = filemap_write_and_wait(inode->i_mapping); mapping_set_error(inode->i_mapping, rc); rc = invalidate_inode_pages2(inode->i_mapping); ...if there's an error on writeback, it'll reset that error on the mapping to make sure we can report it later. However, I was mistaken...this patch doesn't report any writeback errors here, just ones on invalidation, so it may be ok. I wonder though...would it be less racy to set up a launder_page function for CIFS and just call invalidate_inode_pages2 here without the filemap_write_and_wait? -- 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
