On Wed, 30 Mar 2011 16:06:11 -0500
Steve French <smfrench@xxxxxxxxx> wrote:
> On Wed, Mar 30, 2011 at 3:55 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Mon, 28 Mar 2011 22:15:53 +0400
> > 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_file_strict_mmap);
> >> 2) don't do it (cifs_getattrs, cifs_strict_fsync, cifs_lseek).
> >>
> >> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> >> ---
> >> Â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.
>
> I am not convinced that launder page is needed (it might make it
> easier but hard to tell)
>From invalidate_inode_pages2_range:
---------------[snip]--------------------
BUG_ON(page_mapped(page));
ret2 = do_launder_page(mapping, page);
if (ret2 == 0) {
if (!invalidate_complete_page2(mapping, page))
ret2 = -EBUSY;
}
---------------[snip]--------------------
...when the above code runs, the page is locked. It can't be mapped
into anyone's page tables until it's unlocked. So, we launder the page
(basically, write it out), and then invalidate it. CIFS doesn't use
PagePrivate, and after launder_page it should be clean. So
invalidate_complete_page2 should then work -- no EBUSY.
The only catch is that launder_page can fail and you do have to deal
with that case.
> but I agree that special casing one
> rc out of:
>
> rc = invalidate_inode_pages2(inode->i_mapping);
>
> is odd. Seems like the problem is that some callers of
> invalidate_mapping don't care about the rc, some care
> about the rc from filemap_fdatawrite so they can return
> write errors (disk full, server down etc.) to the user and
> some may even care whether all pages are freed
> this time around (ie whether invalidate_inode_page2
> partially vs. totally succeeded).
>
Yes. Perhaps it's time to consider breaking up cifs_invalidate_mapping.
The function made sense when we didn't have to worry about the return
code from invalidate_remote_inode, but I don't think we can just drop
invalidate_inode_pages2 in its place trivially like this without making
a mess.
--
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
