hmm.
looks Andrew Morton should return BKL in ext3_get_block_handle() in -mm tree?
this BKL protects ext3_alloc_branch() -> ext3_alloc_block() -> ext3_new_block()
call chain. or we may implement new protection schema where each jh has some
reference alike 'used by transaction N'
Andrew?
>>>>> Stephen C Tweedie (SCT) writes:
SCT> Not with BKL. Without it, yes, that's definitely a risk, and you need
SCT> some locking for the access to b_committed_data. Without that, even if
SCT> you keep the jh->b_committed_data field valid, you risk freeing the old
SCT> copy that another thread is using.
Alex Tomas <[email protected]> wrote:
>
> looks Andrew Morton should return BKL in ext3_get_block_handle() in -mm tree?
> this BKL protects ext3_alloc_branch() -> ext3_alloc_block() -> ext3_new_block()
> call chain. or we may implement new protection schema where each jh has some
> reference alike 'used by transaction N'
Can this be solved by spinlocking the relevant buffer_head, in a similar
way to your recent changes to journal_add_journal_head()?
>>>>> Andrew Morton (AM) writes:
AM> Alex Tomas <[email protected]> wrote:
>>
>> looks Andrew Morton should return BKL in ext3_get_block_handle() in -mm tree?
>> this BKL protects ext3_alloc_branch() -> ext3_alloc_block() -> ext3_new_block()
>> call chain. or we may implement new protection schema where each jh has some
>> reference alike 'used by transaction N'
AM> Can this be solved by spinlocking the relevant buffer_head, in a similar
AM> way to your recent changes to journal_add_journal_head()?
yes, we may protect it by such lock, but this lock have to be held all time
ext3_new_block() uses some b_committed_data because last one may be freed
during current ext3_new_block(). I don't think it's good.
probably, another solution is do not free b_committed_data if it's referenced
by new transaction and make copy from b_frozen instead of freeing it.
Alex Tomas <[email protected]> wrote:
>
> yes, we may protect it by such lock, but this lock have to be held all time
> ext3_new_block() uses some b_committed_data because last one may be freed
> during current ext3_new_block(). I don't think it's good.
Take a closer look - I don't think it'll be too messy, and certainly the
hold times will not be a problem.
hi!
I think current journal_release_buffer() which is used by ext3 allocator
in -mm tree has a bug. look, it won't decrement credits if concurrent
thread already put buffer on metadata list. but this may ext3_try_to_allocate()
may overflow handle's credist.
so, jbd will hit J_ASSERT_JH(jh, handle->h_buffer_credits > 0) somewhere
void journal_release_buffer (handle_t *handle, struct buffer_head *bh)
{
.....
if (jh->b_jlist == BJ_Reserved && jh->b_transaction == transaction &&
!buffer_jdirty(jh2bh(jh))) {
JBUFFER_TRACE(jh, "unused: refiling it");
handle->h_buffer_credits++;
__journal_refile_buffer(jh);
}
....
}
here is the patch against 2.5.69-mm6:
diff -puNr linux-2.5.69-mm6/fs/jbd/transaction.c edited/fs/jbd/transaction.c
--- linux-2.5.69-mm6/fs/jbd/transaction.c Fri May 16 18:03:20 2003
+++ b_commited_data-race/fs/jbd/transaction.c Fri May 23 11:10:21 2003
@@ -1146,10 +1146,10 @@ void journal_release_buffer (handle_t *h
if (jh->b_jlist == BJ_Reserved && jh->b_transaction == transaction &&
!buffer_jdirty(jh2bh(jh))) {
JBUFFER_TRACE(jh, "unused: refiling it");
- handle->h_buffer_credits++;
__journal_refile_buffer(jh);
}
spin_unlock(&journal_datalist_lock);
+ handle->h_buffer_credits++;
JBUFFER_TRACE(jh, "exit");
unlock_journal(journal);
Alex Tomas <[email protected]> wrote:
>
> I think current journal_release_buffer() which is used by ext3 allocator
> in -mm tree has a bug. look, it won't decrement credits if concurrent
> thread already put buffer on metadata list. but this may ext3_try_to_allocate()
> may overflow handle's credist.
Yes, that is so.
But some other handle has gained itself a free buffer. That is actually
harmless, except for one thing: as the handles are closed down,
->t_outstanding_credits will be too small. We could conceivably end up
overflowing the journal.
umm, one possible solution to that is to rework the t_outstanding_credits
logic so that we instead record:
number of buffers attached to the transaction +
sum of the initial size of all currently-running handles.
as each handle is closed off, we subtract its initial size from the above
metric. Any buffers which that handle happened to add to the lists would
have already been accounted for, when they were added.
I _think_ that'll work...
On May 23, 2003 01:26 -0700, Andrew Morton wrote:
> umm, one possible solution to that is to rework the t_outstanding_credits
> logic so that we instead record:
>
> number of buffers attached to the transaction +
> sum of the initial size of all currently-running handles.
>
> as each handle is closed off, we subtract its initial size from the above
> metric. Any buffers which that handle happened to add to the lists would
> have already been accounted for, when they were added.
One other benefit of doing it that way is that having the original handle
size kept in the handle means that you can properly flag errors when a
nested transaction is "started" on an existing handle and the new start
wants more credits than were actually in the handle.
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/