From: Eric Sandeen on
Badari Pulavarty wrote:

> This is exactly the solution I proposed earlier (to check
> buffer_mapped() before calling submit_bh()).
> But at that time, Jan pointed out that the whole handling is wrong.
>
> But if this is the only case we need to handle, I am okay with this band
> aid :)

Doh!

And we come full circle... ok let me go reread that thread, it got long
enough I had to swap out... :)

-Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Badari Pulavarty on
Eric Sandeen wrote:
> Andrew Morton wrote:
>
>
>> On Thu, 12 Oct 2006 14:28:20 +0200
>> Jan Kara <jack(a)suse.cz> wrote:
>>
>>
>>
>>> Where can we call
>>> journal_dirty_data() without PageLock?
>>>
>>>
>> block_write_full_page() will unlock the page, so ext3_writepage()
>> will run journal_dirty_data_fn() against an unlocked page.
>>
>> I haven't looked into the exact details of the race, but it should
>> be addressable via jbd_lock_bh_state() or j_list_lock coverage
>>
> I'm testing with something like this now; seem sane?
>
> journal_dirty_data & journal_unmap_data both check do
> jbd_lock_bh_state(bh) close to the top... journal_dirty_data_fn has checked
> buffer_mapped before getting into journal_dirty_data, but that state may
> change before the lock is grabbed. Similarly re-check after we drop the lock.
>
>
This is exactly the solution I proposed earlier (to check
buffer_mapped() before calling submit_bh()).
But at that time, Jan pointed out that the whole handling is wrong.

But if this is the only case we need to handle, I am okay with this band
aid :)

Thanks,
Badari

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Badari Pulavarty on
Eric Sandeen wrote:
> Badari Pulavarty wrote:
>
>
>> This is exactly the solution I proposed earlier (to check
>> buffer_mapped() before calling submit_bh()).
>> But at that time, Jan pointed out that the whole handling is wrong.
>>
>> But if this is the only case we need to handle, I am okay with this band
>> aid :)
>>
>
> Doh!
>
> And we come full circle... ok let me go reread that thread, it got long
> enough I had to swap out... :)

Don't bother. Lets see if this is the only case that needs fixing and
move forward..

Thanks,
Badari


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jan Kara on
> Eric Sandeen wrote:
> >Andrew Morton wrote:
> >
> >
> >>On Thu, 12 Oct 2006 14:28:20 +0200
> >>Jan Kara <jack(a)suse.cz> wrote:
> >>
> >>
> >>
> >>>Where can we call
> >>>journal_dirty_data() without PageLock?
> >>>
> >>>
> >>block_write_full_page() will unlock the page, so ext3_writepage()
> >>will run journal_dirty_data_fn() against an unlocked page.
> >>
> >>I haven't looked into the exact details of the race, but it should
> >>be addressable via jbd_lock_bh_state() or j_list_lock coverage
> >>
> >I'm testing with something like this now; seem sane?
> >
> >journal_dirty_data & journal_unmap_data both check do
> >jbd_lock_bh_state(bh) close to the top... journal_dirty_data_fn has
> >checked buffer_mapped before getting into journal_dirty_data, but that
> >state may
> >change before the lock is grabbed. Similarly re-check after we drop the
> >lock.
> >
> >
> This is exactly the solution I proposed earlier (to check
> buffer_mapped() before calling submit_bh()).
> But at that time, Jan pointed out that the whole handling is wrong.
Yes, and it was. However it turned out that there are more problems
than I thought ;).

> But if this is the only case we need to handle, I am okay with this band
> aid :)
I think Eric's patch may be a part of it. But we still need to check whether
the buffer is not after EOF before submitting it (or better said just
after we manage to lock the buffer). Because while we are waiting for
the buffer lock, journal_unmap_buffer() can still come and steal the
buffer - at least the write-out in journal_dirty_data() definitely needs
the check if I haven't overlooked something.

Honza
--
Jan Kara <jack(a)suse.cz>
SuSE CR Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Eric Sandeen on
Jan Kara wrote:

>> This is exactly the solution I proposed earlier (to check
>> buffer_mapped() before calling submit_bh()).
>> But at that time, Jan pointed out that the whole handling is wrong.
> Yes, and it was. However it turned out that there are more problems
> than I thought ;).
>
>> But if this is the only case we need to handle, I am okay with this band
>> aid :)
> I think Eric's patch may be a part of it. But we still need to check whether
> the buffer is not after EOF before submitting it (or better said just
> after we manage to lock the buffer). Because while we are waiting for
> the buffer lock, journal_unmap_buffer() can still come and steal the
> buffer - at least the write-out in journal_dirty_data() definitely needs
> the check if I haven't overlooked something.

Ok, let me think on that today. My first reaction is that if we have
the bh state lock and pay attention to mapped in journal_dirty_data(),
then any blocks past EOF which have gotten unmapped by
journal_unmap_buffer will be recognized as such (because they are now
unmapped... without needing to check for past EOF...) and we'll be fine.

As a datapoint, davej's stresstest (several fsx's and fsstresses)
survived an overnight run on his box, which used to panic in < 2 hrs.
Survived about 6 hours on my box until I intentionally stopped it; my
box had added a write/truncate test in a loop, with a bunch of periodic
syncs as well....

-Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/