[linux-cifs-client] tail -f /mounted/share/filename is not
working
Shirish Pargaonkar
shirishpargaonkar at gmail.com
Mon Apr 20 10:36:16 GMT 2009
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>
>
More information about the linux-cifs-client
mailing list