Version 0.8.60 for kernel 2.4.21 released
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 19 lines · Stephen C. Tweedie
Hi,
Quoted 4 lines · Andreas Gruenbacher
- The extended attribute locking strategy is now more fine grained. The getxattr and listxattr inode operations no longer take the inode semaphore. This reduces contention between extended attribute operations and data accesses considerably.
Nice!
However,
split-0.8.60/jbd-credits-fix.patchmakes a broken change to the jbd logic, as it admits in the comment:Quoted 4 lines
This logic effectively gives another transaction handle a free buffer credit. These could conceivably accumulate and cause a journal overflow. This is a separate problem and needs changes to the
t_outstanding_creditsaccounting and the logic instart_this_handle.
Quoted 5 lines
That's a bit of a problem!
journal_releaseis _supposed_ to work the old way. It's a way of releasing buffers optimistically, without breaking any guarantees --- if you're lucky enough that the buffer can be freed immediately then you get a credit back, but there is no guarantee.
Please note that the comment is verbatim from one of Andrew's patches that went into 2.5, so the problem is not only a problem in the xattr patch. I am taking Andrew into the CC.
We have the following pattern in 2.5 ext3:
for(;;) { /* several iterations */
ext3_journal_get_write_access(handle, bh);
if (this_bh_is_okay(bh)) {
use_this_bh(bh);
ext3_journal_dirty_metadata(handle, bh);
break;
}
journal_release_buffer(handle, bh);
}This means that get_write_access -- release_buffer must leave the handle with a constant number of credits. This pattern is used in more than one place.
Quoted 2 lines
There can't be any guarantee, because another task may already be relying on the existing credit for that buffer.
Indeed. And it should have been charged for that buffer in the first place. I think that each get_write_access that succeeds should cost one credit, no matter whether the block actually needed to be journaled or not.
The effect of the current accounting logic is that some handles get accounted for buffers they don't require, wile those handles that actually get the credits get them for free.
The itch with charging each handle individually would be to reduce the problem of over-accounting. Two handles that share a block would both be charged, leaving less known available room in the journal. This doesn't seem like a problem for short lived handles. For long lived handles, ext3_journal_extend could most likely work around this problem, assuming that no buffers from before the extend will be revoked afterwards.
Quoted 7 lines
The core problem here is the reliance on the bh lock for data synchronisation, which forces you to declare the transaction credit before you've done the locking which will tell you whether or not you need that credit in the first place. Using alternative locking would avoid that entirely. 2.5 uses the idea of locking based on different bits in the bh state, for example; that sounds like it would avoid your particular problem here nicely.
That would work, and even with fewer pains. I will have to look if this can be easily ported to 2.4 as well. AFAIK sleeping on b_wait waiting on other bits than BH_Lock in 2.4 might deadlock, right?
Thanks for your feedback.
Cheers,
Andreas.
Quoted 7 lines · Andreas Gruenbacher
Quoted 7 lines
That's a bit of a problem!
journal_releaseis _supposed_ to work the old way. It's a way of releasing buffers optimistically, without breaking any guarantees --- if you're lucky enough that the buffer can be freed immediately then you get a credit back, but there is no guarantee.
Well the bigger problem was:
CPU0 CPU1 get_write_access() get_write_access() release_buffer() journal_dirty_metadata -> it's not journalled. Splat.
I don't think release_buffer() was ever correct. Unless all callers happened to be under lock_super() or something. Of course, it's #if'ed away in 2.4 anyway.
Quoted 2 lines
The effect of the current accounting logic is that some handles get accounted for buffers they don't require,
How so?
Quoted 2 lines
wile those handles that actually
get the credits get them for free.
yup. And t_outstanding_credits ends up wrong. It's a rare occurrence and we have quite a lot of reserve so it doesn't hit in practice.
Simpler would be to just bump t_outstanding_credits each time we move a buffer from BJ_Reserved to BJ_Metadata.
Quoted 11 lines
Quoted 7 lines
The core problem here is the reliance on the bh lock for data synchronisation, which forces you to declare the transaction credit before you've done the locking which will tell you whether or not you need that credit in the first place. Using alternative locking would avoid that entirely. 2.5 uses the idea of locking based on different bits in the bh state, for example; that sounds like it would avoid your particular problem here nicely.
That would work, and even with fewer pains. I will have to look if this can be easily ported to 2.4 as well. AFAIK sleeping on
b_waitwaiting on other bits thanBH_Lockin 2.4 might deadlock, right?
Yup. Sleeping locks are a scalability disaster, so they're all spinlocks in there. With the exception of lock_super() in ext3_orphan_add/del, and that lock_super() is being a problem too.
So I'm OK with the optimistic try-it-oh-shucks approach to get_write_access(). But the credit accounting does need to be rethought and redone. I've 90% done that twice, ended up throwing them away.
Quoted 19 lines · Andrew Morton
Quoted 7 lines · Andreas Gruenbacher
Quoted 7 lines
That's a bit of a problem!
journal_releaseis _supposed_ to work the old way. It's a way of releasing buffers optimistically, without breaking any guarantees --- if you're lucky enough that the buffer can be freed immediately then you get a credit back, but there is no guarantee.Well the bigger problem was:
CPU0 CPU1 get_write_access() get_write_access() release_buffer() journal_dirty_metadata -> it's not journalled. Splat.
Quoted 3 lines
I don't think
release_buffer()was ever correct. Unless all callers happened to be underlock_super()or something. Of course, it's #if'ed away in 2.4 anyway.
Indeed.
Quoted 4 lines
Quoted 2 lines
The effect of the current accounting logic is that some handles get accounted for buffers they don't require,
How so?
In your above example, at the end the left task is still be charged while the right task gets this buffer for free. It must not be, or else the number of iterations of get_write_access -- release_buffer must be limited.
Quoted 5 lines
Quoted 2 lines
wile those handles that actually
get the credits get them for free.yup. And
t_outstanding_creditsends up wrong. It's a rare occurrence and we have quite a lot of reserve so it doesn't hit in practice.
t_outstanding_credits ends up correct again after all the handles are stopped. But that's not good enough; the problem is the time window in between.
Quoted 2 lines
Simpler would be to just bump
t_outstanding_creditseach time we move a buffer fromBJ_ReservedtoBJ_Metadata.
Alright. That would still leave us with the funny credits parameters of get_write_access, release_buffer, etc. I think they should really go away.
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
The core problem here is the reliance on the bh lock for data synchronisation, which forces you to declare the transaction credit before you've done the locking which will tell you whether or not you need that credit in the first place. Using alternative locking would avoid that entirely. 2.5 uses the idea of locking based on different bits in the bh state, for example; that sounds like it would avoid your particular problem here nicely.
That would work, and even with fewer pains. I will have to look if this can be easily ported to 2.4 as well. AFAIK sleeping on
b_waitwaiting on other bits thanBH_Lockin 2.4 might deadlock, right?
Quoted 7 lines
Yup. Sleeping locks are a scalability disaster, so they're all spinlocks in there. With the exception of
lock_super()in ext3_orphan_add/del, and thatlock_super()is being a problem too.So I'm OK with the optimistic try-it-oh-shucks approach to
get_write_access(). But the credit accounting does need to be rethought and redone. I've 90% done that twice, ended up throwing them away.
To quota a recent and quite popular movie, I know *exactly* what you mean :-)
Cheers.
------------------------------------------------------------------------
Andreas Gruenbacher, [email protected]
Contact information: http://www.bestbits.at/~ag/