Messages in this thread | | | Date | Thu, 18 May 2006 17:11:45 +0200 | From | Zoltan Menyhart <> | Subject | Re: [PATCH] Change ll_rw_block() calls in JBD |
| |
Getting better :-)
> + was_dirty = buffer_dirty(bh);
Why do not we use "buffer_jbddirty()"?
> + if (was_dirty && test_set_buffer_locked(bh)) { > + BUFFER_TRACE(bh, "needs blocking lock"); > + get_bh(bh);
Why do you need a "get_bh(bh)"? "bh" is attached to "jh". Can it go away while waiting for the buffer lock? ("jh" in on "t_sync_datalist", it cannot go away.)
> + spin_unlock(&journal->j_list_lock); > + lock_buffer(bh); > + spin_lock(&journal->j_list_lock); > + /* Someone already cleaned up the buffer? Restart. */ > + if (!buffer_jbd(bh) || jh->b_jlist != BJ_SyncData) {
Who (else) can take away the journal head, remove our "jh" from the synch. data list?
> + else { > + BUFFER_TRACE(bh, "needs writeout, submitting"); > __journal_temp_unlink_buffer(jh); > __journal_file_buffer(jh, commit_transaction, > BJ_Locked);
A simple "__journal_file_buffer(...)" could be enough as it includes:
if (jh->b_transaction) __journal_temp_unlink_buffer(jh);
Would not it be more easy to read like this?
if ((!was_dirty && buffer_locked(bh)) || (was_dirty && test_clear_buffer_dirty(bh))) { BUFFER_TRACE(bh, "needs writeout, submitting"); __journal_temp_unlink_buffer(jh); __journal_file_buffer(jh, commit_transaction, BJ_Locked); jbd_unlock_bh_state(bh); if (was_dirty) { get_bh(bh); bh->b_end_io = end_buffer_write_sync; submit_bh(WRITE, bh); } } else {
BUFFER_TRACE(bh, "writeout complete: unfile"); __journal_unfile_buffer(jh); jbd_unlock_bh_state(bh); journal_remove_journal_head(bh); if (was_dirty) unlock_buffer(bh); put_bh(bh); }
As synch. data handling is a compact stuff, cannot it be moved out from "journal_commit_transaction()" as e.g. "journal_write_revoke_records()"? (Just for a better readability...)
Thanks,
Zoltan
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |