[linux-cifs-client] tail -f /mounted/share/filename is not working

[Shirish Pargaonkar]

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.

I do not use kmail, have never usd it but will attempt to set it up
and use it over
samba shares via cifs client to see if the problem (crashes) can be recreated.