Re: [PATCH] Btrfs: fix dio write vs buffered read race V2

[Miao Xie <miaox@xxxxxxxxxxxxxx>]

On Thu, 28 Jun 2012 08:34:23 -0400, Josef Bacik wrote:
> On Wed, Jun 27, 2012 at 09:35:08PM -0600, Miao Xie wrote:
>> On Tue, 26 Jun 2012 09:42:56 -0400, Josef Bacik wrote:
>>> From: Josef Bacik <josef@xxxxxxxxxx>
>>>
>>> Miao pointed out there's a problem with mixing dio writes and buffered
>>> reads.  If the read happens between us invalidating the page range and
>>> actually locking the extent we can bring in pages into page cache.  Then
>>> once the write finishes if somebody tries to read again it will just find
>>> uptodate pages and we'll read stale data.  So we need to lock the extent and
>>> check for uptodate bits in the range.  If there are uptodate bits we need to
>>> unlock and invalidate again.  This will keep this race from happening since
>>> we will hold the extent locked until we create the ordered extent, and then
>>> teh read side always waits for ordered extents.  Thanks,
>>
>> This patch still can not work well. It is because we don't update i_size in time.
>> 	Writer		Worker		Reader
>> 	lock_extent
>> 	do direct io
>> 	end io
>> 			finish io
>> 			unlock_extent
>> 					lock_extent
>> 					check the pos is beyond EOF or not
>> 					  beyond EOF, zero the page and set it uptodate
>> 					unlock_extent
>> 	update i_size
>>
>> So I think we must update the i_size in time, and I wrote a small patch to do it:
>>
> 
> We should probably be updating i_size when we create an extent past EOF in the
> write stuff, not during endio, I will work this out and fold it into my patch.
> Good catch.

It is better that update i_size in endio, I think. because during endio, we are sure that
the data is flushed into the disk successfully, and can update i_size at ease. and if the
error happens when flushing the data into the disk, we also needn't reset i_size.

Thanks
Miao
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html