On Mon, Dec 1, 2008 at 5:55 AM, Jeff Layton <jlayton@redhat.com> wrote:
quoted text
> On Mon, 1 Dec 2008 12:32:26 +0100
> Nick Piggin <npiggin@suse.de> 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@suse.de> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html