2011/3/27 Steve French <smfrench@xxxxxxxxx>:
> On Sun, Mar 27, 2011 at 5:48 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> On Sat, 26 Mar 2011 11:14:45 +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 | 23 ++++++++++++++++-------
>>> 4 files changed, 38 insertions(+), 13 deletions(-)
>>>
>>
>> -----------------[snip]---------------------
>>
>>> 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;
>>> + }
>>> + }
>>>
>>
>> Now that I've had some more time to think on this patch, I don't think
>> this is what we want at all. The problem in a nutshell is that an -EBUSY
>> return makes no sense here.
>>
>> Suppose I run fsync() on a file, and everything is written out
>> successfully, but some page is redirtied in the meantime and can't be
>> invalidated. This then returns -EBUSY, but why should the task doing the
>> fsync care that the pages can't be invalidated? It shouldn't, AFAICT.
>> The thread getting the -EBUSY ought to be the one that next attempts to
>> access the page.
>
> you are right. it doesn't care about EBUSY, that the filemap_fdatawrite*
> succeeds is more important. But the next thread that
> accesses the range may need to recognize that invalid_mapping = true
> though. I thought Pavel said that the strict cache read was going
> to be changed so it frees the mapping when the mapping is marked
> invalid
>
No, I was not going to provide such a check for cifs_strict_read -
only cifs_revalidate for future cifs_file_aio_read (non-strictcache
mode). There is no needs to do it for strict part because if we don't
have an oplock for reading (like in this case) we read directly from
the server and don't use a cache.
As for fsycn logic - as I understand this call - it ought to
synchronize data between a cache and storage device. But if we can't
free pages, we don't successfully complete this synchronization - so,
reporting -EBUSY error here doesn't seem wrong to me (may be we should
set -EIO error code - according to the man page). If I am mistaken,
please, explain your points.
--
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
