On Fri, 25 Mar 2011 22:47:23 +0300 Pavel Shilovsky <[email protected]> wrote:
> 2011/3/25 Jeff Layton <[email protected]>: > > 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? > > > > It can make sense but I think that it should be a separate patch. If > everything is ok excepts things you commented I can provide the next > version of the patch. > That's seems reasonable. -- 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
