On Tue, 22 Mar 2011 11:24:02 -0500
Steve French <smfrench@xxxxxxxxx> wrote:
> On Tue, Mar 22, 2011 at 10:17 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Tue, 22 Mar 2011 09:50:43 -0500
> > Steve French <smfrench@xxxxxxxxx> wrote:
> >
> >> 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))
> >>
> >
> > 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 <jlayton@xxxxxxxxxx>
--
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
