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

[Miao Xie <miaox@xxxxxxxxxxxxxx>]

On Fri, 29 Jun 2012 09:05:10 -0400, Chris Mason wrote:
> On Thu, Jun 28, 2012 at 08:18:35PM -0600, Miao Xie wrote:
>> 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.
> 
> I think the i_size update should happen sooner.  The rest of the
> filesystems work that way, and it will have fewer interaction problems
> with the VM.

Thanks for your explanation.

Regards
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