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

Nick Piggin
2005-04-28T01:29:35Z

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.