Re: tail -f /mounted/share/filename is not working

On Fri, Mar 20, 2009 at 10:50 PM, Günter Kukkukk <[email protected]> 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
> [email protected]
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

I do not think calling/forcing invalidate_remote_inode() in
cifs_revalidate() is the solution, it will just get rid of uptodate
pages from the mapping of this inode, and cifs client will end up
reading the page/pages from server.
Has to be a better fix.