On Mon, Dec 1, 2008 at 5:55 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > On Mon, 1 Dec 2008 12:32:26 +0100 > Nick Piggin <npiggin@xxxxxxx> wrote: > >> 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. >> > > Ahh good point...the page would be locked there. If it's impossible for > PageUptodate to be flipped on while the page lock is held then this is > probably safe enough. > > I'd still prefer that we handle the situation where both bits are set > in cifs_write_end. Some defensive coding is warranted here I think. > That can wait until 2.6.29 though. For now, the patch in Steve's tree > should be fine, IMO. This (why this defensive code is fine) is similar to what we discussed. I agree that we can leave it as is. -- Thanks, Steve -- 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
