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

Shirish S Pargaonkar shirishp at us.ibm.com
Mon Nov 17 15:42:18 GMT 2008


Jeff Layton <jlayton at redhat.com> wrote on 11/17/2008 09:34:58 AM:

> On Mon, 17 Nov 2008 09:26:57 -0600
> Dave Kleikamp <shaggy at linux.vnet.ibm.com> wrote:
> 
> > On Mon, 2008-11-17 at 10:19 -0500, Jeff Layton wrote:
> > > On Mon, 17 Nov 2008 07:02:47 -0600
> > > Dave Kleikamp <shaggy at linux.vnet.ibm.com> wrote:
> > > 
> > > > cifs: prevent cifs_writepages() from skipping unwritten pages
> > > > 
> > > > In order to write contiguous pages whenever possible, 
cifs_writepages()
> > > > asks pagevec_lookup_tag() for more pages than it may write at one 
time.
> > > > Normally, it then resets index just past the last page written
> before calling
> > > > pagevec_lookup_tag() again.
> > > > 
> > > > If cifs_writepages() can't write the first page returned, it 
> wasn't resetting
> > > > index, and the next call to pagevec_lookup_tag() resulted in 
> skipping all of
> > > > the pages it previously returned, even though 
> cifs_writepages() did nothing
> > > > with them.  This can result in data loss when the file 
> descriptor is about
> > > > to be closed.
> > > > 
> > > > This patch ensures that index gets set back to the first 
> returned page so
> > > > that none get skipped.
> > > > 
> > > > Signed-off-by: Dave Kleikamp <shaggy at linux.vnet.ibm.com>
> > > > Cc: Shirish S Pargaonkar <shirishp at us.ibm.com>
> > > > Cc: Steve French <sfrench at us.ibm.com>
> > > > Cc: Jeff Layton <jlayton at redhat.com>
> > > > 
> > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > > index 1540ada..9815e03 100644
> > > > --- a/fs/cifs/file.c
> > > > +++ b/fs/cifs/file.c
> > > > @@ -1404,7 +1404,10 @@ retry:
> > > >           if ((wbc->nr_to_write -= n_iov) <= 0)
> > > >              done = 1;
> > > >           index = next;
> > > > -      }
> > > > +      } else
> > > > +         /* Need to re-find the pages we skipped */
> > > > +         index = pvec.pages[0]->index;
> > > > +
> > > >        pagevec_release(&pvec);
> > > >     }
> > > >     if (!scanned && !done) {
> > > > 
> > > 
> > > Patch looks sane to me and fairly obvious once you know what 
> you're looking for...
> > > 
> > > Do we know what condition is occurring to make it break out of 
> the loop before n_iov can be incremented?
> > 
> > I'm not positive, but I suspect memory pressure is causing pages to be
> > written, maybe by pdflush, so a racing thread starts to write the page
> > before we lock it.  By the time we lock it and wait for writeback, 
it's
> > clean.
> 
> Thanks, makes sense...
> 
> Acked-by: Jeff Layton <jlayton at redhat.com>

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!

Regards,

Shirish
-------------- next part --------------
HTML attachment scrubbed and removed


More information about the linux-cifs-client mailing list