On Tue, Mar 22, 2011 at 12:42 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > On Tue, 22 Mar 2011 12:13:03 -0500 > Steve French <smfrench@xxxxxxxxx> wrote: > >> On Tue, Mar 22, 2011 at 12:03 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: >> > On Tue, 22 Mar 2011 11:46:54 -0500 >> > Steve French <smfrench@xxxxxxxxx> wrote: >> > >> >> On Tue, Mar 22, 2011 at 11:42 AM, Steve French <smfrench@xxxxxxxxx> wrote: >> >> > Isn't there a lock across lseek? >> >> > >> >> >>> but how could we have PageDirty? We just did filemap_write_and_wait >> >> >>> (unless some private mmap case where it may be ok?). >> >> >>> >> >> >> >> >> >> Nothing prevents the page from being redirtied after writeback. >> >> >> >> If it is redirtied, we immediately do a filemap_fdatawrite anyway (in >> >> the write/aio_write path) so there isn't a delay in writing data to >> >> the server. >> >> >> > >> > The bottom line is that you're asking to revalidate the file, finding >> > that it has changed and are trying to invalidate the pagecache. Some of >> > those pages are may not be invalidated though, so you won't end up with >> > new data from the server if you go to do a read. >> > >> > The whole point of this patch is to fix data-integrity issues, right? >> > So why only do a half-assed job of that? >> >> The data gets written (immediately). The page gets freed either >> immediately or (in the unlikely event it was busy) the next time we do >> a seek or revalidate or any of the other places that checks the >> invalidate flag. Not sure how there is a data integrity issue. If we >> do return EBUSY - it makes no sense on seek (we got the updated file >> size) - the only case it might make sense on is strictcache read which >> was racing with an mmapped write and in that case the data the client >> has is up to date and the next read will toss the page out of cache >> since the mapping is marked invalid. Seems a lot of complexity to >> return EBUSY to strictcache read since there isn't anything it can do >> about it other than spin and retry and we would retry anyway on the >> next operation on that file. >> > > The fact that data gets written immediately is immaterial. It's still > in cache so a pagefault can find it, and it's still potentially wrong. > I'm also not clear on why you're focused on seeks. > > What matters is that a page got dirtied while it had data on it that > we were getting ready to toss out. The right course of action at that > point is to try and write out the data and then return an error Other than for strict_fsync (which looks like it should be able to return an error and doesn't) - return an error to who? We save errors on filemap_fdatawrite in the mapping. Of the other callers of invalidate_mapping: the most common ones: revalidate doesn't care that write and read are racing with each other, and lseek doesn't care, and there is no data integrity issue. All the data is written immediately, the page is tossed from the cache as soon as it is not in the middle of a write (presumably the next operation on it). Read is getting the most current copy of the page out of the client's page cache (since it is in the middle of a local write, the client's copy is more current anyway). If there were an error on filemap_fdatawrite it is saved in the mapping (doesn't that returned it on the next write or flush). It does look like we should return an error back on strict_fsync though if filemap_fdatawrite failed. If tossing the page out of the page cache is delayed until the next read, write or seek that seems ok to me - but we should make sure that fsync (and deferred errors returned on write) does return errors - and there may be a hole in the case of strict fsync -- Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
