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