* Re: attempt to fix race in bigalloc code path
@ 2011-08-22 13:54 Yongqiang Yang
2011-08-22 17:30 ` Aditya Kali
0 siblings, 1 reply; 3+ messages in thread
From: Yongqiang Yang @ 2011-08-22 13:54 UTC (permalink / raw)
To: adityakali; +Cc: Jan Kara, Ted Ts'o, Ext4 Developers List
Hi,
I can not find the patch - attempt to fix race in bigalloc code path -
in the mailing list.
I don't think this patch can work. set_buffers_da_mapped() is called
under i_data_sem, so it can not lock page. If the page is not locked,
calling page_has_buffers() is not secure as Jan pointed out
previously. If a page is partially mapped(block size < page size),
setting BH_Da_Mapped without locking page is not safe too.
BTW: find_delalloc_range() does not lock page as ext4_fiemap_cb().
As Jan said, it can crash system. BUT we can not lock page in both
scenarios, because i_data_sem is hold. So delayed extent list is
necessary. I have finished the code implementing delayed extent list,
which needs some further testing.
What are your opinions?
--
Best Wishes
Yongqiang Yang
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: attempt to fix race in bigalloc code path
2011-08-22 13:54 attempt to fix race in bigalloc code path Yongqiang Yang
@ 2011-08-22 17:30 ` Aditya Kali
2011-08-23 1:12 ` Yongqiang Yang
0 siblings, 1 reply; 3+ messages in thread
From: Aditya Kali @ 2011-08-22 17:30 UTC (permalink / raw)
To: Yongqiang Yang; +Cc: Jan Kara, Ted Ts'o, Ext4 Developers List
On Mon, Aug 22, 2011 at 6:54 AM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
> Hi,
>
> I can not find the patch - attempt to fix race in bigalloc code path -
> in the mailing list.
>
> I don't think this patch can work. set_buffers_da_mapped() is called
> under i_data_sem, so it can not lock page. If the page is not locked,
> calling page_has_buffers() is not secure as Jan pointed out
> previously. If a page is partially mapped(block size < page size),
> setting BH_Da_Mapped without locking page is not safe too.
>
The BH_Da_Mapped is set only during page writeback when the page is
locked (in write_cache_pages_da). So, the setting of BH_Da_Mapped
should be safe, no ?
> BTW: find_delalloc_range() does not lock page as ext4_fiemap_cb().
> As Jan said, it can crash system. BUT we can not lock page in both
> scenarios, because i_data_sem is hold. So delayed extent list is
> necessary. I have finished the code implementing delayed extent list,
> which needs some further testing.
>
>
> What are your opinions?
>
I agree that having a delayed extent list will greatly simply this
solution in this case.
>
> --
> Best Wishes
> Yongqiang Yang
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: attempt to fix race in bigalloc code path
2011-08-22 17:30 ` Aditya Kali
@ 2011-08-23 1:12 ` Yongqiang Yang
0 siblings, 0 replies; 3+ messages in thread
From: Yongqiang Yang @ 2011-08-23 1:12 UTC (permalink / raw)
To: Aditya Kali; +Cc: Jan Kara, Ted Ts'o, Ext4 Developers List
On Tue, Aug 23, 2011 at 1:30 AM, Aditya Kali <adityakali@google.com> wrote:
> On Mon, Aug 22, 2011 at 6:54 AM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
>> Hi,
>>
>> I can not find the patch - attempt to fix race in bigalloc code path -
>> in the mailing list.
>>
>> I don't think this patch can work. set_buffers_da_mapped() is called
>> under i_data_sem, so it can not lock page. If the page is not locked,
>> calling page_has_buffers() is not secure as Jan pointed out
>> previously. If a page is partially mapped(block size < page size),
>> setting BH_Da_Mapped without locking page is not safe too.
>>
> The BH_Da_Mapped is set only during page writeback when the page is
> locked (in write_cache_pages_da). So, the setting of BH_Da_Mapped
> should be safe, no ?
Sorry for noise and thank you for your explanation, you are right.
IMHO, we can clear BH_Delayed and set BH_Mapped instead.
Yongqiang.
>
>> BTW: find_delalloc_range() does not lock page as ext4_fiemap_cb().
>> As Jan said, it can crash system. BUT we can not lock page in both
>> scenarios, because i_data_sem is hold. So delayed extent list is
>> necessary. I have finished the code implementing delayed extent list,
>> which needs some further testing.
>>
>>
>> What are your opinions?
>>
> I agree that having a delayed extent list will greatly simply this
> solution in this case.
>
>>
>> --
>> Best Wishes
>> Yongqiang Yang
>>
>
--
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-08-23 1:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-22 13:54 attempt to fix race in bigalloc code path Yongqiang Yang
2011-08-22 17:30 ` Aditya Kali
2011-08-23 1:12 ` Yongqiang Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).