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) 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).
--
Thanks,
Steve
--
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
