Messages in this thread | | | Date | Fri, 19 May 2006 03:30:23 +0200 | From | Jan Kara <> | Subject | Re: [PATCH] Change ll_rw_block() calls in JBD |
| |
> Getting better :-) > > >+ was_dirty = buffer_dirty(bh); > > Why do not we use "buffer_jbddirty()"? I think Stephen has already explained it... We have a data buffer and hence we use buffer_dirty()
> > >+ 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? For two of the above comments: Under memory pressure data buffers can be written out earlier and then released by __journal_try_to_free_buffer() as they are not dirty any more. The above checks protect us against this.
> >+ 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); Yes, you are right here.
> 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); > } So you basically mean switching those two branches of if.. OK, maybe.
> 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...) Yes, probably moving it to a new function may improve the readability. Thanks for suggestions.
Honza -- Jan Kara <jack@suse.cz> SuSE CR Labs - 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/
| |