Re: [patch] fix the 2nd buffer race properly

[Nick Piggin <nickpiggin@yahoo.com.au>]

Andrew Morton wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:

>> > That's all a bit too complex.   How's about this instead?
>> > 
>>
>> Well you really don't need to hold the page locked for that long.
> 
> 
> Is a rare case, so there's no perfomance issue here.
> 

Well it is for buffered writes, right? And in general it will be OK,
but if you run into queue/memory congestion when submitting the IO,
it will be locked for a lot longer than required.

> I do prefer the idea of simply keeping other threads of control out of the
> page until this thread has finished playing with its buffers.
> 

That's exactly what my patch does too!

> (The buffer-ring walk we have in there is racy against page reclaim, too. 
> If only the first buffer is dirty, we inspect the other buffers after
> PageWriteback has potentially cleared.)
> 

Well we do have a reference on the buffers, so in this particular case
perhaps not. But we have no mutual exclusion on the page or buffers
so I agree it could be racy against a lot of things.

> 
>> block_read_full_page, nobh_prepare_write both use the same sort of
>> array of buffer heads logic - I think it makes sense not to touch
>> any buffers after submitting them all for IO...?
> 
> 
> Well.  Most code in there uses the ->b_this_page walk.
> 

block_read_full_page does the walk in order to gather up the buffers.
They then get submitted for IO via the buffer head array walk.

I prefer my patch. I don't think it is particularly complex.

-- 
SUSE Labs, Novell Inc.