[linux-cifs-client] Re: fsx-linux failing with latest cifs-2.6 git tree

[Dave Kleikamp]

On Fri, 2008-11-21 at 20:02 -0600, Steve French wrote:
> On Fri, Nov 21, 2008 at 7:51 PM, Jeff Layton <jlayton at redhat.com> wrote:
> > On Sat, 22 Nov 2008 00:53:46 +0100
> > Nick Piggin <npiggin at suse.de> wrote:
> >
> >> On Fri, Nov 21, 2008 at 05:50:17PM -0500, Jeff Layton wrote:
i
> >> > Thoughts?
> >>
> >> You just have to be very careful when marking a page uptodate. This
> >> is why I removed that earlier hunk.
> >>
> >> 1) the actual write may not cover as much space as we were told here.
> >
> > Under what circumstances would that occur? The places where write_begin
> > and write_end get called look like they use the same lengths...
> >
> >> 2) the page no wlooks like this I think?
> >>
> >> 0                       offset+len          PAGE_CACHE_SIZE
> >> |---- uninitialized data ---|---- zeroes ---|
> >>
> >> Then if you SetPageUptodate, if you are using the generic_mapping_read,
> >> it can come and read the page even without locking it. If you are
> >> not using the generic pagecache operations at all, then you could do
> >> something like this if you are careful, but it still seems a bit
> >> risky.
> >>
> >> Or am I wrong about the data being uninitialized?
> >
> > Bear with me here -- page flag handling makes me dizzy...
> >
> > The latest patch that I sent only skips the read if:
> >
> > 1) the page lies beyond the current end of the file
> >
> > 2) if the page sits on top of the end of the file and the write would
> > overwrite all of the data that we would read in.
> >
> > The first part should be OK I think. A pagecache read should not be
> > able to go beyond EOF, should it? The second one is a problem. Ideally
> > we'd like to be able to get away w/o doing a read in that case, but I
> > guess we'd have to delay setting the page uptodate until after the
> > write data has been copied to it. cifs_write_end seems to expect
> > that the page is uptodate when it gets it though.
> If cifs_write_end gets a page that is not uptodate (e.g. if we don't
> have readaccess to the file) then it could be really slowww ...
> because write end will have to send each write to the server
> individually (even if the app is only 1 byte at a time) ... I don't
> mind threads racing with each other doing reads/writes as long as they
> get valid data ... but wish we could mark them up to date when we
> finish copying the write data into them so it wouldn't be so slow

Nick's point is that it isn't really safe to mark the page Uptodate
until cifs_write_end(), since the data being overwritten hasn't yet
been.

How about cifs_write_begin() marking the page PageChecked instead of
PageUptodate, of course leaving it PageUptodate if it already is?  Then
cifs_write_end() only has to handle the slow path when the page is
neither PageUptodate or PageChecked.

PageChecked == PG_owner_priv_1, which is reserved for whatever the
filesystem wants to use it for.

> But ... we also need to be checking to make sure we can cache reads
> (at least) since otherwise we could have an i_size that is up to 1
> second old
> 
> 
> Thanks,
> 
> Steve
-- 
David Kleikamp
IBM Linux Technology Center