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?
>
>> 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?
>> }
>>
>> 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.
--
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
