2011/3/25 Pavel Shilovsky <piastry@xxxxxxxxxxx>:
> 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?
>
>>
>>> tcon = tlink_tcon(smbfile->tlink);
>>> if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
>>> @@ -1916,8 +1921,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..d3abfc5 100644
>>> --- a/fs/cifs/inode.c
>>> +++ b/fs/cifs/inode.c
>>> @@ -1683,27 +1683,25 @@ 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;
>>> -
>>> if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
>>> /* write back any cached data */
>>> rc = filemap_write_and_wait(inode->i_mapping);
>>> 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);
>>> - cifs_i->invalid_mapping = true;
>>> - }
>>> + if (rc)
>>> + cFYI(1, "%s: could not invalidate inode %p", __func__,
>>> + inode);
>>> + cifs_i->invalid_mapping = (rc != 0) ? true : false;
>>
>> This is racy. You could end up writing back, invalidating pages, etc.
>> successfully. Another thread could come along afterward and then set
>> invalid_mapping = true. Then you go and set it to false, thinking that
>> everything is OK.
>>
>> I think what you really want to do is mapping to false first, then
>> reset it to true if writeback or page invalidation fails.
>>
>
> But in your case it's racy again:
> 1) t1 sets it to false
> 2) t2 sets it to false
> 3) t1 does filemap_write and invalidate - OK
> 4) t2 does filemap_write and invalidate - ERROR (!)
> 5) t2 marks invalid_mapping as true
> 6) t1 marks invalid_mapping as false - and we miss a ERROR from the 4rd line.
>
> May be we should protect it through a special mapping lock?
Sorry, you are right - there are no problems with it because on the
line 6 we don't set anything as you suggest. So - this question is
over.
>
>>> }
>>>
>>> cifs_fscache_reset_inode_cookie(inode);
>>> + return rc;
>>> }
>>>
>>> int cifs_revalidate_file(struct file *filp)
>>> @@ -1722,7 +1720,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 +1762,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 +1774,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 file attributes and don't need to
>>> + * aware about busy pages (-EBUSY error code).
>>> + */
>>> + if (err == -EBUSY)
>>> + err = 0;
>>>
>>> if (!err) {
>>> generic_fillattr(dentry->d_inode, stat);
>>
>
> --
> Best regards,
> Pavel Shilovsky.
>
--
Best regards,
Pavel Shilovsky.
--
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
