On Mon, Apr 20, 2009 at 5:36 AM, Shirish Pargaonkar
<shirishpargaonkar at gmail.com> wrote:
> On Thu, Apr 9, 2009 at 12:15 PM, Jeff Layton <jlayton at redhat.com> wrote:
>> On Thu, 9 Apr 2009 10:56:26 -0500
>> Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
>>
>>> On Thu, Apr 9, 2009 at 7:27 AM, Jeff Layton <jlayton at redhat.com> wrote:
>>> > On Wed, 8 Apr 2009 22:57:40 -0500
>>> > Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
>>> >
>>> >> On Wed, Apr 8, 2009 at 12:06 AM, Wilhelm Meier <wilhelm.meier at fh-kl.de> wrote:
>>> >> > Am Mittwoch 08 April 2009 schrieb Shirish Pargaonkar:
>>> >> >> On Fri, Mar 20, 2009 at 10:50 PM, Günter Kukkukk <linux at kukkukk.com>
>>> >> > wrote:
>>> >> >> > Am Freitag, 20. März 2009 schrieb Suresh Jayaraman:
>>> >> >> >> � wrote:
>>> >> >> >> > Hi Steve, Jeff,
>>> >> >> >> >
>>> >> >> >> > this one is on my todo list for more than a year now.
>>> >> >> >>
>>> >> >> >> May be this behavior has changed now? I tried to reproduce this
>>> >> >> >> on 2.6.27.19 with directio option against a Samba-3.0.26 server.
>>> >> >> >> I could not reproduce this at all (I used tail instead of
>>> >> >> >> statgk). I'm seeing the stuff written with negligible delay on
>>> >> >> >> the client.
>>> >> >> >>
>>> >> >> >> Are you able to reproduce this with the recent kernels? If yes,
>>> >> >> >> are you getting more clues as to what is happening with a
>>> >> >> >> tcpdump/cifsFYI ?
>>> >> >> >
>>> >> >> > About a year ago i had some short irc talk with Jeff about this
>>> >> >> > tail -f .. problem, but we dropped it, cause other stuff had
>>> >> >> > higher priority.
>>> >> >> >
>>> >> >> > I actually do my tests on Steve's recent git kernel tree - so
>>> >> >> > 2.6.29-rc7 - against recent samba_v-3-3-test.
>>> >> >> > As i already mentioned
>>> >> >> > "Btw - when using the mount option "directio", all _seems_ to
>>> >> >> > be fine." the tail -f ... problem is only seen here when
>>> >> >> > "directio" is _not_ used.
>>> >> >> >
>>> >> >> > The wire traffic to the server is only a repeated
>>> >> >> > QUERY_PATH_INFO (Query File Unix Basic)
>>> >> >> > as requested periodically from the clients program fstat()
>>> >> >> >
>>> >> >> > Btw - my statgk applet has some advantages over 'tail' when
>>> >> >> > debugging this, by also listing the size (..) change - and
>>> >> >> > showing the ability to really read the appended line(s) - and
>>> >> >> > list it as ASCII and also as some first hex bytes (which are
>>> >> >> > often nil here).
>>> >> >> >
>>> >> >> > In inode.c --> cifs_revalidate() the var "invalidate_inode" is
>>> >> >> > correctly set true.
>>> >> >> >
>>> >> >> > To me, the problematic part is inside this code fragment:
>>> >> >> >
>>> >> >> > -----------------------
>>> >> >> > if (invalidate_inode) {
>>> >> >> > /* shrink_dcache not necessary now that cifs dentry ops
>>> >> >> > are exported for negative dentries */
>>> >> >> > /* if (S_ISDIR(direntry->d_inode->i_mode))
>>> >> >> > shrink_dcache_parent(direntry); */
>>> >> >> > if (S_ISREG(direntry->d_inode->i_mode)) {
>>> >> >> > if (direntry->d_inode->i_mapping) {
>>> >> >> > wbrc =
>>> >> >> > filemap_fdatawait(direntry->d_inode->i_mapping); if (wbrc)
>>> >> >> >
>>> >> >> > CIFS_I(direntry->d_inode)->write_behind_rc = wbrc; }
>>> >> >> > /* may eventually have to do this for open
>>> >> >> > files too */ if (list_empty(&(cifsInode->openFileList))) { /*
>>> >> >> > changed on server - flush read ahead pages */ cFYI(1,
>>> >> >> > ("Invalidating read ahead data on " "closed file"));
>>> >> >> > invalidate_remote_inode(direntry->d_inode); }
>>> >> >> > }
>>> >> >> > }
>>> >> >> >
>>> >> >> > -------------------------------
>>> >> >> >
>>> >> >> > The code path to use
>>> >> >> > invalidate_remote_inode(direntry->d_inode);
>>> >> >> > is not hit.
>>> >> >> > (when i force "invalidate_remote_inode(direntry->d_inode);" here,
>>> >> >> > all looks much better)
>>> >> >> >
>>> >> >> > Sorry, when i'm wrong here. :-)
>>> >> >> >
>>> >> >> > Cheers, Günter
>>> >> >> >
>>> >> >> >> > How to test:
>>> >> >> >> > server side: use "./wrt filename" or "./wrt -s 100 filename"
>>> >> >> >> > to start writing/appending to a file on a
>>> >> >> >> > (samba) exported share (regarding the '-s' option, I though
>>> >> >> >> > writing large data would/could trigger some cifs "re-read
>>> >> >> >> > server" internals (page dirty...)) cifs client side: (one
>>> >> >> >> > might use 'tail -f /mnt/xyz/filename' as well) use ./statgk
>>> >> >> >> > /mnt/xyz/filename
>>> >> >> >> >
>>> >> >> >> > On the cifs client side "statgk" uses fstat() to get possible
>>> >> >> >> > _updated_ file info from the server (filesize, times, ...) -
>>> >> >> >> > cifs *realizes* (!) the size (times) change, but no "read
>>> >> >> >> > additional data" from the server is done.
>>> >> >> >>
>>> >> >> >> Thanks,
>>> >> >> >
>>> >> >> > _______________________________________________
>>> >> >> > linux-cifs-client mailing list
>>> >> >> > linux-cifs-client at lists.samba.org
>>> >> >> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>> >> >>
>>> >> >> I think we should take out the check for closed files for that
>>> >> >> inode if (list_empty(&(cifsInode->openFileList))) {
>>> >> >> and invalidate pages of the inode instead of gating on whether the
>>> >> >> file is closed or not, if invalidate_inode gets marked for whatever
>>> >> >> reason (e.g. change in file size).
>>> >> >
>>> >> > Do you experience this behaviour also with expanding / reading the
>>> >> > file on the client side?
>>> >> > The code-excerpt above doesn't show this case, but we have serious
>>> >> > problems using kmail on cifs and the problems looks the same: kmail
>>> >> > crashes with incorrect index-files, using directio corrects this!
>>> >> >
>>> >> > --
>>> >> > Wilhelm
>>> >> > _______________________________________________
>>> >> > linux-cifs-client mailing list
>>> >> > linux-cifs-client at lists.samba.org
>>> >> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>> >> >
>>> >>
>>> >> At least, as far as size is concerned, cifs client should either initiate
>>> >> read pages for the inode from the server if the server file size has increased
>>> >> or discard pages from the inode's address map if the server file size
>>> >> has shrunk,
>>> >> as it realizes file size change during cifs_revalidate.
>>> >> That probably would prevent kmail crashes.
>>> >>
>>> >> If the file is modified without a change in size, on the server, I am
>>> >> not sure what is
>>> >> the best way to bring over that change.
>>> >>
>>> >
>>> > Generally, you check and see if the mtime has changed and invalidate
>>> > all the clean pages of the file when it has. At least that's how it's
>>>
>>> Jeff,
>>>
>>> cifs client does this except that it does not invalidate the clean pages
>>> i.e. whenever either mtime or size changes, cifs flushes out the dirty pages
>>> but does not call invalidate_remote_inode for open files. Doing that definitely
>>> fixes the tail -f problem.
>>>
>>> But that is a huge penalty i.e. we are reading entire file for a
>>> pagefull (or less)
>>> change. And in case of large files, even one byte of change (mtime) will incurr
>>> huge network traffick. Not sure how locking helps, need to understand
>>> that better.
>>>
>>
>> There really is no other option. If you detect that the file has
>> changed on the server, you have no idea which parts of of that file has
>> changed and which haven't. Obviously, you don't want to invalidate the
>> file if the mtime changed due to a write that you just did. You do want
>> to invalidate the file if you QPathInfo and find that the time changed
>> from what you expected it to be.
>>
>> If tail -f isn't working right then this sounds like it's broken. CIFS
>> could benefit I think by moving the cache validation code into helper
>> functions. Then it's just a matter of making sure we revalidate the
>> file in the right places and at the right times.
>>
>> NFS uses posix locks as cache consistency points. On F_SETLK, it
>> does a getattr to ensure that it has up-to-date file attributes. On
>> F_UNLCK, it flushes all of the dirty data to the server. This behavior
>> isn't mandated by posix, but it helps ensure good cache consistency when
>> applications are using NFS as shared storage and posix locks to
>> serialize that access.
>>
>> Doing the same with CIFS would also probably be a good thing.
>
> Jeff,
>
> tail is broken and so is more (and less also). I am not sure how
> getting (only) file attributes
> will help. We need the (updated) file data. Perhaps once cifs
> realizes a change
> has occurred e.g. mtime has changed, cifs should re-read the pages
> that are not dirty that it has
> in the address mapping for that inode.
>
> With more, less, and tail, there are no dirty pages to flush to the server.
>
>>
>> --
>> Jeff Layton <jlayton at redhat.com>
>>
>
Perhaps, calling invalidate_remote_inode is a good option, purge the
uptodate pages from page cache and the inode's address map and
re-read as they are needed.