On Tue, Mar 22, 2011 at 7:04 AM, Jeff Layton <[email protected]> wrote: > On Tue, 22 Mar 2011 15:01:11 +0300 > Pavel Shilovsky <[email protected]> wrote: > >> 2011/3/22 Jeff Layton <[email protected]>: >> > On Wed, 16 Mar 2011 01:55:32 +0300 >> > Pavel Shilovsky <[email protected]> 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 <[email protected]> >> >> --- >> >> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
