On Fri, 19 Nov 2010 14:55:57 +0300
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2010/11/19 Pavel Shilovsky <piastryyy@xxxxxxxxx>:
> > 2010/11/19 Pavel Shilovsky <piastryyy@xxxxxxxxx>:
> >> 2010/11/15 Jeff Layton <jlayton@xxxxxxxxxx>:
> >>>
> >>> Not directly related to your patch, but what exactly is the purpose of
> >>> the "open_inode_helper"? It looks like "pile o' random junk that we do
> >>
> >> I agree that we can replace it with simply calling get_query_info from
> >> cifs_open.
> >>
> >>> for non-posix opens". Maybe it would be best to eliminate that function
> >>> altogether and further unify the posix and non-posix open code. Steve,
> >>> care to comment?
> >>>
> >>> Now that I'm done ranting, I don't think the invalidate mapping call
> >>> here is unnecessary. We will likely have already invalidated the cache
> >>> during the d_revalidate phase, and you're going to do it again below in
> >>> cifs_new_fileinfo.
> >>>
> >>
> >> I investigated it - you was right about unnecessary calling
> >> cifs_invalidate_mapping from open_inode_helper and cifs_new_fileinfo.
> >> But I found the following problem:
> >>
> >> When we execute script close2_problem.py (see in attachment) several
> >> times (e.x. for i in `seq 1 20`; do ./close2_problem.py; done) with
> >> (!) cifsFYI switched off it shows 'a' instead of 'x' (about 1-2 times
> >> for 20 runs). If we switch cifsFYI on it is almost always 'x' (true) -
> >> I have caught 'a' in this case one or two times for many-many runs.
> >> What do you think about it?
> >
> > I continued the investigation and discovered that if we use
> > invalidate_inode_pages2 instead of invalidate_remote_inode - the
> > problem disappears! So, this is the patch we should apply to solve it
> > (I don't post it as a separate patch? because I think it needs more
> > discussing):
> >
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index 53cce8c..f20f200 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -1591,7 +1591,10 @@ cifs_invalidate_mapping(struct inode *inode)
> > if (rc)
> > cifs_i->write_behind_rc = rc;
> > }
> > - invalidate_remote_inode(inode);
> > +
> > + if (inode->i_mapping)
> > + invalidate_inode_pages2(inode->i_mapping);
> > +
> > cifs_fscache_reset_inode_cookie(inode);
> > }
> >
>
> Sorry, "invalidate" patch above was based on v2.6.36 git tree. Here
> are the version from cifs master branch:
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index ff7d299..66d3ba3 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1679,12 +1679,16 @@ 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) {
> - rc = filemap_write_and_wait(inode->i_mapping);
> - mapping_set_error(inode->i_mapping, rc);
> + if (inode->i_mapping) {
> + /* write back any cached data */
> + if (inode->i_mapping->nrpages != 0) {
> + rc = filemap_write_and_wait(inode->i_mapping);
> + mapping_set_error(inode->i_mapping, rc);
> + }
> +
> + invalidate_inode_pages2(inode->i_mapping);
> }
> - invalidate_remote_inode(inode);
> +
> cifs_fscache_reset_inode_cookie(inode);
> }
>
>
> If everything is ok with it I will post it as a separate patch in right format.
>
I don't think we should apply any patches to fix a problem that we
don't really understand. If it mostly goes away with cifsFYI turned on
then it sounds like a race condition of some sort. It looks like
invalidate_inode_pages2 invalidates locked pages whereas
invalidate_remote_inode doesn't, so that may be a hint, or it could
just change the timing of the race.
You'll need to ascertain the nature of the race to ensure that the fix
is really fixing the problem and not just papering over the bug.
--
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
