[linux-cifs-client] Re: [PATCH] cifs: prevent cifs_writepages() from skipping unwritten pages

[Dave Kleikamp]

On Mon, 2008-11-17 at 11:02 -0600, Shirish S Pargaonkar wrote:
> 
> 
> Dave Kleikamp <shaggy at linux.vnet.ibm.com> wrote on 11/17/2008 09:59:07
> AM:
> 
> > On Mon, 2008-11-17 at 09:42 -0600, Shirish S Pargaonkar wrote:
> > 
> > > Shaggy, 
> > > 
> > > But if the pdflush grabbed it to write, it would percolate down
> to 
> > > calling cifs_writepages right?  If so, how come we do not see
> those 
> > > at the server, we would have seen samba server logging those write
> > > andx 
> > > and perhaps the offsets would be out of order but there in the
> samba 
> > > log file nonetheless! 
> > > 
> > You're more familiar with what's been logged, but let me explain
> what I
> > think may be happening, and you can tell me if it fits your
> > observations.
> > 
> > The cifs_flush thread gets into cifs_writepages and
> pagevec_lookup_tag()
> > returns 14 pages, let's say pages 0-13.
> >  -----
> > The pdflush thread writes some smaller number of pages, 0-4.
> >  -----
> > After waiting on page 0, the cifs_flush thread sees that it's no
> longer
> > dirty, and calls pagevec_lookup_tag() again, but index has been set
> to
> > 14, so it gets back pages 14-27.  Pages 5-13 get missed.
> > -- 
> > David Kleikamp
> > IBM Linux Technology Center
> > 
> 
> I see the opposite, i.e. the upper pages are not received by samba
> server. 
> For example, after writing 56KB at the 56K boundary, 
> 8 pages are not received by the samba server but rest of them are. 
> 
> Here is an example of what I see... 
> 
> This is the log level 10 output on the samba server. 
> 
> 
> real_write_file (39.test): pos = 19267584, size = 57344, returned
> 57344 
> real_write_file (39.test): pos = 19324928, size = 57344, returned
> 57344 
> real_write_file (39.test): pos = 19382272, size = 57344, returned
> 57344 
> real_write_file (39.test): pos = 19439616, size = 57344, returned
> 57344 
> real_write_file (39.test): pos = 19496960, size = 57344, returned
> 57344 
> real_write_file (39.test): pos = 19587072, size = 57344, returned
> 57344 
> real_write_file (39.test): pos = 19644416, size = 57344, returned
> 57344 
> real_write_file (39.test): pos = 19701760, size = 57344, returned
> 57344 
> 
> which matches with the hole in the file as the hex dump of the file
> shows 
> 
> 12a5fe0: 6db6 6db6 6db6 6db6 6db6 6db6 6db6 6db6  m.m.m.m.m.m.m.m. 
> 12a5ff0: 6db6 6db6 6db6 6db6 6db6 6db6 6db6 6db6  m.m.m.m.m.m.m.m. 
> 12a6000: 0000 0000 0000 0000 0000 0000 0000 0000  ................ 
> 12a6010: 0000 0000 0000 0000 0000 0000 0000 0000  ................ 
>                         ......... 
>                        ......... 
> 12adfe0: 0000 0000 0000 0000 0000 0000 0000 0000  ................ 
> 12adff0: 0000 0000 0000 0000 0000 0000 0000 0000  ................ 
> 12ae000: 6db6 6db6 6db6 6db6 6db6 6db6 6db6 6db6  m.m.m.m.m.m.m.m. 
> 12ae010: 6db6 6db6 6db6 6db6 6db6 6db6 6db6 6db6  m.m.m.m.m.m.m.m. 
> 12ae020: 6db6 6db6 6db6 6db6 6db6 6db6 6db6 6db6  m.m.m.m.m.m.m.m. 
> 
> I do not see the top 8 pages starting at position 19554304 but see 
> 14 pages starting at position 19587072 instead. 
> Upto position 19496960, all position (offsets in write andx) have
> been 
> on 56K boundary but not starting at position 19587072 and IMHO that 
> is because top first 8 pages of 14 pages starting at 19554304 did 
> never arrive at the samba server. 
> If so, we could have seen an entry like this in the samba server log 
>  real_write_file (39.test): pos = 19554304, size = 32768, returned
> 32768 

Now I'm confused.  If cifs_writepages breaks out of the for loop for
some reason other than that the page is already marked non-dirty, then
why does it resume 8 pages later and not 14?  How does my patch fix it?
Going back to try the same page succeeds the next time?

I could see something like this during write-behind, if pages are being
flushed out while the testcase is still writing to the file, but I would
expect a later call to cifs_flush when the file is closed to go back and
clean it up afterwards.

> Regards, 
> 
> Shirish 
-- 
David Kleikamp
IBM Linux Technology Center