On Fri, 2008-11-21 at 20:02 -0600, Steve French wrote: > On Fri, Nov 21, 2008 at 7:51 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Sat, 22 Nov 2008 00:53:46 +0100 > > Nick Piggin <npiggin@xxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
