On Tue, 22 Mar 2011 11:24:02 -0500 Steve French <[email protected]> wrote:
> On Tue, Mar 22, 2011 at 10:17 AM, Jeff Layton <[email protected]> wrote: > > On Tue, 22 Mar 2011 09:50:43 -0500 > > Steve French <[email protected]> wrote: > > > >> 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)) > >> > > > > It does however make sense to return an error if you're going to > > invalidate a page and can't because the page is dirty: > > > > ret2 = do_launder_page(mapping, page); > > if (ret2 == 0) { > > if (!invalidate_complete_page2(mapping, > > page)) > > ret2 = -EBUSY; > > } > > > > do_launder_page returns 0 if there is no launder_page op... > > > > invalidate_complete_page does this: > > > > if (PageDirty(page)) > > goto failed; > > > > ...so the fact that we don't have a launder_page operation doesn't mean > > that we'll never return an error here. > > > > It makes no sense at all to do a cERROR printk for this. It'll make > > users go "huh?", and won't help us to debug anything. > > but how could we have PageDirty? We just did filemap_write_and_wait > (unless some private mmap case where it may be ok?). > Nothing prevents the page from being redirtied after writeback. -- Jeff Layton <[email protected]> -- 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
