On Mon, Dec 01, 2008 at 06:28:49AM -0500, Jeff Layton wrote: > On Mon, 1 Dec 2008 09:44:35 +0100 > Nick Piggin <npiggin@xxxxxxx> wrote: > > > I think it actually is a problem. Suppose PageChecked is never cleared > > > like you say, we flush the page and then do a partial page write again. > > > We do a readpage this time and it fails, but the copy of data to the > > > page works. Now we hit cifs_write_end and PageChecked is set, but > > > the unwritten parts of the page actually aren't up to date. Data > > > corruption ensues... > > > > > > I agree that we should drop that patch. We might be able to make > > > cifs_write_end more efficient, but we'll need to be more careful > > > with PageChecked. > > > > Oh? I admittedly haven't looked at the source code after applying > > your latest patch, but I thought it should not be possible to have > > a leaking PageChecked. The page is under the page lock the whole > > time, so a concurrent write should not be an issue...? > > > > But a concurrent write and read is, right? > > Suppose we do a successful cifs_write_begin and set PageChecked. Another > thread then incurs a page fault and does a readpage before we copy the > data to the page. Won't we then call write_end with both PageChecked and > PageUptodate set? > > That write will be fine, of course. PageChecked is still true though, > and I think that sets up the problem I was describing... Unless cifs is doing something different from the usual case, it should lock the page over the readpage operation (the end IO handler would typically unlock the page after doing a SetPageUptodate). So concurrent reads should be protected with page lock as well. -- 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
