On Fri, 25 Mar 2011 22:47:23 +0300
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> 2011/3/25 Jeff Layton <jlayton@xxxxxxxxxx>:
> > On Fri, 25 Mar 2011 07:56:07 +0300
> > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> >
> >> 2011/3/25 Jeff Layton <jlayton@xxxxxxxxxx>:
> >> > 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
> >>
> >> 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 <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
