On Tue, Mar 22, 2011 at 7:04 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Tue, 22 Mar 2011 15:01:11 +0300
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> 2011/3/22 Jeff Layton <jlayton@xxxxxxxxxx>:
>> > On Wed, 16 Mar 2011 01:55:32 +0300
>> > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>> >
>> >> Use invalidate_inode_pages2 that don't leave pages even if shrink_page_list()
>> >> has a temp ref on them. It prevents a data coherency problem when
>> >> cifs_invalidate_mapping didn't invalidate pages but the client thinks that a data
>> >> from the cache is uptodate according to an oplock level (exclusive or II).
>> >>
>> >> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>> >> ---
>> >> fs/cifs/inode.c | 10 ++++++++--
>> >> 1 files changed, 8 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> >> index 41e5651..adb6324 100644
>> >> --- a/fs/cifs/inode.c
>> >> +++ b/fs/cifs/inode.c
>> >> @@ -1691,12 +1691,18 @@ cifs_invalidate_mapping(struct inode *inode)
>> >>
>> >> cifs_i->invalid_mapping = false;
>> >>
>> >> - /* write back any cached data */
>> >> 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;
>> >> + }
>> >> }
>> >> - invalidate_remote_inode(inode);
>> >> +
>> >> cifs_fscache_reset_inode_cookie(inode);
>> >> }
>> >>
>> >
>> > Hi Pavel,
>> >
>> > I noticed that Steve has merged this patch, but it doesn't seem like
>> > you ever made the change to have EBUSY percolate up to userspace. Are
>> > you still planning to fix that?
>> >
>>
>> Steve NACK'ed this and we came to agreement to mark a mapping as
>> invalid in this case for now and let the client try invalidate it
>> again when it needs. Of course, it can bring problems with a data
>> coherency if you need to use a cache very strictly.
>>
>
> Ahh, I must have missed the discussion around that NAK, and I don't see
> any record of it in the list archives. Steve, can you clarify why you
> didn't think that approach was acceptible?
Pavel and I discussed the code paths in detail via IRC or chat. I
would have to go back and look through the code again, but I thought
the issue was that returning
EBUSY (from invalidate_inode_pages2) doesn't make any sense to
revalidate_file (e.g. why would a seek return EBUSY even though we
successfully updated the correct file size?) who can't use the rc, but
by resetting the mapping to invalid_mapping invalidate_inode_pages2
will still get invoked again later (which is all we would do in the
caller if we passed it back).
In practice it probably doesn't make much difference for cifs anyway
(passing the return code back from invalidate_inode_pages2) because we
don't have a "launder_page" operation for cifs and therefore don't
return an error, and for the other case it doesn't seem correct to
fail a seek because of (page_has_private(page))
--
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
